Plugin: Enable service account based on plugin settings on init (#77193)

* Disable plugin service account

* Revert extsvc injection

* handle plugin state changes

* Use isProxyEnabled

* Remove plugininteg changes

* Change update function to also work for mysql 😩

* Plugin: enable service account based on plugin settings on
initialization

* Remove misleading comment

* Fix tests

* test message

* Clean up tests

* Simplify tests

* Re-order imports

* Remove unecessary comment

* Enable datasource plugins by default

Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>

---------

Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>

pkg/plugins/auth/models.go

@@ -13,5 +13,5 @@ type ExternalService struct {
 }
 
 type ExternalServiceRegistry interface {
-	RegisterExternalService(ctx context.Context, name string, svc *plugindef.ExternalServiceRegistration) (*ExternalService, error)
+	RegisterExternalService(ctx context.Context, name string, pType plugindef.Type, svc *plugindef.ExternalServiceRegistration) (*ExternalService, error)
 }

pkg/plugins/manager/fakes/fakes.go

@@ -437,7 +437,7 @@ type FakeAuthService struct {
 	Result *auth.ExternalService
 }
 
-func (f *FakeAuthService) RegisterExternalService(ctx context.Context, name string, svc *plugindef.ExternalServiceRegistration) (*auth.ExternalService, error) {
+func (f *FakeAuthService) RegisterExternalService(ctx context.Context, name string, pType plugindef.Type, svc *plugindef.ExternalServiceRegistration) (*auth.ExternalService, error) {
 	return f.Result, nil
 }
 

pkg/services/pluginsintegration/pipeline/steps.go

@@ -12,6 +12,7 @@ import (
 	"github.com/grafana/grafana/pkg/plugins/manager/pipeline/initialization"
 	"github.com/grafana/grafana/pkg/plugins/manager/pipeline/validation"
 	"github.com/grafana/grafana/pkg/plugins/manager/signature"
+	"github.com/grafana/grafana/pkg/plugins/plugindef"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
 	"github.com/grafana/grafana/pkg/services/pluginsintegration/pluginerrs"
 )
@@ -40,7 +41,7 @@ func newExternalServiceRegistration(cfg *config.Cfg, serviceRegistry auth.Extern
 func (r *ExternalServiceRegistration) Register(ctx context.Context, p *plugins.Plugin) (*plugins.Plugin, error) {
 	if p.ExternalServiceRegistration != nil &&
 		(r.cfg.Features.IsEnabled(featuremgmt.FlagExternalServiceAuth) || r.cfg.Features.IsEnabled(featuremgmt.FlagExternalServiceAccounts)) {
-		s, err := r.externalServiceRegistry.RegisterExternalService(ctx, p.ID, p.ExternalServiceRegistration)
+		s, err := r.externalServiceRegistry.RegisterExternalService(ctx, p.ID, plugindef.Type(p.Type), p.ExternalServiceRegistration)
 		if err != nil {
 			r.log.Error("Could not register an external service. Initialization skipped", "pluginId", p.ID, "error", err)
 			return nil, err

pkg/services/pluginsintegration/serviceregistration/serviceregistration.go

@@ -2,26 +2,42 @@ package serviceregistration
 
 import (
 	"context"
+	"errors"
 
 	"github.com/grafana/grafana/pkg/plugins/auth"
 	"github.com/grafana/grafana/pkg/plugins/plugindef"
 	"github.com/grafana/grafana/pkg/services/accesscontrol"
 	"github.com/grafana/grafana/pkg/services/extsvcauth"
+	"github.com/grafana/grafana/pkg/services/pluginsintegration/pluginsettings"
 )
 
 type Service struct {
-	os extsvcauth.ExternalServiceRegistry
+	reg         extsvcauth.ExternalServiceRegistry
+	settingsSvc pluginsettings.Service
 }
 
-func ProvideService(os extsvcauth.ExternalServiceRegistry) *Service {
+func ProvideService(reg extsvcauth.ExternalServiceRegistry, settingsSvc pluginsettings.Service) *Service {
 	s := &Service{
-		os: os,
+		reg:         reg,
+		settingsSvc: settingsSvc,
 	}
 	return s
 }
 
 // RegisterExternalService is a simplified wrapper around SaveExternalService for the plugin use case.
-func (s *Service) RegisterExternalService(ctx context.Context, svcName string, svc *plugindef.ExternalServiceRegistration) (*auth.ExternalService, error) {
+func (s *Service) RegisterExternalService(ctx context.Context, svcName string, pType plugindef.Type, svc *plugindef.ExternalServiceRegistration) (*auth.ExternalService, error) {
+	// Datasource plugins can only be enabled
+	enabled := true
+	// App plugins can be disabled
+	if pType == plugindef.TypeApp {
+		settings, err := s.settingsSvc.GetPluginSettingByPluginID(ctx, &pluginsettings.GetByPluginIDArgs{PluginID: svcName})
+		if err != nil && !errors.Is(err, pluginsettings.ErrPluginSettingNotFound) {
+			return nil, err
+		}
+
+		enabled = (settings != nil) && settings.Enabled
+	}
+
 	impersonation := extsvcauth.ImpersonationCfg{}
 	if svc.Impersonation != nil {
 		impersonation.Permissions = toAccessControlPermissions(svc.Impersonation.Permissions)
@@ -38,9 +54,9 @@ func (s *Service) RegisterExternalService(ctx context.Context, svcName string, s
 	}
 
 	self := extsvcauth.SelfCfg{}
+	self.Enabled = enabled
 	if len(svc.Permissions) > 0 {
 		self.Permissions = toAccessControlPermissions(svc.Permissions)
-		self.Enabled = true
 	}
 
 	registration := &extsvcauth.ExternalServiceRegistration{
@@ -56,7 +72,7 @@ func (s *Service) RegisterExternalService(ctx context.Context, svcName string, s
 		registration.OAuthProviderCfg = &extsvcauth.OAuthProviderCfg{Key: &extsvcauth.KeyOption{Generate: true}}
 	}
 
-	extSvc, err := s.os.SaveExternalService(ctx, registration)
+	extSvc, err := s.reg.SaveExternalService(ctx, registration)
 	if err != nil || extSvc == nil {
 		return nil, err
 	}

pkg/services/serviceaccounts/extsvcaccounts/models.go

@@ -36,6 +36,7 @@ type SaveCredentialsCmd struct {
 }
 
 type saveCmd struct {
+	Enabled     bool
 	ExtSvcSlug  string
 	OrgID       int64
 	Permissions []ac.Permission

pkg/services/serviceaccounts/extsvcaccounts/service.go

@@ -144,7 +144,7 @@ func (esa *ExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cmd *
 		return 0, errRetrieve
 	}
 
-	if !cmd.Enabled || len(cmd.Permissions) == 0 {
+	if len(cmd.Permissions) == 0 {
 		if saID > 0 {
 			if err := esa.deleteExtSvcAccount(ctx, cmd.OrgID, cmd.ExtSvcSlug, saID); err != nil {
 				esa.logger.Error("Error occurred while deleting service account",
@@ -155,15 +155,15 @@ func (esa *ExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cmd *
 			}
 			esa.metrics.deletedCount.Inc()
 		}
-		esa.logger.Info("Skipping service account creation",
+		esa.logger.Info("Skipping service account creation, no permission",
 			"service", cmd.ExtSvcSlug,
-			"enabled", cmd.Enabled,
 			"permission count", len(cmd.Permissions),
 			"saID", saID)
 		return 0, nil
 	}
 
 	saID, errSave := esa.saveExtSvcAccount(ctx, &saveCmd{
+		Enabled:     cmd.Enabled,
 		ExtSvcSlug:  cmd.ExtSvcSlug,
 		OrgID:       cmd.OrgID,
 		Permissions: cmd.Permissions,
@@ -194,6 +194,12 @@ func (esa *ExtSvcAccountsService) saveExtSvcAccount(ctx context.Context, cmd *sa
 		cmd.SaID = sa.Id
 	}
 
+	// Enable or disable the service account
+	esa.logger.Debug("Set service account state", "service", cmd.ExtSvcSlug, "saID", cmd.SaID, "enabled", cmd.Enabled)
+	if err := esa.saSvc.EnableServiceAccount(ctx, cmd.OrgID, cmd.SaID, cmd.Enabled); err != nil {
+		return 0, err
+	}
+
 	// update the service account's permissions
 	esa.logger.Debug("Update role permissions", "service", cmd.ExtSvcSlug, "saID", cmd.SaID)
 	if err := esa.acSvc.SaveExternalServiceRole(ctx, ac.SaveExternalServiceRoleCommand{

pkg/services/serviceaccounts/extsvcaccounts/service_test.go

@@ -70,17 +70,23 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) {
 		name    string
 		init    func(env *TestEnv)
 		cmd     sa.ManageExtSvcAccountCmd
-		checks  func(t *testing.T, env *TestEnv)
 		want    int64
 		wantErr bool
 	}{
 		{
-			name: "should remove service account when disabled",
+			name: "should disable service account",
 			init: func(env *TestEnv) {
 				// A previous service account was attached to this slug
-				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, mock.Anything, mock.Anything).Return(extSvcAccID, nil)
+				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, extSvcOrgID, sa.ExtSvcPrefix+extSvcSlug).Return(extSvcAccID, nil)
-				env.SaSvc.On("DeleteServiceAccount", mock.Anything, mock.Anything, mock.Anything).Return(nil)
+				env.SaSvc.On("EnableServiceAccount", mock.Anything, extSvcOrgID, extSvcAccID, false).Return(nil)
-				env.AcStore.On("DeleteExternalServiceRole", mock.Anything, mock.Anything).Return(nil)
+				env.AcStore.On("SaveExternalServiceRole",
+					mock.Anything,
+					mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool {
+						return cmd.ServiceAccountID == extSvcAccID && cmd.ExternalServiceID == extSvcSlug &&
+							cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 &&
+							cmd.Permissions[0] == extSvcPerms[0]
+					})).
+					Return(nil)
 			},
 			cmd: sa.ManageExtSvcAccountCmd{
 				ExtSvcSlug:  extSvcSlug,
@@ -88,26 +94,16 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) {
 				OrgID:       extSvcOrgID,
 				Permissions: extSvcPerms,
 			},
-			checks: func(t *testing.T, env *TestEnv) {
+			want:    extSvcAccID,
-				env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything,
-					mock.MatchedBy(func(orgID int64) bool { return orgID == extSvcOrgID }),
-					mock.MatchedBy(func(slug string) bool { return slug == sa.ExtSvcPrefix+extSvcSlug }))
-				env.SaSvc.AssertCalled(t, "DeleteServiceAccount", mock.Anything,
-					mock.MatchedBy(func(orgID int64) bool { return orgID == extSvcOrgID }),
-					mock.MatchedBy(func(saID int64) bool { return saID == extSvcAccID }))
-				env.AcStore.AssertCalled(t, "DeleteExternalServiceRole", mock.Anything,
-					mock.MatchedBy(func(slug string) bool { return slug == extSvcSlug }))
-			},
-			want:    0,
 			wantErr: false,
 		},
 		{
 			name: "should remove service account when no permission",
 			init: func(env *TestEnv) {
 				// A previous service account was attached to this slug
-				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, mock.Anything, mock.Anything).Return(extSvcAccID, nil)
+				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, extSvcOrgID, sa.ExtSvcPrefix+extSvcSlug).Return(extSvcAccID, nil)
-				env.SaSvc.On("DeleteServiceAccount", mock.Anything, mock.Anything, mock.Anything).Return(nil)
+				env.SaSvc.On("DeleteServiceAccount", mock.Anything, extSvcOrgID, extSvcAccID).Return(nil)
-				env.AcStore.On("DeleteExternalServiceRole", mock.Anything, mock.Anything).Return(nil)
+				env.AcStore.On("DeleteExternalServiceRole", mock.Anything, extSvcSlug).Return(nil)
 			},
 			cmd: sa.ManageExtSvcAccountCmd{
 				ExtSvcSlug:  extSvcSlug,
@@ -115,16 +111,6 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) {
 				OrgID:       extSvcOrgID,
 				Permissions: []ac.Permission{},
 			},
-			checks: func(t *testing.T, env *TestEnv) {
-				env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything,
-					mock.MatchedBy(func(orgID int64) bool { return orgID == extSvcOrgID }),
-					mock.MatchedBy(func(slug string) bool { return slug == sa.ExtSvcPrefix+extSvcSlug }))
-				env.SaSvc.AssertCalled(t, "DeleteServiceAccount", mock.Anything,
-					mock.MatchedBy(func(orgID int64) bool { return orgID == extSvcOrgID }),
-					mock.MatchedBy(func(saID int64) bool { return saID == extSvcAccID }))
-				env.AcStore.AssertCalled(t, "DeleteExternalServiceRole", mock.Anything,
-					mock.MatchedBy(func(slug string) bool { return slug == extSvcSlug }))
-			},
 			want:    0,
 			wantErr: false,
 		},
@@ -132,11 +118,24 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) {
 			name: "should create new service account",
 			init: func(env *TestEnv) {
 				// No previous service account was attached to this slug
-				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, mock.Anything, mock.Anything).
+				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, extSvcOrgID, sa.ExtSvcPrefix+extSvcSlug).
 					Return(int64(0), sa.ErrServiceAccountNotFound.Errorf("mock"))
-				env.SaSvc.On("CreateServiceAccount", mock.Anything, mock.Anything, mock.Anything).
+				env.SaSvc.On("CreateServiceAccount",
+					mock.Anything,
+					extSvcOrgID,
+					mock.MatchedBy(func(cmd *sa.CreateServiceAccountForm) bool {
+						return cmd.Name == sa.ExtSvcPrefix+extSvcSlug && *cmd.Role == roletype.RoleNone
+					})).
 					Return(extSvcAccount, nil)
-				env.AcStore.On("SaveExternalServiceRole", mock.Anything, mock.Anything).Return(nil)
+				env.SaSvc.On("EnableServiceAccount", mock.Anything, extSvcOrgID, extSvcAccount.Id, true).Return(nil)
+				env.AcStore.On("SaveExternalServiceRole",
+					mock.Anything,
+					mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool {
+						return cmd.ServiceAccountID == extSvcAccount.Id && cmd.ExternalServiceID == extSvcSlug &&
+							cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 &&
+							cmd.Permissions[0] == extSvcPerms[0]
+					})).
+					Return(nil)
 			},
 			cmd: sa.ManageExtSvcAccountCmd{
 				ExtSvcSlug:  extSvcSlug,
@@ -144,23 +143,6 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) {
 				OrgID:       extSvcOrgID,
 				Permissions: extSvcPerms,
 			},
-			checks: func(t *testing.T, env *TestEnv) {
-				env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything,
-					mock.MatchedBy(func(orgID int64) bool { return orgID == extSvcOrgID }),
-					mock.MatchedBy(func(slug string) bool { return slug == sa.ExtSvcPrefix+extSvcSlug }))
-				env.SaSvc.AssertCalled(t, "CreateServiceAccount", mock.Anything,
-					mock.MatchedBy(func(orgID int64) bool { return orgID == extSvcOrgID }),
-					mock.MatchedBy(func(cmd *sa.CreateServiceAccountForm) bool {
-						return cmd.Name == sa.ExtSvcPrefix+extSvcSlug && *cmd.Role == roletype.RoleNone
-					}),
-				)
-				env.AcStore.AssertCalled(t, "SaveExternalServiceRole", mock.Anything,
-					mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool {
-						return cmd.ServiceAccountID == extSvcAccount.Id && cmd.ExternalServiceID == extSvcSlug &&
-							cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 &&
-							cmd.Permissions[0] == extSvcPerms[0]
-					}))
-			},
 			want:    extSvcAccID,
 			wantErr: false,
 		},
@@ -168,9 +150,17 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) {
 			name: "should update service account",
 			init: func(env *TestEnv) {
 				// A previous service account was attached to this slug
-				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, mock.Anything, mock.Anything).
+				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, extSvcOrgID, sa.ExtSvcPrefix+extSvcSlug).
 					Return(int64(11), nil)
-				env.AcStore.On("SaveExternalServiceRole", mock.Anything, mock.Anything).Return(nil)
+				env.SaSvc.On("EnableServiceAccount", mock.Anything, extSvcOrgID, int64(11), true).Return(nil)
+				env.AcStore.On("SaveExternalServiceRole",
+					mock.Anything,
+					mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool {
+						return cmd.ServiceAccountID == int64(11) && cmd.ExternalServiceID == extSvcSlug &&
+							cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 &&
+							cmd.Permissions[0] == extSvcPerms[0]
+					})).
+					Return(nil)
 			},
 			cmd: sa.ManageExtSvcAccountCmd{
 				ExtSvcSlug:  extSvcSlug,
@@ -178,17 +168,6 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) {
 				OrgID:       extSvcOrgID,
 				Permissions: extSvcPerms,
 			},
-			checks: func(t *testing.T, env *TestEnv) {
-				env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything,
-					mock.MatchedBy(func(orgID int64) bool { return orgID == extSvcOrgID }),
-					mock.MatchedBy(func(slug string) bool { return slug == sa.ExtSvcPrefix+extSvcSlug }))
-				env.AcStore.AssertCalled(t, "SaveExternalServiceRole", mock.Anything,
-					mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool {
-						return cmd.ServiceAccountID == int64(11) && cmd.ExternalServiceID == extSvcSlug &&
-							cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 &&
-							cmd.Permissions[0] == extSvcPerms[0]
-					}))
-			},
 			want:    11,
 			wantErr: false,
 		},
@@ -210,10 +189,6 @@ func TestExtSvcAccountsService_ManageExtSvcAccount(t *testing.T) {
 			}
 			require.NoError(t, err)
 
-			if tt.checks != nil {
-				tt.checks(t, env)
-			}
-
 			require.Equal(t, tt.want, got)
 		})
 	}
