From 215eb31c7a6f8c79624eb149dcc2cfdde2b5bfb4 Mon Sep 17 00:00:00 2001 From: Stephanie Hingtgen Date: Fri, 10 Jan 2025 07:35:38 -0700 Subject: [PATCH] Annotations: Fix usage of dashboard table for permissions (#98774) --- .../accesscontrol/accesscontrol.go | 30 ++++----- .../accesscontrol/accesscontrol_test.go | 66 ++++++++++++++----- .../annotations/accesscontrol/models.go | 5 -- .../annotationsimpl/annotations.go | 4 +- .../annotationsimpl/annotations_test.go | 61 +++++++++++------ .../publicdashboards/service/common_test.go | 2 +- 6 files changed, 107 insertions(+), 61 deletions(-) diff --git a/pkg/services/annotations/accesscontrol/accesscontrol.go b/pkg/services/annotations/accesscontrol/accesscontrol.go index cc2f7e980bd..d29eed7d1e7 100644 --- a/pkg/services/annotations/accesscontrol/accesscontrol.go +++ b/pkg/services/annotations/accesscontrol/accesscontrol.go @@ -7,6 +7,7 @@ import ( "github.com/grafana/grafana/pkg/infra/db" ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/annotations" + "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/dashboards/dashboardaccess" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/sqlstore/permissions" @@ -29,12 +30,14 @@ var ( type AuthService struct { db db.DB features featuremgmt.FeatureToggles + dashSvc dashboards.DashboardService } -func NewAuthService(db db.DB, features featuremgmt.FeatureToggles) *AuthService { +func NewAuthService(db db.DB, features featuremgmt.FeatureToggles, dashSvc dashboards.DashboardService) *AuthService { return &AuthService{ db: db, features: features, + dashSvc: dashSvc, } } @@ -133,26 +136,21 @@ func (authz *AuthService) dashboardsWithVisibleAnnotations(ctx context.Context, }) } - sb := &searchstore.Builder{Dialect: authz.db.GetDialect(), Filters: filters, Features: authz.features} - // This is a limit for a batch size, not for the end query result. - var limit int64 = 1000 - if query.Page == 0 { - query.Page = 1 - } - sql, params := sb.ToSQL(limit, query.Page) - - visibleDashboards := make(map[string]int64) - var res []dashboardProjection - - err = authz.db.WithDbSession(ctx, func(sess *db.Session) error { - return sess.SQL(sql, params...).Find(&res) + dashs, err := authz.dashSvc.SearchDashboards(ctx, &dashboards.FindPersistedDashboardsQuery{ + OrgId: query.SignedInUser.GetOrgID(), + Filters: filters, + SignedInUser: query.SignedInUser, + Page: query.Page, + Type: filterType, + Limit: 1000, }) if err != nil { return nil, err } - for _, p := range res { - visibleDashboards[p.UID] = p.ID + visibleDashboards := make(map[string]int64) + for _, d := range dashs { + visibleDashboards[d.UID] = d.ID } return visibleDashboards, nil diff --git a/pkg/services/annotations/accesscontrol/accesscontrol_test.go b/pkg/services/annotations/accesscontrol/accesscontrol_test.go index f3d82853b7c..3933b590311 100644 --- a/pkg/services/annotations/accesscontrol/accesscontrol_test.go +++ b/pkg/services/annotations/accesscontrol/accesscontrol_test.go @@ -7,13 +7,25 @@ import ( "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/infra/db" + "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" + accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" "github.com/grafana/grafana/pkg/services/annotations" "github.com/grafana/grafana/pkg/services/annotations/testutil" + "github.com/grafana/grafana/pkg/services/authz/zanzana" "github.com/grafana/grafana/pkg/services/dashboards" + "github.com/grafana/grafana/pkg/services/dashboards/database" + dashboardsservice "github.com/grafana/grafana/pkg/services/dashboards/service" "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/services/folder/folderimpl" + "github.com/grafana/grafana/pkg/services/guardian" + "github.com/grafana/grafana/pkg/services/quota/quotatest" + "github.com/grafana/grafana/pkg/services/supportbundles/supportbundlestest" + "github.com/grafana/grafana/pkg/services/tag/tagimpl" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/tests/testsuite" ) @@ -28,27 +40,46 @@ func TestIntegrationAuthorize(t *testing.T) { } sql, cfg := db.InitTestDBWithCfg(t) - - dash1 := testutil.CreateDashboard(t, sql, cfg, featuremgmt.WithFeatures(), dashboards.SaveDashboardCommand{ - UserID: 1, - OrgID: 1, - Dashboard: simplejson.NewFromAny(map[string]any{ - "title": "Dashboard 1", - }), - }) - - dash2 := testutil.CreateDashboard(t, sql, cfg, featuremgmt.WithFeatures(), dashboards.SaveDashboardCommand{ - UserID: 1, - OrgID: 1, - Dashboard: simplejson.NewFromAny(map[string]any{ - "title": "Dashboard 2", - }), - }) + origNewDashboardGuardian := guardian.New + defer func() { guardian.New = origNewDashboardGuardian }() + guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true}) + folderStore := folderimpl.ProvideDashboardFolderStore(sql) + fStore := folderimpl.ProvideStore(sql) + dashStore, err := database.ProvideDashboardStore(sql, cfg, featuremgmt.WithFeatures(), tagimpl.ProvideService(sql)) + require.NoError(t, err) + ac := acimpl.ProvideAccessControl(featuremgmt.WithFeatures(), zanzana.NewNoopClient()) + folderSvc := folderimpl.ProvideService(fStore, accesscontrolmock.New(), bus.ProvideBus(tracing.InitializeTracerForTest()), + dashStore, folderStore, sql, featuremgmt.WithFeatures(), + supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) + dashSvc, err := dashboardsservice.ProvideDashboardServiceImpl(cfg, dashStore, folderStore, featuremgmt.WithFeatures(), accesscontrolmock.NewMockedPermissionsService(), accesscontrolmock.NewMockedPermissionsService(), + ac, folderSvc, fStore, nil, zanzana.NewNoopClient(), nil, nil, nil, quotatest.New(false, nil), nil) + require.NoError(t, err) u := &user.SignedInUser{ UserID: 1, OrgID: 1, } + + dash1, err := dashSvc.SaveDashboard(context.Background(), &dashboards.SaveDashboardDTO{ + User: u, + OrgID: 1, + Dashboard: &dashboards.Dashboard{ + Title: "Dashboard 1", + Data: simplejson.New(), + }, + }, false) + require.NoError(t, err) + + dash2, err := dashSvc.SaveDashboard(context.Background(), &dashboards.SaveDashboardDTO{ + User: u, + OrgID: 1, + Dashboard: &dashboards.Dashboard{ + Title: "Dashboard 2", + Data: simplejson.New(), + }, + }, false) + require.NoError(t, err) + role := testutil.SetupRBACRole(t, sql, u) type testCase struct { @@ -172,8 +203,7 @@ func TestIntegrationAuthorize(t *testing.T) { t.Run(tc.name, func(t *testing.T) { u.Permissions = map[int64]map[string][]string{1: tc.permissions} testutil.SetupRBACPermission(t, sql, role, u) - - authz := NewAuthService(sql, featuremgmt.WithFeatures(tc.featureToggle)) + authz := NewAuthService(sql, featuremgmt.WithFeatures(tc.featureToggle), dashSvc) query := annotations.ItemQuery{SignedInUser: u, OrgID: 1} resources, err := authz.Authorize(context.Background(), query) diff --git a/pkg/services/annotations/accesscontrol/models.go b/pkg/services/annotations/accesscontrol/models.go index 8a2c7ed01e5..02b91653bd6 100644 --- a/pkg/services/annotations/accesscontrol/models.go +++ b/pkg/services/annotations/accesscontrol/models.go @@ -11,8 +11,3 @@ type AccessResources struct { // Skip filtering SkipAccessControlFilter bool } - -type dashboardProjection struct { - ID int64 `xorm:"id"` - UID string `xorm:"uid"` -} diff --git a/pkg/services/annotations/annotationsimpl/annotations.go b/pkg/services/annotations/annotationsimpl/annotations.go index 22c0ec796bb..3f7eed0056b 100644 --- a/pkg/services/annotations/annotationsimpl/annotations.go +++ b/pkg/services/annotations/annotationsimpl/annotations.go @@ -5,6 +5,7 @@ import ( "github.com/grafana/grafana/pkg/services/annotations/accesscontrol" "github.com/grafana/grafana/pkg/services/annotations/annotationsimpl/loki" + "github.com/grafana/grafana/pkg/services/dashboards" alertingStore "github.com/grafana/grafana/pkg/services/ngalert/store" "github.com/grafana/grafana/pkg/infra/db" @@ -31,6 +32,7 @@ func ProvideService( tagService tag.Service, tracer tracing.Tracer, ruleStore *alertingStore.DBstore, + dashSvc dashboards.DashboardService, ) *RepositoryImpl { l := log.New("annotations") l.Debug("Initializing annotations service") @@ -51,7 +53,7 @@ func ProvideService( return &RepositoryImpl{ db: db, features: features, - authZ: accesscontrol.NewAuthService(db, features), + authZ: accesscontrol.NewAuthService(db, features, dashSvc), reader: read, writer: write, } diff --git a/pkg/services/annotations/annotationsimpl/annotations_test.go b/pkg/services/annotations/annotationsimpl/annotations_test.go index dd1d7de5171..9f7fbe77164 100644 --- a/pkg/services/annotations/annotationsimpl/annotations_test.go +++ b/pkg/services/annotations/annotationsimpl/annotations_test.go @@ -16,16 +16,19 @@ import ( "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" + accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" "github.com/grafana/grafana/pkg/services/annotations" "github.com/grafana/grafana/pkg/services/annotations/testutil" "github.com/grafana/grafana/pkg/services/authz/zanzana" "github.com/grafana/grafana/pkg/services/dashboards" - dashboardstore "github.com/grafana/grafana/pkg/services/dashboards/database" + "github.com/grafana/grafana/pkg/services/dashboards/database" + dashboardsservice "github.com/grafana/grafana/pkg/services/dashboards/service" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/folder/folderimpl" "github.com/grafana/grafana/pkg/services/guardian" alertingStore "github.com/grafana/grafana/pkg/services/ngalert/store" + "github.com/grafana/grafana/pkg/services/quota/quotatest" "github.com/grafana/grafana/pkg/services/supportbundles/supportbundlestest" "github.com/grafana/grafana/pkg/services/tag/tagimpl" "github.com/grafana/grafana/pkg/services/user" @@ -49,8 +52,22 @@ func TestIntegrationAnnotationListingWithRBAC(t *testing.T) { features := featuremgmt.WithFeatures() tagService := tagimpl.ProvideService(sql) ruleStore := alertingStore.SetupStoreForTesting(t, sql) + origNewDashboardGuardian := guardian.New + defer func() { guardian.New = origNewDashboardGuardian }() + guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{}) + folderStore := folderimpl.ProvideDashboardFolderStore(sql) + fStore := folderimpl.ProvideStore(sql) + dashStore, err := database.ProvideDashboardStore(sql, cfg, featuremgmt.WithFeatures(), tagimpl.ProvideService(sql)) + require.NoError(t, err) + ac := acimpl.ProvideAccessControl(featuremgmt.WithFeatures(), zanzana.NewNoopClient()) + folderSvc := folderimpl.ProvideService(fStore, accesscontrolmock.New(), bus.ProvideBus(tracing.InitializeTracerForTest()), + dashStore, folderStore, sql, featuremgmt.WithFeatures(), + supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) + dashSvc, err := dashboardsservice.ProvideDashboardServiceImpl(cfg, dashStore, folderStore, featuremgmt.WithFeatures(), accesscontrolmock.NewMockedPermissionsService(), accesscontrolmock.NewMockedPermissionsService(), + ac, folderSvc, fStore, nil, zanzana.NewNoopClient(), nil, nil, nil, quotatest.New(false, nil), nil) + require.NoError(t, err) - repo := ProvideService(sql, cfg, features, tagService, tracing.InitializeTracerForTest(), ruleStore) + repo := ProvideService(sql, cfg, features, tagService, tracing.InitializeTracerForTest(), ruleStore, dashSvc) dashboard1 := testutil.CreateDashboard(t, sql, cfg, features, dashboards.SaveDashboardCommand{ UserID: 1, @@ -70,8 +87,6 @@ func TestIntegrationAnnotationListingWithRBAC(t *testing.T) { }), }) - var err error - dash1Annotation := &annotations.Item{ OrgID: 1, DashboardID: 1, @@ -208,7 +223,7 @@ func TestIntegrationAnnotationListingWithInheritedRBAC(t *testing.T) { allDashboards := make([]dashInfo, 0, folder.MaxNestedFolderDepth+1) annotationsTexts := make([]string, 0, folder.MaxNestedFolderDepth+1) - setupFolderStructure := func() db.DB { + setupFolderStructure := func() (db.DB, dashboards.DashboardService) { sql, cfg := db.InitTestDBWithCfg(t) // enable nested folders so that the folder table is populated for all the tests @@ -216,7 +231,7 @@ func TestIntegrationAnnotationListingWithInheritedRBAC(t *testing.T) { tagService := tagimpl.ProvideService(sql) - dashStore, err := dashboardstore.ProvideDashboardStore(sql, cfg, features, tagService) + dashStore, err := database.ProvideDashboardStore(sql, cfg, features, tagService) require.NoError(t, err) origNewGuardian := guardian.New @@ -227,8 +242,12 @@ func TestIntegrationAnnotationListingWithInheritedRBAC(t *testing.T) { ac := acimpl.ProvideAccessControl(features, zanzana.NewNoopClient()) fStore := folderimpl.ProvideStore(sql) + folderStore := folderimpl.ProvideDashboardFolderStore(sql) folderSvc := folderimpl.ProvideService(fStore, ac, bus.ProvideBus(tracing.InitializeTracerForTest()), dashStore, - folderimpl.ProvideDashboardFolderStore(sql), sql, features, supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) + folderStore, sql, features, supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) + dashSvc, err := dashboardsservice.ProvideDashboardServiceImpl(cfg, dashStore, folderStore, features, accesscontrolmock.NewMockedPermissionsService(), accesscontrolmock.NewMockedPermissionsService(), + ac, folderSvc, fStore, nil, zanzana.NewNoopClient(), nil, nil, nil, quotatest.New(false, nil), nil) + require.NoError(t, err) cfg.AnnotationMaximumTagsLength = 60 @@ -253,16 +272,18 @@ func TestIntegrationAnnotationListingWithInheritedRBAC(t *testing.T) { t.Fail() } - dashboard := testutil.CreateDashboard(t, sql, cfg, features, dashboards.SaveDashboardCommand{ - UserID: usr.UserID, - OrgID: orgID, - IsFolder: false, - Dashboard: simplejson.NewFromAny(map[string]any{ - "title": fmt.Sprintf("Dashboard under %s", f.UID), - }), - FolderID: f.ID, // nolint:staticcheck - FolderUID: f.UID, - }) + dashboard, err := dashSvc.SaveDashboard(context.Background(), &dashboards.SaveDashboardDTO{ + User: usr, + OrgID: orgID, + Dashboard: &dashboards.Dashboard{ + IsFolder: false, + Title: fmt.Sprintf("Dashboard under %s", f.UID), + Data: simplejson.New(), + FolderID: f.ID, // nolint:staticcheck + FolderUID: f.UID, + }, + }, false) + require.NoError(t, err) allDashboards = append(allDashboards, dashInfo{UID: dashboard.UID, ID: dashboard.ID}) @@ -282,10 +303,10 @@ func TestIntegrationAnnotationListingWithInheritedRBAC(t *testing.T) { } role = testutil.SetupRBACRole(t, sql, usr) - return sql + return sql, dashSvc } - sql := setupFolderStructure() + sql, dashSvc := setupFolderStructure() testCases := []struct { desc string @@ -319,7 +340,7 @@ func TestIntegrationAnnotationListingWithInheritedRBAC(t *testing.T) { cfg := setting.NewCfg() cfg.AnnotationMaximumTagsLength = 60 ruleStore := alertingStore.SetupStoreForTesting(t, sql) - repo := ProvideService(sql, cfg, tc.features, tagimpl.ProvideService(sql), tracing.InitializeTracerForTest(), ruleStore) + repo := ProvideService(sql, cfg, tc.features, tagimpl.ProvideService(sql), tracing.InitializeTracerForTest(), ruleStore, dashSvc) usr.Permissions = map[int64]map[string][]string{1: tc.permissions} testutil.SetupRBACPermission(t, sql, role, usr) diff --git a/pkg/services/publicdashboards/service/common_test.go b/pkg/services/publicdashboards/service/common_test.go index 4e0acf61c70..82992ed3622 100644 --- a/pkg/services/publicdashboards/service/common_test.go +++ b/pkg/services/publicdashboards/service/common_test.go @@ -35,7 +35,7 @@ func newPublicDashboardServiceImpl( } tagService := tagimpl.ProvideService(store) if annotationsRepo == nil { - annotationsRepo = annotationsimpl.ProvideService(store, cfg, featuremgmt.WithFeatures(), tagService, tracing.InitializeTracerForTest(), nil) + annotationsRepo = annotationsimpl.ProvideService(store, cfg, featuremgmt.WithFeatures(), tagService, tracing.InitializeTracerForTest(), nil, dashboardService) } if publicDashboardStore == nil {