public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

RBAC: Remove dashboard guardians pt 3 (#102558)

Browse Source 
* remove usage of New dashboard guardian

* fix tests
This commit is contained in:
Ieva
2025-03-21 10:44:16 +00:00
committed by GitHub
parent 6922315d7c
commit 73436e3d55
5 changed files with 51 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/api/annotations.go
+10 -19
View File
@@ -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
}
pkg/api/annotations_test.go
+20 -19
View File
@@ -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})
}
pkg/api/dashboard_snapshot.go
+9 -15
View File
@@ -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)
}
}
pkg/api/dashboard_snapshot_test.go
+3 -3
View File
@@ -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
}
pkg/api/folder_test.go
+9
View File
@@ -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})
}