@@ -242,12 +217,20 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
 		wantErr bool
 	}{
 		{
-			name: "should remove service account when disabled",
+			name: "should disable service account",
 			init: func(env *TestEnv) {
 				// A previous service account was attached to this slug
-				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, mock.Anything, mock.Anything).Return(extSvcAccID, nil)
+				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug).
-				env.SaSvc.On("DeleteServiceAccount", mock.Anything, mock.Anything, mock.Anything).Return(nil)
+					Return(extSvcAccID, nil)
-				env.AcStore.On("DeleteExternalServiceRole", mock.Anything, mock.Anything).Return(nil)
+				env.SaSvc.On("EnableServiceAccount", mock.Anything, tmpOrgID, extSvcAccID, false).Return(nil)
+				env.AcStore.On("SaveExternalServiceRole",
+					mock.Anything,
+					mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool {
+						return cmd.ServiceAccountID == extSvcAccID && cmd.ExternalServiceID == extSvcSlug &&
+							cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 &&
+							cmd.Permissions[0] == extSvcPerms[0]
+					})).
+					Return(nil)
 				// A token was previously stored in the secret store
 				_ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, "ExtSvcSecretToken")
 			},
@@ -259,27 +242,26 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
 				},
 			},
 			checks: func(t *testing.T, env *TestEnv) {
-				env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything,
-					mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }),
-					mock.MatchedBy(func(slug string) bool { return slug == sa.ExtSvcPrefix+extSvcSlug }))
-				env.SaSvc.AssertCalled(t, "DeleteServiceAccount", mock.Anything,
-					mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }),
-					mock.MatchedBy(func(saID int64) bool { return saID == extSvcAccID }))
-				env.AcStore.AssertCalled(t, "DeleteExternalServiceRole", mock.Anything,
-					mock.MatchedBy(func(slug string) bool { return slug == extSvcSlug }))
 				_, ok, _ := env.SkvStore.Get(context.Background(), tmpOrgID, extSvcSlug, kvStoreType)
