AccessControl: Remove permissions on data source delete (#45504)

* AccessControl: Remove permissions on datasource delete

* Ensure legacy behavior is preserved

pkg/services/sqlstore/datasource.go

@@ -2,6 +2,7 @@ package sqlstore
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -10,6 +11,7 @@ import (
 	"github.com/grafana/grafana/pkg/events"
 	"github.com/grafana/grafana/pkg/infra/metrics"
 	"github.com/grafana/grafana/pkg/models"
+	ac "github.com/grafana/grafana/pkg/services/accesscontrol"
 	"github.com/grafana/grafana/pkg/util/errutil"
 	"xorm.io/xorm"
 )
@@ -20,6 +22,11 @@ func (ss *SQLStore) GetDataSource(ctx context.Context, query *models.GetDataSour
 	metrics.MDBDataSourceQueryByID.Inc()
 
 	return ss.WithDbSession(ctx, func(sess *DBSession) error {
+		return ss.getDataSource(ctx, query, sess)
+	})
+}
+
+func (ss *SQLStore) getDataSource(ctx context.Context, query *models.GetDataSourceQuery, sess *DBSession) error {
 	if query.OrgId == 0 || (query.Id == 0 && len(query.Name) == 0 && len(query.Uid) == 0) {
 		return models.ErrDataSourceIdentifierNotSet
 	}
@@ -37,7 +44,6 @@ func (ss *SQLStore) GetDataSource(ctx context.Context, query *models.GetDataSour
 	query.Result = datasource
 
 	return nil
-	})
 }
 
 func (ss *SQLStore) GetDataSources(ctx context.Context, query *models.GetDataSourcesQuery) error {
@@ -82,32 +88,34 @@ func (ss *SQLStore) GetDefaultDataSource(ctx context.Context, query *models.GetD
 }
 
 // DeleteDataSource removes a datasource by org_id as well as either uid (preferred), id, or name
-// and is added to the bus.
+// and is added to the bus. It also removes permissions related to the team.
 func (ss *SQLStore) DeleteDataSource(ctx context.Context, cmd *models.DeleteDataSourceCommand) error {
-	params := make([]interface{}, 0)
-
-	makeQuery := func(sql string, p ...interface{}) {
-		params = append(params, sql)
-		params = append(params, p...)
-	}
-
-	switch {
-	case cmd.OrgID == 0:
-		return models.ErrDataSourceIdentifierNotSet
-	case cmd.UID != "":
-		makeQuery("DELETE FROM data_source WHERE uid=? and org_id=?", cmd.UID, cmd.OrgID)
-	case cmd.ID != 0:
-		makeQuery("DELETE FROM data_source WHERE id=? and org_id=?", cmd.ID, cmd.OrgID)
-	case cmd.Name != "":
-		makeQuery("DELETE FROM data_source WHERE name=? and org_id=?", cmd.Name, cmd.OrgID)
-	default:
-		return models.ErrDataSourceIdentifierNotSet
-	}
-
 	return ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error {
-		result, err := sess.Exec(params...)
+		dsQuery := &models.GetDataSourceQuery{Id: cmd.ID, Uid: cmd.UID, Name: cmd.Name, OrgId: cmd.OrgID}
+		errGettingDS := ss.getDataSource(ctx, dsQuery, sess)
+
+		if errGettingDS != nil && !errors.Is(errGettingDS, models.ErrDataSourceNotFound) {
+			return errGettingDS
+		}
+
+		ds := dsQuery.Result
+		if ds != nil {
+			// Delete the data source
+			result, err := sess.Exec("DELETE FROM data_source WHERE org_id=? AND id=?", ds.OrgId, ds.Id)
+			if err != nil {
+				return err
+			}
+
 			cmd.DeletedDatasourcesCount, _ = result.RowsAffected()
 
+			// Remove associated AccessControl permissions
+			if _, errDeletingPerms := sess.Exec("DELETE FROM permission WHERE scope=?",
+				ac.Scope("datasources", "id", fmt.Sprint(dsQuery.Result.Id))); errDeletingPerms != nil {
+				return errDeletingPerms
+			}
+		}
+
+		// Publish data source deletion event
 		sess.publishAfterCommit(&events.DataSourceDeleted{
 			Timestamp: time.Now(),
 			Name:      cmd.Name,
@@ -116,7 +124,7 @@ func (ss *SQLStore) DeleteDataSource(ctx context.Context, cmd *models.DeleteData
 			OrgID:     cmd.OrgID,
 		})
 
-		return err
+		return nil
 	})
 }
 

pkg/services/sqlstore/datasource_test.go

@@ -6,6 +6,7 @@ package sqlstore
 import (
 	"context"
 	"errors"
+	"fmt"
 	"strconv"
 	"testing"
 	"time"
@@ -13,6 +14,7 @@ import (
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/events"
 	"github.com/grafana/grafana/pkg/models"
+	ac "github.com/grafana/grafana/pkg/services/accesscontrol"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -226,8 +228,10 @@ func TestDataAccess(t *testing.T) {
 			sqlStore := InitTestDB(t)
 			ds := initDatasource(sqlStore)
 
-			err := sqlStore.DeleteDataSource(context.Background(), &models.DeleteDataSourceCommand{ID: ds.Id, OrgID: 123123})
+			err := sqlStore.DeleteDataSource(context.Background(),
+				&models.DeleteDataSourceCommand{ID: ds.Id, OrgID: 123123})
 			require.NoError(t, err)
+
 			query := models.GetDataSourcesQuery{OrgId: 10}
 			err = sqlStore.GetDataSources(context.Background(), &query)
 			require.NoError(t, err)
@@ -246,7 +250,8 @@ func TestDataAccess(t *testing.T) {
 			return nil
 		})
 
-		err := sqlStore.DeleteDataSource(context.Background(), &models.DeleteDataSourceCommand{ID: ds.Id, UID: "nisse-uid", Name: "nisse", OrgID: 123123})
+		err := sqlStore.DeleteDataSource(context.Background(),
+			&models.DeleteDataSourceCommand{ID: ds.Id, UID: "nisse-uid", Name: "nisse", OrgID: int64(123123)})
 		require.NoError(t, err)
 
 		require.Eventually(t, func() bool {
@@ -273,6 +278,42 @@ func TestDataAccess(t *testing.T) {
 		require.Equal(t, 0, len(query.Result))
 	})
 
+	t.Run("DeleteDataSourceAccessControlPermissions", func(t *testing.T) {
+		sqlStore := InitTestDB(t)
+		ds := initDatasource(sqlStore)
+
+		// Init associated permission
+		errAddPermissions := sqlStore.WithTransactionalDbSession(context.TODO(), func(sess *DBSession) error {
+			_, err := sess.Table("permission").Insert(ac.Permission{
+				RoleID:  1,
+				Action:  "datasources:read",
+				Scope:   ac.Scope("datasources", "id", fmt.Sprintf("%d", ds.Id)),
+				Updated: time.Now(),
+				Created: time.Now(),
+			})
+			return err
+		})
+		require.NoError(t, errAddPermissions)
+		query := models.GetDataSourcesQuery{OrgId: 10}
+
+		errDeletingDS := sqlStore.DeleteDataSource(context.Background(),
+			&models.DeleteDataSourceCommand{Name: ds.Name, OrgID: ds.OrgId},
+		)
+		require.NoError(t, errDeletingDS)
+
+		// Check associated permission
+		permCount := int64(0)
+		errGetPermissions := sqlStore.WithTransactionalDbSession(context.TODO(), func(sess *DBSession) 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")
+
+		require.Equal(t, 0, len(query.Result))
+	})
+
 	t.Run("GetDataSources", func(t *testing.T) {
 		t.Run("Number of data sources returned limited to 6 per organization", func(t *testing.T) {
 			sqlStore := InitTestDB(t)