Service accounts: refactor errors (#50917)

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 {
+	cmd := serviceaccounts.CreateServiceAccountForm{}
-		Name string `json:"name" binding:"Required"`
-	}
-	cmd := 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{}):
+	case errors.Is(err, database.ErrServiceAccountAlreadyExists):
-		return response.Error(http.StatusBadRequest, "Failed due to %s", err)
+		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)
 	}

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) {

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)

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 service account token")
-		return fmt.Errorf("API key is not linked to service account")
 	}
 
 	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
 }

pkg/services/serviceaccounts/database/errors.go

@@ -1,52 +1,12 @@
 package database
 
 import (
-	"fmt"
+	"errors"
-
-	"github.com/grafana/grafana/pkg/models"
 )
 
-type ErrSAInvalidName struct {
+var (
-}
+	ErrServiceAccountAlreadyExists = errors.New("service account already exists")
-
+	ErrServiceAccountTokenNotFound = errors.New("service account token not found")
-func (e *ErrSAInvalidName) Error() string {
+	ErrInvalidTokenExpiration      = errors.New("invalid SecondsToLive value")
-	return "service account name already in use"
+	ErrDuplicateToken              = errors.New("service account token with given name already exists in the organization")
-}
+)
-
-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
-}

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()
+		affected, err := result.RowsAffected()
-		if err != nil {
+		if affected == 0 {
-			return err
+			return ErrServiceAccountTokenNotFound
-		} else if n == 0 {
-			return &ErrMissingSAToken{}
 		}
-		return nil
+
+		return err
 	})
 }
 

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"`