-				require.False(t, ok, "secret should have been removed from store")
+				require.True(t, ok, "secret should have been kept in store")
+			},
+			want: &extsvcauth.ExternalService{
+				Name:   extSvcSlug,
+				ID:     extSvcSlug,
+				Secret: "not empty",
 			},
-			want:    nil,
 			wantErr: false,
 		},
 		{
 			name: "should remove service account when no permission",
 			init: func(env *TestEnv) {
 				// A previous service account was attached to this slug
-				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, mock.Anything, mock.Anything).Return(extSvcAccID, nil)
+				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug).
-				env.SaSvc.On("DeleteServiceAccount", mock.Anything, mock.Anything, mock.Anything).Return(nil)
+					Return(extSvcAccID, nil)
-				env.AcStore.On("DeleteExternalServiceRole", mock.Anything, mock.Anything).Return(nil)
+				env.SaSvc.On("DeleteServiceAccount", mock.Anything, tmpOrgID, extSvcAccID).Return(nil)
+				env.AcStore.On("DeleteExternalServiceRole", mock.Anything, extSvcSlug).Return(nil)
+				// A token was previously stored in the secret store
+				_ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, "ExtSvcSecretToken")
 			},
 			cmd: extsvcauth.ExternalServiceRegistration{
 				Name: extSvcSlug,
@@ -289,14 +271,8 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
 				},
 			},
 			checks: func(t *testing.T, env *TestEnv) {
-				env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything,
+				_, ok, _ := env.SkvStore.Get(context.Background(), tmpOrgID, extSvcSlug, kvStoreType)
-					mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }),
+				require.False(t, ok, "secret should have been removed from store")
-					mock.MatchedBy(func(slug string) bool { return slug == sa.ExtSvcPrefix+extSvcSlug }))
-				env.SaSvc.AssertCalled(t, "DeleteServiceAccount", mock.Anything,
-					mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }),
-					mock.MatchedBy(func(saID int64) bool { return saID == extSvcAccID }))
-				env.AcStore.AssertCalled(t, "DeleteExternalServiceRole", mock.Anything,
-					mock.MatchedBy(func(slug string) bool { return slug == extSvcSlug }))
 			},
 			want:    nil,
 			wantErr: false,
