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

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 {

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"},

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 {

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)

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)

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 {
+func Provision(ctx context.Context, configDirectory string, dsService BaseDataSourceService, correlationsStore CorrelationsStore, orgService org.Service) error {
-	dc := newDatasourceProvisioner(log.New("provisioning.datasources"), store, correlationsStore, orgService)
+	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) {
+		if err != nil {
-			return err
+			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}
+		cmd := &datasources.DeleteDataSourceCommand{OrgID: ds.OrgID, Name: ds.Name, UID: existingDs.UID, SkipPublish: skipPublish}
-		if err := dc.store.DeleteDataSource(ctx, cmd); err != nil {
+		if err := dc.dsService.DeleteDataSource(ctx, cmd); err != nil {
 			return err
 		}
 

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