From 25b30aeb6d4309fcbd3391c104a55346d57023a3 Mon Sep 17 00:00:00 2001 From: Gabriel MABILLE Date: Fri, 27 Oct 2023 14:27:06 +0200 Subject: [PATCH] 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 :weary: * 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 --------- Co-authored-by: Andres Martinez Gotor --- pkg/plugins/auth/models.go | 2 +- pkg/plugins/manager/fakes/fakes.go | 2 +- .../pluginsintegration/pipeline/steps.go | 3 +- .../serviceregistration.go | 28 ++- .../serviceaccounts/extsvcaccounts/models.go | 1 + .../serviceaccounts/extsvcaccounts/service.go | 12 +- .../extsvcaccounts/service_test.go | 217 ++++++++---------- pkg/services/serviceaccounts/models.go | 2 +- 8 files changed, 130 insertions(+), 137 deletions(-) diff --git a/pkg/plugins/auth/models.go b/pkg/plugins/auth/models.go index 8c0f902203a..add4b46d504 100644 --- a/pkg/plugins/auth/models.go +++ b/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) } diff --git a/pkg/plugins/manager/fakes/fakes.go b/pkg/plugins/manager/fakes/fakes.go index 908a3cfd506..60ac22c8d1c 100644 --- a/pkg/plugins/manager/fakes/fakes.go +++ b/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 } diff --git a/pkg/services/pluginsintegration/pipeline/steps.go b/pkg/services/pluginsintegration/pipeline/steps.go index 44bd572dba3..97354b9e06f 100644 --- a/pkg/services/pluginsintegration/pipeline/steps.go +++ b/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 diff --git a/pkg/services/pluginsintegration/serviceregistration/serviceregistration.go b/pkg/services/pluginsintegration/serviceregistration/serviceregistration.go index 396e43f2565..722891c111e 100644 --- a/pkg/services/pluginsintegration/serviceregistration/serviceregistration.go +++ b/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 } diff --git a/pkg/services/serviceaccounts/extsvcaccounts/models.go b/pkg/services/serviceaccounts/extsvcaccounts/models.go index 94154cddc0d..e10df1bd261 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/models.go +++ b/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 diff --git a/pkg/services/serviceaccounts/extsvcaccounts/service.go b/pkg/services/serviceaccounts/extsvcaccounts/service.go index 0562b5f74db..24d00430e3c 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/service.go +++ b/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{ diff --git a/pkg/services/serviceaccounts/extsvcaccounts/service_test.go b/pkg/services/serviceaccounts/extsvcaccounts/service_test.go index 4adfd06f3ae..636c0287ac7 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/service_test.go +++ b/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("DeleteServiceAccount", mock.Anything, mock.Anything, mock.Anything).Return(nil) - env.AcStore.On("DeleteExternalServiceRole", mock.Anything, mock.Anything).Return(nil) + env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, extSvcOrgID, sa.ExtSvcPrefix+extSvcSlug).Return(extSvcAccID, nil) + env.SaSvc.On("EnableServiceAccount", mock.Anything, extSvcOrgID, 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) }, 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) { - 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, + want: extSvcAccID, 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("DeleteServiceAccount", mock.Anything, mock.Anything, mock.Anything).Return(nil) - env.AcStore.On("DeleteExternalServiceRole", mock.Anything, mock.Anything).Return(nil) + env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, extSvcOrgID, sa.ExtSvcPrefix+extSvcSlug).Return(extSvcAccID, nil) + env.SaSvc.On("DeleteServiceAccount", mock.Anything, extSvcOrgID, extSvcAccID).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("DeleteServiceAccount", mock.Anything, mock.Anything, mock.Anything).Return(nil) - env.AcStore.On("DeleteExternalServiceRole", mock.Anything, mock.Anything).Return(nil) + env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug). + Return(extSvcAccID, 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("DeleteServiceAccount", mock.Anything, mock.Anything, mock.Anything).Return(nil) - env.AcStore.On("DeleteExternalServiceRole", mock.Anything, mock.Anything).Return(nil) + env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug). + Return(extSvcAccID, 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, - 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") }, 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, diff --git a/pkg/services/serviceaccounts/models.go b/pkg/services/serviceaccounts/models.go index 411b6f995b1..90b74ee6634 100644 --- a/pkg/services/serviceaccounts/models.go +++ b/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 }