@@ -305,13 +281,26 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
 			name: "should create new service account",
 			init: func(env *TestEnv) {
 				// No previous service account was attached to this slug
-				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, mock.Anything, mock.Anything).
+				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug).
 					Return(int64(0), sa.ErrServiceAccountNotFound.Errorf("mock"))
-				env.SaSvc.On("CreateServiceAccount", mock.Anything, mock.Anything, mock.Anything).
+				env.SaSvc.On("CreateServiceAccount",
+					mock.Anything,
+					tmpOrgID,
+					mock.MatchedBy(func(cmd *sa.CreateServiceAccountForm) bool {
+						return cmd.Name == sa.ExtSvcPrefix+extSvcSlug && *cmd.Role == roletype.RoleNone
+					})).
 					Return(extSvcAccount, nil)
+				env.SaSvc.On("EnableServiceAccount", mock.Anything, extsvcauth.TmpOrgID, extSvcAccID, true).Return(nil)
 				// Api Key was added without problem
 				env.SaSvc.On("AddServiceAccountToken", mock.Anything, mock.Anything, mock.Anything).Return(&apikey.APIKey{}, nil)
-				env.AcStore.On("SaveExternalServiceRole", mock.Anything, mock.Anything).Return(nil)
+				env.AcStore.On("SaveExternalServiceRole",
+					mock.Anything,
+					mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool {
+						return cmd.ServiceAccountID == extSvcAccount.Id && cmd.ExternalServiceID == extSvcSlug &&
+							cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 &&
+							cmd.Permissions[0] == extSvcPerms[0]
+					})).
+					Return(nil)
 			},
 			cmd: extsvcauth.ExternalServiceRegistration{
 				Name: extSvcSlug,
@@ -320,23 +309,6 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
 					Permissions: extSvcPerms,
 				},
 			},
