From 47e248ceabfe4ba6770064d510261d2fc4741fea Mon Sep 17 00:00:00 2001 From: Gabriel MABILLE Date: Thu, 17 Feb 2022 14:06:34 +0100 Subject: [PATCH] AccessControl: Remove permissions on data source delete (#45504) * AccessControl: Remove permissions on datasource delete * Ensure legacy behavior is preserved --- pkg/services/sqlstore/datasource.go | 90 +++++++++++++----------- pkg/services/sqlstore/datasource_test.go | 45 +++++++++++- 2 files changed, 92 insertions(+), 43 deletions(-) diff --git a/pkg/services/sqlstore/datasource.go b/pkg/services/sqlstore/datasource.go index 0ad11bb52f0..7f3f0cd3219 100644 --- a/pkg/services/sqlstore/datasource.go +++ b/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,26 +22,30 @@ func (ss *SQLStore) GetDataSource(ctx context.Context, query *models.GetDataSour metrics.MDBDataSourceQueryByID.Inc() return ss.WithDbSession(ctx, func(sess *DBSession) error { - if query.OrgId == 0 || (query.Id == 0 && len(query.Name) == 0 && len(query.Uid) == 0) { - return models.ErrDataSourceIdentifierNotSet - } - - datasource := &models.DataSource{Name: query.Name, OrgId: query.OrgId, Id: query.Id, Uid: query.Uid} - has, err := sess.Get(datasource) - - if err != nil { - sqlog.Error("Failed getting data source", "err", err, "uid", query.Uid, "id", query.Id, "name", query.Name, "orgId", query.OrgId) - return err - } else if !has { - return models.ErrDataSourceNotFound - } - - query.Result = datasource - - return nil + 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 + } + + datasource := &models.DataSource{Name: query.Name, OrgId: query.OrgId, Id: query.Id, Uid: query.Uid} + has, err := sess.Get(datasource) + + if err != nil { + sqlog.Error("Failed getting data source", "err", err, "uid", query.Uid, "id", query.Id, "name", query.Name, "orgId", query.OrgId) + return err + } else if !has { + return models.ErrDataSourceNotFound + } + + query.Result = datasource + + return nil +} + func (ss *SQLStore) GetDataSources(ctx context.Context, query *models.GetDataSourcesQuery) error { var sess *xorm.Session return ss.WithDbSession(ctx, func(dbSess *DBSession) 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...) - cmd.DeletedDatasourcesCount, _ = result.RowsAffected() + 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 }) } diff --git a/pkg/services/sqlstore/datasource_test.go b/pkg/services/sqlstore/datasource_test.go index b53a7703f56..46e36620f01 100644 --- a/pkg/services/sqlstore/datasource_test.go +++ b/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)