Service accounts: Creation logic simplification (#63884)

* SA creation improvements

* PR feedback - put salt and rand back in and remove an unneeded line:

pkg/services/serviceaccounts/api/api.go

@@ -16,7 +16,6 @@ import (
 	contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model"
 	"github.com/grafana/grafana/pkg/services/org"
 	"github.com/grafana/grafana/pkg/services/serviceaccounts"
-	"github.com/grafana/grafana/pkg/services/serviceaccounts/database"
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/grafana/grafana/pkg/util"
 	"github.com/grafana/grafana/pkg/web"
@@ -133,7 +132,7 @@ func (api *ServiceAccountsAPI) CreateServiceAccount(c *contextmodel.ReqContext)
 
 	serviceAccount, err := api.service.CreateServiceAccount(c.Req.Context(), c.OrgID, &cmd)
 	switch {
-	case errors.Is(err, database.ErrServiceAccountAlreadyExists):
+	case errors.Is(err, serviceaccounts.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)

pkg/services/serviceaccounts/api/token.go

@@ -12,7 +12,6 @@ import (
 	"github.com/grafana/grafana/pkg/services/apikey"
 	contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model"
 	"github.com/grafana/grafana/pkg/services/serviceaccounts"
-	"github.com/grafana/grafana/pkg/services/serviceaccounts/database"
 	"github.com/grafana/grafana/pkg/web"
 )
 
@@ -177,10 +176,10 @@ func (api *ServiceAccountsAPI) CreateToken(c *contextmodel.ReqContext) response.
 
 	apiKey, err := api.service.AddServiceAccountToken(c.Req.Context(), saID, &cmd)
 	if err != nil {
-		if errors.Is(err, database.ErrInvalidTokenExpiration) {
+		if errors.Is(err, serviceaccounts.ErrInvalidTokenExpiration) {
 			return response.Error(http.StatusBadRequest, err.Error(), nil)
 		}
-		if errors.Is(err, database.ErrDuplicateToken) {
+		if errors.Is(err, serviceaccounts.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/errors.go

@@ -1,13 +0,0 @@
-package database
-
-import (
-	"errors"
-)
-
-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")
-	ErrServiceAccountAndTokenMismatch = errors.New("API token does not belong to the given service account")
-)

pkg/services/serviceaccounts/database/store.go

@@ -3,7 +3,6 @@ package database
 //nolint:goimports
 import (
 	"context"
-	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -55,40 +54,17 @@ func (s *ServiceAccountsStoreImpl) CreateServiceAccount(ctx context.Context, org
 	if saForm.Role != nil {
 		role = *saForm.Role
 	}
-	var newSA *user.User
-	createErr := s.sqlStore.WithTransactionalDbSession(ctx, func(sess *db.Session) (err error) {
-		var errUser error
-		newSA, errUser = s.userService.CreateServiceAccount(ctx, &user.CreateUserCommand{
-			Login:            generatedLogin,
-			OrgID:            orgId,
-			Name:             saForm.Name,
-			IsDisabled:       isDisabled,
-			IsServiceAccount: true,
-			SkipOrgSetup:     true,
-		})
-		if errUser != nil {
-			return errUser
-		}
 
-		errAddOrgUser := s.orgService.AddOrgUser(ctx, &org.AddOrgUserCommand{
+	newSA, err := s.userService.CreateServiceAccount(ctx, &user.CreateUserCommand{
-			Role:                      role,
+		Login:            generatedLogin,
-			OrgID:                     orgId,
+		OrgID:            orgId,
-			UserID:                    newSA.ID,
+		Name:             saForm.Name,
-			AllowAddingServiceAccount: true,
+		IsDisabled:       isDisabled,
-		})
+		IsServiceAccount: true,
-		if errAddOrgUser != nil {
+		DefaultOrgRole:   string(role),
-			return errAddOrgUser
-		}
-
-		return nil
 	})
-
+	if err != nil {
-	if createErr != nil {
+		return nil, fmt.Errorf("failed to create service account: %w", err)
-		if errors.Is(createErr, user.ErrUserAlreadyExists) {
-			return nil, ErrServiceAccountAlreadyExists
-		}
-
-		return nil, fmt.Errorf("failed to create service account: %w", createErr)
 	}
 
 	return &serviceaccounts.ServiceAccountDTO{
@@ -473,7 +449,7 @@ func (s *ServiceAccountsStoreImpl) RevertApiKey(ctx context.Context, saId int64,
 	}
 
 	if *key.ServiceAccountId != saId {
-		return ErrServiceAccountAndTokenMismatch
+		return serviceaccounts.ErrServiceAccountAndTokenMismatch
 	}
 
 	tokens, err := s.ListTokens(ctx, &serviceaccounts.GetSATokensQuery{

pkg/services/serviceaccounts/database/store_test.go

@@ -328,7 +328,7 @@ func TestStore_RevertApiKey(t *testing.T) {
 			desc:                        "should fail reverting to api key when the token is assigned to a different service account",
 			key:                         tests.TestApiKey{Name: "Test1", Role: org.RoleEditor, OrgId: 1},
 			forceMismatchServiceAccount: true,
-			expectedErr:                 ErrServiceAccountAndTokenMismatch,
+			expectedErr:                 serviceaccounts.ErrServiceAccountAndTokenMismatch,
 		},
 	}
 

pkg/services/serviceaccounts/database/token_store.go

@@ -58,9 +58,9 @@ func (s *ServiceAccountsStoreImpl) AddServiceAccountToken(ctx context.Context, s
 		if err := s.apiKeyService.AddAPIKey(ctx, addKeyCmd); err != nil {
 			switch {
 			case errors.Is(err, apikey.ErrDuplicate):
-				return ErrDuplicateToken
+				return serviceaccounts.ErrDuplicateToken
 			case errors.Is(err, apikey.ErrInvalidExpiration):
-				return ErrInvalidTokenExpiration
+				return serviceaccounts.ErrInvalidTokenExpiration
 			}
 
 			return err
@@ -81,7 +81,7 @@ func (s *ServiceAccountsStoreImpl) DeleteServiceAccountToken(ctx context.Context
 		}
 		affected, err := result.RowsAffected()
 		if affected == 0 {
-			return ErrServiceAccountTokenNotFound
+			return serviceaccounts.ErrServiceAccountTokenNotFound
 		}
 
 		return err
@@ -98,7 +98,7 @@ func (s *ServiceAccountsStoreImpl) RevokeServiceAccountToken(ctx context.Context
 		}
 		affected, err := result.RowsAffected()
 		if affected == 0 {
-			return ErrServiceAccountTokenNotFound
+			return serviceaccounts.ErrServiceAccountTokenNotFound
 		}
 
 		return err

pkg/services/serviceaccounts/models.go

@@ -6,6 +6,7 @@ import (
 	"github.com/grafana/grafana/pkg/services/accesscontrol"
 	"github.com/grafana/grafana/pkg/services/org"
 	"github.com/grafana/grafana/pkg/services/user"
+	"github.com/grafana/grafana/pkg/util/errutil"
 )
 
 var (
@@ -22,6 +23,14 @@ const (
 	ActionPermissionsWrite = "serviceaccounts.permissions:write"
 )
 
+var (
+	ErrServiceAccountAlreadyExists    = errutil.NewBase(errutil.StatusBadRequest, "serviceaccounts.ErrAlreadyExists", errutil.WithPublicMessage("service account already exists"))
+	ErrServiceAccountTokenNotFound    = errutil.NewBase(errutil.StatusNotFound, "serviceaccounts.ErrTokenNotFound", errutil.WithPublicMessage("service account token not found"))
+	ErrInvalidTokenExpiration         = errutil.NewBase(errutil.StatusValidationFailed, "serviceaccounts.ErrInvalidInput", errutil.WithPublicMessage("invalid SecondsToLive value"))
+	ErrDuplicateToken                 = errutil.NewBase(errutil.StatusBadRequest, "serviceaccounts.ErrTokenAlreadyExists", errutil.WithPublicMessage("service account token with given name already exists in the organization"))
+	ErrServiceAccountAndTokenMismatch = errutil.NewBase(errutil.StatusBadRequest, "serviceaccounts.ErrToeknMismatch", errutil.WithPublicMessage("API token does not belong to the given service account"))
+)
+
 type ServiceAccount struct {
 	Id int64
 }

pkg/services/thumbs/crawler_auth.go

@@ -9,7 +9,6 @@ import (
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/services/org"
 	"github.com/grafana/grafana/pkg/services/serviceaccounts"
-	"github.com/grafana/grafana/pkg/services/serviceaccounts/database"
 )
 
 type CrawlerAuthSetupService interface {
@@ -98,7 +97,7 @@ func (o *OSSCrawlerAuthSetupService) Setup(ctx context.Context) (CrawlerAuth, er
 		}
 
 		serviceAccount, err := o.serviceAccounts.CreateServiceAccount(ctx, orgId, &saForm)
-		accountAlreadyExists := errors.Is(err, database.ErrServiceAccountAlreadyExists)
+		accountAlreadyExists := errors.Is(err, serviceaccounts.ErrServiceAccountAlreadyExists)
 
 		if !accountAlreadyExists && err != nil {
 			o.log.Error("Failed to create the service account", "err", err, "accountName", serviceAccountNameOrg, "orgId", orgId)

pkg/services/user/userimpl/user.go

@@ -14,6 +14,7 @@ import (
 	ac "github.com/grafana/grafana/pkg/services/accesscontrol"
 	"github.com/grafana/grafana/pkg/services/org"
 	"github.com/grafana/grafana/pkg/services/quota"
+	"github.com/grafana/grafana/pkg/services/serviceaccounts"
 	"github.com/grafana/grafana/pkg/services/supportbundles"
 	"github.com/grafana/grafana/pkg/services/team"
 	"github.com/grafana/grafana/pkg/services/user"
@@ -467,26 +468,12 @@ func (s *Service) getOrgIDForNewUser(ctx context.Context, cmd *user.CreateUserCo
 	return orgID, err
 }
 
-// CreateServiceAccount is a copy of Create with a single difference; it will create the OrgUser service account.
+// CreateServiceAccount creates a service account in the user table and adds service account to an organisation in the org_user table
 func (s *Service) CreateServiceAccount(ctx context.Context, cmd *user.CreateUserCommand) (*user.User, error) {
-	cmdOrg := org.GetOrgIDForNewUserCommand{
+	cmd.Email = cmd.Login
-		Email:        cmd.Email,
+	err := s.store.LoginConflict(ctx, cmd.Login, cmd.Email, s.cfg.CaseInsensitiveLogin)
-		Login:        cmd.Login,
-		OrgID:        cmd.OrgID,
-		OrgName:      cmd.OrgName,
-		SkipOrgSetup: cmd.SkipOrgSetup,
-	}
-	orgID, err := s.orgService.GetIDForNewUser(ctx, cmdOrg)
 	if err != nil {
-		return nil, err
+		return nil, serviceaccounts.ErrServiceAccountAlreadyExists
-	}
-	if cmd.Email == "" {
-		cmd.Email = cmd.Login
-	}
-
-	err = s.store.LoginConflict(ctx, cmd.Login, cmd.Email, s.cfg.CaseInsensitiveLogin)
-	if err != nil {
-		return nil, err
 	}
 
 	// create user
@@ -494,15 +481,12 @@ func (s *Service) CreateServiceAccount(ctx context.Context, cmd *user.CreateUser
 		Email:            cmd.Email,
 		Name:             cmd.Name,
 		Login:            cmd.Login,
-		Company:          cmd.Company,
-		IsAdmin:          cmd.IsAdmin,
 		IsDisabled:       cmd.IsDisabled,
 		OrgID:            cmd.OrgID,
-		EmailVerified:    cmd.EmailVerified,
 		Created:          time.Now(),
 		Updated:          time.Now(),
 		LastSeenAt:       time.Now().AddDate(-10, 0, 0),
-		IsServiceAccount: cmd.IsServiceAccount,
+		IsServiceAccount: true,
 	}
 
 	salt, err := util.GetRandomString(10)
@@ -516,40 +500,22 @@ func (s *Service) CreateServiceAccount(ctx context.Context, cmd *user.CreateUser
 	}
 	usr.Rands = rands
 
-	if len(cmd.Password) > 0 {
-		encodedPassword, err := util.EncodePassword(cmd.Password, usr.Salt)
-		if err != nil {
-			return nil, err
-		}
-		usr.Password = encodedPassword
-	}
-
 	_, err = s.store.Insert(ctx, usr)
 	if err != nil {
 		return nil, err
 	}
 
 	// create org user link
-	if !cmd.SkipOrgSetup {
+	orgCmd := &org.AddOrgUserCommand{
-		orgCmd := &org.AddOrgUserCommand{
+		OrgID:                     cmd.OrgID,
-			OrgID:                     orgID,
+		UserID:                    usr.ID,
-			UserID:                    usr.ID,
+		Role:                      org.RoleType(cmd.DefaultOrgRole),
-			Role:                      org.RoleAdmin,
+		AllowAddingServiceAccount: true,
-			AllowAddingServiceAccount: true,
-		}
-
-		if s.cfg.AutoAssignOrg && !usr.IsAdmin {
-			if len(cmd.DefaultOrgRole) > 0 {
-				orgCmd.Role = org.RoleType(cmd.DefaultOrgRole)
-			} else {
-				orgCmd.Role = org.RoleType(s.cfg.AutoAssignOrgRole)
-			}
-		}
-
-		if err = s.orgService.AddOrgUser(ctx, orgCmd); err != nil {
-			return nil, err
-		}
 	}
+	if err = s.orgService.AddOrgUser(ctx, orgCmd); err != nil {
+		return nil, err
+	}
+
 	return usr, nil
 }