[v11.3.x] Service Accounts: Run service account creation in transaction (#94803)

Service Accounts: Run service account creation in transaction (#94744)

* run service account creation DB queries in transaction

* extract the signed in user from the context

* undo unneeded change

* don't error out if a user is not found

* Update pkg/services/serviceaccounts/manager/service.go

Co-authored-by: Misi <mgyongyosi@users.noreply.github.com>

* Update pkg/services/serviceaccounts/manager/service.go

Co-authored-by: Karl Persson <kalle.persson@grafana.com>

---------

Co-authored-by: Misi <mgyongyosi@users.noreply.github.com>
Co-authored-by: Karl Persson <kalle.persson@grafana.com>
(cherry picked from commit ca1fd028a2)

Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>

pkg/services/serviceaccounts/api/api.go

@@ -101,17 +101,6 @@ func (api *ServiceAccountsAPI) CreateServiceAccount(c *contextmodel.ReqContext)
 
 	if api.cfg.RBAC.PermissionsOnCreation("service-account") {
 		if c.SignedInUser.IsIdentityType(claims.TypeUser) {
-			userID, err := c.SignedInUser.GetInternalID()
-			if err != nil {
-				return response.Error(http.StatusInternalServerError, "Failed to parse user id", err)
-			}
-
-			if _, err := api.permissionService.SetUserPermission(c.Req.Context(),
-				c.SignedInUser.GetOrgID(), accesscontrol.User{ID: userID},
-				strconv.FormatInt(serviceAccount.Id, 10), "Admin"); err != nil {
-				return response.Error(http.StatusInternalServerError, "Failed to set permissions for service account creator", err)
-			}
-
 			// Clear permission cache for the user who's created the service account, so that new permissions are fetched for their next call
 			// Required for cases when caller wants to immediately interact with the newly created object
 			api.accesscontrolService.ClearUserPermissionCache(c.SignedInUser)

pkg/services/serviceaccounts/manager/service.go

@@ -4,8 +4,12 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strconv"
 	"time"
 
+	"github.com/grafana/authlib/claims"
+
+	"github.com/grafana/grafana/pkg/apimachinery/identity"
 	"github.com/grafana/grafana/pkg/infra/db"
 	"github.com/grafana/grafana/pkg/infra/kvstore"
 	"github.com/grafana/grafana/pkg/infra/log"
@@ -29,6 +33,8 @@ type ServiceAccountsService struct {
 	acService   accesscontrol.Service
 	permissions accesscontrol.ServiceAccountPermissionsService
 
+	cfg               *setting.Cfg
+	db                db.DB
 	store             store
 	log               log.Logger
 	backgroundLog     log.Logger
@@ -58,6 +64,8 @@ func ProvideServiceAccountsService(
 		orgService,
 	)
 	s := &ServiceAccountsService{
+		cfg:           cfg,
+		db:            store,
 		acService:     acService,
 		permissions:   permissions,
 		store:         serviceAccountsStore,
@@ -150,7 +158,39 @@ func (sa *ServiceAccountsService) CreateServiceAccount(ctx context.Context, orgI
 	if err := validOrgID(orgID); err != nil {
 		return nil, err
 	}
-	return sa.store.CreateServiceAccount(ctx, orgID, saForm)
+
+	var serviceAccount *serviceaccounts.ServiceAccountDTO
+	err := sa.db.InTransaction(ctx, func(ctx context.Context) error {
+		var err error
+		serviceAccount, err = sa.store.CreateServiceAccount(ctx, orgID, saForm)
+		if err != nil {
+			return err
+		}
+
+		user, err := identity.GetRequester(ctx)
+		if err == nil && sa.cfg.RBAC.PermissionsOnCreation("service-account") {
+			if user.IsIdentityType(claims.TypeUser) {
+				userID, err := user.GetInternalID()
+				if err != nil {
+					return err
+				}
+
+				if _, err := sa.permissions.SetUserPermission(ctx,
+					orgID, accesscontrol.User{ID: userID},
+					strconv.FormatInt(serviceAccount.Id, 10), "Admin"); err != nil {
+					return err
+				}
+			}
+		}
+
+		return nil
+	})
+
+	if err != nil {
+		return nil, err
+	}
+
+	return serviceAccount, nil
 }
 
 func (sa *ServiceAccountsService) RetrieveServiceAccount(ctx context.Context, orgID int64, serviceAccountID int64) (*serviceaccounts.ServiceAccountProfileDTO, error) {

pkg/services/serviceaccounts/manager/service_test.go

@@ -4,13 +4,15 @@ import (
 	"context"
 	"testing"
 
-	"github.com/grafana/grafana/pkg/services/accesscontrol/actest"
 	"github.com/stretchr/testify/require"
 
+	"github.com/grafana/grafana/pkg/infra/db"
 	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/services/accesscontrol/actest"
 	"github.com/grafana/grafana/pkg/services/apikey"
 	"github.com/grafana/grafana/pkg/services/org"
 	"github.com/grafana/grafana/pkg/services/serviceaccounts"
+	"github.com/grafana/grafana/pkg/tests/testsuite"
 )
 
 type FakeServiceAccountStore struct {
@@ -116,11 +118,22 @@ func (f *SecretsCheckerFake) CheckTokens(ctx context.Context) error {
 	return f.ExpectedError
 }
 
+func TestMain(m *testing.M) {
+	testsuite.Run(m)
+}
+
 func TestProvideServiceAccount_DeleteServiceAccount(t *testing.T) {
 	storeMock := newServiceAccountStoreFake()
 	acSvc := actest.FakeService{}
 	pSvc := &actest.FakePermissionsService{}
-	svc := ServiceAccountsService{acSvc, pSvc, storeMock, log.NewNopLogger(), log.NewNopLogger(), &SecretsCheckerFake{}, false, 0}
+	svc := ServiceAccountsService{
+		acService:         acSvc,
+		permissions:       pSvc,
+		store:             storeMock,
+		db:                db.InitTestDB(t),
+		log:               log.NewNopLogger(),
+		secretScanEnabled: false,
+	}
 	testOrgId := 1
 
 	t.Run("should create service account", func(t *testing.T) {

pkg/services/serviceaccounts/manager/stats_test.go

@@ -16,7 +16,15 @@ func Test_UsageStats(t *testing.T) {
 	acSvc := actest.FakeService{}
 	pSvc := actest.FakePermissionsService{}
 	storeMock := newServiceAccountStoreFake()
-	svc := ServiceAccountsService{acSvc, &pSvc, storeMock, log.NewNopLogger(), log.NewNopLogger(), &SecretsCheckerFake{}, true, 5}
+	svc := ServiceAccountsService{
+		acService:          acSvc,
+		permissions:        &pSvc,
+		store:              storeMock,
+		log:                log.NewNopLogger(),
+		secretScanService:  &SecretsCheckerFake{},
+		secretScanEnabled:  true,
+		secretScanInterval: 5,
+	}
 	err := svc.DeleteServiceAccount(context.Background(), 1, 1)
 	require.NoError(t, err)