From 73436e3d55f384a85417ab47a8af56510b710d8e Mon Sep 17 00:00:00 2001 From: Ieva Date: Fri, 21 Mar 2025 10:44:16 +0000 Subject: [PATCH] RBAC: Remove dashboard guardians pt 3 (#102558) * remove usage of New dashboard guardian * fix tests --- pkg/api/annotations.go | 29 ++++++++-------------- pkg/api/annotations_test.go | 39 +++++++++++++++--------------- pkg/api/dashboard_snapshot.go | 24 +++++++----------- pkg/api/dashboard_snapshot_test.go | 6 ++--- pkg/api/folder_test.go | 9 +++++++ 5 files changed, 51 insertions(+), 56 deletions(-) diff --git a/pkg/api/annotations.go b/pkg/api/annotations.go index 4720e25e91f..5873504d7c9 100644 --- a/pkg/api/annotations.go +++ b/pkg/api/annotations.go @@ -16,7 +16,6 @@ import ( "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/folder" - "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/web" @@ -278,7 +277,7 @@ func (hs *HTTPServer) UpdateAnnotation(c *contextmodel.ReqContext) response.Resp } if !hs.Features.IsEnabled(c.Req.Context(), featuremgmt.FlagAnnotationPermissionUpdate) { - if canSave, err := hs.canSaveAnnotation(c, annotation); err != nil || !canSave { + if canSave, err := hs.canSaveAnnotation(c, hs.AccessControl, annotation); err != nil || !canSave { return dashboardGuardianResponse(err) } } @@ -336,7 +335,7 @@ func (hs *HTTPServer) PatchAnnotation(c *contextmodel.ReqContext) response.Respo } if !hs.Features.IsEnabled(c.Req.Context(), featuremgmt.FlagAnnotationPermissionUpdate) { - if canSave, err := hs.canSaveAnnotation(c, annotation); err != nil || !canSave { + if canSave, err := hs.canSaveAnnotation(c, hs.AccessControl, annotation); err != nil || !canSave { return dashboardGuardianResponse(err) } } @@ -502,7 +501,7 @@ func (hs *HTTPServer) DeleteAnnotationByID(c *contextmodel.ReqContext) response. return resp } - if canSave, err := hs.canSaveAnnotation(c, annotation); err != nil || !canSave { + if canSave, err := hs.canSaveAnnotation(c, hs.AccessControl, annotation); err != nil || !canSave { return dashboardGuardianResponse(err) } } @@ -518,25 +517,17 @@ func (hs *HTTPServer) DeleteAnnotationByID(c *contextmodel.ReqContext) response. return response.Success("Annotation deleted") } -func (hs *HTTPServer) canSaveAnnotation(c *contextmodel.ReqContext, annotation *annotations.ItemDTO) (bool, error) { +func (hs *HTTPServer) canSaveAnnotation(c *contextmodel.ReqContext, ac accesscontrol.AccessControl, annotation *annotations.ItemDTO) (bool, error) { if annotation.GetType() == annotations.Dashboard { - return canEditDashboard(c, annotation.DashboardID) + return canEditDashboard(c, ac, annotation.DashboardID) } else { return true, nil } } -func canEditDashboard(c *contextmodel.ReqContext, dashboardID int64) (bool, error) { - guard, err := guardian.New(c.Req.Context(), dashboardID, c.SignedInUser.GetOrgID(), c.SignedInUser) - if err != nil { - return false, err - } - - if canEdit, err := guard.CanEdit(); err != nil || !canEdit { - return false, err - } - - return true, nil +func canEditDashboard(c *contextmodel.ReqContext, ac accesscontrol.AccessControl, dashboardID int64) (bool, error) { + evaluator := accesscontrol.EvalPermission(dashboards.ActionDashboardsWrite, dashboards.ScopeDashboardsProvider.GetResourceScope(strconv.FormatInt(dashboardID, 10))) + return ac.Evaluate(c.Req.Context(), c.SignedInUser, evaluator) } func findAnnotationByID(ctx context.Context, repo annotations.Repository, annotationID int64, user *user.SignedInUser) (*annotations.ItemDTO, response.Response) { @@ -680,7 +671,7 @@ func (hs *HTTPServer) canCreateAnnotation(c *contextmodel.ReqContext, dashboardI return canSave, err } - return canEditDashboard(c, dashboardId) + return canEditDashboard(c, hs.AccessControl, dashboardId) } else { // organization annotations evaluator := accesscontrol.EvalPermission(accesscontrol.ActionAnnotationsCreate, accesscontrol.ScopeAnnotationsTypeOrganization) return hs.AccessControl.Evaluate(c.Req.Context(), c.SignedInUser, evaluator) @@ -708,7 +699,7 @@ func (hs *HTTPServer) canMassDeleteAnnotations(c *contextmodel.ReqContext, dashb return false, err } - canSave, err = canEditDashboard(c, dashboardID) + canSave, err = canEditDashboard(c, hs.AccessControl, dashboardID) if err != nil || !canSave { return false, err } diff --git a/pkg/api/annotations_test.go b/pkg/api/annotations_test.go index 83e11eb91cf..fa4f119f13b 100644 --- a/pkg/api/annotations_test.go +++ b/pkg/api/annotations_test.go @@ -19,7 +19,6 @@ import ( "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/folder/foldertest" - "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/web/webtest" ) @@ -110,7 +109,10 @@ func TestAPI_Annotations(t *testing.T) { path: "/api/annotations/2", method: http.MethodPut, expectedCode: http.StatusOK, - permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsWrite, Scope: accesscontrol.ScopeAnnotationsTypeDashboard}}, + permissions: []accesscontrol.Permission{ + {Action: accesscontrol.ActionAnnotationsWrite, Scope: accesscontrol.ScopeAnnotationsTypeDashboard}, + {Action: dashboards.ActionDashboardsWrite, Scope: dashboards.ScopeDashboardsAll}, + }, }, { desc: "should not be able to update dashboard annotation without correct permission", @@ -162,7 +164,10 @@ func TestAPI_Annotations(t *testing.T) { path: "/api/annotations/2", method: http.MethodPatch, expectedCode: http.StatusOK, - permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsWrite, Scope: accesscontrol.ScopeAnnotationsTypeDashboard}}, + permissions: []accesscontrol.Permission{ + {Action: accesscontrol.ActionAnnotationsWrite, Scope: accesscontrol.ScopeAnnotationsTypeDashboard}, + {Action: dashboards.ActionDashboardsWrite, Scope: dashboards.ScopeDashboardsAll}, + }, }, { desc: "should not be able to patch dashboard annotation without correct permission", @@ -215,7 +220,10 @@ func TestAPI_Annotations(t *testing.T) { method: http.MethodPost, body: "{\"dashboardId\": 2,\"text\": \"test\"}", expectedCode: http.StatusOK, - permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsCreate, Scope: accesscontrol.ScopeAnnotationsTypeDashboard}}, + permissions: []accesscontrol.Permission{ + {Action: accesscontrol.ActionAnnotationsCreate, Scope: accesscontrol.ScopeAnnotationsTypeDashboard}, + {Action: dashboards.ActionDashboardsWrite, Scope: dashboards.ScopeDashboardsAll}, + }, }, { desc: "should not be able to create dashboard annotation without correct permission", @@ -273,7 +281,10 @@ func TestAPI_Annotations(t *testing.T) { path: "/api/annotations/2", method: http.MethodDelete, expectedCode: http.StatusOK, - permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsDelete, Scope: accesscontrol.ScopeAnnotationsTypeDashboard}}, + permissions: []accesscontrol.Permission{ + {Action: accesscontrol.ActionAnnotationsDelete, Scope: accesscontrol.ScopeAnnotationsTypeDashboard}, + {Action: dashboards.ActionDashboardsWrite, Scope: dashboards.ScopeDashboardsAll}, + }, }, { desc: "should not be able to delete dashboard annotation without correct permission", @@ -341,7 +352,10 @@ func TestAPI_Annotations(t *testing.T) { body: "{\"dashboardId\": 2, \"panelId\": 1}", method: http.MethodPost, expectedCode: http.StatusOK, - permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionAnnotationsDelete, Scope: accesscontrol.ScopeAnnotationsTypeDashboard}}, + permissions: []accesscontrol.Permission{ + {Action: accesscontrol.ActionAnnotationsDelete, Scope: accesscontrol.ScopeAnnotationsTypeDashboard}, + {Action: dashboards.ActionDashboardsWrite, Scope: dashboards.ScopeDashboardsAll}, + }, }, { desc: "should not be able to mass delete dashboard annotations without correct permission", @@ -382,10 +396,6 @@ func TestAPI_Annotations(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - // Don't need access to dashboards if annotationPermissionUpdate is enabled - if len(tt.featureFlags) == 0 { - setUpRBACGuardian(t) - } server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = setting.NewCfg() repo := annotationstest.NewFakeAnnotationsRepo() @@ -518,12 +528,3 @@ func TestService_AnnotationTypeScopeResolver(t *testing.T) { }) } } - -func setUpRBACGuardian(t *testing.T) { - origNewGuardian := guardian.New - t.Cleanup(func() { - guardian.New = origNewGuardian - }) - - guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanEditValue: true, CanViewValue: true}) -} diff --git a/pkg/api/dashboard_snapshot.go b/pkg/api/dashboard_snapshot.go index b583c550da7..4ebe1e8708f 100644 --- a/pkg/api/dashboard_snapshot.go +++ b/pkg/api/dashboard_snapshot.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "net/http" + "strconv" "time" "github.com/grafana/grafana/pkg/api/dtos" @@ -17,7 +18,6 @@ import ( "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/dashboardsnapshots" "github.com/grafana/grafana/pkg/services/featuremgmt" - "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/util/errhttp" "github.com/grafana/grafana/pkg/web" @@ -226,21 +226,15 @@ func (hs *HTTPServer) DeleteDashboardSnapshot(c *contextmodel.ReqContext) respon dashboardID := queryResult.Dashboard.Get("id").MustInt64() if dashboardID != 0 { - g, err := guardian.New(c.Req.Context(), dashboardID, c.SignedInUser.GetOrgID(), c.SignedInUser) - if err != nil { - if !errors.Is(err, dashboards.ErrDashboardNotFound) { - return response.Err(err) - } - } else { - canEdit, err := g.CanEdit() - // check for permissions only if the dashboard is found - if err != nil && !errors.Is(err, dashboards.ErrDashboardNotFound) { - return response.Error(http.StatusInternalServerError, "Error while checking permissions for snapshot", err) - } + evaluator := ac.EvalPermission(dashboards.ActionDashboardsWrite, dashboards.ScopeDashboardsProvider.GetResourceScope(strconv.FormatInt(dashboardID, 10))) + canEdit, err := hs.AccessControl.Evaluate(c.Req.Context(), c.SignedInUser, evaluator) + // check for permissions only if the dashboard is found + if err != nil && !errors.Is(err, dashboards.ErrDashboardNotFound) { + return response.Error(http.StatusInternalServerError, "Error while checking permissions for snapshot", err) + } - if !canEdit && queryResult.UserID != c.SignedInUser.UserID && !errors.Is(err, dashboards.ErrDashboardNotFound) { - return response.Error(http.StatusForbidden, "Access denied to this snapshot", nil) - } + if !canEdit && queryResult.UserID != c.SignedInUser.UserID && !errors.Is(err, dashboards.ErrDashboardNotFound) { + return response.Error(http.StatusForbidden, "Access denied to this snapshot", nil) } } diff --git a/pkg/api/dashboard_snapshot_test.go b/pkg/api/dashboard_snapshot_test.go index 24dcd3ef4ec..01d9e6ce4d0 100644 --- a/pkg/api/dashboard_snapshot_test.go +++ b/pkg/api/dashboard_snapshot_test.go @@ -15,13 +15,12 @@ import ( "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/infra/db/dbtest" - "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" + "github.com/grafana/grafana/pkg/services/accesscontrol/actest" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/dashboardsnapshots" "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" @@ -41,7 +40,7 @@ func TestHTTPServer_DeleteDashboardSnapshot(t *testing.T) { hs.DashboardService = svc hs.AccessControl = acimpl.ProvideAccessControl(featuremgmt.WithFeatures()) - guardian.InitAccessControlGuardian(hs.Cfg, hs.AccessControl, hs.DashboardService, hs.folderService, log.NewNopLogger()) + hs.AccessControl.RegisterScopeAttributeResolver(dashboards.NewDashboardIDScopeResolver(svc, nil)) }) } @@ -378,6 +377,7 @@ func buildHttpServer(d dashboardsnapshots.Service, snapshotEnabled bool) *HTTPSe Cfg: &setting.Cfg{ SnapshotEnabled: snapshotEnabled, }, + AccessControl: actest.FakeAccessControl{ExpectedEvaluate: true}, } return hs } diff --git a/pkg/api/folder_test.go b/pkg/api/folder_test.go index 840ae125594..96e8b75a0ec 100644 --- a/pkg/api/folder_test.go +++ b/pkg/api/folder_test.go @@ -768,3 +768,12 @@ func TestSetDefaultPermissionsWhenCreatingFolder(t *testing.T) { }) } } + +func setUpRBACGuardian(t *testing.T) { + origNewGuardian := guardian.New + t.Cleanup(func() { + guardian.New = origNewGuardian + }) + + guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanEditValue: true, CanViewValue: true}) +}