ServiceAccounts: Don't create new orgs for service accounts (#51819)

* Org: use constants for status codes

* ServiceAccounts: Avoid creating new orgs for service accounts

* Document createUserBehavior

* Update pkg/services/sqlstore/org_users_test.go

* add doc string to flag

pkg/api/org.go

@@ -34,10 +34,10 @@ func (hs *HTTPServer) GetOrgByName(c *models.ReqContext) response.Response {
 	org, err := hs.SQLStore.GetOrgByName(web.Params(c.Req)[":name"])
 	if err != nil {
 		if errors.Is(err, models.ErrOrgNotFound) {
-			return response.Error(404, "Organization not found", err)
+			return response.Error(http.StatusNotFound, "Organization not found", err)
 		}
 
-		return response.Error(500, "Failed to get organization", err)
+		return response.Error(http.StatusInternalServerError, "Failed to get organization", err)
 	}
 	result := models.OrgDetailsDTO{
 		Id:   org.Id,
@@ -60,9 +60,9 @@ func (hs *HTTPServer) getOrgHelper(ctx context.Context, orgID int64) response.Re
 
 	if err := hs.SQLStore.GetOrgById(ctx, &query); err != nil {
 		if errors.Is(err, models.ErrOrgNotFound) {
-			return response.Error(404, "Organization not found", err)
+			return response.Error(http.StatusNotFound, "Organization not found", err)
 		}
-		return response.Error(500, "Failed to get organization", err)
+		return response.Error(http.StatusInternalServerError, "Failed to get organization", err)
 	}
 
 	org := query.Result
@@ -90,15 +90,15 @@ func (hs *HTTPServer) CreateOrg(c *models.ReqContext) response.Response {
 	}
 	acEnabled := !hs.AccessControl.IsDisabled()
 	if !acEnabled && !(setting.AllowUserOrgCreate || c.IsGrafanaAdmin) {
-		return response.Error(403, "Access denied", nil)
+		return response.Error(http.StatusForbidden, "Access denied", nil)
 	}
 
 	cmd.UserId = c.UserId
 	if err := hs.SQLStore.CreateOrg(c.Req.Context(), &cmd); err != nil {
 		if errors.Is(err, models.ErrOrgNameTaken) {
-			return response.Error(409, "Organization name taken", err)
+			return response.Error(http.StatusConflict, "Organization name taken", err)
 		}
-		return response.Error(500, "Failed to create organization", err)
+		return response.Error(http.StatusInternalServerError, "Failed to create organization", err)
 	}
 
 	metrics.MApiOrgCreate.Inc()
@@ -135,9 +135,9 @@ func (hs *HTTPServer) updateOrgHelper(ctx context.Context, form dtos.UpdateOrgFo
 	cmd := models.UpdateOrgCommand{Name: form.Name, OrgId: orgID}
 	if err := hs.SQLStore.UpdateOrg(ctx, &cmd); err != nil {
 		if errors.Is(err, models.ErrOrgNameTaken) {
-			return response.Error(400, "Organization name taken", err)
+			return response.Error(http.StatusBadRequest, "Organization name taken", err)
 		}
-		return response.Error(500, "Failed to update organization", err)
+		return response.Error(http.StatusInternalServerError, "Failed to update organization", err)
 	}
 
 	return response.Success("Organization updated")
@@ -179,7 +179,7 @@ func (hs *HTTPServer) updateOrgAddressHelper(ctx context.Context, form dtos.Upda
 	}
 
 	if err := hs.SQLStore.UpdateOrgAddress(ctx, &cmd); err != nil {
-		return response.Error(500, "Failed to update org address", err)
+		return response.Error(http.StatusInternalServerError, "Failed to update org address", err)
 	}
 
 	return response.Success("Address updated")
@@ -193,14 +193,14 @@ func (hs *HTTPServer) DeleteOrgByID(c *models.ReqContext) response.Response {
 	}
 	// before deleting an org, check if user does not belong to the current org
 	if c.OrgId == orgID {
-		return response.Error(400, "Can not delete org for current user", nil)
+		return response.Error(http.StatusBadRequest, "Can not delete org for current user", nil)
 	}
 
 	if err := hs.SQLStore.DeleteOrg(c.Req.Context(), &models.DeleteOrgCommand{Id: orgID}); err != nil {
 		if errors.Is(err, models.ErrOrgNotFound) {
-			return response.Error(404, "Failed to delete organization. ID not found", nil)
+			return response.Error(http.StatusNotFound, "Failed to delete organization. ID not found", nil)
 		}
-		return response.Error(500, "Failed to update organization", err)
+		return response.Error(http.StatusInternalServerError, "Failed to update organization", err)
 	}
 	return response.Success("Organization deleted")
 }
@@ -221,7 +221,7 @@ func (hs *HTTPServer) SearchOrgs(c *models.ReqContext) response.Response {
 	}
 
 	if err := hs.SQLStore.SearchOrgs(c.Req.Context(), &query); err != nil {
-		return response.Error(500, "Failed to search orgs", err)
+		return response.Error(http.StatusInternalServerError, "Failed to search orgs", err)
 	}
 
 	return response.JSON(http.StatusOK, query.Result)

pkg/models/org_user.go

@@ -102,6 +102,9 @@ type AddOrgUserCommand struct {
 
 	OrgId  int64 `json:"-"`
 	UserId int64 `json:"-"`
+
+	// internal use: avoid adding service accounts to orgs via user routes
+	AllowAddingServiceAccount bool `json:"-"`
 }
 
 type UpdateOrgUserCommand struct {

pkg/services/serviceaccounts/database/database.go

@@ -32,29 +32,50 @@ func NewServiceAccountsStore(store *sqlstore.SQLStore, kvStore kvstore.KVStore)
 }
 
 // CreateServiceAccount creates service account
-func (s *ServiceAccountsStoreImpl) CreateServiceAccount(ctx context.Context, orgId int64, name string) (saDTO *serviceaccounts.ServiceAccountDTO, err error) {
+func (s *ServiceAccountsStoreImpl) CreateServiceAccount(ctx context.Context, orgId int64, name string) (*serviceaccounts.ServiceAccountDTO, error) {
 	generatedLogin := "sa-" + strings.ToLower(name)
 	generatedLogin = strings.ReplaceAll(generatedLogin, " ", "-")
-	cmd := user.CreateUserCommand{
-		Login:            generatedLogin,
-		OrgID:            orgId,
-		Name:             name,
-		IsServiceAccount: true,
-	}
 
-	newuser, err := s.sqlStore.CreateUser(ctx, cmd)
-	if err != nil {
-		if errors.Is(err, models.ErrUserAlreadyExists) {
+	var newSA *user.User
+	createErr := s.sqlStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) (err error) {
+		var errUser error
+		newSA, errUser = s.sqlStore.CreateUser(ctx, user.CreateUserCommand{
+			Login:            generatedLogin,
+			OrgID:            orgId,
+			Name:             name,
+			IsServiceAccount: true,
+			SkipOrgSetup:     true,
+		})
+		if errUser != nil {
+			return errUser
+		}
+
+		errAddOrgUser := s.sqlStore.AddOrgUser(ctx, &models.AddOrgUserCommand{
+			Role:                      models.ROLE_VIEWER,
+			OrgId:                     orgId,
+			UserId:                    newSA.ID,
+			AllowAddingServiceAccount: true,
+		})
+		if errAddOrgUser != nil {
+			return errAddOrgUser
+		}
+
+		return nil
+	})
+
+	if createErr != nil {
+		if errors.Is(createErr, models.ErrUserAlreadyExists) {
 			return nil, ErrServiceAccountAlreadyExists
 		}
-		return nil, fmt.Errorf("failed to create service account: %w", err)
+
+		return nil, fmt.Errorf("failed to create service account: %w", createErr)
 	}
 
 	return &serviceaccounts.ServiceAccountDTO{
-		Id:     newuser.ID,
-		Name:   newuser.Name,
-		Login:  newuser.Login,
-		OrgId:  newuser.OrgID,
+		Id:     newSA.ID,
+		Name:   newSA.Name,
+		Login:  newSA.Login,
+		OrgId:  newSA.OrgID,
 		Tokens: 0,
 	}, nil
 }

pkg/services/serviceaccounts/database/database_test.go

@@ -13,12 +13,28 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestStore_CreateServiceAccount(t *testing.T) {
+// Service Account should not create an org on its own
+func TestStore_CreateServiceAccountOrgNonExistant(t *testing.T) {
 	_, store := setupTestDatabase(t)
 	t.Run("create service account", func(t *testing.T) {
 		serviceAccountName := "new Service Account"
 		serviceAccountOrgId := int64(1)
 
+		_, err := store.CreateServiceAccount(context.Background(), serviceAccountOrgId, serviceAccountName)
+		require.Error(t, err)
+	})
+}
+
+func TestStore_CreateServiceAccount(t *testing.T) {
+	_, store := setupTestDatabase(t)
+	orgQuery := &models.CreateOrgCommand{Name: sqlstore.MainOrgName}
+	err := store.sqlStore.CreateOrg(context.Background(), orgQuery)
+	require.NoError(t, err)
+
+	t.Run("create service account", func(t *testing.T) {
+		serviceAccountName := "new Service Account"
+		serviceAccountOrgId := orgQuery.Result.Id
+
 		saDTO, err := store.CreateServiceAccount(context.Background(), serviceAccountOrgId, serviceAccountName)
 		require.NoError(t, err)
 		assert.Equal(t, "sa-new-service-account", saDTO.Login)

pkg/services/sqlstore/org_users.go

@@ -16,7 +16,12 @@ func (ss *SQLStore) AddOrgUser(ctx context.Context, cmd *models.AddOrgUserComman
 	return ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error {
 		// check if user exists
 		var user user.User
-		if exists, err := sess.ID(cmd.UserId).Where(notServiceAccountFilter(ss)).Get(&user); err != nil {
+		session := sess.ID(cmd.UserId)
+		if !cmd.AllowAddingServiceAccount {
+			session = session.Where(notServiceAccountFilter(ss))
+		}
+
+		if exists, err := session.Get(&user); err != nil {
 			return err
 		} else if !exists {
 			return models.ErrUserNotFound

pkg/services/sqlstore/org_users_test.go

@@ -148,6 +148,61 @@ func TestSQLStore_SearchOrgUsers(t *testing.T) {
 	}
 }
 
+func TestSQLStore_AddOrgUser(t *testing.T) {
+	var orgID int64 = 1
+	store := InitTestDB(t)
+
+	// create org and admin
+	_, err := store.CreateUser(context.Background(), user.CreateUserCommand{
+		Login: "admin",
+		OrgID: orgID,
+	})
+	require.NoError(t, err)
+
+	// create a service account with no org
+	sa, err := store.CreateUser(context.Background(), user.CreateUserCommand{
+		Login:            "sa-no-org",
+		IsServiceAccount: true,
+		SkipOrgSetup:     true,
+	})
+
+	require.NoError(t, err)
+	require.Equal(t, int64(-1), sa.OrgID)
+
+	// assign the sa to the org but without the override. should fail
+	err = store.AddOrgUser(context.Background(), &models.AddOrgUserCommand{
+		Role:   "Viewer",
+		OrgId:  orgID,
+		UserId: sa.ID,
+	})
+	require.Error(t, err)
+
+	// assign the sa to the org with the override. should succeed
+	err = store.AddOrgUser(context.Background(), &models.AddOrgUserCommand{
+		Role:                      "Viewer",
+		OrgId:                     orgID,
+		UserId:                    sa.ID,
+		AllowAddingServiceAccount: true,
+	})
+
+	require.NoError(t, err)
+
+	// assert the org has been correctly set
+	saFound := new(user.User)
+	err = store.WithDbSession(context.Background(), func(sess *DBSession) error {
+		has, err := sess.ID(sa.ID).Get(saFound)
+		if err != nil {
+			return err
+		} else if !has {
+			return models.ErrUserNotFound
+		}
+		return nil
+	})
+
+	require.NoError(t, err)
+	require.Equal(t, saFound.OrgID, orgID)
+}
+
 func TestSQLStore_RemoveOrgUser(t *testing.T) {
 	store := InitTestDB(t)
 

pkg/services/sqlstore/user.go

@@ -68,6 +68,11 @@ func (ss *SQLStore) userCaseInsensitiveLoginConflict(ctx context.Context, sess *
 }
 
 // createUser creates a user in the database
+// if autoAssignOrg is enabled then args.OrgID will be used
+// to add to an existing Org with id=args.OrgID
+// if autoAssignOrg is disabled then args.OrgName will be used
+// to create a new Org with name=args.OrgName.
+// If a org already exists with that name, it will error
 func (ss *SQLStore) createUser(ctx context.Context, sess *DBSession, args user.CreateUserCommand) (user.User, error) {
 	var usr user.User
 	var orgID int64 = -1