From 859148942ed9293b26afd8a5d853e78091386252 Mon Sep 17 00:00:00 2001 From: Alexander Zobnin Date: Thu, 16 Jun 2022 17:02:03 +0300 Subject: [PATCH] Service accounts: refactor errors (#50917) --- pkg/services/serviceaccounts/api/api.go | 11 ++-- pkg/services/serviceaccounts/api/api_test.go | 2 +- pkg/services/serviceaccounts/api/token.go | 5 +- .../serviceaccounts/database/database.go | 11 ++-- .../serviceaccounts/database/errors.go | 54 +++---------------- .../serviceaccounts/database/token_store.go | 15 +++--- pkg/services/serviceaccounts/models.go | 4 ++ 7 files changed, 31 insertions(+), 71 deletions(-) diff --git a/pkg/services/serviceaccounts/api/api.go b/pkg/services/serviceaccounts/api/api.go index 23876d6cf20..dd44dc2f359 100644 --- a/pkg/services/serviceaccounts/api/api.go +++ b/pkg/services/serviceaccounts/api/api.go @@ -86,18 +86,15 @@ func (api *ServiceAccountsAPI) RegisterAPIEndpoints( // POST /api/serviceaccounts func (api *ServiceAccountsAPI) CreateServiceAccount(c *models.ReqContext) response.Response { - type createServiceAccountForm struct { - Name string `json:"name" binding:"Required"` - } - cmd := createServiceAccountForm{} + cmd := serviceaccounts.CreateServiceAccountForm{} if err := web.Bind(c.Req, &cmd); err != nil { return response.Error(http.StatusBadRequest, "Bad request data", err) } serviceAccount, err := api.store.CreateServiceAccount(c.Req.Context(), c.OrgId, cmd.Name) switch { - case errors.Is(err, &database.ErrSAInvalidName{}): - return response.Error(http.StatusBadRequest, "Failed due to %s", err) + case errors.Is(err, database.ErrServiceAccountAlreadyExists): + return response.Error(http.StatusBadRequest, "Failed to create service account", err) case err != nil: return response.Error(http.StatusInternalServerError, "Failed to create service account", err) } @@ -143,7 +140,7 @@ func (api *ServiceAccountsAPI) UpdateServiceAccount(c *models.ReqContext) respon return response.Error(http.StatusBadRequest, "Service Account ID is invalid", err) } - var cmd serviceaccounts.UpdateServiceAccountForm + cmd := serviceaccounts.UpdateServiceAccountForm{} if err := web.Bind(c.Req, &cmd); err != nil { return response.Error(http.StatusBadRequest, "Bad request data", err) } diff --git a/pkg/services/serviceaccounts/api/api_test.go b/pkg/services/serviceaccounts/api/api_test.go index e82ef65faa3..cd4a3b42523 100644 --- a/pkg/services/serviceaccounts/api/api_test.go +++ b/pkg/services/serviceaccounts/api/api_test.go @@ -74,7 +74,7 @@ func TestServiceAccountsAPI_CreateServiceAccount(t *testing.T) { { desc: "not ok - duplicate name", body: map[string]interface{}{"name": "New SA"}, - wantError: "service account name already in use", + wantError: "service account already exists", acmock: tests.SetupMockAccesscontrol( t, func(c context.Context, siu *models.SignedInUser, _ accesscontrol.Options) ([]accesscontrol.Permission, error) { diff --git a/pkg/services/serviceaccounts/api/token.go b/pkg/services/serviceaccounts/api/token.go index d1c24151cdd..d154983b044 100644 --- a/pkg/services/serviceaccounts/api/token.go +++ b/pkg/services/serviceaccounts/api/token.go @@ -11,6 +11,7 @@ import ( apikeygenprefix "github.com/grafana/grafana/pkg/components/apikeygenprefixed" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/serviceaccounts" + "github.com/grafana/grafana/pkg/services/serviceaccounts/database" "github.com/grafana/grafana/pkg/web" ) @@ -120,10 +121,10 @@ func (api *ServiceAccountsAPI) CreateToken(c *models.ReqContext) response.Respon cmd.Key = newKeyInfo.HashedKey if err := api.store.AddServiceAccountToken(c.Req.Context(), saID, &cmd); err != nil { - if errors.Is(err, models.ErrInvalidApiKeyExpiration) { + if errors.Is(err, database.ErrInvalidTokenExpiration) { return response.Error(http.StatusBadRequest, err.Error(), nil) } - if errors.Is(err, models.ErrDuplicateApiKey) { + if errors.Is(err, database.ErrDuplicateToken) { return response.Error(http.StatusConflict, err.Error(), nil) } return response.Error(http.StatusInternalServerError, "Failed to add service account token", err) diff --git a/pkg/services/serviceaccounts/database/database.go b/pkg/services/serviceaccounts/database/database.go index 019247a7472..7a29ca0ebb1 100644 --- a/pkg/services/serviceaccounts/database/database.go +++ b/pkg/services/serviceaccounts/database/database.go @@ -44,7 +44,7 @@ func (s *ServiceAccountsStoreImpl) CreateServiceAccount(ctx context.Context, org newuser, err := s.sqlStore.CreateUser(ctx, cmd) if err != nil { if errors.Is(err, models.ErrUserAlreadyExists) { - return nil, &ErrSAInvalidName{} + return nil, ErrServiceAccountAlreadyExists } return nil, fmt.Errorf("failed to create service account: %w", err) } @@ -439,16 +439,15 @@ func (s *ServiceAccountsStoreImpl) RevertApiKey(ctx context.Context, keyId int64 key := query.Result if key.ServiceAccountId == nil { - // TODO: better error message - return fmt.Errorf("API key is not linked to service account") + return fmt.Errorf("API key is not service account token") } tokens, err := s.ListTokens(ctx, key.OrgId, *key.ServiceAccountId) if err != nil { - return fmt.Errorf("cannot revert API key: %w", err) + return fmt.Errorf("cannot revert token: %w", err) } if len(tokens) > 1 { - return fmt.Errorf("cannot revert API key: service account contains more than one token") + return fmt.Errorf("cannot revert token: service account contains more than one token") } err = s.sqlStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { @@ -473,7 +472,7 @@ func (s *ServiceAccountsStoreImpl) RevertApiKey(ctx context.Context, keyId int64 }) if err != nil { - return fmt.Errorf("cannot revert to API key: %w", err) + return fmt.Errorf("cannot revert token to API key: %w", err) } return nil } diff --git a/pkg/services/serviceaccounts/database/errors.go b/pkg/services/serviceaccounts/database/errors.go index 8001b2bd889..d143e4f5a77 100644 --- a/pkg/services/serviceaccounts/database/errors.go +++ b/pkg/services/serviceaccounts/database/errors.go @@ -1,52 +1,12 @@ package database import ( - "fmt" - - "github.com/grafana/grafana/pkg/models" + "errors" ) -type ErrSAInvalidName struct { -} - -func (e *ErrSAInvalidName) Error() string { - return "service account name already in use" -} - -func (e *ErrSAInvalidName) Unwrap() error { - return models.ErrUserAlreadyExists -} - -type ErrMissingSAToken struct { -} - -func (e *ErrMissingSAToken) Error() string { - return "service account token not found" -} - -func (e *ErrMissingSAToken) Unwrap() error { - return models.ErrApiKeyNotFound -} - -type ErrInvalidExpirationSAToken struct { -} - -func (e *ErrInvalidExpirationSAToken) Error() string { - return "service account token not found" -} - -func (e *ErrInvalidExpirationSAToken) Unwrap() error { - return models.ErrInvalidApiKeyExpiration -} - -type ErrDuplicateSAToken struct { - name string -} - -func (e *ErrDuplicateSAToken) Error() string { - return fmt.Sprintf("service account token %s already exists in the organization", e.name) -} - -func (e *ErrDuplicateSAToken) Unwrap() error { - return models.ErrDuplicateApiKey -} +var ( + ErrServiceAccountAlreadyExists = errors.New("service account already exists") + ErrServiceAccountTokenNotFound = errors.New("service account token not found") + ErrInvalidTokenExpiration = errors.New("invalid SecondsToLive value") + ErrDuplicateToken = errors.New("service account token with given name already exists in the organization") +) diff --git a/pkg/services/serviceaccounts/database/token_store.go b/pkg/services/serviceaccounts/database/token_store.go index ce4e860d34f..e777f16078c 100644 --- a/pkg/services/serviceaccounts/database/token_store.go +++ b/pkg/services/serviceaccounts/database/token_store.go @@ -35,7 +35,7 @@ func (s *ServiceAccountsStoreImpl) AddServiceAccountToken(ctx context.Context, s key := models.ApiKey{OrgId: cmd.OrgId, Name: cmd.Name} exists, _ := sess.Get(&key) if exists { - return &ErrDuplicateSAToken{cmd.Name} + return ErrDuplicateToken } updated := time.Now() @@ -44,7 +44,7 @@ func (s *ServiceAccountsStoreImpl) AddServiceAccountToken(ctx context.Context, s v := updated.Add(time.Second * time.Duration(cmd.SecondsToLive)).Unix() expires = &v } else if cmd.SecondsToLive < 0 { - return &ErrInvalidExpirationSAToken{} + return ErrInvalidTokenExpiration } token := models.ApiKey{ @@ -74,13 +74,12 @@ func (s *ServiceAccountsStoreImpl) DeleteServiceAccountToken(ctx context.Context if err != nil { return err } - n, err := result.RowsAffected() - if err != nil { - return err - } else if n == 0 { - return &ErrMissingSAToken{} + affected, err := result.RowsAffected() + if affected == 0 { + return ErrServiceAccountTokenNotFound } - return nil + + return err }) } diff --git a/pkg/services/serviceaccounts/models.go b/pkg/services/serviceaccounts/models.go index 5acb745df6e..884e69a7910 100644 --- a/pkg/services/serviceaccounts/models.go +++ b/pkg/services/serviceaccounts/models.go @@ -23,6 +23,10 @@ type ServiceAccount struct { Id int64 } +type CreateServiceAccountForm struct { + Name string `json:"name" binding:"Required"` +} + type UpdateServiceAccountForm struct { Name *string `json:"name"` Role *models.RoleType `json:"role"`