From ec9c35fae5dce0f716bc2f73b0eaca4d9bc44819 Mon Sep 17 00:00:00 2001 From: Ieva Date: Mon, 21 Aug 2023 14:26:49 +0100 Subject: [PATCH] Chore: clean up access control for data sources (#73010) * move DS guardian interfaces to OSS, move allow guardian to OSS * update codeowner file --- .github/CODEOWNERS | 2 +- pkg/api/datasources.go | 21 +---- pkg/api/datasources_test.go | 6 +- pkg/api/frontendsettings.go | 2 +- pkg/api/http_server.go | 8 +- pkg/server/wire.go | 2 + pkg/server/wireexts_oss.go | 11 +-- pkg/services/alerting/extractor.go | 33 ++++---- pkg/services/alerting/extractor_test.go | 76 +------------------ .../datasources/guardian/allow_guardian.go | 19 +++++ pkg/services/datasources/guardian/provider.go | 25 ++++++ .../permissions/datasource_permissions.go | 25 ------ .../datasource_permissions_mocks.go | 21 ----- pkg/services/datasources/service/cache.go | 32 +++++++- .../publicdashboards/api/common_test.go | 3 +- .../publicdashboards/api/query_test.go | 3 +- 16 files changed, 109 insertions(+), 180 deletions(-) create mode 100644 pkg/services/datasources/guardian/allow_guardian.go create mode 100644 pkg/services/datasources/guardian/provider.go delete mode 100644 pkg/services/datasources/permissions/datasource_permissions.go delete mode 100644 pkg/services/datasources/permissions/datasource_permissions_mocks.go diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 5e190d75bff..c8324965490 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -549,7 +549,7 @@ lerna.json @grafana/frontend-ops /pkg/services/authn/ @grafana/grafana-authnz-team /pkg/services/signingkeys/ @grafana/grafana-authnz-team /pkg/services/dashboards/accesscontrol.go @grafana/grafana-authnz-team -/pkg/services/datasources/permissions/ @grafana/grafana-authnz-team +/pkg/services/datasources/guardian/ @grafana/grafana-authnz-team /pkg/services/guardian/ @grafana/grafana-authnz-team /pkg/services/ldap/ @grafana/grafana-authnz-team /pkg/services/login/ @grafana/grafana-authnz-team diff --git a/pkg/api/datasources.go b/pkg/api/datasources.go index 2607566bd96..4e9c27fdc1d 100644 --- a/pkg/api/datasources.go +++ b/pkg/api/datasources.go @@ -19,9 +19,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/datasources" - "github.com/grafana/grafana/pkg/services/datasources/permissions" "github.com/grafana/grafana/pkg/services/pluginsintegration/plugincontext" - "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/web" @@ -50,7 +48,7 @@ func (hs *HTTPServer) GetDataSources(c *contextmodel.ReqContext) response.Respon return response.Error(500, "Failed to query datasources", err) } - filtered, err := hs.filterDatasourcesByQueryPermission(c.Req.Context(), c.SignedInUser, dataSources) + filtered, err := hs.dsGuardian.New(c.SignedInUser.OrgID, c.SignedInUser).FilterDatasourcesByQueryPermissions(dataSources) if err != nil { return response.Error(500, "Failed to query datasources", err) } @@ -866,23 +864,6 @@ func (hs *HTTPServer) checkDatasourceHealth(c *contextmodel.ReqContext, ds *data return response.JSON(http.StatusOK, payload) } -func (hs *HTTPServer) filterDatasourcesByQueryPermission(ctx context.Context, user *user.SignedInUser, ds []*datasources.DataSource) ([]*datasources.DataSource, error) { - query := datasources.DatasourcesPermissionFilterQuery{ - User: user, - Datasources: ds, - } - - dataSources, err := hs.DatasourcePermissionsService.FilterDatasourcesBasedOnQueryPermissions(ctx, &query) - if err != nil { - if !errors.Is(err, permissions.ErrNotImplemented) { - return nil, err - } - return ds, nil - } - - return dataSources, nil -} - // swagger:parameters checkDatasourceHealthByID type CheckDatasourceHealthByIDParams struct { // in:path diff --git a/pkg/api/datasources_test.go b/pkg/api/datasources_test.go index 5d4f97b809a..d8ded39f2a8 100644 --- a/pkg/api/datasources_test.go +++ b/pkg/api/datasources_test.go @@ -21,7 +21,7 @@ import ( "github.com/grafana/grafana/pkg/services/accesscontrol/actest" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/datasources" - "github.com/grafana/grafana/pkg/services/datasources/permissions" + "github.com/grafana/grafana/pkg/services/datasources/guardian" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/web" "github.com/grafana/grafana/pkg/web/webtest" @@ -35,7 +35,6 @@ const ( func TestDataSourcesProxy_userLoggedIn(t *testing.T) { mockSQLStore := dbtest.NewFakeDB() - mockDatasourcePermissionService := permissions.NewMockDatasourcePermissionService() loggedInUserScenario(t, "When calling GET on", "/api/datasources/", "/api/datasources/", func(sc *scenarioContext) { // Stubs the database query ds := []*datasources.DataSource{ @@ -44,7 +43,6 @@ func TestDataSourcesProxy_userLoggedIn(t *testing.T) { {Name: "BBB"}, {Name: "aaa"}, } - mockDatasourcePermissionService.DsResult = ds // handler func being tested hs := &HTTPServer{ @@ -53,7 +51,7 @@ func TestDataSourcesProxy_userLoggedIn(t *testing.T) { DataSourcesService: &dataSourcesServiceMock{ expectedDatasources: ds, }, - DatasourcePermissionsService: mockDatasourcePermissionService, + dsGuardian: guardian.ProvideGuardian(), } sc.handlerFunc = hs.GetDataSources sc.fakeReq("GET", "/api/datasources").exec() diff --git a/pkg/api/frontendsettings.go b/pkg/api/frontendsettings.go index 10b69b5ef18..f116a7c6b8f 100644 --- a/pkg/api/frontendsettings.go +++ b/pkg/api/frontendsettings.go @@ -286,7 +286,7 @@ func (hs *HTTPServer) getFSDataSources(c *contextmodel.ReqContext, availablePlug // If RBAC is enabled, it will filter out all datasources for a public user, so we need to skip it orgDataSources = dataSources } else { - filtered, err := hs.filterDatasourcesByQueryPermission(c.Req.Context(), c.SignedInUser, dataSources) + filtered, err := hs.dsGuardian.New(c.SignedInUser.OrgID, c.SignedInUser).FilterDatasourcesByQueryPermissions(dataSources) if err != nil { return nil, err } diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index f830f89c194..5257f33a108 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -49,7 +49,7 @@ import ( dashver "github.com/grafana/grafana/pkg/services/dashboardversion" "github.com/grafana/grafana/pkg/services/datasourceproxy" "github.com/grafana/grafana/pkg/services/datasources" - "github.com/grafana/grafana/pkg/services/datasources/permissions" + "github.com/grafana/grafana/pkg/services/datasources/guardian" "github.com/grafana/grafana/pkg/services/encryption" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/folder" @@ -174,7 +174,7 @@ type HTTPServer struct { DashboardService dashboards.DashboardService dashboardProvisioningService dashboards.DashboardProvisioningService folderService folder.Service - DatasourcePermissionsService permissions.DatasourcePermissionsService + dsGuardian guardian.DatasourceGuardianProvider AlertNotificationService *alerting.AlertNotificationService dashboardsnapshotsService dashboardsnapshots.Service PluginSettings pluginSettings.Service @@ -232,7 +232,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi authInfoService login.AuthInfoService, storageService store.StorageService, httpEntityStore httpentitystore.HTTPEntityStore, notificationService *notifications.NotificationService, dashboardService dashboards.DashboardService, dashboardProvisioningService dashboards.DashboardProvisioningService, folderService folder.Service, - datasourcePermissionsService permissions.DatasourcePermissionsService, alertNotificationService *alerting.AlertNotificationService, + dsGuardian guardian.DatasourceGuardianProvider, alertNotificationService *alerting.AlertNotificationService, dashboardsnapshotsService dashboardsnapshots.Service, pluginSettings pluginSettings.Service, avatarCacheServer *avatar.AvatarCacheServer, preferenceService pref.Service, teamsPermissionsService accesscontrol.TeamPermissionsService, folderPermissionsService accesscontrol.FolderPermissionsService, @@ -317,7 +317,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi DashboardService: dashboardService, dashboardProvisioningService: dashboardProvisioningService, folderService: folderService, - DatasourcePermissionsService: datasourcePermissionsService, + dsGuardian: dsGuardian, teamPermissionsService: teamsPermissionsService, AlertNotificationService: alertNotificationService, dashboardsnapshotsService: dashboardsnapshotsService, diff --git a/pkg/server/wire.go b/pkg/server/wire.go index 3169120cac9..f0ed4e99e92 100644 --- a/pkg/server/wire.go +++ b/pkg/server/wire.go @@ -254,6 +254,8 @@ var wireBasicSet = wire.NewSet( elasticsearch.ProvideService, pyroscope.ProvideService, parca.ProvideService, + datasourceservice.ProvideCacheService, + wire.Bind(new(datasources.CacheService), new(*datasourceservice.CacheServiceImpl)), encryptionservice.ProvideEncryptionService, wire.Bind(new(encryption.Internal), new(*encryptionservice.Service)), secretsManager.ProvideSecretsService, diff --git a/pkg/server/wireexts_oss.go b/pkg/server/wireexts_oss.go index 5498d8e1c75..903356c9487 100644 --- a/pkg/server/wireexts_oss.go +++ b/pkg/server/wireexts_oss.go @@ -5,7 +5,6 @@ package server import ( "github.com/google/wire" - "github.com/grafana/grafana/pkg/plugins" "github.com/grafana/grafana/pkg/registry" "github.com/grafana/grafana/pkg/registry/backgroundsvcs" @@ -18,9 +17,7 @@ import ( "github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/services/auth/authimpl" "github.com/grafana/grafana/pkg/services/caching" - "github.com/grafana/grafana/pkg/services/datasources" - "github.com/grafana/grafana/pkg/services/datasources/permissions" - datasourceservice "github.com/grafana/grafana/pkg/services/datasources/service" + "github.com/grafana/grafana/pkg/services/datasources/guardian" "github.com/grafana/grafana/pkg/services/encryption" encryptionprovider "github.com/grafana/grafana/pkg/services/encryption/provider" "github.com/grafana/grafana/pkg/services/kmsproviders" @@ -63,8 +60,6 @@ var wireExtsBasicSet = wire.NewSet( wire.Bind(new(provisioning.ProvisioningService), new(*provisioning.ProvisioningServiceImpl)), backgroundsvcs.ProvideBackgroundServiceRegistry, wire.Bind(new(registry.BackgroundServiceRegistry), new(*backgroundsvcs.BackgroundServiceRegistry)), - datasourceservice.ProvideCacheService, - wire.Bind(new(datasources.CacheService), new(*datasourceservice.CacheServiceImpl)), migrations.ProvideOSSMigrations, wire.Bind(new(registry.DatabaseMigrator), new(*migrations.OSSMigrations)), authinfoservice.ProvideOSSUserProtectionService, @@ -79,8 +74,8 @@ var wireExtsBasicSet = wire.NewSet( wire.Bind(new(kmsproviders.Service), new(osskmsproviders.Service)), ldap.ProvideGroupsService, wire.Bind(new(ldap.Groups), new(*ldap.OSSGroups)), - permissions.ProvideDatasourcePermissionsService, - wire.Bind(new(permissions.DatasourcePermissionsService), new(*permissions.OSSDatasourcePermissionsService)), + guardian.ProvideGuardian, + wire.Bind(new(guardian.DatasourceGuardianProvider), new(*guardian.OSSProvider)), usagestatssvcs.ProvideUsageStatsProvidersRegistry, wire.Bind(new(registry.UsageStatsProvidersRegistry), new(*usagestatssvcs.UsageStatsProvidersRegistry)), ossaccesscontrol.ProvideDatasourcePermissionsService, diff --git a/pkg/services/alerting/extractor.go b/pkg/services/alerting/extractor.go index a718c69a816..a5e4510a29a 100644 --- a/pkg/services/alerting/extractor.go +++ b/pkg/services/alerting/extractor.go @@ -10,7 +10,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/alerting/models" "github.com/grafana/grafana/pkg/services/datasources" - "github.com/grafana/grafana/pkg/services/datasources/permissions" + "github.com/grafana/grafana/pkg/services/datasources/guardian" ) type DashAlertExtractor interface { @@ -20,18 +20,18 @@ type DashAlertExtractor interface { // DashAlertExtractorService extracts alerts from the dashboard json. type DashAlertExtractorService struct { - datasourcePermissionsService permissions.DatasourcePermissionsService - datasourceService datasources.DataSourceService - alertStore AlertStore - log log.Logger + dsGuardian guardian.DatasourceGuardianProvider + datasourceService datasources.DataSourceService + alertStore AlertStore + log log.Logger } -func ProvideDashAlertExtractorService(datasourcePermissionsService permissions.DatasourcePermissionsService, datasourceService datasources.DataSourceService, store AlertStore) *DashAlertExtractorService { +func ProvideDashAlertExtractorService(dsGuardian guardian.DatasourceGuardianProvider, datasourceService datasources.DataSourceService, store AlertStore) *DashAlertExtractorService { return &DashAlertExtractorService{ - datasourcePermissionsService: datasourcePermissionsService, - datasourceService: datasourceService, - alertStore: store, - log: log.New("alerting.extractor"), + dsGuardian: dsGuardian, + datasourceService: datasourceService, + alertStore: store, + log: log.New("alerting.extractor"), } } @@ -210,17 +210,10 @@ func (e *DashAlertExtractorService) getAlertFromPanels(ctx context.Context, json return nil, err } - dsFilterQuery := datasources.DatasourcesPermissionFilterQuery{ - User: dashAlertInfo.User, - Datasources: []*datasources.DataSource{datasource}, - } - - dataSources, err := e.datasourcePermissionsService.FilterDatasourcesBasedOnQueryPermissions(ctx, &dsFilterQuery) + canQuery, err := e.dsGuardian.New(dashAlertInfo.OrgID, dashAlertInfo.User, *datasource).CanQuery(datasource.ID) if err != nil { - if !errors.Is(err, permissions.ErrNotImplemented) { - return nil, err - } - } else if len(dataSources) == 0 { + return nil, err + } else if !canQuery { return nil, datasources.ErrDataSourceAccessDenied } diff --git a/pkg/services/alerting/extractor_test.go b/pkg/services/alerting/extractor_test.go index fa15c9190c3..df1f5489aae 100644 --- a/pkg/services/alerting/extractor_test.go +++ b/pkg/services/alerting/extractor_test.go @@ -2,12 +2,10 @@ package alerting import ( "context" - "errors" "os" "testing" "time" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/components/simplejson" @@ -16,7 +14,7 @@ import ( "github.com/grafana/grafana/pkg/services/alerting/models" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/datasources" - "github.com/grafana/grafana/pkg/services/datasources/permissions" + "github.com/grafana/grafana/pkg/services/datasources/guardian" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/setting" @@ -34,18 +32,13 @@ func TestAlertRuleExtraction(t *testing.T) { json, err := os.ReadFile("./testdata/graphite-alert.json") require.Nil(t, err) - dsPermissions := permissions.NewMockDatasourcePermissionService() - dsPermissions.DsResult = []*datasources.DataSource{ - { - ID: 1, - }, - } + dsGuardian := guardian.ProvideGuardian() dsService := &fakeDatasourceService{ExpectedDatasource: defaultDs} db := dbtest.NewFakeDB() cfg := &setting.Cfg{} store := ProvideAlertStore(db, localcache.ProvideService(), cfg, nil, featuremgmt.WithFeatures()) - extractor := ProvideDashAlertExtractorService(dsPermissions, dsService, store) + extractor := ProvideDashAlertExtractorService(dsGuardian, dsService, store) t.Run("Parsing alert rules from dashboard json", func(t *testing.T) { dashJSON, err := simplejson.NewJson(json) @@ -306,69 +299,6 @@ func TestAlertRuleExtraction(t *testing.T) { }) } -func TestFilterPermissionsErrors(t *testing.T) { - RegisterCondition("query", func(model *simplejson.Json, index int) (Condition, error) { - return &FakeCondition{}, nil - }) - - // mock data - defaultDs := &datasources.DataSource{ID: 12, OrgID: 1, Name: "I am default", IsDefault: true, UID: "def-uid"} - - json, err := os.ReadFile("./testdata/graphite-alert.json") - require.Nil(t, err) - dashJSON, err := simplejson.NewJson(json) - require.Nil(t, err) - - dsPermissions := permissions.NewMockDatasourcePermissionService() - dsService := &fakeDatasourceService{ExpectedDatasource: defaultDs} - extractor := ProvideDashAlertExtractorService(dsPermissions, dsService, nil) - - tc := []struct { - name string - result []*datasources.DataSource - err error - expectedErr error - }{ - { - "Data sources are filtered and return results don't return an error", - []*datasources.DataSource{defaultDs}, - nil, - nil, - }, - { - "Data sources are filtered but return empty results should return error", - nil, - nil, - datasources.ErrDataSourceAccessDenied, - }, - { - "Using default OSS implementation doesn't return an error", - nil, - permissions.ErrNotImplemented, - nil, - }, - { - "Returning an error different from ErrNotImplemented should fails", - nil, - errors.New("random error"), - errors.New("random error"), - }, - } - - for _, test := range tc { - t.Run(test.name, func(t *testing.T) { - dsPermissions.DsResult = test.result - dsPermissions.ErrResult = test.err - _, err = extractor.GetAlerts(WithUAEnabled(context.Background(), true), DashAlertInfo{ - User: nil, - Dash: dashboards.NewDashboardFromJson(dashJSON), - OrgID: 1, - }) - assert.Equal(t, err, test.expectedErr) - }) - } -} - type fakeDatasourceService struct { ExpectedDatasource *datasources.DataSource datasources.DataSourceService diff --git a/pkg/services/datasources/guardian/allow_guardian.go b/pkg/services/datasources/guardian/allow_guardian.go new file mode 100644 index 00000000000..add832b43cb --- /dev/null +++ b/pkg/services/datasources/guardian/allow_guardian.go @@ -0,0 +1,19 @@ +package guardian + +import ( + "github.com/grafana/grafana/pkg/services/datasources" +) + +var _ DatasourceGuardian = new(AllowGuardian) + +// AllowGuardian is used whenever an enterprise build is running without a license. +// It allows every one to Query all data sources and will not filter out any of them +type AllowGuardian struct{} + +func (n AllowGuardian) CanQuery(datasourceID int64) (bool, error) { + return true, nil +} + +func (n AllowGuardian) FilterDatasourcesByQueryPermissions(ds []*datasources.DataSource) ([]*datasources.DataSource, error) { + return ds, nil +} diff --git a/pkg/services/datasources/guardian/provider.go b/pkg/services/datasources/guardian/provider.go new file mode 100644 index 00000000000..5be645ae4a1 --- /dev/null +++ b/pkg/services/datasources/guardian/provider.go @@ -0,0 +1,25 @@ +package guardian + +import ( + "github.com/grafana/grafana/pkg/services/datasources" + "github.com/grafana/grafana/pkg/services/user" +) + +type DatasourceGuardianProvider interface { + New(orgID int64, user *user.SignedInUser, dataSources ...datasources.DataSource) DatasourceGuardian +} + +type DatasourceGuardian interface { + CanQuery(datasourceID int64) (bool, error) + FilterDatasourcesByQueryPermissions([]*datasources.DataSource) ([]*datasources.DataSource, error) +} + +func ProvideGuardian() *OSSProvider { + return &OSSProvider{} +} + +type OSSProvider struct{} + +func (p *OSSProvider) New(orgID int64, user *user.SignedInUser, dataSources ...datasources.DataSource) DatasourceGuardian { + return &AllowGuardian{} +} diff --git a/pkg/services/datasources/permissions/datasource_permissions.go b/pkg/services/datasources/permissions/datasource_permissions.go deleted file mode 100644 index 20e59772ea0..00000000000 --- a/pkg/services/datasources/permissions/datasource_permissions.go +++ /dev/null @@ -1,25 +0,0 @@ -package permissions - -import ( - "context" - "errors" - - "github.com/grafana/grafana/pkg/services/datasources" -) - -var ErrNotImplemented = errors.New("not implemented") - -type DatasourcePermissionsService interface { - FilterDatasourcesBasedOnQueryPermissions(ctx context.Context, cmd *datasources.DatasourcesPermissionFilterQuery) ([]*datasources.DataSource, error) -} - -// dummy method -func (hs *OSSDatasourcePermissionsService) FilterDatasourcesBasedOnQueryPermissions(ctx context.Context, cmd *datasources.DatasourcesPermissionFilterQuery) ([]*datasources.DataSource, error) { - return nil, ErrNotImplemented -} - -type OSSDatasourcePermissionsService struct{} - -func ProvideDatasourcePermissionsService() *OSSDatasourcePermissionsService { - return &OSSDatasourcePermissionsService{} -} diff --git a/pkg/services/datasources/permissions/datasource_permissions_mocks.go b/pkg/services/datasources/permissions/datasource_permissions_mocks.go deleted file mode 100644 index 73b527b3446..00000000000 --- a/pkg/services/datasources/permissions/datasource_permissions_mocks.go +++ /dev/null @@ -1,21 +0,0 @@ -package permissions - -import ( - "context" - - "github.com/grafana/grafana/pkg/services/datasources" -) - -type mockDatasourcePermissionService struct { - DsResult []*datasources.DataSource - DsUidResult []string - ErrResult error -} - -func (m *mockDatasourcePermissionService) FilterDatasourcesBasedOnQueryPermissions(ctx context.Context, cmd *datasources.DatasourcesPermissionFilterQuery) ([]*datasources.DataSource, error) { - return m.DsResult, m.ErrResult -} - -func NewMockDatasourcePermissionService() *mockDatasourcePermissionService { - return &mockDatasourcePermissionService{} -} diff --git a/pkg/services/datasources/service/cache.go b/pkg/services/datasources/service/cache.go index 4e5c954d9d4..a97b9cbcadc 100644 --- a/pkg/services/datasources/service/cache.go +++ b/pkg/services/datasources/service/cache.go @@ -9,6 +9,7 @@ import ( "github.com/grafana/grafana/pkg/infra/localcache" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/datasources" + "github.com/grafana/grafana/pkg/services/datasources/guardian" "github.com/grafana/grafana/pkg/services/user" ) @@ -16,12 +17,13 @@ const ( DefaultCacheTTL = 5 * time.Second ) -func ProvideCacheService(cacheService *localcache.CacheService, sqlStore db.DB) *CacheServiceImpl { +func ProvideCacheService(cacheService *localcache.CacheService, sqlStore db.DB, dsGuardian guardian.DatasourceGuardianProvider) *CacheServiceImpl { return &CacheServiceImpl{ logger: log.New("datasources"), cacheTTL: DefaultCacheTTL, CacheService: cacheService, SQLStore: sqlStore, + dsGuardian: dsGuardian, } } @@ -30,6 +32,7 @@ type CacheServiceImpl struct { cacheTTL time.Duration CacheService *localcache.CacheService SQLStore db.DB + dsGuardian guardian.DatasourceGuardianProvider } func (dc *CacheServiceImpl) GetDatasource( @@ -44,6 +47,9 @@ func (dc *CacheServiceImpl) GetDatasource( if cached, found := dc.CacheService.Get(cacheKey); found { ds := cached.(*datasources.DataSource) if ds.OrgID == user.OrgID { + if err := dc.canQuery(user, ds); err != nil { + return nil, err + } return ds, nil } } @@ -62,6 +68,11 @@ func (dc *CacheServiceImpl) GetDatasource( dc.CacheService.Set(uidKey(ds.OrgID, ds.UID), ds, time.Second*5) } dc.CacheService.Set(cacheKey, ds, dc.cacheTTL) + + if err = dc.canQuery(user, ds); err != nil { + return nil, err + } + return ds, nil } @@ -83,6 +94,9 @@ func (dc *CacheServiceImpl) GetDatasourceByUID( if cached, found := dc.CacheService.Get(uidCacheKey); found { ds := cached.(*datasources.DataSource) if ds.OrgID == user.OrgID { + if err := dc.canQuery(user, ds); err != nil { + return nil, err + } return ds, nil } } @@ -98,6 +112,11 @@ func (dc *CacheServiceImpl) GetDatasourceByUID( dc.CacheService.Set(uidCacheKey, ds, dc.cacheTTL) dc.CacheService.Set(idKey(ds.ID), ds, dc.cacheTTL) + + if err = dc.canQuery(user, ds); err != nil { + return nil, err + } + return ds, nil } @@ -108,3 +127,14 @@ func idKey(id int64) string { func uidKey(orgID int64, uid string) string { return fmt.Sprintf("ds-orgid-uid-%d-%s", orgID, uid) } + +func (dc *CacheServiceImpl) canQuery(user *user.SignedInUser, ds *datasources.DataSource) error { + guardian := dc.dsGuardian.New(user.OrgID, user, *ds) + if canQuery, err := guardian.CanQuery(ds.ID); err != nil || !canQuery { + if err != nil { + return err + } + return datasources.ErrDataSourceAccessDenied + } + return nil +} diff --git a/pkg/services/publicdashboards/api/common_test.go b/pkg/services/publicdashboards/api/common_test.go index 0252b43e63f..e25249401bf 100644 --- a/pkg/services/publicdashboards/api/common_test.go +++ b/pkg/services/publicdashboards/api/common_test.go @@ -22,6 +22,7 @@ import ( contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/datasources" fakeDatasources "github.com/grafana/grafana/pkg/services/datasources/fakes" + "github.com/grafana/grafana/pkg/services/datasources/guardian" datasourceService "github.com/grafana/grafana/pkg/services/datasources/service" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/pluginsintegration/plugincontext" @@ -100,7 +101,7 @@ func buildQueryDataService(t *testing.T, cs datasources.CacheService, fpc *fakeP // default cache service if cs == nil { - cs = datasourceService.ProvideCacheService(localcache.ProvideService(), store) + cs = datasourceService.ProvideCacheService(localcache.ProvideService(), store, guardian.ProvideGuardian()) } // default fakePluginClient diff --git a/pkg/services/publicdashboards/api/query_test.go b/pkg/services/publicdashboards/api/query_test.go index fdf48a87705..af827e266d7 100644 --- a/pkg/services/publicdashboards/api/query_test.go +++ b/pkg/services/publicdashboards/api/query_test.go @@ -22,6 +22,7 @@ import ( "github.com/grafana/grafana/pkg/services/dashboards" dashboardStore "github.com/grafana/grafana/pkg/services/dashboards/database" "github.com/grafana/grafana/pkg/services/datasources" + "github.com/grafana/grafana/pkg/services/datasources/guardian" datasourcesService "github.com/grafana/grafana/pkg/services/datasources/service" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/publicdashboards" @@ -263,7 +264,7 @@ func TestIntegrationUnauthenticatedUserCanGetPubdashPanelQueryData(t *testing.T) } db := db.InitTestDB(t) - cacheService := datasourcesService.ProvideCacheService(localcache.ProvideService(), db) + cacheService := datasourcesService.ProvideCacheService(localcache.ProvideService(), db, guardian.ProvideGuardian()) qds := buildQueryDataService(t, cacheService, nil, db) dsStore := datasourcesService.CreateStore(db, log.New("publicdashboards.test")) _, _ = dsStore.AddDataSource(context.Background(), &datasources.AddDataSourceCommand{