From c16f502ec5fddc92fcc43ed0c7c8be2c3900712a Mon Sep 17 00:00:00 2001 From: Ieva Date: Mon, 3 Jun 2024 16:19:53 +0300 Subject: [PATCH] Access Control: Clean up permissions for deprovisioned data sources (#88483) * make sure that DS permissions get correctly cleaned up when a DS is deleted through provisioning * don't attempt to delete a DS if it's not found * fixes for tests * fix ds tests * rename DS service used by DS provisioner to BaseDataSourceService to avoid confusions with the full DS service --- .../accesscontrol/mock/service_mock.go | 2 +- .../datasources/service/datasource_test.go | 65 +++++++++++++++++++ pkg/services/datasources/service/store.go | 7 -- .../datasources/service/store_test.go | 41 ------------ .../datasources/config_reader_test.go | 4 +- .../provisioning/datasources/datasources.go | 36 +++++----- pkg/services/provisioning/provisioning.go | 4 +- 7 files changed, 90 insertions(+), 69 deletions(-) diff --git a/pkg/services/accesscontrol/mock/service_mock.go b/pkg/services/accesscontrol/mock/service_mock.go index 618b0cf14ed..6232e7df8a5 100644 --- a/pkg/services/accesscontrol/mock/service_mock.go +++ b/pkg/services/accesscontrol/mock/service_mock.go @@ -46,7 +46,7 @@ func (m *MockPermissionsService) SetPermissions(ctx context.Context, orgID int64 func (m *MockPermissionsService) DeleteResourcePermissions(ctx context.Context, orgID int64, resourceID string) error { mockedArgs := m.Called(ctx, orgID, resourceID) - return mockedArgs.Error(1) + return mockedArgs.Error(0) } func (m *MockPermissionsService) MapActions(permission accesscontrol.ResourcePermission) string { diff --git a/pkg/services/datasources/service/datasource_test.go b/pkg/services/datasources/service/datasource_test.go index f86e28d6291..0fa7457351b 100644 --- a/pkg/services/datasources/service/datasource_test.go +++ b/pkg/services/datasources/service/datasource_test.go @@ -542,6 +542,71 @@ func TestService_UpdateDataSource(t *testing.T) { }) } +func TestService_DeleteDataSource(t *testing.T) { + t.Run("should not return an error if data source doesn't exist", func(t *testing.T) { + sqlStore := db.InitTestDB(t) + secretsService := secretsmng.SetupTestService(t, fakes.NewFakeSecretsStore()) + secretsStore := secretskvs.NewSQLSecretsKVStore(sqlStore, secretsService, log.New("test.logger")) + quotaService := quotatest.New(false, nil) + permissionSvc := acmock.NewMockedPermissionsService() + permissionSvc.On("DeleteResourcePermissions", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + + dsService, err := ProvideService(sqlStore, secretsService, secretsStore, &setting.Cfg{}, featuremgmt.WithFeatures(), acmock.New(), permissionSvc, quotaService, &pluginstore.FakePluginStore{}, &pluginfakes.FakePluginClient{}, nil) + require.NoError(t, err) + + cmd := &datasources.DeleteDataSourceCommand{ + UID: uuid.New().String(), + ID: 1, + OrgID: 1, + } + + err = dsService.DeleteDataSource(context.Background(), cmd) + require.NoError(t, err) + }) + + t.Run("should successfully delete a data source that exists", func(t *testing.T) { + sqlStore := db.InitTestDB(t) + secretsService := secretsmng.SetupTestService(t, fakes.NewFakeSecretsStore()) + secretsStore := secretskvs.NewSQLSecretsKVStore(sqlStore, secretsService, log.New("test.logger")) + quotaService := quotatest.New(false, nil) + + permissionSvc := acmock.NewMockedPermissionsService() + permissionSvc.On("SetPermissions", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]accesscontrol.ResourcePermission{}, nil).Once() + permissionSvc.On("DeleteResourcePermissions", mock.Anything, mock.Anything, mock.Anything).Return(nil).Once() + + dsService, err := ProvideService(sqlStore, secretsService, secretsStore, &setting.Cfg{}, featuremgmt.WithFeatures(), acmock.New(), permissionSvc, quotaService, &pluginstore.FakePluginStore{}, &pluginfakes.FakePluginClient{}, nil) + require.NoError(t, err) + + // First add the datasource + ds, err := dsService.AddDataSource(context.Background(), &datasources.AddDataSourceCommand{ + OrgID: 1, + Name: "test", + Type: "test", + UserID: 0, + }) + require.NoError(t, err) + + cmd := &datasources.DeleteDataSourceCommand{ + ID: ds.ID, + UID: ds.UID, + OrgID: 1, + } + + err = dsService.DeleteDataSource(context.Background(), cmd) + require.NoError(t, err) + + // Data source doesn't exist anymore + ds, err = dsService.GetDataSource(context.Background(), &datasources.GetDataSourceQuery{ + OrgID: 1, + UID: ds.UID, + }) + require.Nil(t, ds) + require.ErrorIs(t, err, datasources.ErrDataSourceNotFound) + + permissionSvc.AssertExpectations(t) + }) +} + func TestService_NameScopeResolver(t *testing.T) { retriever := &dataSourceMockRetriever{[]*datasources.DataSource{ {Name: "test-datasource", UID: "1"}, diff --git a/pkg/services/datasources/service/store.go b/pkg/services/datasources/service/store.go index 1e9214e14f6..27bbc95935c 100644 --- a/pkg/services/datasources/service/store.go +++ b/pkg/services/datasources/service/store.go @@ -14,7 +14,6 @@ import ( "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/metrics" - ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/quota" @@ -162,12 +161,6 @@ func (ss *SqlStore) DeleteDataSource(ctx context.Context, cmd *datasources.Delet } cmd.DeletedDatasourcesCount, _ = result.RowsAffected() - - // Remove associated AccessControl permissions - if _, errDeletingPerms := sess.Exec("DELETE FROM permission WHERE scope=?", - ac.Scope(datasources.ScopeProvider.GetResourceScope(ds.UID))); errDeletingPerms != nil { - return errDeletingPerms - } } if cmd.UpdateSecretFn != nil { diff --git a/pkg/services/datasources/service/store_test.go b/pkg/services/datasources/service/store_test.go index d93bd0e18d0..e0cd2e06dc2 100644 --- a/pkg/services/datasources/service/store_test.go +++ b/pkg/services/datasources/service/store_test.go @@ -12,7 +12,6 @@ import ( "github.com/grafana/grafana/pkg/events" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" - ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/featuremgmt" ) @@ -350,46 +349,6 @@ func TestIntegrationDataAccess(t *testing.T) { require.Equal(t, 0, len(dataSources)) }) - t.Run("DeleteDataSourceAccessControlPermissions", func(t *testing.T) { - store := db.InitTestDB(t) - ds := initDatasource(store) - ss := SqlStore{db: store} - - // Init associated permission - errAddPermissions := store.WithTransactionalDbSession(context.TODO(), func(sess *db.Session) error { - _, err := sess.Table("permission").Insert(ac.Permission{ - RoleID: 1, - Action: "datasources:read", - Scope: datasources.ScopeProvider.GetResourceScope(ds.UID), - Updated: time.Now(), - Created: time.Now(), - }) - return err - }) - require.NoError(t, errAddPermissions) - query := datasources.GetDataSourcesQuery{OrgID: 10} - - errDeletingDS := ss.DeleteDataSource(context.Background(), - &datasources.DeleteDataSourceCommand{Name: ds.Name, OrgID: ds.OrgID}, - ) - require.NoError(t, errDeletingDS) - - // Check associated permission - permCount := int64(0) - errGetPermissions := store.WithTransactionalDbSession(context.TODO(), func(sess *db.Session) error { - var err error - permCount, err = sess.Table("permission").Count() - return err - }) - require.NoError(t, errGetPermissions) - require.Zero(t, permCount, "permissions associated to the data source should have been removed") - - dataSources, err := ss.GetDataSources(context.Background(), &query) - - require.NoError(t, err) - require.Equal(t, 0, len(dataSources)) - }) - t.Run("GetDataSources", func(t *testing.T) { t.Run("Number of data sources returned limited to 6 per organization", func(t *testing.T) { db := db.InitTestDB(t) diff --git a/pkg/services/provisioning/datasources/config_reader_test.go b/pkg/services/provisioning/datasources/config_reader_test.go index e35820da35d..e18f96639de 100644 --- a/pkg/services/provisioning/datasources/config_reader_test.go +++ b/pkg/services/provisioning/datasources/config_reader_test.go @@ -125,7 +125,7 @@ func TestDatasourceAsConfig(t *testing.T) { }) t.Run("Remove one datasource should have removed old datasource", func(t *testing.T) { - store := &spyStore{} + store := &spyStore{items: []*datasources.DataSource{{Name: "old-data-source", OrgID: 1, UID: "old-data-source"}}} orgFake := &orgtest.FakeOrgService{} correlationsStore := &mockCorrelationsStore{} dc := newDatasourceProvisioner(logger, store, correlationsStore, orgFake) @@ -142,7 +142,7 @@ func TestDatasourceAsConfig(t *testing.T) { }) t.Run("Two configured datasource and purge others", func(t *testing.T) { - store := &spyStore{items: []*datasources.DataSource{{Name: "old-graphite", OrgID: 1, ID: 1}, {Name: "old-graphite2", OrgID: 1, ID: 2}}} + store := &spyStore{items: []*datasources.DataSource{{Name: "old-graphite", OrgID: 1, ID: 1}, {Name: "old-graphite3", OrgID: 1, ID: 2}}} orgFake := &orgtest.FakeOrgService{} correlationsStore := &mockCorrelationsStore{} dc := newDatasourceProvisioner(logger, store, correlationsStore, orgFake) diff --git a/pkg/services/provisioning/datasources/datasources.go b/pkg/services/provisioning/datasources/datasources.go index e0468725def..40ab9e3246f 100644 --- a/pkg/services/provisioning/datasources/datasources.go +++ b/pkg/services/provisioning/datasources/datasources.go @@ -12,7 +12,7 @@ import ( jsoniter "github.com/json-iterator/go" ) -type Store interface { +type BaseDataSourceService interface { GetDataSource(ctx context.Context, query *datasources.GetDataSourceQuery) (*datasources.DataSource, error) GetPrunableProvisionedDataSources(ctx context.Context) ([]*datasources.DataSource, error) AddDataSource(ctx context.Context, cmd *datasources.AddDataSourceCommand) (*datasources.DataSource, error) @@ -35,8 +35,8 @@ var ( // Provision scans a directory for provisioning config files // and provisions the datasource in those files. -func Provision(ctx context.Context, configDirectory string, store Store, correlationsStore CorrelationsStore, orgService org.Service) error { - dc := newDatasourceProvisioner(log.New("provisioning.datasources"), store, correlationsStore, orgService) +func Provision(ctx context.Context, configDirectory string, dsService BaseDataSourceService, correlationsStore CorrelationsStore, orgService org.Service) error { + dc := newDatasourceProvisioner(log.New("provisioning.datasources"), dsService, correlationsStore, orgService) return dc.applyChanges(ctx, configDirectory) } @@ -45,15 +45,15 @@ func Provision(ctx context.Context, configDirectory string, store Store, correla type DatasourceProvisioner struct { log log.Logger cfgProvider *configReader - store Store + dsService BaseDataSourceService correlationsStore CorrelationsStore } -func newDatasourceProvisioner(log log.Logger, store Store, correlationsStore CorrelationsStore, orgService org.Service) DatasourceProvisioner { +func newDatasourceProvisioner(log log.Logger, dsService BaseDataSourceService, correlationsStore CorrelationsStore, orgService org.Service) DatasourceProvisioner { return DatasourceProvisioner{ log: log, cfgProvider: &configReader{log: log, orgService: orgService}, - store: store, + dsService: dsService, correlationsStore: correlationsStore, } } @@ -65,7 +65,7 @@ func (dc *DatasourceProvisioner) provisionDataSources(ctx context.Context, cfg * for _, ds := range cfg.Datasources { cmd := &datasources.GetDataSourceQuery{OrgID: ds.OrgID, Name: ds.Name} - dataSource, err := dc.store.GetDataSource(ctx, cmd) + dataSource, err := dc.dsService.GetDataSource(ctx, cmd) if err != nil && !errors.Is(err, datasources.ErrDataSourceNotFound) { return err } @@ -73,14 +73,14 @@ func (dc *DatasourceProvisioner) provisionDataSources(ctx context.Context, cfg * if errors.Is(err, datasources.ErrDataSourceNotFound) { insertCmd := createInsertCommand(ds) dc.log.Info("inserting datasource from configuration", "name", insertCmd.Name, "uid", insertCmd.UID) - _, err = dc.store.AddDataSource(ctx, insertCmd) + _, err = dc.dsService.AddDataSource(ctx, insertCmd) if err != nil { return err } } else { updateCmd := createUpdateCommand(ds, dataSource.ID) dc.log.Debug("updating datasource from configuration", "name", updateCmd.Name, "uid", updateCmd.UID) - if _, err := dc.store.UpdateDataSource(ctx, updateCmd); err != nil { + if _, err := dc.dsService.UpdateDataSource(ctx, updateCmd); err != nil { if errors.Is(err, datasources.ErrDataSourceUpdatingOldVersion) { dc.log.Debug("ignoring old version of datasource", "name", updateCmd.Name, "uid", updateCmd.UID) } else { @@ -96,7 +96,7 @@ func (dc *DatasourceProvisioner) provisionDataSources(ctx context.Context, cfg * func (dc *DatasourceProvisioner) provisionCorrelations(ctx context.Context, cfg *configs) error { for _, ds := range cfg.Datasources { cmd := &datasources.GetDataSourceQuery{OrgID: ds.OrgID, Name: ds.Name} - dataSource, err := dc.store.GetDataSource(ctx, cmd) + dataSource, err := dc.dsService.GetDataSource(ctx, cmd) if errors.Is(err, datasources.ErrDataSourceNotFound) { return err @@ -154,7 +154,7 @@ func (dc *DatasourceProvisioner) applyChanges(ctx context.Context, configPath st } } - prunableProvisionedDataSources, err := dc.store.GetPrunableProvisionedDataSources(ctx) + prunableProvisionedDataSources, err := dc.dsService.GetPrunableProvisionedDataSources(ctx) if err != nil { return err } @@ -238,17 +238,21 @@ func makeCreateCorrelationCommand(correlation map[string]any, SourceUID string, func (dc *DatasourceProvisioner) deleteDatasources(ctx context.Context, dsToDelete []*deleteDatasourceConfig, willExistAfterProvisioning map[DataSourceMapKey]bool) error { for _, ds := range dsToDelete { getDsQuery := &datasources.GetDataSourceQuery{Name: ds.Name, OrgID: ds.OrgID} - _, err := dc.store.GetDataSource(ctx, getDsQuery) + existingDs, err := dc.dsService.GetDataSource(ctx, getDsQuery) - if err != nil && !errors.Is(err, datasources.ErrDataSourceNotFound) { - return err + if err != nil { + if errors.Is(err, datasources.ErrDataSourceNotFound) { + continue + } else { + return err + } } // Skip publishing the event as the data source is not really deleted, it will be re-created during provisioning // This is to avoid cleaning up any resources related to the data source (e.g. correlations) skipPublish := willExistAfterProvisioning[DataSourceMapKey{Name: ds.Name, OrgId: ds.OrgID}] - cmd := &datasources.DeleteDataSourceCommand{OrgID: ds.OrgID, Name: ds.Name, SkipPublish: skipPublish} - if err := dc.store.DeleteDataSource(ctx, cmd); err != nil { + cmd := &datasources.DeleteDataSourceCommand{OrgID: ds.OrgID, Name: ds.Name, UID: existingDs.UID, SkipPublish: skipPublish} + if err := dc.dsService.DeleteDataSource(ctx, cmd); err != nil { return err } diff --git a/pkg/services/provisioning/provisioning.go b/pkg/services/provisioning/provisioning.go index a4d95f07205..8a63fa3bdea 100644 --- a/pkg/services/provisioning/provisioning.go +++ b/pkg/services/provisioning/provisioning.go @@ -118,7 +118,7 @@ func NewProvisioningServiceImpl() *ProvisioningServiceImpl { // Used for testing purposes func newProvisioningServiceImpl( newDashboardProvisioner dashboards.DashboardProvisionerFactory, - provisionDatasources func(context.Context, string, datasources.Store, datasources.CorrelationsStore, org.Service) error, + provisionDatasources func(context.Context, string, datasources.BaseDataSourceService, datasources.CorrelationsStore, org.Service) error, provisionPlugins func(context.Context, string, pluginstore.Store, pluginsettings.Service, org.Service) error, ) *ProvisioningServiceImpl { return &ProvisioningServiceImpl{ @@ -142,7 +142,7 @@ type ProvisioningServiceImpl struct { pollingCtxCancel context.CancelFunc newDashboardProvisioner dashboards.DashboardProvisionerFactory dashboardProvisioner dashboards.DashboardProvisioner - provisionDatasources func(context.Context, string, datasources.Store, datasources.CorrelationsStore, org.Service) error + provisionDatasources func(context.Context, string, datasources.BaseDataSourceService, datasources.CorrelationsStore, org.Service) error provisionPlugins func(context.Context, string, pluginstore.Store, pluginsettings.Service, org.Service) error provisionAlerting func(context.Context, prov_alerting.ProvisionerConfig) error mutex sync.Mutex