From 11d8bcbea9728779c4b820efb945f26ac4e8bc59 Mon Sep 17 00:00:00 2001 From: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> Date: Thu, 15 Dec 2022 16:34:17 +0200 Subject: [PATCH] Guardian: Introduce additional constructors (#59577) * Guardian: Use dashboard UID instead of ID * Apply suggestions from code review Introduce several guardian constructors and each time use the most appropriate one. --- pkg/api/alerting.go | 5 +- pkg/api/annotations.go | 7 +- pkg/api/annotations_test.go | 79 +++++++++-- pkg/api/common_test.go | 33 +++-- pkg/api/dashboard.go | 83 +++++++----- pkg/api/dashboard_permission.go | 13 +- pkg/api/dashboard_permission_test.go | 8 +- pkg/api/dashboard_snapshot.go | 6 +- pkg/api/dashboard_snapshot_test.go | 34 ++++- pkg/api/dashboard_test.go | 10 ++ pkg/api/folder.go | 23 +++- pkg/api/folder_permission.go | 11 +- pkg/api/folder_test.go | 7 + pkg/models/dashboards.go | 9 -- .../comments/commentmodel/permissions.go | 20 ++- .../dashboards/service/dashboard_service.go | 32 ++++- .../dashboard_service_integration_test.go | 20 +-- pkg/services/folder/folderimpl/folder.go | 39 +++++- .../guardian/accesscontrol_guardian.go | 123 ++++++++++++++---- .../guardian/accesscontrol_guardian_test.go | 16 ++- pkg/services/guardian/guardian.go | 108 +++++++++++++-- pkg/services/guardian/guardian_test.go | 34 ++++- pkg/services/guardian/guardian_util_test.go | 38 +++++- pkg/services/guardian/provider.go | 21 ++- pkg/services/libraryelements/api.go | 6 +- pkg/services/libraryelements/guard.go | 10 +- .../librarypanels/librarypanels_test.go | 12 +- pkg/services/live/features/dashboard.go | 12 +- pkg/services/ngalert/store/alert_rule.go | 6 +- pkg/services/thumbs/service.go | 46 +++---- 30 files changed, 678 insertions(+), 193 deletions(-) diff --git a/pkg/api/alerting.go b/pkg/api/alerting.go index 01806447a96..285d2005a9c 100644 --- a/pkg/api/alerting.go +++ b/pkg/api/alerting.go @@ -693,7 +693,10 @@ func (hs *HTTPServer) PauseAlert(legacyAlertingEnabled *bool) func(c *models.Req return response.Error(500, "Get Alert failed", err) } - guardian := guardian.New(c.Req.Context(), query.Result.DashboardId, c.OrgID, c.SignedInUser) + guardian, err := guardian.New(c.Req.Context(), query.Result.DashboardId, c.OrgID, c.SignedInUser) + if err != nil { + return response.ErrOrFallback(http.StatusInternalServerError, "Error while creating permission guardian", err) + } if canEdit, err := guardian.CanEdit(); err != nil || !canEdit { if err != nil { return response.Error(500, "Error while checking permissions for Alert", err) diff --git a/pkg/api/annotations.go b/pkg/api/annotations.go index f41bbc89093..ad09335177a 100644 --- a/pkg/api/annotations.go +++ b/pkg/api/annotations.go @@ -504,7 +504,11 @@ func (hs *HTTPServer) canSaveAnnotation(c *models.ReqContext, annotation *annota } func canEditDashboard(c *models.ReqContext, dashboardID int64) (bool, error) { - guard := guardian.New(c.Req.Context(), dashboardID, c.OrgID, c.SignedInUser) + guard, err := guardian.New(c.Req.Context(), dashboardID, c.OrgID, c.SignedInUser) + if err != nil { + return false, err + } + if canEdit, err := guard.CanEdit(); err != nil || !canEdit { return false, err } @@ -606,6 +610,7 @@ func (hs *HTTPServer) canCreateAnnotation(c *models.ReqContext, dashboardId int6 return canSave, err } } + return canEditDashboard(c, dashboardId) } else { // organization annotations if !hs.AccessControl.IsDisabled() { diff --git a/pkg/api/annotations_test.go b/pkg/api/annotations_test.go index 2284fd2cf4a..26534dd4025 100644 --- a/pkg/api/annotations_test.go +++ b/pkg/api/annotations_test.go @@ -14,6 +14,7 @@ import ( "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/api/routing" + "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol" @@ -24,6 +25,7 @@ import ( "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/grafana/grafana/pkg/services/team/teamtest" + "github.com/grafana/grafana/pkg/services/user" ) func TestAnnotationsAPIEndpoint(t *testing.T) { @@ -154,8 +156,9 @@ func TestAnnotationsAPIEndpoint(t *testing.T) { t.Run("When user is an Org Viewer", func(t *testing.T) { role := org.RoleViewer + dashSvc := dashboards.NewFakeDashboardService(t) t.Run("Should not be allowed to save an annotation", func(t *testing.T) { - postAnnotationScenario(t, "When calling POST on", "/api/annotations", "/api/annotations", role, cmd, store, nil, func(sc *scenarioContext) { + postAnnotationScenario(t, "When calling POST on", "/api/annotations", "/api/annotations", role, cmd, store, dashSvc, func(sc *scenarioContext) { setUpACL() sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() assert.Equal(t, 403, sc.resp.Code) @@ -186,7 +189,8 @@ func TestAnnotationsAPIEndpoint(t *testing.T) { t.Run("When user is an Org Editor", func(t *testing.T) { role := org.RoleEditor t.Run("Should be able to save an annotation", func(t *testing.T) { - postAnnotationScenario(t, "When calling POST on", "/api/annotations", "/api/annotations", role, cmd, store, nil, func(sc *scenarioContext) { + dashSvc := dashboards.NewFakeDashboardService(t) + postAnnotationScenario(t, "When calling POST on", "/api/annotations", "/api/annotations", role, cmd, store, dashSvc, func(sc *scenarioContext) { setUpACL() sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() assert.Equal(t, 200, sc.resp.Code) @@ -220,12 +224,6 @@ func TestAnnotationsAPIEndpoint(t *testing.T) { mockStore := mockstore.NewSQLStoreMock() t.Run("Should be able to do anything", func(t *testing.T) { - postAnnotationScenario(t, "When calling POST on", "/api/annotations", "/api/annotations", role, cmd, store, nil, func(sc *scenarioContext) { - setUpACL() - sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() - assert.Equal(t, 200, sc.resp.Code) - }) - dashSvc := dashboards.NewFakeDashboardService(t) dashSvc.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { q := args.Get(1).(*models.GetDashboardQuery) @@ -234,6 +232,12 @@ func TestAnnotationsAPIEndpoint(t *testing.T) { Uid: q.Uid, } }).Return(nil) + postAnnotationScenario(t, "When calling POST on", "/api/annotations", "/api/annotations", role, cmd, store, dashSvc, func(sc *scenarioContext) { + setUpACL() + sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() + assert.Equal(t, 200, sc.resp.Code) + }) + postAnnotationScenario(t, "When calling POST on", "/api/annotations", "/api/annotations", role, dashboardUIDCmd, mockStore, dashSvc, func(sc *scenarioContext) { setUpACL() sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() @@ -387,7 +391,31 @@ func TestAPI_Annotations_AccessControl(t *testing.T) { _, err := sc.hs.orgService.CreateWithMember(context.Background(), &org.CreateOrgCommand{Name: "TestOrg", UserID: testUserID}) require.NoError(t, err) - dashboardAnnotation := &annotations.Item{Id: 1, DashboardId: 1} + origNewGuardian := guardian.New + t.Cleanup(func() { + guardian.New = origNewGuardian + }) + + // Create a dashboard + guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true}) + sc.dashboardPermissionsService.On("SetPermissions", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("string"), mock.Anything).Return([]accesscontrol.ResourcePermission{}, nil) + cmd := &dashboards.SaveDashboardDTO{ + OrgId: testOrgID, + User: &user.SignedInUser{UserID: testUserID, OrgID: testOrgID}, + Dashboard: &models.Dashboard{ + OrgId: testOrgID, + Title: "1 test dash", + Data: simplejson.NewFromAny(map[string]interface{}{}), + }} + dashboard, err := sc.hs.DashboardService.SaveDashboard(context.Background(), cmd, false) + require.NoError(t, err) + require.NotNil(t, dashboard) + t.Cleanup(func() { + err := sc.hs.DashboardService.DeleteDashboard(context.Background(), dashboard.Id, dashboard.OrgId) + require.NoError(t, err) + }) + + dashboardAnnotation := &annotations.Item{Id: 1, DashboardId: dashboard.Id} organizationAnnotation := &annotations.Item{Id: 2, DashboardId: 0} _ = sc.hs.annotationsRepo.Save(context.Background(), dashboardAnnotation) @@ -796,6 +824,30 @@ func TestAPI_MassDeleteAnnotations_AccessControl(t *testing.T) { method string } + origNewGuardian := guardian.New + t.Cleanup(func() { + guardian.New = origNewGuardian + }) + + // Create a dashboard + guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true}) + sc.dashboardPermissionsService.On("SetPermissions", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("string"), mock.Anything).Return([]accesscontrol.ResourcePermission{}, nil) + cmd := &dashboards.SaveDashboardDTO{ + OrgId: testOrgID, + User: &user.SignedInUser{UserID: testUserID, OrgID: testOrgID}, + Dashboard: &models.Dashboard{ + OrgId: testOrgID, + Title: "1 test dash", + Data: simplejson.NewFromAny(map[string]interface{}{}), + }} + dashboard, err := sc.hs.DashboardService.SaveDashboard(context.Background(), cmd, false) + require.NoError(t, err) + require.NotNil(t, dashboard) + t.Cleanup(func() { + err := sc.hs.DashboardService.DeleteDashboard(context.Background(), dashboard.Id, dashboard.OrgId) + require.NoError(t, err) + }) + tests := []struct { name string args args @@ -834,7 +886,7 @@ func TestAPI_MassDeleteAnnotations_AccessControl(t *testing.T) { url: "/api/annotations/mass-delete", method: http.MethodPost, body: mockRequestBody(dtos.MassDeleteAnnotationsCmd{ - DashboardId: 1, + DashboardId: dashboard.Id, PanelId: 1, }), }, @@ -932,6 +984,13 @@ func setUpACL() { {Role: &editorRole, Permission: models.PERMISSION_EDIT}, } }).Return(nil) + dashSvc.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { + q := args.Get(1).(*models.GetDashboardQuery) + q.Result = &models.Dashboard{ + Id: q.Id, + Uid: q.Uid, + } + }).Return(nil) guardian.InitLegacyGuardian(store, dashSvc, teamSvc) } diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index 10928bca1ed..4dec04f21ef 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -305,9 +305,11 @@ type accessControlScenarioContext struct { // cfg is the setting provider cfg *setting.Cfg - dashboardsStore dashboards.Store - teamService team.Service - userService user.Service + dashboardsStore dashboards.Store + teamService team.Service + userService user.Service + folderPermissionsService *accesscontrolmock.MockPermissionsService + dashboardPermissionsService *accesscontrolmock.MockPermissionsService } func setAccessControlPermissions(acmock *accesscontrolmock.Mock, perms []accesscontrol.Permission, org int64) { @@ -417,6 +419,9 @@ func setupHTTPServerWithCfgDb( teamPermissionService, err := ossaccesscontrol.ProvideTeamPermissions(cfg, routeRegister, db, ac, license, acService, teamService, userSvc) require.NoError(t, err) + folderPermissionsService := accesscontrolmock.NewMockedPermissionsService() + dashboardPermissionsService := accesscontrolmock.NewMockedPermissionsService() + // Create minimal HTTP Server hs := &HTTPServer{ Cfg: cfg, @@ -432,7 +437,7 @@ func setupHTTPServerWithCfgDb( searchUsersService: searchusers.ProvideUsersService(filters.ProvideOSSSearchUserFilter(), usertest.NewUserServiceFake()), DashboardService: dashboardservice.ProvideDashboardService( cfg, dashboardsStore, nil, features, - accesscontrolmock.NewMockedPermissionsService(), accesscontrolmock.NewMockedPermissionsService(), ac, + folderPermissionsService, dashboardPermissionsService, ac, ), preferenceService: preftest.NewPreferenceServiceFake(), userService: userSvc, @@ -469,15 +474,17 @@ func setupHTTPServerWithCfgDb( hs.RouteRegister.Register(m.Router) return accessControlScenarioContext{ - server: m, - initCtx: initCtx, - hs: hs, - acmock: acmock, - db: db, - cfg: cfg, - dashboardsStore: dashboardsStore, - teamService: teamService, - userService: userSvc, + server: m, + initCtx: initCtx, + hs: hs, + acmock: acmock, + db: db, + cfg: cfg, + dashboardsStore: dashboardsStore, + teamService: teamService, + userService: userSvc, + dashboardPermissionsService: dashboardPermissionsService, + folderPermissionsService: folderPermissionsService, } } diff --git a/pkg/api/dashboard.go b/pkg/api/dashboard.go index 42b2d976114..1dd8e021ece 100644 --- a/pkg/api/dashboard.go +++ b/pkg/api/dashboard.go @@ -133,7 +133,11 @@ func (hs *HTTPServer) GetDashboard(c *models.ReqContext) response.Response { return response.Error(500, "Error while loading dashboard, dashboard data is invalid", nil) } } - guardian := guardian.New(c.Req.Context(), dash.Id, c.OrgID, c.SignedInUser) + guardian, err := guardian.NewByDashboard(c.Req.Context(), dash, c.OrgID, c.SignedInUser) + if err != nil { + return response.Err(err) + } + if canView, err := guardian.CanView(); err != nil || !canView { return dashboardGuardianResponse(err) } @@ -308,13 +312,17 @@ func (hs *HTTPServer) deleteDashboard(c *models.ReqContext) response.Response { if rsp != nil { return rsp } - guardian := guardian.New(c.Req.Context(), dash.Id, c.OrgID, c.SignedInUser) + guardian, err := guardian.NewByDashboard(c.Req.Context(), dash, c.OrgID, c.SignedInUser) + if err != nil { + return response.Err(err) + } + if canDelete, err := guardian.CanDelete(); err != nil || !canDelete { return dashboardGuardianResponse(err) } // disconnect all library elements for this dashboard - err := hs.LibraryElementService.DisconnectElementsFromDashboard(c.Req.Context(), dash.Id) + err = hs.LibraryElementService.DisconnectElementsFromDashboard(c.Req.Context(), dash.Id) if err != nil { hs.log.Error("Failed to disconnect library elements", "dashboard", dash.Id, "user", c.SignedInUser.UserID, "error", err) } @@ -630,32 +638,32 @@ func (hs *HTTPServer) GetDashboardVersions(c *models.ReqContext) response.Respon if err != nil { return response.Error(http.StatusBadRequest, "dashboardId is invalid", err) } - } else { - q := models.GetDashboardQuery{ - OrgId: c.SignedInUser.OrgID, - Uid: dashUID, - } - if err := hs.DashboardService.GetDashboard(c.Req.Context(), &q); err != nil { - return response.Error(http.StatusBadRequest, "failed to get dashboard by UID", err) - } - dashID = q.Result.Id } - guardian := guardian.New(c.Req.Context(), dashID, c.OrgID, c.SignedInUser) + + dash, rsp := hs.getDashboardHelper(c.Req.Context(), c.OrgID, dashID, dashUID) + if rsp != nil { + return rsp + } + + guardian, err := guardian.NewByDashboard(c.Req.Context(), dash, c.OrgID, c.SignedInUser) + if err != nil { + return response.Err(err) + } if canSave, err := guardian.CanSave(); err != nil || !canSave { return dashboardGuardianResponse(err) } query := dashver.ListDashboardVersionsQuery{ OrgID: c.OrgID, - DashboardID: dashID, - DashboardUID: dashUID, + DashboardID: dash.Id, + DashboardUID: dash.Uid, Limit: c.QueryInt("limit"), Start: c.QueryInt("start"), } res, err := hs.dashboardVersionService.List(c.Req.Context(), &query) if err != nil { - return response.Error(404, fmt.Sprintf("No versions found for dashboardId %d", dashID), err) + return response.Error(404, fmt.Sprintf("No versions found for dashboardId %d", dash.Id), err) } for _, version := range res { @@ -708,23 +716,24 @@ func (hs *HTTPServer) GetDashboardVersion(c *models.ReqContext) response.Respons var err error dashUID := web.Params(c.Req)[":uid"] + var dash *models.Dashboard if dashUID == "" { dashID, err = strconv.ParseInt(web.Params(c.Req)[":dashboardId"], 10, 64) if err != nil { return response.Error(http.StatusBadRequest, "dashboardId is invalid", err) } - } else { - q := models.GetDashboardQuery{ - OrgId: c.SignedInUser.OrgID, - Uid: dashUID, - } - if err := hs.DashboardService.GetDashboard(c.Req.Context(), &q); err != nil { - return response.Error(http.StatusBadRequest, "failed to get dashboard by UID", err) - } - dashID = q.Result.Id } - guardian := guardian.New(c.Req.Context(), dashID, c.OrgID, c.SignedInUser) + dash, rsp := hs.getDashboardHelper(c.Req.Context(), c.OrgID, dashID, dashUID) + if rsp != nil { + return rsp + } + + guardian, err := guardian.NewByDashboard(c.Req.Context(), dash, c.OrgID, c.SignedInUser) + if err != nil { + return response.Err(err) + } + if canSave, err := guardian.CanSave(); err != nil || !canSave { return dashboardGuardianResponse(err) } @@ -732,13 +741,13 @@ func (hs *HTTPServer) GetDashboardVersion(c *models.ReqContext) response.Respons version, _ := strconv.ParseInt(web.Params(c.Req)[":id"], 10, 32) query := dashver.GetDashboardVersionQuery{ OrgID: c.OrgID, - DashboardID: dashID, + DashboardID: dash.Id, Version: int(version), } res, err := hs.dashboardVersionService.Get(c.Req.Context(), &query) if err != nil { - return response.Error(500, fmt.Sprintf("Dashboard version %d not found for dashboardId %d", query.Version, dashID), err) + return response.Error(500, fmt.Sprintf("Dashboard version %d not found for dashboardId %d", query.Version, dash.Id), err) } creator := anonString @@ -843,13 +852,21 @@ func (hs *HTTPServer) CalculateDashboardDiff(c *models.ReqContext) response.Resp if err := web.Bind(c.Req, &apiOptions); err != nil { return response.Error(http.StatusBadRequest, "bad request data", err) } - guardianBase := guardian.New(c.Req.Context(), apiOptions.Base.DashboardId, c.OrgID, c.SignedInUser) + guardianBase, err := guardian.New(c.Req.Context(), apiOptions.Base.DashboardId, c.OrgID, c.SignedInUser) + if err != nil { + return response.Err(err) + } + if canSave, err := guardianBase.CanSave(); err != nil || !canSave { return dashboardGuardianResponse(err) } if apiOptions.Base.DashboardId != apiOptions.New.DashboardId { - guardianNew := guardian.New(c.Req.Context(), apiOptions.New.DashboardId, c.OrgID, c.SignedInUser) + guardianNew, err := guardian.New(c.Req.Context(), apiOptions.New.DashboardId, c.OrgID, c.SignedInUser) + if err != nil { + return response.Err(err) + } + if canSave, err := guardianNew.CanSave(); err != nil || !canSave { return dashboardGuardianResponse(err) } @@ -964,11 +981,11 @@ func (hs *HTTPServer) RestoreDashboardVersion(c *models.ReqContext) response.Res return rsp } - if dash != nil && dash.Id != 0 { - dashID = dash.Id + guardian, err := guardian.NewByDashboard(c.Req.Context(), dash, c.OrgID, c.SignedInUser) + if err != nil { + return response.Err(err) } - guardian := guardian.New(c.Req.Context(), dashID, c.OrgID, c.SignedInUser) if canSave, err := guardian.CanSave(); err != nil || !canSave { return dashboardGuardianResponse(err) } diff --git a/pkg/api/dashboard_permission.go b/pkg/api/dashboard_permission.go index eccae6871b0..7307bc940aa 100644 --- a/pkg/api/dashboard_permission.go +++ b/pkg/api/dashboard_permission.go @@ -56,12 +56,11 @@ func (hs *HTTPServer) GetDashboardPermissionList(c *models.ReqContext) response. return rsp } - if dashID == 0 { - dashID = dash.Id + g, err := guardian.NewByDashboard(c.Req.Context(), dash, c.OrgID, c.SignedInUser) + if err != nil { + return response.Err(err) } - g := guardian.New(c.Req.Context(), dashID, c.OrgID, c.SignedInUser) - if canAdmin, err := g.CanAdmin(); err != nil || !canAdmin { return dashboardGuardianResponse(err) } @@ -147,11 +146,11 @@ func (hs *HTTPServer) UpdateDashboardPermissions(c *models.ReqContext) response. return rsp } - if dashUID != "" { - dashID = dash.Id + g, err := guardian.NewByDashboard(c.Req.Context(), dash, c.OrgID, c.SignedInUser) + if err != nil { + return response.Err(err) } - g := guardian.New(c.Req.Context(), dashID, c.OrgID, c.SignedInUser) if canAdmin, err := g.CanAdmin(); err != nil || !canAdmin { return dashboardGuardianResponse(err) } diff --git a/pkg/api/dashboard_permission_test.go b/pkg/api/dashboard_permission_test.go index 78597574522..3b0ec28cb8a 100644 --- a/pkg/api/dashboard_permission_test.go +++ b/pkg/api/dashboard_permission_test.go @@ -27,7 +27,13 @@ func TestDashboardPermissionAPIEndpoint(t *testing.T) { t.Run("Dashboard permissions test", func(t *testing.T) { settings := setting.NewCfg() dashboardStore := &dashboards.FakeDashboardStore{} - dashboardStore.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Return(nil, nil) + dashboardStore.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { + q := args.Get(1).(*models.GetDashboardQuery) + q.Result = &models.Dashboard{ + Id: q.Id, + Uid: q.Uid, + } + }).Return(nil, nil) defer dashboardStore.AssertExpectations(t) features := featuremgmt.WithFeatures() diff --git a/pkg/api/dashboard_snapshot.go b/pkg/api/dashboard_snapshot.go index 47ae50544ab..5bb65285c90 100644 --- a/pkg/api/dashboard_snapshot.go +++ b/pkg/api/dashboard_snapshot.go @@ -342,7 +342,11 @@ func (hs *HTTPServer) DeleteDashboardSnapshot(c *models.ReqContext) response.Res dashboardID := query.Result.Dashboard.Get("id").MustInt64() if dashboardID != 0 { - guardian := guardian.New(c.Req.Context(), dashboardID, c.OrgID, c.SignedInUser) + guardian, err := guardian.New(c.Req.Context(), dashboardID, c.OrgID, c.SignedInUser) + if err != nil { + return response.Err(err) + } + canEdit, err := guardian.CanEdit() // check for permissions only if the dashboard is found if err != nil && !errors.Is(err, dashboards.ErrDashboardNotFound) { diff --git a/pkg/api/dashboard_snapshot_test.go b/pkg/api/dashboard_snapshot_test.go index f0ea1ffaba5..f560bd77670 100644 --- a/pkg/api/dashboard_snapshot_test.go +++ b/pkg/api/dashboard_snapshot_test.go @@ -73,7 +73,15 @@ func TestDashboardSnapshotAPIEndpoint_singleSnapshot(t *testing.T) { teamSvc := &teamtest.FakeService{} dashSvc := dashboards.NewFakeDashboardService(t) + dashSvc.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { + q := args.Get(1).(*models.GetDashboardQuery) + q.Result = &models.Dashboard{ + Id: q.Id, + Uid: q.Uid, + } + }).Return(nil).Maybe() dashSvc.On("GetDashboardACLInfoList", mock.Anything, mock.AnythingOfType("*models.GetDashboardACLInfoListQuery")).Return(nil).Maybe() + hs.DashboardService = dashSvc guardian.InitLegacyGuardian(sc.sqlStore, dashSvc, teamSvc) sc.fakeReqWithParams("DELETE", sc.url, map[string]string{"key": "12345"}).exec() @@ -121,13 +129,28 @@ func TestDashboardSnapshotAPIEndpoint_singleSnapshot(t *testing.T) { loggedInUserScenarioWithRole(t, "Should be able to delete a snapshot when calling DELETE on", "DELETE", "/api/snapshots/12345", "/api/snapshots/:key", org.RoleEditor, func(sc *scenarioContext) { - guardian.InitLegacyGuardian(sc.sqlStore, dashSvc, teamSvc) var externalRequest *http.Request ts := setupRemoteServer(func(rw http.ResponseWriter, req *http.Request) { rw.WriteHeader(200) externalRequest = req }) - hs := &HTTPServer{dashboardsnapshotsService: setUpSnapshotTest(t, 0, ts.URL)} + dashSvc := dashboards.NewFakeDashboardService(t) + dashSvc.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { + q := args.Get(1).(*models.GetDashboardQuery) + q.Result = &models.Dashboard{ + Id: q.Id, + OrgId: q.OrgId, + } + }).Return(nil).Maybe() + dashSvc.On("GetDashboardACLInfoList", mock.Anything, mock.AnythingOfType("*models.GetDashboardACLInfoListQuery")).Run(func(args mock.Arguments) { + q := args.Get(1).(*models.GetDashboardACLInfoListQuery) + q.Result = []*models.DashboardACLInfoDTO{ + {Role: &viewerRole, Permission: models.PERMISSION_VIEW}, + {Role: &editorRole, Permission: models.PERMISSION_EDIT}, + } + }).Return(nil) + guardian.InitLegacyGuardian(sc.sqlStore, dashSvc, teamSvc) + hs := &HTTPServer{dashboardsnapshotsService: setUpSnapshotTest(t, 0, ts.URL), DashboardService: dashSvc} sc.handlerFunc = hs.DeleteDashboardSnapshot sc.fakeReqWithParams("DELETE", sc.url, map[string]string{"key": "12345"}).exec() @@ -146,8 +169,9 @@ func TestDashboardSnapshotAPIEndpoint_singleSnapshot(t *testing.T) { loggedInUserScenarioWithRole(t, "Should be able to delete a snapshot when calling DELETE on", "DELETE", "/api/snapshots/12345", "/api/snapshots/:key", org.RoleEditor, func(sc *scenarioContext) { d := setUpSnapshotTest(t, testUserID, "") - hs := &HTTPServer{dashboardsnapshotsService: d} + dashSvc := dashboards.NewFakeDashboardService(t) + hs := &HTTPServer{dashboardsnapshotsService: d, DashboardService: dashSvc} sc.handlerFunc = hs.DeleteDashboardSnapshot sc.fakeReqWithParams("DELETE", sc.url, map[string]string{"key": "12345"}).exec() @@ -169,7 +193,9 @@ func TestDashboardSnapshotAPIEndpoint_singleSnapshot(t *testing.T) { rw.WriteHeader(500) _, writeErr = rw.Write([]byte(`{"message":"Failed to get dashboard snapshot"}`)) }) - hs := &HTTPServer{dashboardsnapshotsService: setUpSnapshotTest(t, testUserID, ts.URL)} + + dashSvc := dashboards.NewFakeDashboardService(t) + hs := &HTTPServer{dashboardsnapshotsService: setUpSnapshotTest(t, testUserID, ts.URL), DashboardService: dashSvc} sc.handlerFunc = hs.DeleteDashboardSnapshot sc.fakeReqWithParams("DELETE", sc.url, map[string]string{"key": "12345"}).exec() diff --git a/pkg/api/dashboard_test.go b/pkg/api/dashboard_test.go index e4f45cb0a43..43b877f1e3c 100644 --- a/pkg/api/dashboard_test.go +++ b/pkg/api/dashboard_test.go @@ -808,6 +808,13 @@ func TestDashboardAPIEndpoint(t *testing.T) { teamSvc := &teamtest.FakeService{} dashSvc := dashboards.NewFakeDashboardService(t) dashSvc.On("GetDashboardACLInfoList", mock.Anything, mock.AnythingOfType("*models.GetDashboardACLInfoListQuery")).Return(nil) + dashSvc.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { + q := args.Get(1).(*models.GetDashboardQuery) + q.Result = &models.Dashboard{ + OrgId: q.OrgId, + Id: q.Id, + } + }).Return(nil) guardian.InitLegacyGuardian(&sqlmock, dashSvc, teamSvc) } @@ -1152,6 +1159,8 @@ func postDiffScenario(t *testing.T, desc string, url string, routePattern string role org.RoleType, fn scenarioFunc, sqlmock db.DB, fakeDashboardVersionService *dashvertest.FakeDashboardVersionService) { t.Run(fmt.Sprintf("%s %s", desc, url), func(t *testing.T) { cfg := setting.NewCfg() + + dashSvc := dashboards.NewFakeDashboardService(t) hs := HTTPServer{ Cfg: cfg, ProvisioningService: provisioning.NewProvisioningServiceMock(context.Background()), @@ -1163,6 +1172,7 @@ func postDiffScenario(t *testing.T, desc string, url string, routePattern string dashboardVersionService: fakeDashboardVersionService, Features: featuremgmt.WithFeatures(), Kinds: corekind.NewBase(nil), + DashboardService: dashSvc, } sc := setupScenarioContext(t, url) diff --git a/pkg/api/folder.go b/pkg/api/folder.go index 688222c3474..74dd028b0b9 100644 --- a/pkg/api/folder.go +++ b/pkg/api/folder.go @@ -73,7 +73,11 @@ func (hs *HTTPServer) GetFolderByUID(c *models.ReqContext) response.Response { return apierrors.ToFolderErrorResponse(err) } - g := guardian.New(c.Req.Context(), folder.ID, c.OrgID, c.SignedInUser) + g, err := guardian.NewByUID(c.Req.Context(), folder.UID, c.OrgID, c.SignedInUser) + if err != nil { + return response.Err(err) + } + return response.JSON(http.StatusOK, hs.newToFolderDto(c, g, folder)) } @@ -99,7 +103,10 @@ func (hs *HTTPServer) GetFolderByID(c *models.ReqContext) response.Response { return apierrors.ToFolderErrorResponse(err) } - g := guardian.New(c.Req.Context(), folder.ID, c.OrgID, c.SignedInUser) + g, err := guardian.NewByUID(c.Req.Context(), folder.UID, c.OrgID, c.SignedInUser) + if err != nil { + return response.Err(err) + } return response.JSON(http.StatusOK, hs.newToFolderDto(c, g, folder)) } @@ -135,7 +142,11 @@ func (hs *HTTPServer) CreateFolder(c *models.ReqContext) response.Response { hs.accesscontrolService.ClearUserPermissionCache(c.SignedInUser) } - g := guardian.New(c.Req.Context(), folder.ID, c.OrgID, c.SignedInUser) + g, err := guardian.NewByUID(c.Req.Context(), folder.UID, c.OrgID, c.SignedInUser) + if err != nil { + return response.Err(err) + } + // TODO set ParentUID if nested folders are enabled return response.JSON(http.StatusOK, hs.newToFolderDto(c, g, folder)) } @@ -190,7 +201,11 @@ func (hs *HTTPServer) UpdateFolder(c *models.ReqContext) response.Response { if err != nil { return apierrors.ToFolderErrorResponse(err) } - g := guardian.New(c.Req.Context(), result.ID, c.OrgID, c.SignedInUser) + g, err := guardian.NewByUID(c.Req.Context(), result.UID, c.OrgID, c.SignedInUser) + if err != nil { + return response.Err(err) + } + return response.JSON(http.StatusOK, hs.newToFolderDto(c, g, result)) } diff --git a/pkg/api/folder_permission.go b/pkg/api/folder_permission.go index c48dda8f108..850e5281c74 100644 --- a/pkg/api/folder_permission.go +++ b/pkg/api/folder_permission.go @@ -34,7 +34,10 @@ func (hs *HTTPServer) GetFolderPermissionList(c *models.ReqContext) response.Res return apierrors.ToFolderErrorResponse(err) } - g := guardian.New(c.Req.Context(), folder.ID, c.OrgID, c.SignedInUser) + g, err := guardian.New(c.Req.Context(), folder.ID, c.OrgID, c.SignedInUser) + if err != nil { + return response.Err(err) + } if canAdmin, err := g.CanAdmin(); err != nil || !canAdmin { return apierrors.ToFolderErrorResponse(dashboards.ErrFolderAccessDenied) @@ -95,7 +98,11 @@ func (hs *HTTPServer) UpdateFolderPermissions(c *models.ReqContext) response.Res return apierrors.ToFolderErrorResponse(err) } - g := guardian.New(c.Req.Context(), folder.ID, c.OrgID, c.SignedInUser) + g, err := guardian.New(c.Req.Context(), folder.ID, c.OrgID, c.SignedInUser) + if err != nil { + return response.Err(err) + } + canAdmin, err := g.CanAdmin() if err != nil { return apierrors.ToFolderErrorResponse(err) diff --git a/pkg/api/folder_test.go b/pkg/api/folder_test.go index 777fe6a281e..b1ae041e5ff 100644 --- a/pkg/api/folder_test.go +++ b/pkg/api/folder_test.go @@ -240,6 +240,13 @@ func createFolderScenario(t *testing.T, desc string, url string, routePattern st q := args.Get(1).(*models.GetDashboardACLInfoListQuery) q.Result = aclMockResp }).Return(nil) + dashSvc.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { + q := args.Get(1).(*models.GetDashboardQuery) + q.Result = &models.Dashboard{ + Id: q.Id, + Uid: q.Uid, + } + }).Return(nil) store := mockstore.NewSQLStoreMock() guardian.InitLegacyGuardian(store, dashSvc, teamSvc) hs := HTTPServer{ diff --git a/pkg/models/dashboards.go b/pkg/models/dashboards.go index a89f451e007..d4aabc68e6c 100644 --- a/pkg/models/dashboards.go +++ b/pkg/models/dashboards.go @@ -54,15 +54,6 @@ func (d *Dashboard) SetVersion(version int) { d.Data.Set("version", version) } -// GetDashboardIdForSavePermissionCheck return the dashboard id to be used for checking permission of dashboard -func (d *Dashboard) GetDashboardIdForSavePermissionCheck() int64 { - if d.Id == 0 { - return d.FolderId - } - - return d.Id -} - // NewDashboard creates a new dashboard func NewDashboard(title string) *Dashboard { dash := &Dashboard{} diff --git a/pkg/services/comments/commentmodel/permissions.go b/pkg/services/comments/commentmodel/permissions.go index 5756ca081a3..a57d19645e8 100644 --- a/pkg/services/comments/commentmodel/permissions.go +++ b/pkg/services/comments/commentmodel/permissions.go @@ -57,7 +57,10 @@ func (c *PermissionChecker) CheckReadPermissions(ctx context.Context, orgId int6 if err != nil { return false, err } - guard := guardian.New(ctx, dash.Id, orgId, signedInUser) + guard, err := guardian.NewByDashboard(ctx, dash, orgId, signedInUser) + if err != nil { + return false, err + } if ok, err := guard.CanView(); err != nil || !ok { return false, nil } @@ -81,7 +84,10 @@ func (c *PermissionChecker) CheckReadPermissions(ctx context.Context, orgId int6 if err != nil { return false, err } - guard := guardian.New(ctx, dash.Id, orgId, signedInUser) + guard, err := guardian.NewByDashboard(ctx, dash, orgId, signedInUser) + if err != nil { + return false, err + } if ok, err := guard.CanView(); err != nil || !ok { return false, nil } @@ -103,7 +109,10 @@ func (c *PermissionChecker) CheckWritePermissions(ctx context.Context, orgId int if err != nil { return false, err } - guard := guardian.New(ctx, dash.Id, orgId, signedInUser) + guard, err := guardian.NewByDashboard(ctx, dash, orgId, signedInUser) + if err != nil { + return false, err + } if ok, err := guard.CanEdit(); err != nil || !ok { return false, nil } @@ -133,7 +142,10 @@ func (c *PermissionChecker) CheckWritePermissions(ctx context.Context, orgId int if err != nil { return false, nil } - guard := guardian.New(ctx, dash.Id, orgId, signedInUser) + guard, err := guardian.NewByDashboard(ctx, dash, orgId, signedInUser) + if err != nil { + return false, err + } if ok, err := guard.CanEdit(); err != nil || !ok { return false, nil } diff --git a/pkg/services/dashboards/service/dashboard_service.go b/pkg/services/dashboards/service/dashboard_service.go index 881f2bc09ab..8a3f7ebf55b 100644 --- a/pkg/services/dashboards/service/dashboard_service.go +++ b/pkg/services/dashboards/service/dashboard_service.go @@ -17,6 +17,7 @@ import ( "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/services/org" + "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" ) @@ -120,7 +121,10 @@ func (dr *DashboardServiceImpl) BuildSaveDashboardCommand(ctx context.Context, d if isParentFolderChanged { // Check that the user is allowed to add a dashboard to the folder - guardian := guardian.New(ctx, dash.Id, dto.OrgId, dto.User) + guardian, err := guardian.NewByDashboard(ctx, dash, dto.OrgId, dto.User) + if err != nil { + return nil, err + } if canSave, err := guardian.CanCreate(dash.FolderId, dash.IsFolder); err != nil || !canSave { if err != nil { return nil, err @@ -140,7 +144,11 @@ func (dr *DashboardServiceImpl) BuildSaveDashboardCommand(ctx context.Context, d } } - guard := guardian.New(ctx, dash.GetDashboardIdForSavePermissionCheck(), dto.OrgId, dto.User) + guard, err := getGuardianForSavePermissionCheck(ctx, dash, dto.User) + if err != nil { + return nil, err + } + if dash.Id == 0 { if canCreate, err := guard.CanCreate(dash.FolderId, dash.IsFolder); err != nil || !canCreate { if err != nil { @@ -183,6 +191,26 @@ func (dr *DashboardServiceImpl) DeleteOrphanedProvisionedDashboards(ctx context. return dr.dashboardStore.DeleteOrphanedProvisionedDashboards(ctx, cmd) } +// getGuardianForSavePermissionCheck returns the guardian to be used for checking permission of dashboard +// It replaces deleted Dashboard.GetDashboardIdForSavePermissionCheck() +func getGuardianForSavePermissionCheck(ctx context.Context, d *models.Dashboard, user *user.SignedInUser) (guardian.DashboardGuardian, error) { + newDashboard := d.Id == 0 + + if newDashboard { + // if it's a new dashboard/folder check the parent folder permissions + guard, err := guardian.New(ctx, d.FolderId, d.OrgId, user) + if err != nil { + return nil, err + } + return guard, nil + } + guard, err := guardian.NewByDashboard(ctx, d, d.OrgId, user) + if err != nil { + return nil, err + } + return guard, nil +} + func validateDashboardRefreshInterval(dash *models.Dashboard) error { if setting.MinRefreshInterval == "" { return nil diff --git a/pkg/services/dashboards/service/dashboard_service_integration_test.go b/pkg/services/dashboards/service/dashboard_service_integration_test.go index 210085b565d..e70e4f391c2 100644 --- a/pkg/services/dashboards/service/dashboard_service_integration_test.go +++ b/pkg/services/dashboards/service/dashboard_service_integration_test.go @@ -108,7 +108,7 @@ func TestIntegrationIntegratedDashboardService(t *testing.T) { err := callSaveWithError(t, cmd, sqlStore) assert.Equal(t, dashboards.ErrDashboardUpdateAccessDenied, err) - assert.Equal(t, int64(0), sc.dashboardGuardianMock.DashId) + assert.Equal(t, "", sc.dashboardGuardianMock.DashUID) assert.Equal(t, cmd.OrgId, sc.dashboardGuardianMock.OrgId) assert.Equal(t, cmd.UserId, sc.dashboardGuardianMock.User.UserID) }) @@ -128,7 +128,7 @@ func TestIntegrationIntegratedDashboardService(t *testing.T) { err := callSaveWithError(t, cmd, sc.sqlStore) require.Equal(t, dashboards.ErrDashboardUpdateAccessDenied, err) - assert.Equal(t, sc.otherSavedFolder.Id, sc.dashboardGuardianMock.DashId) + assert.Equal(t, sc.otherSavedFolder.Id, sc.dashboardGuardianMock.DashID) assert.Equal(t, cmd.OrgId, sc.dashboardGuardianMock.OrgId) assert.Equal(t, cmd.UserId, sc.dashboardGuardianMock.User.UserID) }) @@ -148,7 +148,7 @@ func TestIntegrationIntegratedDashboardService(t *testing.T) { err := callSaveWithError(t, cmd, sc.sqlStore) require.Equal(t, dashboards.ErrDashboardUpdateAccessDenied, err) - assert.Equal(t, sc.savedDashInFolder.Id, sc.dashboardGuardianMock.DashId) + assert.Equal(t, sc.savedDashInFolder.Uid, sc.dashboardGuardianMock.DashUID) assert.Equal(t, cmd.OrgId, sc.dashboardGuardianMock.OrgId) assert.Equal(t, cmd.UserId, sc.dashboardGuardianMock.User.UserID) }) @@ -169,7 +169,7 @@ func TestIntegrationIntegratedDashboardService(t *testing.T) { err := callSaveWithError(t, cmd, sc.sqlStore) require.Equal(t, dashboards.ErrDashboardUpdateAccessDenied, err) - assert.Equal(t, sc.savedDashInFolder.Id, sc.dashboardGuardianMock.DashId) + assert.Equal(t, sc.savedDashInFolder.Uid, sc.dashboardGuardianMock.DashUID) assert.Equal(t, cmd.OrgId, sc.dashboardGuardianMock.OrgId) assert.Equal(t, cmd.UserId, sc.dashboardGuardianMock.User.UserID) }) @@ -190,7 +190,7 @@ func TestIntegrationIntegratedDashboardService(t *testing.T) { err := callSaveWithError(t, cmd, sc.sqlStore) assert.Equal(t, dashboards.ErrDashboardUpdateAccessDenied, err) - assert.Equal(t, sc.savedDashInGeneralFolder.Id, sc.dashboardGuardianMock.DashId) + assert.Equal(t, sc.savedDashInGeneralFolder.Uid, sc.dashboardGuardianMock.DashUID) assert.Equal(t, cmd.OrgId, sc.dashboardGuardianMock.OrgId) assert.Equal(t, cmd.UserId, sc.dashboardGuardianMock.User.UserID) }) @@ -211,7 +211,7 @@ func TestIntegrationIntegratedDashboardService(t *testing.T) { err := callSaveWithError(t, cmd, sc.sqlStore) require.Equal(t, dashboards.ErrDashboardUpdateAccessDenied, err) - assert.Equal(t, sc.savedDashInFolder.Id, sc.dashboardGuardianMock.DashId) + assert.Equal(t, sc.savedDashInFolder.Uid, sc.dashboardGuardianMock.DashUID) assert.Equal(t, cmd.OrgId, sc.dashboardGuardianMock.OrgId) assert.Equal(t, cmd.UserId, sc.dashboardGuardianMock.User.UserID) }) @@ -232,7 +232,7 @@ func TestIntegrationIntegratedDashboardService(t *testing.T) { err := callSaveWithError(t, cmd, sc.sqlStore) require.Equal(t, dashboards.ErrDashboardUpdateAccessDenied, err) - assert.Equal(t, sc.savedDashInGeneralFolder.Id, sc.dashboardGuardianMock.DashId) + assert.Equal(t, sc.savedDashInGeneralFolder.Uid, sc.dashboardGuardianMock.DashUID) assert.Equal(t, cmd.OrgId, sc.dashboardGuardianMock.OrgId) assert.Equal(t, cmd.UserId, sc.dashboardGuardianMock.User.UserID) }) @@ -253,7 +253,7 @@ func TestIntegrationIntegratedDashboardService(t *testing.T) { err := callSaveWithError(t, cmd, sc.sqlStore) assert.Equal(t, dashboards.ErrDashboardUpdateAccessDenied, err) - assert.Equal(t, sc.savedDashInFolder.Id, sc.dashboardGuardianMock.DashId) + assert.Equal(t, sc.savedDashInFolder.Uid, sc.dashboardGuardianMock.DashUID) assert.Equal(t, cmd.OrgId, sc.dashboardGuardianMock.OrgId) assert.Equal(t, cmd.UserId, sc.dashboardGuardianMock.User.UserID) }) @@ -274,7 +274,7 @@ func TestIntegrationIntegratedDashboardService(t *testing.T) { err := callSaveWithError(t, cmd, sc.sqlStore) require.Equal(t, dashboards.ErrDashboardUpdateAccessDenied, err) - assert.Equal(t, sc.savedDashInGeneralFolder.Id, sc.dashboardGuardianMock.DashId) + assert.Equal(t, sc.savedDashInGeneralFolder.Uid, sc.dashboardGuardianMock.DashUID) assert.Equal(t, cmd.OrgId, sc.dashboardGuardianMock.OrgId) assert.Equal(t, cmd.UserId, sc.dashboardGuardianMock.User.UserID) }) @@ -295,7 +295,7 @@ func TestIntegrationIntegratedDashboardService(t *testing.T) { err := callSaveWithError(t, cmd, sc.sqlStore) require.Equal(t, dashboards.ErrDashboardUpdateAccessDenied, err) - assert.Equal(t, sc.savedDashInFolder.Id, sc.dashboardGuardianMock.DashId) + assert.Equal(t, sc.savedDashInFolder.Uid, sc.dashboardGuardianMock.DashUID) assert.Equal(t, cmd.OrgId, sc.dashboardGuardianMock.OrgId) assert.Equal(t, cmd.UserId, sc.dashboardGuardianMock.User.UserID) }) diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index 56cc6e4a78b..338c5008ca9 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/pkg/services/folder/folderimpl/folder.go @@ -104,7 +104,14 @@ func (s *Service) Get(ctx context.Context, cmd *folder.GetFolderQuery) (*folder. return nil, err } - g := guardian.New(ctx, f.ID, f.OrgID, cmd.SignedInUser) + // do not get guardian by the folder ID because it differs from the nested folder ID + // and the legacy folder ID has been associated with the permissions: + // use the folde UID instead that is the same for both + g, err := guardian.NewByUID(ctx, f.UID, f.OrgID, cmd.SignedInUser) + if err != nil { + return nil, err + } + if canView, err := g.CanView(); err != nil || !canView { if err != nil { return nil, toFolderError(err) @@ -166,7 +173,14 @@ func (s *Service) getFolderByID(ctx context.Context, user *user.SignedInUser, id return nil, err } - g := guardian.New(ctx, dashFolder.ID, orgID, user) + // do not get guardian by the folder ID because it differs from the nested folder ID + // and the legacy folder ID has been associated with the permissions: + // use the folde UID instead that is the same for both + g, err := guardian.NewByUID(ctx, dashFolder.UID, orgID, user) + if err != nil { + return nil, err + } + if canView, err := g.CanView(); err != nil || !canView { if err != nil { return nil, toFolderError(err) @@ -183,7 +197,14 @@ func (s *Service) getFolderByUID(ctx context.Context, user *user.SignedInUser, o return nil, err } - g := guardian.New(ctx, dashFolder.ID, orgID, user) + // do not get guardian by the folder ID because it differs from the nested folder ID + // and the legacy folder ID has been associated with the permissions: + // use the folde UID instead that is the same for both + g, err := guardian.NewByUID(ctx, dashFolder.UID, orgID, user) + if err != nil { + return nil, err + } + if canView, err := g.CanView(); err != nil || !canView { if err != nil { return nil, toFolderError(err) @@ -200,7 +221,11 @@ func (s *Service) getFolderByTitle(ctx context.Context, user *user.SignedInUser, return nil, err } - g := guardian.New(ctx, dashFolder.ID, orgID, user) + g, err := guardian.NewByUID(ctx, dashFolder.UID, orgID, user) + if err != nil { + return nil, err + } + if canView, err := g.CanView(); err != nil || !canView { if err != nil { return nil, toFolderError(err) @@ -426,7 +451,11 @@ func (s *Service) DeleteFolder(ctx context.Context, cmd *folder.DeleteFolderComm return err } - guard := guardian.New(ctx, dashFolder.ID, cmd.OrgID, cmd.SignedInUser) + guard, err := guardian.NewByUID(ctx, dashFolder.UID, cmd.OrgID, cmd.SignedInUser) + if err != nil { + return err + } + if canSave, err := guard.CanDelete(); err != nil || !canSave { if err != nil { return toFolderError(err) diff --git a/pkg/services/guardian/accesscontrol_guardian.go b/pkg/services/guardian/accesscontrol_guardian.go index d781ab8d0fd..e2f1018dae3 100644 --- a/pkg/services/guardian/accesscontrol_guardian.go +++ b/pkg/services/guardian/accesscontrol_guardian.go @@ -2,6 +2,7 @@ package guardian import ( "context" + "errors" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" @@ -21,30 +22,106 @@ var permissionMap = map[string]models.PermissionType{ var _ DashboardGuardian = new(AccessControlDashboardGuardian) +// NewAccessControlDashboardGuardianByDashboard creates a dashboard guardian by the provided dashboardId. func NewAccessControlDashboardGuardian( ctx context.Context, dashboardId int64, user *user.SignedInUser, store db.DB, ac accesscontrol.AccessControl, folderPermissionsService accesscontrol.FolderPermissionsService, dashboardPermissionsService accesscontrol.DashboardPermissionsService, dashboardService dashboards.DashboardService, -) *AccessControlDashboardGuardian { +) (*AccessControlDashboardGuardian, error) { + var dashboard *models.Dashboard + if dashboardId != 0 { + q := &models.GetDashboardQuery{ + Id: dashboardId, + OrgId: user.OrgID, + } + + if err := dashboardService.GetDashboard(ctx, q); err != nil { + if errors.Is(err, dashboards.ErrDashboardNotFound) { + return nil, ErrGuardianDashboardNotFound.Errorf("failed to get dashboard by UID: %w", err) + } + return nil, ErrGuardianGetDashboardFailure.Errorf("failed to get dashboard by UID: %w", err) + } + dashboard = q.Result + } + return &AccessControlDashboardGuardian{ ctx: ctx, log: log.New("dashboard.permissions"), - dashboardID: dashboardId, + dashboard: dashboard, user: user, store: store, ac: ac, folderPermissionsService: folderPermissionsService, dashboardPermissionsService: dashboardPermissionsService, dashboardService: dashboardService, + }, nil +} + +// NewAccessControlDashboardGuardianByDashboard creates a dashboard guardian by the provided dashboardUID. +func NewAccessControlDashboardGuardianByUID( + ctx context.Context, dashboardUID string, user *user.SignedInUser, + store db.DB, ac accesscontrol.AccessControl, + folderPermissionsService accesscontrol.FolderPermissionsService, + dashboardPermissionsService accesscontrol.DashboardPermissionsService, + dashboardService dashboards.DashboardService, +) (*AccessControlDashboardGuardian, error) { + var dashboard *models.Dashboard + if dashboardUID != "" { + q := &models.GetDashboardQuery{ + Uid: dashboardUID, + OrgId: user.OrgID, + } + + if err := dashboardService.GetDashboard(ctx, q); err != nil { + if errors.Is(err, dashboards.ErrDashboardNotFound) { + return nil, ErrGuardianDashboardNotFound.Errorf("failed to get dashboard by UID: %w", err) + } + return nil, ErrGuardianGetDashboardFailure.Errorf("failed to get dashboard by UID: %w", err) + } + dashboard = q.Result } + + return &AccessControlDashboardGuardian{ + ctx: ctx, + log: log.New("dashboard.permissions"), + dashboard: dashboard, + user: user, + store: store, + ac: ac, + folderPermissionsService: folderPermissionsService, + dashboardPermissionsService: dashboardPermissionsService, + dashboardService: dashboardService, + }, nil +} + +// NewAccessControlDashboardGuardianByDashboard creates a dashboard guardian by the provided dashboard. +// This constructor should be preferred over the other two if the dashboard in available +// since it avoids querying the database for fetching the dashboard. +func NewAccessControlDashboardGuardianByDashboard( + ctx context.Context, dashboard *models.Dashboard, user *user.SignedInUser, + store db.DB, ac accesscontrol.AccessControl, + folderPermissionsService accesscontrol.FolderPermissionsService, + dashboardPermissionsService accesscontrol.DashboardPermissionsService, + dashboardService dashboards.DashboardService, +) (*AccessControlDashboardGuardian, error) { + return &AccessControlDashboardGuardian{ + ctx: ctx, + log: log.New("dashboard.permissions"), + dashboard: dashboard, + user: user, + store: store, + ac: ac, + folderPermissionsService: folderPermissionsService, + dashboardPermissionsService: dashboardPermissionsService, + dashboardService: dashboardService, + }, nil } type AccessControlDashboardGuardian struct { ctx context.Context log log.Logger - dashboardID int64 dashboard *models.Dashboard user *user.SignedInUser store db.DB @@ -55,8 +132,8 @@ type AccessControlDashboardGuardian struct { } func (a *AccessControlDashboardGuardian) CanSave() (bool, error) { - if err := a.loadDashboard(); err != nil { - return false, err + if a.dashboard == nil { + return false, ErrGuardianDashboardNotFound } if a.dashboard.IsFolder { @@ -69,9 +146,10 @@ func (a *AccessControlDashboardGuardian) CanSave() (bool, error) { } func (a *AccessControlDashboardGuardian) CanEdit() (bool, error) { - if err := a.loadDashboard(); err != nil { - return false, err + if a.dashboard == nil { + return false, ErrGuardianDashboardNotFound } + if setting.ViewersCanEdit { return a.CanView() } @@ -86,8 +164,8 @@ func (a *AccessControlDashboardGuardian) CanEdit() (bool, error) { } func (a *AccessControlDashboardGuardian) CanView() (bool, error) { - if err := a.loadDashboard(); err != nil { - return false, err + if a.dashboard == nil { + return false, ErrGuardianDashboardNotFound } if a.dashboard.IsFolder { @@ -100,8 +178,8 @@ func (a *AccessControlDashboardGuardian) CanView() (bool, error) { } func (a *AccessControlDashboardGuardian) CanAdmin() (bool, error) { - if err := a.loadDashboard(); err != nil { - return false, err + if a.dashboard == nil { + return false, ErrGuardianDashboardNotFound } if a.dashboard.IsFolder { @@ -118,8 +196,8 @@ func (a *AccessControlDashboardGuardian) CanAdmin() (bool, error) { } func (a *AccessControlDashboardGuardian) CanDelete() (bool, error) { - if err := a.loadDashboard(); err != nil { - return false, err + if a.dashboard == nil { + return false, ErrGuardianDashboardNotFound } if a.dashboard.IsFolder { @@ -145,11 +223,11 @@ func (a *AccessControlDashboardGuardian) CanCreate(folderID int64, isFolder bool func (a *AccessControlDashboardGuardian) evaluate(evaluator accesscontrol.Evaluator) (bool, error) { ok, err := a.ac.Evaluate(a.ctx, a.user, evaluator) if err != nil { - a.log.Debug("Failed to evaluate access control to folder or dashboard", "error", err, "userId", a.user.UserID, "id", a.dashboardID) + a.log.Debug("Failed to evaluate access control to folder or dashboard", "error", err, "userId", a.user.UserID, "id", a.dashboard.Id) } if !ok && err == nil { - a.log.Debug("Access denied to folder or dashboard", "userId", a.user.UserID, "id", a.dashboardID, "permissions", evaluator.GoString()) + a.log.Debug("Access denied to folder or dashboard", "userId", a.user.UserID, "id", a.dashboard.Id, "permissions", evaluator.GoString()) } return ok, err @@ -162,8 +240,8 @@ func (a *AccessControlDashboardGuardian) CheckPermissionBeforeUpdate(permission // GetACL translate access control permissions to dashboard acl info func (a *AccessControlDashboardGuardian) GetACL() ([]*models.DashboardACLInfoDTO, error) { - if err := a.loadDashboard(); err != nil { - return nil, err + if a.dashboard == nil { + return nil, ErrGuardianGetDashboardFailure } var svc accesscontrol.PermissionsService @@ -254,17 +332,6 @@ func (a *AccessControlDashboardGuardian) GetHiddenACL(cfg *setting.Cfg) ([]*mode return hiddenACL, nil } -func (a *AccessControlDashboardGuardian) loadDashboard() error { - if a.dashboard == nil { - query := &models.GetDashboardQuery{Id: a.dashboardID, OrgId: a.user.OrgID} - if err := a.dashboardService.GetDashboard(a.ctx, query); err != nil { - return err - } - a.dashboard = query.Result - } - return nil -} - func (a *AccessControlDashboardGuardian) loadParentFolder(folderID int64) (*models.Dashboard, error) { if folderID == 0 { return &models.Dashboard{Uid: accesscontrol.GeneralFolderUID}, nil diff --git a/pkg/services/guardian/accesscontrol_guardian_test.go b/pkg/services/guardian/accesscontrol_guardian_test.go index 39c0496a19c..41ad86297be 100644 --- a/pkg/services/guardian/accesscontrol_guardian_test.go +++ b/pkg/services/guardian/accesscontrol_guardian_test.go @@ -616,9 +616,21 @@ func setupAccessControlGuardianTest(t *testing.T, uid string, permissions []acce setting.NewCfg(), routing.NewRouteRegister(), store, ac, license, &dashboards.FakeDashboardStore{}, ac, teamSvc, userSvc) require.NoError(t, err) if dashboardSvc == nil { - dashboardSvc = &dashboards.FakeDashboardService{} + fakeDashboardService := dashboards.NewFakeDashboardService(t) + fakeDashboardService.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { + q := args.Get(1).(*models.GetDashboardQuery) + q.Result = &models.Dashboard{ + Id: q.Id, + Uid: q.Uid, + OrgId: q.OrgId, + } + }).Return(nil) + dashboardSvc = fakeDashboardService } - return NewAccessControlDashboardGuardian(context.Background(), dash.Id, &user.SignedInUser{OrgID: 1}, store, ac, folderPermissions, dashboardPermissions, dashboardSvc), dash + + g, err := NewAccessControlDashboardGuardian(context.Background(), dash.Id, &user.SignedInUser{OrgID: 1}, store, ac, folderPermissions, dashboardPermissions, dashboardSvc) + require.NoError(t, err) + return g, dash } func testDashSvc(t *testing.T) dashboards.DashboardService { diff --git a/pkg/services/guardian/guardian.go b/pkg/services/guardian/guardian.go index 16ce9e10b6e..a548ec08d4e 100644 --- a/pkg/services/guardian/guardian.go +++ b/pkg/services/guardian/guardian.go @@ -12,11 +12,14 @@ import ( "github.com/grafana/grafana/pkg/services/team" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/util/errutil" ) var ( - ErrGuardianPermissionExists = errors.New("permission already exists") - ErrGuardianOverride = errors.New("you can only override a permission to be higher") + ErrGuardianPermissionExists = errors.New("permission already exists") + ErrGuardianOverride = errors.New("you can only override a permission to be higher") + ErrGuardianGetDashboardFailure = errutil.NewBase(errutil.StatusInternal, "guardian.getDashboardFailure", errutil.WithPublicMessage("Failed to get dashboard")) + ErrGuardianDashboardNotFound = errutil.NewBase(errutil.StatusNotFound, "guardian.dashboardNotFound") ) // DashboardGuardian to be used for guard against operations without access on dashboard and acl @@ -54,11 +57,38 @@ type dashboardGuardianImpl struct { // New factory for creating a new dashboard guardian instance // When using access control this function is replaced on startup and the AccessControlDashboardGuardian is returned -var New = func(ctx context.Context, dashId int64, orgId int64, user *user.SignedInUser) DashboardGuardian { +var New = func(ctx context.Context, dashId int64, orgId int64, user *user.SignedInUser) (DashboardGuardian, error) { panic("no guardian factory implementation provided") } -func newDashboardGuardian(ctx context.Context, dashId int64, orgId int64, user *user.SignedInUser, store db.DB, dashSvc dashboards.DashboardService, teamSvc team.Service) *dashboardGuardianImpl { +// NewByUID factory for creating a new dashboard guardian instance +// When using access control this function is replaced on startup and the AccessControlDashboardGuardian is returned +var NewByUID = func(ctx context.Context, dashUID string, orgId int64, user *user.SignedInUser) (DashboardGuardian, error) { + panic("no guardian factory implementation provided") +} + +// NewByDashboard factory for creating a new dashboard guardian instance +// When using access control this function is replaced on startup and the AccessControlDashboardGuardian is returned +var NewByDashboard = func(ctx context.Context, dash *models.Dashboard, orgId int64, user *user.SignedInUser) (DashboardGuardian, error) { + panic("no guardian factory implementation provided") +} + +// newDashboardGuardian creates a dashboard guardian by the provided dashId. +func newDashboardGuardian(ctx context.Context, dashId int64, orgId int64, user *user.SignedInUser, store db.DB, dashSvc dashboards.DashboardService, teamSvc team.Service) (*dashboardGuardianImpl, error) { + if dashId != 0 { + q := &models.GetDashboardQuery{ + Id: dashId, + OrgId: orgId, + } + + if err := dashSvc.GetDashboard(ctx, q); err != nil { + if errors.Is(err, dashboards.ErrDashboardNotFound) { + return nil, ErrGuardianDashboardNotFound.Errorf("failed to get dashboard by UID: %w", err) + } + return nil, ErrGuardianGetDashboardFailure.Errorf("failed to get dashboard by UID: %w", err) + } + } + return &dashboardGuardianImpl{ user: user, dashId: dashId, @@ -68,7 +98,53 @@ func newDashboardGuardian(ctx context.Context, dashId int64, orgId int64, user * store: store, dashboardService: dashSvc, teamService: teamSvc, + }, nil +} + +// newDashboardGuardianByUID creates a dashboard guardian by the provided dashUID. +func newDashboardGuardianByUID(ctx context.Context, dashUID string, orgId int64, user *user.SignedInUser, store db.DB, dashSvc dashboards.DashboardService, teamSvc team.Service) (*dashboardGuardianImpl, error) { + dashID := int64(0) + if dashUID != "" { + q := &models.GetDashboardQuery{ + Uid: dashUID, + OrgId: orgId, + } + + if err := dashSvc.GetDashboard(ctx, q); err != nil { + if errors.Is(err, dashboards.ErrDashboardNotFound) { + return nil, ErrGuardianDashboardNotFound.Errorf("failed to get dashboard by UID: %w", err) + } + return nil, ErrGuardianGetDashboardFailure.Errorf("failed to get dashboard by UID: %w", err) + } + dashID = q.Result.Id } + + return &dashboardGuardianImpl{ + user: user, + dashId: dashID, + orgId: orgId, + log: log.New("dashboard.permissions"), + ctx: ctx, + store: store, + dashboardService: dashSvc, + teamService: teamSvc, + }, nil +} + +// newDashboardGuardianByDashboard creates a dashboard guardian by the provided dashboard. +// This constructor should be preferred over the other two if the dashboard in available +// since it avoids querying the database for fetching the dashboard. +func newDashboardGuardianByDashboard(ctx context.Context, dash *models.Dashboard, orgId int64, user *user.SignedInUser, store db.DB, dashSvc dashboards.DashboardService, teamSvc team.Service) (*dashboardGuardianImpl, error) { + return &dashboardGuardianImpl{ + user: user, + dashId: dash.Id, + orgId: orgId, + log: log.New("dashboard.permissions"), + ctx: ctx, + store: store, + dashboardService: dashSvc, + teamService: teamSvc, + }, nil } func (g *dashboardGuardianImpl) CanSave() (bool, error) { @@ -319,7 +395,8 @@ func (g *dashboardGuardianImpl) GetHiddenACL(cfg *setting.Cfg) ([]*models.Dashbo // nolint:unused type FakeDashboardGuardian struct { - DashId int64 + DashID int64 + DashUID string OrgId int64 User *user.SignedInUser CanSaveValue bool @@ -379,10 +456,25 @@ func (g *FakeDashboardGuardian) GetHiddenACL(cfg *setting.Cfg) ([]*models.Dashbo // nolint:unused func MockDashboardGuardian(mock *FakeDashboardGuardian) { - New = func(_ context.Context, dashId int64, orgId int64, user *user.SignedInUser) DashboardGuardian { + New = func(_ context.Context, dashID int64, orgId int64, user *user.SignedInUser) (DashboardGuardian, error) { mock.OrgId = orgId - mock.DashId = dashId + mock.DashID = dashID mock.User = user - return mock + return mock, nil + } + + NewByUID = func(_ context.Context, dashUID string, orgId int64, user *user.SignedInUser) (DashboardGuardian, error) { + mock.OrgId = orgId + mock.DashUID = dashUID + mock.User = user + return mock, nil + } + + NewByDashboard = func(_ context.Context, dash *models.Dashboard, orgId int64, user *user.SignedInUser) (DashboardGuardian, error) { + mock.OrgId = orgId + mock.DashUID = dash.Uid + mock.DashID = dash.Id + mock.User = user + return mock, nil } } diff --git a/pkg/services/guardian/guardian_test.go b/pkg/services/guardian/guardian_test.go index c8ca0bb6302..85b82e44252 100644 --- a/pkg/services/guardian/guardian_test.go +++ b/pkg/services/guardian/guardian_test.go @@ -23,6 +23,7 @@ const ( orgID = int64(1) defaultDashboardID = int64(-1) dashboardID = int64(1) + dashboardUID = "uid" parentFolderID = int64(2) childDashboardID = int64(3) userID = int64(1) @@ -697,6 +698,14 @@ func TestGuardianGetHiddenACL(t *testing.T) { {Inherited: true, UserId: 3, UserLogin: "user3", Permission: models.PERMISSION_VIEW}, } }).Return(nil) + dashSvc.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { + q := args.Get(1).(*models.GetDashboardQuery) + q.Result = &models.Dashboard{ + Id: q.Id, + Uid: q.Uid, + OrgId: q.OrgId, + } + }).Return(nil) cfg := setting.NewCfg() cfg.HiddenUsers = map[string]struct{}{"user2": {}} @@ -707,7 +716,8 @@ func TestGuardianGetHiddenACL(t *testing.T) { UserID: 1, Login: "user1", } - g := newDashboardGuardian(context.Background(), dashboardID, orgID, user, store, dashSvc, &teamtest.FakeService{}) + g, err := newDashboardGuardian(context.Background(), dashboardID, orgID, user, store, dashSvc, &teamtest.FakeService{}) + require.NoError(t, err) hiddenACL, err := g.GetHiddenACL(cfg) require.NoError(t, err) @@ -723,7 +733,16 @@ func TestGuardianGetHiddenACL(t *testing.T) { Login: "user1", IsGrafanaAdmin: true, } - g := newDashboardGuardian(context.Background(), dashboardID, orgID, user, store, &dashboards.FakeDashboardService{}, &teamtest.FakeService{}) + dashSvc := dashboards.NewFakeDashboardService(t) + dashSvc.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { + q := args.Get(1).(*models.GetDashboardQuery) + q.Result = &models.Dashboard{ + Id: q.Id, + Uid: q.Uid, + } + }).Return(nil) + g, err := newDashboardGuardian(context.Background(), dashboardID, orgID, user, store, dashSvc, &teamtest.FakeService{}) + require.NoError(t, err) hiddenACL, err := g.GetHiddenACL(cfg) require.NoError(t, err) @@ -750,6 +769,14 @@ func TestGuardianGetACLWithoutDuplicates(t *testing.T) { {Inherited: false, UserId: 6, UserLogin: "user6", Permission: models.PERMISSION_EDIT}, } }).Return(nil) + dashSvc.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { + q := args.Get(1).(*models.GetDashboardQuery) + q.Result = &models.Dashboard{ + Id: q.Id, + Uid: q.Uid, + OrgId: q.OrgId, + } + }).Return(nil) t.Run("Should get acl without duplicates", func(t *testing.T) { user := &user.SignedInUser{ @@ -757,7 +784,8 @@ func TestGuardianGetACLWithoutDuplicates(t *testing.T) { UserID: 1, Login: "user1", } - g := newDashboardGuardian(context.Background(), dashboardID, orgID, user, store, dashSvc, &teamtest.FakeService{}) + g, err := newDashboardGuardian(context.Background(), dashboardID, orgID, user, store, dashSvc, &teamtest.FakeService{}) + require.NoError(t, err) acl, err := g.GetACLWithoutDuplicates() require.NoError(t, err) diff --git a/pkg/services/guardian/guardian_util_test.go b/pkg/services/guardian/guardian_util_test.go index 820e532b5c2..b70b041bc32 100644 --- a/pkg/services/guardian/guardian_util_test.go +++ b/pkg/services/guardian/guardian_util_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/infra/db/dbtest" "github.com/grafana/grafana/pkg/models" @@ -43,7 +44,17 @@ func orgRoleScenario(desc string, t *testing.T, role org.RoleType, fn scenarioFu OrgRole: role, } store := dbtest.NewFakeDB() - guard := newDashboardGuardian(context.Background(), dashboardID, orgID, user, store, &dashboards.FakeDashboardService{}, &teamtest.FakeService{}) + + fakeDashboardService := dashboards.NewFakeDashboardService(t) + fakeDashboardService.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { + q := args.Get(1).(*models.GetDashboardQuery) + q.Result = &models.Dashboard{ + Id: q.Id, + Uid: q.Uid, + } + }).Return(nil) + guard, err := newDashboardGuardian(context.Background(), dashboardID, orgID, user, store, fakeDashboardService, &teamtest.FakeService{}) + require.NoError(t, err) sc := &scenarioContext{ t: t, @@ -65,7 +76,17 @@ func apiKeyScenario(desc string, t *testing.T, role org.RoleType, fn scenarioFun ApiKeyID: 10, } store := dbtest.NewFakeDB() - guard := newDashboardGuardian(context.Background(), dashboardID, orgID, user, store, &dashboards.FakeDashboardService{}, &teamtest.FakeService{}) + dashSvc := dashboards.NewFakeDashboardService(t) + dashSvc.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { + q := args.Get(1).(*models.GetDashboardQuery) + q.Result = &models.Dashboard{ + Id: q.Id, + Uid: q.Uid, + } + }).Return(nil) + guard, err := newDashboardGuardian(context.Background(), dashboardID, orgID, user, store, dashSvc, &teamtest.FakeService{}) + require.NoError(t, err) + sc := &scenarioContext{ t: t, orgRoleScenario: desc, @@ -96,9 +117,20 @@ func permissionScenario(desc string, dashboardID int64, sc *scenarioContext, q := args.Get(1).(*models.GetDashboardACLInfoListQuery) q.Result = permissions }).Return(nil) + dashSvc.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { + q := args.Get(1).(*models.GetDashboardQuery) + q.Result = &models.Dashboard{ + Id: q.Id, + Uid: q.Uid, + OrgId: q.OrgId, + } + }).Return(nil) sc.permissionScenario = desc - sc.g = newDashboardGuardian(context.Background(), dashboardID, sc.givenUser.OrgID, sc.givenUser, store, dashSvc, teamSvc) + g, err := newDashboardGuardian(context.Background(), dashboardID, sc.givenUser.OrgID, sc.givenUser, store, dashSvc, teamSvc) + require.NoError(t, err) + sc.g = g + sc.givenDashboardID = dashboardID sc.givenPermissions = permissions sc.givenTeams = teams diff --git a/pkg/services/guardian/provider.go b/pkg/services/guardian/provider.go index 58c6ffd352e..4dc0cdc71b7 100644 --- a/pkg/services/guardian/provider.go +++ b/pkg/services/guardian/provider.go @@ -4,6 +4,7 @@ import ( "context" "github.com/grafana/grafana/pkg/infra/db" + "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/team" @@ -27,16 +28,32 @@ func ProvideService( } func InitLegacyGuardian(store db.DB, dashSvc dashboards.DashboardService, teamSvc team.Service) { - New = func(ctx context.Context, dashId int64, orgId int64, user *user.SignedInUser) DashboardGuardian { + New = func(ctx context.Context, dashId int64, orgId int64, user *user.SignedInUser) (DashboardGuardian, error) { return newDashboardGuardian(ctx, dashId, orgId, user, store, dashSvc, teamSvc) } + + NewByUID = func(ctx context.Context, dashUID string, orgId int64, user *user.SignedInUser) (DashboardGuardian, error) { + return newDashboardGuardianByUID(ctx, dashUID, orgId, user, store, dashSvc, teamSvc) + } + + NewByDashboard = func(ctx context.Context, dash *models.Dashboard, orgId int64, user *user.SignedInUser) (DashboardGuardian, error) { + return newDashboardGuardianByDashboard(ctx, dash, orgId, user, store, dashSvc, teamSvc) + } } func InitAccessControlGuardian( store db.DB, ac accesscontrol.AccessControl, folderPermissionsService accesscontrol.FolderPermissionsService, dashboardPermissionsService accesscontrol.DashboardPermissionsService, dashboardService dashboards.DashboardService, ) { - New = func(ctx context.Context, dashId int64, orgId int64, user *user.SignedInUser) DashboardGuardian { + New = func(ctx context.Context, dashId int64, orgId int64, user *user.SignedInUser) (DashboardGuardian, error) { return NewAccessControlDashboardGuardian(ctx, dashId, user, store, ac, folderPermissionsService, dashboardPermissionsService, dashboardService) } + + NewByUID = func(ctx context.Context, dashUID string, orgId int64, user *user.SignedInUser) (DashboardGuardian, error) { + return NewAccessControlDashboardGuardianByUID(ctx, dashUID, user, store, ac, folderPermissionsService, dashboardPermissionsService, dashboardService) + } + + NewByDashboard = func(ctx context.Context, dash *models.Dashboard, orgId int64, user *user.SignedInUser) (DashboardGuardian, error) { + return NewAccessControlDashboardGuardianByDashboard(ctx, dash, user, store, ac, folderPermissionsService, dashboardPermissionsService, dashboardService) + } } diff --git a/pkg/services/libraryelements/api.go b/pkg/services/libraryelements/api.go index d6afb11f1a2..08b46cc7bfc 100644 --- a/pkg/services/libraryelements/api.go +++ b/pkg/services/libraryelements/api.go @@ -50,7 +50,7 @@ func (l *LibraryElementService) createHandler(c *models.ReqContext) response.Res } else { folder, err := l.folderService.Get(c.Req.Context(), &folder.GetFolderQuery{OrgID: c.OrgID, UID: cmd.FolderUID, SignedInUser: c.SignedInUser}) if err != nil || folder == nil { - return response.Error(http.StatusBadRequest, "failed to get folder", err) + return response.ErrOrFallback(http.StatusBadRequest, "failed to get folder", err) } cmd.FolderID = folder.ID } @@ -64,7 +64,7 @@ func (l *LibraryElementService) createHandler(c *models.ReqContext) response.Res if element.FolderID != 0 { folder, err := l.folderService.Get(c.Req.Context(), &folder.GetFolderQuery{OrgID: c.OrgID, ID: &element.FolderID, SignedInUser: c.SignedInUser}) if err != nil { - return response.Error(http.StatusInternalServerError, "failed to get folder", err) + return response.ErrOrFallback(http.StatusInternalServerError, "failed to get folder", err) } element.FolderUID = folder.UID element.Meta.FolderUID = folder.UID @@ -270,7 +270,7 @@ func toLibraryElementError(err error, message string) response.Response { if errors.Is(err, errLibraryElementUIDTooLong) { return response.Error(400, errLibraryElementUIDTooLong.Error(), err) } - return response.Error(500, message, err) + return response.ErrOrFallback(http.StatusInternalServerError, message, err) } // swagger:parameters getLibraryElementByUID getLibraryElementConnections diff --git a/pkg/services/libraryelements/guard.go b/pkg/services/libraryelements/guard.go index e216507e84a..6278a514345 100644 --- a/pkg/services/libraryelements/guard.go +++ b/pkg/services/libraryelements/guard.go @@ -6,7 +6,6 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/dashboards" - "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/user" @@ -40,13 +39,12 @@ func (l *LibraryElementService) requireEditPermissionsOnFolder(ctx context.Conte if isGeneralFolder(folderID) && user.HasRole(org.RoleViewer) { return dashboards.ErrFolderAccessDenied } - folder, err := l.folderService.Get(ctx, &folder.GetFolderQuery{ID: &folderID, OrgID: user.OrgID, SignedInUser: user}) + + g, err := guardian.New(ctx, folderID, user.OrgID, user) if err != nil { return err } - g := guardian.New(ctx, folder.ID, user.OrgID, user) - canEdit, err := g.CanEdit() if err != nil { return err @@ -63,13 +61,11 @@ func (l *LibraryElementService) requireViewPermissionsOnFolder(ctx context.Conte return nil } - folder, err := l.folderService.Get(ctx, &folder.GetFolderQuery{ID: &folderID, OrgID: user.OrgID, SignedInUser: user}) + g, err := guardian.New(ctx, folderID, user.OrgID, user) if err != nil { return err } - g := guardian.New(ctx, folder.ID, user.OrgID, user) - canView, err := g.CanView() if err != nil { return err diff --git a/pkg/services/librarypanels/librarypanels_test.go b/pkg/services/librarypanels/librarypanels_test.go index c326e415c5b..90eb5f4125a 100644 --- a/pkg/services/librarypanels/librarypanels_test.go +++ b/pkg/services/librarypanels/librarypanels_test.go @@ -7,6 +7,7 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/api/routing" @@ -764,7 +765,16 @@ func updateFolderACL(t *testing.T, dashboardStore *database.DashboardStore, fold func scenarioWithLibraryPanel(t *testing.T, desc string, fn func(t *testing.T, sc scenarioContext)) { store := dbtest.NewFakeDB() - guardian.InitLegacyGuardian(store, &dashboards.FakeDashboardService{}, &teamtest.FakeService{}) + + dashSvc := dashboards.NewFakeDashboardService(t) + dashSvc.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { + q := args.Get(1).(*models.GetDashboardQuery) + q.Result = &models.Dashboard{ + Id: q.Id, + Uid: q.Uid, + } + }).Return(nil) + guardian.InitLegacyGuardian(store, dashSvc, &teamtest.FakeService{}) t.Helper() testScenario(t, desc, func(t *testing.T, sc scenarioContext) { diff --git a/pkg/services/live/features/dashboard.go b/pkg/services/live/features/dashboard.go index d2372249d55..c2d81dbf3ff 100644 --- a/pkg/services/live/features/dashboard.go +++ b/pkg/services/live/features/dashboard.go @@ -73,7 +73,10 @@ func (h *DashboardHandler) OnSubscribe(ctx context.Context, user *user.SignedInU } dash := query.Result - guard := guardian.New(ctx, dash.Id, user.OrgID, user) + guard, err := guardian.NewByDashboard(ctx, dash, user.OrgID, user) + if err != nil { + return models.SubscribeReply{}, backend.SubscribeStreamStatusPermissionDenied, err + } if canView, err := guard.CanView(); err != nil || !canView { return models.SubscribeReply{}, backend.SubscribeStreamStatusPermissionDenied, nil } @@ -119,7 +122,12 @@ func (h *DashboardHandler) OnPublish(ctx context.Context, user *user.SignedInUse return models.PublishReply{}, backend.PublishStreamStatusNotFound, nil } - guard := guardian.New(ctx, query.Result.Id, user.OrgID, user) + guard, err := guardian.NewByDashboard(ctx, query.Result, user.OrgID, user) + if err != nil { + logger.Error("Failed to create guardian", "err", err) + return models.PublishReply{}, backend.PublishStreamStatusNotFound, fmt.Errorf("internal error") + } + canEdit, err := guard.CanEdit() if err != nil { return models.PublishReply{}, backend.PublishStreamStatusNotFound, fmt.Errorf("internal error") diff --git a/pkg/services/ngalert/store/alert_rule.go b/pkg/services/ngalert/store/alert_rule.go index 6f0df1ca244..6151f9f239b 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -375,7 +375,11 @@ func (st DBstore) GetNamespaceByTitle(ctx context.Context, namespace string, org // if access control is disabled, check that the user is allowed to save in the folder. if withCanSave && st.AccessControl.IsDisabled() { - g := guardian.New(ctx, folder.ID, orgID, user) + g, err := guardian.NewByUID(ctx, folder.UID, orgID, user) + if err != nil { + return folder, err + } + if canSave, err := g.CanSave(); err != nil || !canSave { if err != nil { st.Logger.Error("checking can save permission has failed", "userId", user.UserID, "username", user.Login, "namespace", namespace, "orgId", orgID, "error", err) diff --git a/pkg/services/thumbs/service.go b/pkg/services/thumbs/service.go index 036dcdf6fa1..a8ef7037cb6 100644 --- a/pkg/services/thumbs/service.go +++ b/pkg/services/thumbs/service.go @@ -182,7 +182,12 @@ func (hs *thumbService) parseImageReq(c *models.ReqContext, checkSave bool) *pre } // Check permissions and status - status := hs.getStatus(c, req.UID, checkSave) + status, err := hs.getStatus(c, req.UID, checkSave) + if err != nil { + c.JSON(status, map[string]string{"error": err.Error()}) + return nil + } + if status != 200 { c.JSON(status, map[string]string{"error": fmt.Sprintf("code: %d", status)}) return nil @@ -463,35 +468,24 @@ func (hs *thumbService) CrawlerStatus(c *models.ReqContext) response.Response { } // Ideally this service would not require first looking up the full dashboard just to bet the id! -func (hs *thumbService) getStatus(c *models.ReqContext, uid string, checkSave bool) int { - dashboardID, err := hs.getDashboardId(c, uid) +func (hs *thumbService) getStatus(c *models.ReqContext, uid string, checkSave bool) (int, error) { + guardian, err := guardian.NewByUID(c.Req.Context(), uid, c.OrgID, c.SignedInUser) if err != nil { - return 404 - } - - guardian := guardian.New(c.Req.Context(), dashboardID, c.OrgID, c.SignedInUser) - if checkSave { - if canSave, err := guardian.CanSave(); err != nil || !canSave { - return 403 // forbidden - } - return 200 - } - - if canView, err := guardian.CanView(); err != nil || !canView { - return 403 // forbidden - } - - return 200 // found and OK -} - -func (hs *thumbService) getDashboardId(c *models.ReqContext, uid string) (int64, error) { - query := models.GetDashboardQuery{Uid: uid, OrgId: c.OrgID} - - if err := hs.dashboardService.GetDashboard(c.Req.Context(), &query); err != nil { return 0, err } - return query.Result.Id, nil + if checkSave { + if canSave, err := guardian.CanSave(); err != nil || !canSave { + return 403, nil // forbidden + } + return 200, nil + } + + if canView, err := guardian.CanView(); err != nil || !canView { + return 403, nil // forbidden + } + + return 200, nil // found and OK } func (hs *thumbService) runOnDemandCrawl(parentCtx context.Context, theme models.Theme, mode CrawlerMode, kind ThumbnailKind, authOpts rendering.AuthOpts) {