-			checks: func(t *testing.T, env *TestEnv) {
-				env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything,
-					mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }),
-					mock.MatchedBy(func(slug string) bool { return slug == sa.ExtSvcPrefix+extSvcSlug }))
-				env.SaSvc.AssertCalled(t, "CreateServiceAccount", mock.Anything,
-					mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }),
-					mock.MatchedBy(func(cmd *sa.CreateServiceAccountForm) bool {
-						return cmd.Name == sa.ExtSvcPrefix+extSvcSlug && *cmd.Role == roletype.RoleNone
-					}),
-				)
-				env.AcStore.AssertCalled(t, "SaveExternalServiceRole", mock.Anything,
-					mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool {
-						return cmd.ServiceAccountID == extSvcAccount.Id && cmd.ExternalServiceID == extSvcSlug &&
-							cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 &&
-							cmd.Permissions[0] == extSvcPerms[0]
-					}))
-			},
 			want: &extsvcauth.ExternalService{
 				Name:   extSvcSlug,
 				ID:     extSvcSlug,
@@ -348,9 +320,17 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
 			name: "should update service account",
 			init: func(env *TestEnv) {
 				// A previous service account was attached to this slug
-				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, mock.Anything, mock.Anything).
+				env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug).
 					Return(int64(11), nil)
