diff --git a/pkg/plugins/auth/models.go b/pkg/plugins/auth/models.go index add4b46d504..cc5dc32b81a 100644 --- a/pkg/plugins/auth/models.go +++ b/pkg/plugins/auth/models.go @@ -13,5 +13,7 @@ type ExternalService struct { } type ExternalServiceRegistry interface { - RegisterExternalService(ctx context.Context, name string, pType plugindef.Type, svc *plugindef.ExternalServiceRegistration) (*ExternalService, error) + HasExternalService(ctx context.Context, pluginID string) bool + RegisterExternalService(ctx context.Context, pluginID string, pType plugindef.Type, svc *plugindef.ExternalServiceRegistration) (*ExternalService, error) + RemoveExternalService(ctx context.Context, pluginID string) error } diff --git a/pkg/plugins/manager/fakes/fakes.go b/pkg/plugins/manager/fakes/fakes.go index 60ac22c8d1c..a5ea3761285 100644 --- a/pkg/plugins/manager/fakes/fakes.go +++ b/pkg/plugins/manager/fakes/fakes.go @@ -437,10 +437,18 @@ type FakeAuthService struct { Result *auth.ExternalService } -func (f *FakeAuthService) RegisterExternalService(ctx context.Context, name string, pType plugindef.Type, svc *plugindef.ExternalServiceRegistration) (*auth.ExternalService, error) { +func (f *FakeAuthService) HasExternalService(ctx context.Context, pluginID string) bool { + return f.Result != nil +} + +func (f *FakeAuthService) RegisterExternalService(ctx context.Context, pluginID string, pType plugindef.Type, svc *plugindef.ExternalServiceRegistration) (*auth.ExternalService, error) { return f.Result, nil } +func (f *FakeAuthService) RemoveExternalService(ctx context.Context, pluginID string) error { + return nil +} + type FakeDiscoverer struct { DiscoverFunc func(ctx context.Context, src plugins.PluginSource) ([]*plugins.FoundBundle, error) } diff --git a/pkg/plugins/manager/installer.go b/pkg/plugins/manager/installer.go index 249b01b8a5b..a12454718f5 100644 --- a/pkg/plugins/manager/installer.go +++ b/pkg/plugins/manager/installer.go @@ -6,6 +6,7 @@ import ( "fmt" "github.com/grafana/grafana/pkg/plugins" + "github.com/grafana/grafana/pkg/plugins/auth" "github.com/grafana/grafana/pkg/plugins/config" "github.com/grafana/grafana/pkg/plugins/log" "github.com/grafana/grafana/pkg/plugins/manager/loader" @@ -24,16 +25,18 @@ type PluginInstaller struct { pluginRegistry registry.Service pluginLoader loader.Service log log.Logger + serviceRegistry auth.ExternalServiceRegistry } func ProvideInstaller(cfg *config.Cfg, pluginRegistry registry.Service, pluginLoader loader.Service, - pluginRepo repo.Service) *PluginInstaller { + pluginRepo repo.Service, serviceRegistry auth.ExternalServiceRegistry) *PluginInstaller { return New(pluginRegistry, pluginLoader, pluginRepo, - storage.FileSystem(log.NewPrettyLogger("installer.fs"), cfg.PluginsPath), storage.SimpleDirNameGeneratorFunc) + storage.FileSystem(log.NewPrettyLogger("installer.fs"), cfg.PluginsPath), storage.SimpleDirNameGeneratorFunc, serviceRegistry) } func New(pluginRegistry registry.Service, pluginLoader loader.Service, pluginRepo repo.Service, - pluginStorage storage.ZipExtractor, pluginStorageDirFunc storage.DirNameGeneratorFunc) *PluginInstaller { + pluginStorage storage.ZipExtractor, pluginStorageDirFunc storage.DirNameGeneratorFunc, + serviceRegistry auth.ExternalServiceRegistry) *PluginInstaller { return &PluginInstaller{ pluginLoader: pluginLoader, pluginRegistry: pluginRegistry, @@ -41,6 +44,7 @@ func New(pluginRegistry registry.Service, pluginLoader loader.Service, pluginRep pluginStorage: pluginStorage, pluginStorageDirFunc: pluginStorageDirFunc, log: log.New("plugin.installer"), + serviceRegistry: serviceRegistry, } } @@ -156,6 +160,9 @@ func (m *PluginInstaller) Remove(ctx context.Context, pluginID string) error { } } + if m.serviceRegistry.HasExternalService(ctx, pluginID) { + return m.serviceRegistry.RemoveExternalService(ctx, pluginID) + } return nil } diff --git a/pkg/plugins/manager/installer_test.go b/pkg/plugins/manager/installer_test.go index 7ed606980be..43465cd23ff 100644 --- a/pkg/plugins/manager/installer_test.go +++ b/pkg/plugins/manager/installer_test.go @@ -65,7 +65,7 @@ func TestPluginManager_Add_Remove(t *testing.T) { }, } - inst := New(fakes.NewFakePluginRegistry(), loader, pluginRepo, fs, storage.SimpleDirNameGeneratorFunc) + inst := New(fakes.NewFakePluginRegistry(), loader, pluginRepo, fs, storage.SimpleDirNameGeneratorFunc, &fakes.FakeAuthService{}) err := inst.Add(context.Background(), pluginID, v1, testCompatOpts()) require.NoError(t, err) @@ -171,7 +171,7 @@ func TestPluginManager_Add_Remove(t *testing.T) { }, } - pm := New(reg, &fakes.FakeLoader{}, &fakes.FakePluginRepo{}, &fakes.FakePluginStorage{}, storage.SimpleDirNameGeneratorFunc) + pm := New(reg, &fakes.FakeLoader{}, &fakes.FakePluginRepo{}, &fakes.FakePluginStorage{}, storage.SimpleDirNameGeneratorFunc, &fakes.FakeAuthService{}) err := pm.Add(context.Background(), p.ID, "3.2.0", testCompatOpts()) require.ErrorIs(t, err, plugins.ErrInstallCorePlugin) diff --git a/pkg/services/extsvcauth/models.go b/pkg/services/extsvcauth/models.go index fd69ec4efe5..6b1f3e278a7 100644 --- a/pkg/services/extsvcauth/models.go +++ b/pkg/services/extsvcauth/models.go @@ -17,6 +17,12 @@ const ( type AuthProvider string type ExternalServiceRegistry interface { + // HasExternalService returns whether an external service has been saved with that name. + HasExternalService(ctx context.Context, name string) bool + + // RemoveExternalService removes an external service and its associated resources from the database (ex: service account, token). + RemoveExternalService(ctx context.Context, name string) error + // SaveExternalService creates or updates an external service in the database. Based on the requested auth provider, // it generates client_id, secrets and any additional provider specificities (ex: rsa keys). It also ensures that the // associated service account has the correct permissions. diff --git a/pkg/services/extsvcauth/oauthserver/models.go b/pkg/services/extsvcauth/oauthserver/models.go index cae75077ffe..a5923802170 100644 --- a/pkg/services/extsvcauth/oauthserver/models.go +++ b/pkg/services/extsvcauth/oauthserver/models.go @@ -33,6 +33,8 @@ type OAuth2Server interface { // GetExternalService retrieves an external service from store by client_id. It populates the SelfPermissions and // SignedInUser from the associated service account. GetExternalService(ctx context.Context, id string) (*OAuthExternalService, error) + // RemoveExternalService removes an external service and its associated resources from the store. + RemoveExternalService(ctx context.Context, name string) error // HandleTokenRequest handles the client's OAuth2 query to obtain an access_token by presenting its authorization // grant (ex: client_credentials, jwtbearer). @@ -45,6 +47,7 @@ type OAuth2Server interface { //go:generate mockery --name Store --structname MockStore --outpkg oastest --filename store_mock.go --output ./oastest/ type Store interface { + DeleteExternalService(ctx context.Context, id string) error GetExternalService(ctx context.Context, id string) (*OAuthExternalService, error) GetExternalServiceByName(ctx context.Context, name string) (*OAuthExternalService, error) GetExternalServicePublicKey(ctx context.Context, clientID string) (*jose.JSONWebKey, error) diff --git a/pkg/services/extsvcauth/oauthserver/oasimpl/service.go b/pkg/services/extsvcauth/oauthserver/oasimpl/service.go index 250928ce9ac..2c807dbf257 100644 --- a/pkg/services/extsvcauth/oauthserver/oasimpl/service.go +++ b/pkg/services/extsvcauth/oauthserver/oasimpl/service.go @@ -183,6 +183,33 @@ func (s *OAuth2ServiceImpl) setClientUser(ctx context.Context, client *oauthserv return nil } +func (s *OAuth2ServiceImpl) RemoveExternalService(ctx context.Context, name string) error { + s.logger.Info("Remove external service", "service", name) + + client, err := s.sqlstore.GetExternalServiceByName(ctx, name) + if err != nil { + if errors.Is(err, oauthserver.ErrClientNotFound) { + s.logger.Debug("No external service linked to this name", "name", name) + return nil + } + s.logger.Error("Error fetching external service", "name", name, "error", err.Error()) + return err + } + + // Since we will delete the service, clear cache entry + s.cache.Delete(client.ClientID) + + // Delete the OAuth client info in store + if err := s.sqlstore.DeleteExternalService(ctx, client.ClientID); err != nil { + s.logger.Error("Error deleting external service", "name", name, "error", err.Error()) + return err + } + s.logger.Debug("Deleted external service", "name", name, "client_id", client.ClientID) + + // Remove the associated service account + return s.saService.RemoveExtSvcAccount(ctx, oauthserver.TmpOrgID, slugify.Slugify(name)) +} + // SaveExternalService creates or updates an external service in the database, it generates client_id and secrets and // it ensures that the associated service account has the correct permissions. // Database consistency is not guaranteed, consider changing this in the future. @@ -412,14 +439,13 @@ func (s *OAuth2ServiceImpl) handlePluginStateChanged(ctx context.Context, event s.logger.Info("Plugin state changed", "pluginId", event.PluginId, "enabled", event.Enabled) // Retrieve client associated to the plugin - slug := slugify.Slugify(event.PluginId) - client, err := s.sqlstore.GetExternalServiceByName(ctx, slug) + client, err := s.sqlstore.GetExternalServiceByName(ctx, event.PluginId) if err != nil { if errors.Is(err, oauthserver.ErrClientNotFound) { s.logger.Debug("No external service linked to this plugin", "pluginId", event.PluginId) return nil } - s.logger.Error("Error fetching service", "pluginId", event.PluginId, "error", err) + s.logger.Error("Error fetching service", "pluginId", event.PluginId, "error", err.Error()) return err } diff --git a/pkg/services/extsvcauth/oauthserver/oasimpl/service_test.go b/pkg/services/extsvcauth/oauthserver/oasimpl/service_test.go index 36568b228a1..42a77a24e13 100644 --- a/pkg/services/extsvcauth/oauthserver/oasimpl/service_test.go +++ b/pkg/services/extsvcauth/oauthserver/oasimpl/service_test.go @@ -408,6 +408,51 @@ func assertArrayInMap[K comparable, V string](t *testing.T, m1 map[K][]V, m2 map } } +func TestOAuth2ServiceImpl_RemoveExternalService(t *testing.T) { + const serviceName = "my-ext-service" + const clientID = "RANDOMID" + + dummyClient := &oauthserver.OAuthExternalService{ + Name: serviceName, + ClientID: clientID, + ServiceAccountID: 1, + } + + testCases := []struct { + name string + init func(*TestEnv) + }{ + { + name: "should do nothing on not found", + init: func(env *TestEnv) { + env.OAuthStore.On("GetExternalServiceByName", mock.Anything, serviceName).Return(nil, oauthserver.ErrClientNotFoundFn(serviceName)) + }, + }, + { + name: "should remove the external service and its associated service account", + init: func(env *TestEnv) { + env.OAuthStore.On("GetExternalServiceByName", mock.Anything, serviceName).Return(dummyClient, nil) + env.OAuthStore.On("DeleteExternalService", mock.Anything, clientID).Return(nil) + env.SAService.On("RemoveExtSvcAccount", mock.Anything, oauthserver.TmpOrgID, serviceName).Return(nil) + }, + }, + } + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + env := setupTestEnv(t) + if tt.init != nil { + tt.init(env) + } + + err := env.S.RemoveExternalService(context.Background(), serviceName) + require.NoError(t, err) + + env.OAuthStore.AssertExpectations(t) + env.SAService.AssertExpectations(t) + }) + } +} + func TestTestOAuth2ServiceImpl_handleKeyOptions(t *testing.T) { testCases := []struct { name string diff --git a/pkg/services/extsvcauth/oauthserver/oastest/fakes.go b/pkg/services/extsvcauth/oauthserver/oastest/fakes.go index 71aab89ab68..f492364b075 100644 --- a/pkg/services/extsvcauth/oauthserver/oastest/fakes.go +++ b/pkg/services/extsvcauth/oauthserver/oastest/fakes.go @@ -25,6 +25,10 @@ func (s *FakeService) GetExternalService(ctx context.Context, id string) (*oauth return s.ExpectedClient, s.ExpectedErr } +func (s *FakeService) RemoveExternalService(ctx context.Context, name string) error { + return s.ExpectedErr +} + func (s *FakeService) HandleTokenRequest(rw http.ResponseWriter, req *http.Request) {} func (s *FakeService) HandleIntrospectionRequest(rw http.ResponseWriter, req *http.Request) {} diff --git a/pkg/services/extsvcauth/oauthserver/oastest/store_mock.go b/pkg/services/extsvcauth/oauthserver/oastest/store_mock.go index 17a705e3e33..84c52f5372a 100644 --- a/pkg/services/extsvcauth/oauthserver/oastest/store_mock.go +++ b/pkg/services/extsvcauth/oauthserver/oastest/store_mock.go @@ -16,6 +16,20 @@ type MockStore struct { mock.Mock } +// DeleteExternalService provides a mock function with given fields: ctx, id +func (_m *MockStore) DeleteExternalService(ctx context.Context, id string) error { + ret := _m.Called(ctx, id) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string) error); ok { + r0 = rf(ctx, id) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // GetExternalService provides a mock function with given fields: ctx, id func (_m *MockStore) GetExternalService(ctx context.Context, id string) (*oauthserver.OAuthExternalService, error) { ret := _m.Called(ctx, id) diff --git a/pkg/services/extsvcauth/oauthserver/store/database.go b/pkg/services/extsvcauth/oauthserver/store/database.go index c12c29e25cb..3007b06afa6 100644 --- a/pkg/services/extsvcauth/oauthserver/store/database.go +++ b/pkg/services/extsvcauth/oauthserver/store/database.go @@ -126,6 +126,7 @@ func (s *store) GetExternalService(ctx context.Context, id string) (*oauthserver return err } if !found { + res = nil return oauthserver.ErrClientNotFoundFn(id) } @@ -223,3 +224,18 @@ func (s *store) UpdateExternalServiceGrantTypes(ctx context.Context, clientID, g return err }) } + +func (s *store) DeleteExternalService(ctx context.Context, id string) error { + if id == "" { + return oauthserver.ErrClientRequiredID + } + + return s.db.WithTransactionalDbSession(ctx, func(sess *db.Session) error { + if _, err := sess.Exec(`DELETE FROM oauth_client WHERE client_id = ?`, id); err != nil { + return err + } + + _, err := sess.Exec(`DELETE FROM oauth_impersonate_permission WHERE client_id = ?`, id) + return err + }) +} diff --git a/pkg/services/extsvcauth/oauthserver/store/database_test.go b/pkg/services/extsvcauth/oauthserver/store/database_test.go index 5748ac2d91c..cd8f7078260 100644 --- a/pkg/services/extsvcauth/oauthserver/store/database_test.go +++ b/pkg/services/extsvcauth/oauthserver/store/database_test.go @@ -2,6 +2,7 @@ package store import ( "context" + "errors" "testing" "github.com/go-jose/go-jose/v3" @@ -352,6 +353,88 @@ vuO8AU0bVoUmYMKhozkcCYHudkeS08hEjQIDAQAB } } +func TestStore_RemoveExternalService(t *testing.T) { + ctx := context.Background() + client1 := oauthserver.OAuthExternalService{ + Name: "my-external-service", + ClientID: "ClientID", + ImpersonatePermissions: []accesscontrol.Permission{}, + } + client2 := oauthserver.OAuthExternalService{ + Name: "my-external-service-2", + ClientID: "ClientID2", + ImpersonatePermissions: []accesscontrol.Permission{ + {Action: "dashboards:read", Scope: "folders:*"}, + {Action: "dashboards:read", Scope: "dashboards:*"}, + }, + } + + // Init store + s := &store{db: db.InitTestDB(t, db.InitTestDBOpt{FeatureFlags: []string{featuremgmt.FlagExternalServiceAuth}})} + require.NoError(t, s.SaveExternalService(context.Background(), &client1)) + require.NoError(t, s.SaveExternalService(context.Background(), &client2)) + + // Check presence of clients in store + getState := func(t *testing.T) map[string]bool { + client, err := s.GetExternalService(ctx, "ClientID") + if err != nil && !errors.Is(err, oauthserver.ErrClientNotFound) { + require.Fail(t, "error fetching client") + } + + client2, err := s.GetExternalService(ctx, "ClientID2") + if err != nil && !errors.Is(err, oauthserver.ErrClientNotFound) { + require.Fail(t, "error fetching client") + } + + return map[string]bool{ + "ClientID": client != nil, + "ClientID2": client2 != nil, + } + } + + tests := []struct { + name string + id string + state map[string]bool + wantErr bool + }{ + { + name: "no id provided", + state: map[string]bool{"ClientID": true, "ClientID2": true}, + wantErr: true, + }, + { + name: "not found", + id: "ClientID3", + state: map[string]bool{"ClientID": true, "ClientID2": true}, + wantErr: false, + }, + { + name: "remove client 2", + id: "ClientID2", + state: map[string]bool{"ClientID": true, "ClientID2": false}, + wantErr: false, + }, + { + name: "remove client 1", + id: "ClientID", + state: map[string]bool{"ClientID": false, "ClientID2": false}, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := s.DeleteExternalService(ctx, tt.id) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + require.EqualValues(t, tt.state, getState(t)) + }) + } +} + func compareClientToStored(t *testing.T, s *store, wanted *oauthserver.OAuthExternalService) { ctx := context.Background() stored, err := s.GetExternalService(ctx, wanted.ClientID) diff --git a/pkg/services/extsvcauth/registry/service.go b/pkg/services/extsvcauth/registry/service.go index efeb7c1771d..44d29827689 100644 --- a/pkg/services/extsvcauth/registry/service.go +++ b/pkg/services/extsvcauth/registry/service.go @@ -2,8 +2,10 @@ package registry import ( "context" + "sync" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/infra/slugify" "github.com/grafana/grafana/pkg/services/extsvcauth" "github.com/grafana/grafana/pkg/services/extsvcauth/oauthserver" "github.com/grafana/grafana/pkg/services/featuremgmt" @@ -17,14 +19,53 @@ type Registry struct { logger log.Logger oauthServer oauthserver.OAuth2Server saSvc *extsvcaccounts.ExtSvcAccountsService + + extSvcProviders map[string]extsvcauth.AuthProvider + lock sync.Mutex } func ProvideExtSvcRegistry(oauthServer oauthserver.OAuth2Server, saSvc *extsvcaccounts.ExtSvcAccountsService, features featuremgmt.FeatureToggles) *Registry { return &Registry{ - features: features, - logger: log.New("extsvcauth.registry"), - oauthServer: oauthServer, - saSvc: saSvc, + extSvcProviders: map[string]extsvcauth.AuthProvider{}, + features: features, + lock: sync.Mutex{}, + logger: log.New("extsvcauth.registry"), + oauthServer: oauthServer, + saSvc: saSvc, + } +} + +// HasExternalService returns whether an external service has been saved with that name. +func (r *Registry) HasExternalService(ctx context.Context, name string) bool { + _, ok := r.extSvcProviders[slugify.Slugify(name)] + return ok +} + +// RemoveExternalService removes an external service and its associated resources from the database (ex: service account, token). +func (r *Registry) RemoveExternalService(ctx context.Context, name string) error { + provider, ok := r.extSvcProviders[slugify.Slugify(name)] + if !ok { + r.logger.Debug("external service not found", "service", name) + return nil + } + + switch provider { + case extsvcauth.ServiceAccounts: + if !r.features.IsEnabled(featuremgmt.FlagExternalServiceAccounts) { + r.logger.Debug("Skipping External Service removal, flag disabled", "service", name, "flag", featuremgmt.FlagExternalServiceAccounts) + return nil + } + r.logger.Debug("Routing External Service removal to the External Service Account service", "service", name) + return r.saSvc.RemoveExternalService(ctx, name) + case extsvcauth.OAuth2Server: + if !r.features.IsEnabled(featuremgmt.FlagExternalServiceAuth) { + r.logger.Debug("Skipping External Service removal, flag disabled", "service", name, "flag", featuremgmt.FlagExternalServiceAccounts) + return nil + } + r.logger.Debug("Routing External Service removal to the OAuth2Server", "service", name) + return r.oauthServer.RemoveExternalService(ctx, name) + default: + return extsvcauth.ErrUnknownProvider.Errorf("unknow provider '%v'", provider) } } @@ -32,17 +73,22 @@ func ProvideExtSvcRegistry(oauthServer oauthserver.OAuth2Server, saSvc *extsvcac // it generates client_id, secrets and any additional provider specificities (ex: rsa keys). It also ensures that the // associated service account has the correct permissions. func (r *Registry) SaveExternalService(ctx context.Context, cmd *extsvcauth.ExternalServiceRegistration) (*extsvcauth.ExternalService, error) { + // Record provider in case of removal + r.lock.Lock() + r.extSvcProviders[slugify.Slugify(cmd.Name)] = cmd.AuthProvider + r.lock.Unlock() + switch cmd.AuthProvider { case extsvcauth.ServiceAccounts: if !r.features.IsEnabled(featuremgmt.FlagExternalServiceAccounts) { - r.logger.Warn("Skipping external service authentication, flag disabled", "service", cmd.Name, "flag", featuremgmt.FlagExternalServiceAccounts) + r.logger.Warn("Skipping External Service authentication, flag disabled", "service", cmd.Name, "flag", featuremgmt.FlagExternalServiceAccounts) return nil, nil } r.logger.Debug("Routing the External Service registration to the External Service Account service", "service", cmd.Name) return r.saSvc.SaveExternalService(ctx, cmd) case extsvcauth.OAuth2Server: if !r.features.IsEnabled(featuremgmt.FlagExternalServiceAuth) { - r.logger.Warn("Skipping external service authentication, flag disabled", "service", cmd.Name, "flag", featuremgmt.FlagExternalServiceAuth) + r.logger.Warn("Skipping External Service authentication, flag disabled", "service", cmd.Name, "flag", featuremgmt.FlagExternalServiceAuth) return nil, nil } r.logger.Debug("Routing the External Service registration to the OAuth2Server", "service", cmd.Name) diff --git a/pkg/services/pluginsintegration/pipeline/steps.go b/pkg/services/pluginsintegration/pipeline/steps.go index 97354b9e06f..e8996d136a9 100644 --- a/pkg/services/pluginsintegration/pipeline/steps.go +++ b/pkg/services/pluginsintegration/pipeline/steps.go @@ -39,8 +39,7 @@ func newExternalServiceRegistration(cfg *config.Cfg, serviceRegistry auth.Extern // Register registers the external service with the external service registry, if the feature is enabled. 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)) { + if p.ExternalServiceRegistration != nil { 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) diff --git a/pkg/services/pluginsintegration/serviceregistration/serviceregistration.go b/pkg/services/pluginsintegration/serviceregistration/serviceregistration.go index 662b8160fd6..6b030185120 100644 --- a/pkg/services/pluginsintegration/serviceregistration/serviceregistration.go +++ b/pkg/services/pluginsintegration/serviceregistration/serviceregistration.go @@ -5,32 +5,53 @@ import ( "errors" "github.com/grafana/grafana/pkg/plugins/auth" + "github.com/grafana/grafana/pkg/plugins/config" + "github.com/grafana/grafana/pkg/plugins/log" "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/featuremgmt" "github.com/grafana/grafana/pkg/services/pluginsintegration/pluginsettings" ) type Service struct { - reg extsvcauth.ExternalServiceRegistry - settingsSvc pluginsettings.Service + featureEnabled bool + log log.Logger + reg extsvcauth.ExternalServiceRegistry + settingsSvc pluginsettings.Service } -func ProvideService(reg extsvcauth.ExternalServiceRegistry, settingsSvc pluginsettings.Service) *Service { +func ProvideService(cfg *config.Cfg, reg extsvcauth.ExternalServiceRegistry, settingsSvc pluginsettings.Service) *Service { s := &Service{ - reg: reg, - settingsSvc: settingsSvc, + featureEnabled: cfg.Features.IsEnabled(featuremgmt.FlagExternalServiceAuth) || cfg.Features.IsEnabled(featuremgmt.FlagExternalServiceAccounts), + log: log.New("plugins.external.registration"), + reg: reg, + settingsSvc: settingsSvc, } return s } +func (s *Service) HasExternalService(ctx context.Context, pluginID string) bool { + if !s.featureEnabled { + s.log.Debug("Skipping HasExternalService call. The feature is behind a feature toggle and needs to be enabled.") + return false + } + + return s.reg.HasExternalService(ctx, pluginID) +} + // RegisterExternalService is a simplified wrapper around SaveExternalService for the plugin use case. -func (s *Service) RegisterExternalService(ctx context.Context, svcName string, pType plugindef.Type, svc *plugindef.ExternalServiceRegistration) (*auth.ExternalService, error) { +func (s *Service) RegisterExternalService(ctx context.Context, pluginID string, pType plugindef.Type, svc *plugindef.ExternalServiceRegistration) (*auth.ExternalService, error) { + if !s.featureEnabled { + s.log.Warn("Skipping External Service Registration. The feature is behind a feature toggle and needs to be enabled.") + return nil, nil + } + // 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}) + settings, err := s.settingsSvc.GetPluginSettingByPluginID(ctx, &pluginsettings.GetByPluginIDArgs{PluginID: pluginID}) if err != nil && !errors.Is(err, pluginsettings.ErrPluginSettingNotFound) { return nil, err } @@ -56,7 +77,7 @@ func (s *Service) RegisterExternalService(ctx context.Context, svcName string, p } registration := &extsvcauth.ExternalServiceRegistration{ - Name: svcName, + Name: pluginID, Impersonation: impersonation, Self: self, } @@ -98,3 +119,13 @@ func toAccessControlPermissions(ps []plugindef.Permission) []accesscontrol.Permi } return res } + +// RemoveExternalService removes the external service account associated to a plugin +func (s *Service) RemoveExternalService(ctx context.Context, pluginID string) error { + if !s.featureEnabled { + s.log.Debug("Skipping External Service Removal. The feature is behind a feature toggle and needs to be enabled.") + return nil + } + + return s.reg.RemoveExternalService(ctx, pluginID) +} diff --git a/pkg/services/serviceaccounts/extsvcaccounts/service.go b/pkg/services/serviceaccounts/extsvcaccounts/service.go index 24d00430e3c..4cd92c8dfb3 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/service.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/service.go @@ -126,6 +126,37 @@ func (esa *ExtSvcAccountsService) SaveExternalService(ctx context.Context, cmd * return &extsvcauth.ExternalService{Name: cmd.Name, ID: slug, Secret: token}, nil } +func (esa *ExtSvcAccountsService) RemoveExternalService(ctx context.Context, name string) error { + // This is double proofing, we should never reach here anyway the flags have already been checked. + if !esa.features.IsEnabled(featuremgmt.FlagExternalServiceAccounts) && !esa.features.IsEnabled(featuremgmt.FlagExternalServiceAuth) { + esa.logger.Warn("This feature is behind a feature flag, please set it if you want to save external services") + return nil + } + return esa.RemoveExtSvcAccount(ctx, extsvcauth.TmpOrgID, slugify.Slugify(name)) +} + +func (esa *ExtSvcAccountsService) RemoveExtSvcAccount(ctx context.Context, orgID int64, extSvcSlug string) error { + saID, errRetrieve := esa.saSvc.RetrieveServiceAccountIdByName(ctx, orgID, sa.ExtSvcPrefix+extSvcSlug) + if errRetrieve != nil && !errors.Is(errRetrieve, sa.ErrServiceAccountNotFound) { + return errRetrieve + } + + if saID <= 0 { + esa.logger.Debug("No external service account associated with this service", "service", extSvcSlug, "orgID", orgID) + return nil + } + + if err := esa.deleteExtSvcAccount(ctx, orgID, extSvcSlug, saID); err != nil { + esa.logger.Error("Error occurred while deleting service account", + "service", extSvcSlug, + "saID", saID, + "error", err.Error()) + return err + } + esa.logger.Info("Deleted external service account", "service", extSvcSlug, "orgID", orgID) + return nil +} + // ManageExtSvcAccount creates, updates or deletes the service account associated with an external service func (esa *ExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cmd *sa.ManageExtSvcAccountCmd) (int64, error) { // This is double proofing, we should never reach here anyway the flags have already been checked. @@ -153,7 +184,6 @@ func (esa *ExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cmd * "error", err.Error()) return 0, err } - esa.metrics.deletedCount.Inc() } esa.logger.Info("Skipping service account creation, no permission", "service", cmd.ExtSvcSlug, @@ -173,8 +203,6 @@ func (esa *ExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cmd * esa.logger.Error("Could not save service account", "service", cmd.ExtSvcSlug, "error", errSave.Error()) return 0, errSave } - esa.metrics.savedCount.Inc() - return saID, nil } @@ -212,6 +240,8 @@ func (esa *ExtSvcAccountsService) saveExtSvcAccount(ctx context.Context, cmd *sa return 0, err } + esa.metrics.savedCount.Inc() + return cmd.SaID, nil } @@ -224,7 +254,11 @@ func (esa *ExtSvcAccountsService) deleteExtSvcAccount(ctx context.Context, orgID if err := esa.acSvc.DeleteExternalServiceRole(ctx, slug); err != nil { return err } - return esa.DeleteExtSvcCredentials(ctx, orgID, slug) + if err := esa.DeleteExtSvcCredentials(ctx, orgID, slug); err != nil { + return err + } + esa.metrics.deletedCount.Inc() + return nil } // getExtSvcAccountToken get or create the token of an External Service diff --git a/pkg/services/serviceaccounts/extsvcaccounts/service_test.go b/pkg/services/serviceaccounts/extsvcaccounts/service_test.go index 636c0287ac7..33df87e283a 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/service_test.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/service_test.go @@ -380,3 +380,64 @@ func TestExtSvcAccountsService_SaveExternalService(t *testing.T) { }) } } + +func TestExtSvcAccountsService_RemoveExtSvcAccount(t *testing.T) { + extSvcSlug := "grafana-test-app" + tmpOrgID := int64(1) + extSvcAccID := int64(10) + tests := []struct { + name string + init func(env *TestEnv) + slug string + checks func(t *testing.T, env *TestEnv) + want *extsvcauth.ExternalService + }{ + { + name: "should not fail if the service account does not exist", + init: func(env *TestEnv) { + // No previous service account was attached to this slug + env.SaSvc.On("RetrieveServiceAccountIdByName", mock.Anything, tmpOrgID, sa.ExtSvcPrefix+extSvcSlug). + Return(int64(0), sa.ErrServiceAccountNotFound.Errorf("not found")) + }, + slug: extSvcSlug, + want: nil, + }, + { + name: "should remove service account", + init: func(env *TestEnv) { + // A previous service account was attached to this slug + 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") + }, + slug: extSvcSlug, + checks: func(t *testing.T, env *TestEnv) { + _, ok, _ := env.SkvStore.Get(context.Background(), tmpOrgID, extSvcSlug, kvStoreType) + require.False(t, ok, "secret should have been removed from store") + }, + want: nil, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + + env := setupTestEnv(t) + if tt.init != nil { + tt.init(env) + } + + err := env.S.RemoveExtSvcAccount(ctx, tmpOrgID, tt.slug) + require.NoError(t, err) + + if tt.checks != nil { + tt.checks(t, env) + } + env.SaSvc.AssertExpectations(t) + env.AcStore.AssertExpectations(t) + }) + } +} diff --git a/pkg/services/serviceaccounts/serviceaccounts.go b/pkg/services/serviceaccounts/serviceaccounts.go index caef539eaaa..046e6df2657 100644 --- a/pkg/services/serviceaccounts/serviceaccounts.go +++ b/pkg/services/serviceaccounts/serviceaccounts.go @@ -41,6 +41,8 @@ type ExtSvcAccountsService interface { EnableExtSvcAccount(ctx context.Context, cmd *EnableExtSvcAccountCmd) error // ManageExtSvcAccount creates, updates or deletes the service account associated with an external service ManageExtSvcAccount(ctx context.Context, cmd *ManageExtSvcAccountCmd) (int64, error) + // RemoveExtSvcAccount removes the external service account associated with an external service + RemoveExtSvcAccount(ctx context.Context, orgID int64, extSvcSlug string) error // RetrieveExtSvcAccount fetches an external service account by ID RetrieveExtSvcAccount(ctx context.Context, orgID, saID int64) (*ExtSvcAccount, error) } diff --git a/pkg/services/serviceaccounts/tests/extsvcaccmock.go b/pkg/services/serviceaccounts/tests/extsvcaccmock.go index 50b8d9b56d3..a81afeb50a8 100644 --- a/pkg/services/serviceaccounts/tests/extsvcaccmock.go +++ b/pkg/services/serviceaccounts/tests/extsvcaccmock.go @@ -52,6 +52,20 @@ func (_m *MockExtSvcAccountsService) ManageExtSvcAccount(ctx context.Context, cm return r0, r1 } +// RemoveExtSvcAccount provides a mock function with given fields: ctx, orgID, extSvcSlug +func (_m *MockExtSvcAccountsService) RemoveExtSvcAccount(ctx context.Context, orgID int64, extSvcSlug string) error { + ret := _m.Called(ctx, orgID, extSvcSlug) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, int64, string) error); ok { + r0 = rf(ctx, orgID, extSvcSlug) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // RetrieveExtSvcAccount provides a mock function with given fields: ctx, orgID, saID func (_m *MockExtSvcAccountsService) RetrieveExtSvcAccount(ctx context.Context, orgID int64, saID int64) (*serviceaccounts.ExtSvcAccount, error) { ret := _m.Called(ctx, orgID, saID)