-				env.AcStore.On("SaveExternalServiceRole", mock.Anything, mock.Anything).Return(nil)
+				env.AcStore.On("SaveExternalServiceRole",
+					mock.Anything,
+					mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool {
+						return cmd.ServiceAccountID == int64(11) && cmd.ExternalServiceID == extSvcSlug &&
+							cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 &&
+							cmd.Permissions[0] == extSvcPerms[0]
+					})).
+					Return(nil)
+				env.SaSvc.On("EnableServiceAccount", mock.Anything, extsvcauth.TmpOrgID, int64(11), true).Return(nil)
 				// This time we don't add a token but rely on the secret store
 				_ = env.SkvStore.Set(context.Background(), tmpOrgID, extSvcSlug, kvStoreType, "ExtSvcSecretToken")
 			},
@@ -361,17 +341,6 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) {
 					Permissions: extSvcPerms,
 				},
 			},
-			checks: func(t *testing.T, env *TestEnv) {
-				env.SaSvc.AssertCalled(t, "RetrieveServiceAccountIdByName", mock.Anything,
-					mock.MatchedBy(func(orgID int64) bool { return orgID == tmpOrgID }),
-					mock.MatchedBy(func(slug string) bool { return slug == sa.ExtSvcPrefix+extSvcSlug }))
-				env.AcStore.AssertCalled(t, "SaveExternalServiceRole", mock.Anything,
-					mock.MatchedBy(func(cmd ac.SaveExternalServiceRoleCommand) bool {
-						return cmd.ServiceAccountID == int64(11) && cmd.ExternalServiceID == extSvcSlug &&
-							cmd.OrgID == int64(ac.GlobalOrgID) && len(cmd.Permissions) == 1 &&
-							cmd.Permissions[0] == extSvcPerms[0]
-					}))
-			},
 			want: &extsvcauth.ExternalService{
 				Name:   extSvcSlug,
 				ID:     extSvcSlug,

pkg/services/serviceaccounts/models.go

@@ -191,7 +191,7 @@ type ExtSvcAccount struct {
 
 type ManageExtSvcAccountCmd struct {
 	ExtSvcSlug  string
-	Enabled     bool // disabled: the service account and its permissions will be deleted
+	Enabled     bool
 	OrgID       int64
 	Permissions []accesscontrol.Permission
 }