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

RBAC: Remove folder guardians part 2 (#104645)

Browse Source 
* replace usage of folder guardians with access control evaluators

* remove NewByFolderUID guardian

* bring up to date

* fix test

* more test fixes, and don't fetch the folder before evaluating lib element access

* change what error is returned

* fix alerting test

* try to fix linter errors

* replace the use of newByFolder guardian with direct access control evaluator checks

* remove newByFolder guardian

* remove unintentional changes

* remove unintentional changes

* undo unwanted change
This commit is contained in:
Ieva
2025-05-17 00:25:07 +03:00
committed by GitHub
parent c457a0c750
commit feaaf96269
6 changed files with 57 additions and 146 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/services/folder/folderimpl/folder_unifiedstorage.go
+12 -46
View File
@@ -26,7 +26,6 @@ import (
dashboardsearch "github.com/grafana/grafana/pkg/services/dashboards/service/search"
"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/search/model"
"github.com/grafana/grafana/pkg/services/store/entity"
"github.com/grafana/grafana/pkg/storage/unified/resource"
@@ -123,15 +122,8 @@ func (s *Service) getFromApiServer(ctx context.Context, q *folder.GetFolderQuery
return dashFolder, nil
}
// 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.NewByFolder(ctx, dashFolder, dashFolder.OrgID, q.SignedInUser)
if err != nil {
return nil, err
}
if canView, err := g.CanView(); err != nil || !canView {
evaluator := accesscontrol.EvalPermission(dashboards.ActionFoldersRead, dashboards.ScopeFoldersProvider.GetResourceScopeUID(dashFolder.UID))
if canView, err := s.accessControl.Evaluate(ctx, q.SignedInUser, evaluator); err != nil || !canView {
if err != nil {
return nil, toFolderError(err)
}
@@ -385,28 +377,16 @@ func (s *Service) getChildrenFromApiServer(ctx context.Context, q *folder.GetChi
return s.getRootFoldersFromApiServer(ctx, q)
}
var err error
// TODO: figure out what to do with Guardian
// we only need to check access to the folder
// if the parent is accessible then the subfolders are accessible as well (due to inheritance)
f := &folder.Folder{
UID: q.UID,
}
g, err := guardian.NewByFolder(ctx, f, q.OrgID, q.SignedInUser)
if err != nil {
return nil, err
}
guardianFunc := g.CanView
evaluator := accesscontrol.EvalPermission(dashboards.ActionFoldersRead, dashboards.ScopeFoldersProvider.GetResourceScopeUID(q.UID))
if q.Permission == dashboardaccess.PERMISSION_EDIT {
guardianFunc = g.CanEdit
evaluator = accesscontrol.EvalPermission(dashboards.ActionFoldersWrite, dashboards.ScopeFoldersProvider.GetResourceScopeUID(q.UID))
}
hasAccess, err := guardianFunc()
if err != nil {
return nil, err
}
if !hasAccess {
if hasAccess, err := s.accessControl.Evaluate(ctx, q.SignedInUser, evaluator); err != nil || !hasAccess {
if err != nil {
return nil, err
}
return nil, dashboards.ErrFolderAccessDenied
}
@@ -565,15 +545,8 @@ func (s *Service) updateOnApiServer(ctx context.Context, cmd *folder.UpdateFolde
return nil, dashboards.ErrDashboardTitleEmpty
}
f := &folder.Folder{
UID: cmd.UID,
}
g, err := guardian.NewByFolder(ctx, f, cmd.OrgID, cmd.SignedInUser)
if err != nil {
return nil, err
}
if canSave, err := g.CanSave(); err != nil || !canSave {
evaluator := accesscontrol.EvalPermission(dashboards.ActionFoldersWrite, dashboards.ScopeFoldersProvider.GetResourceScopeUID(cmd.UID))
if hasAccess, err := s.accessControl.Evaluate(ctx, cmd.SignedInUser, evaluator); err != nil || !hasAccess {
if err != nil {
return nil, err
}
@@ -624,15 +597,8 @@ func (s *Service) deleteFromApiServer(ctx context.Context, cmd *folder.DeleteFol
return folder.ErrBadRequest.Errorf("invalid orgID")
}
f := &folder.Folder{
UID: cmd.UID,
}
guard, err := guardian.NewByFolder(ctx, f, cmd.OrgID, cmd.SignedInUser)
if err != nil {
return err
}
if canSave, err := guard.CanDelete(); err != nil || !canSave {
evaluator := accesscontrol.EvalPermission(dashboards.ActionFoldersDelete, dashboards.ScopeFoldersProvider.GetResourceScopeUID(cmd.UID))
if hasAccess, err := s.accessControl.Evaluate(ctx, cmd.SignedInUser, evaluator); err != nil || !hasAccess {
if err != nil {
return toFolderError(err)
}
pkg/services/folder/folderimpl/folder_unifiedstorage_test.go
+13 -47
View File
@@ -32,7 +32,6 @@ import (
dashboardsearch "github.com/grafana/grafana/pkg/services/dashboards/service/search"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/folder"
"github.com/grafana/grafana/pkg/services/guardian"
ngstore "github.com/grafana/grafana/pkg/services/ngalert/store"
"github.com/grafana/grafana/pkg/services/publicdashboards"
"github.com/grafana/grafana/pkg/services/search/model"
@@ -182,11 +181,6 @@ func TestIntegrationFolderServiceViaUnifiedStorage(t *testing.T) {
folderApiServerMock := httptest.NewServer(mux)
defer folderApiServerMock.Close()
origNewGuardian := guardian.New
t.Cleanup(func() {
guardian.New = origNewGuardian
})
db, cfg := sqlstore.InitTestDB(t)
cfg.AppURL = folderApiServerMock.URL
@@ -213,6 +207,8 @@ func TestIntegrationFolderServiceViaUnifiedStorage(t *testing.T) {
[]accesscontrol.Permission{
{Action: dashboards.ActionFoldersCreate, Scope: dashboards.ScopeFoldersAll},
{Action: dashboards.ActionFoldersWrite, Scope: dashboards.ScopeFoldersAll},
{Action: dashboards.ActionFoldersDelete, Scope: dashboards.ScopeFoldersAll},
{Action: dashboards.ActionFoldersRead, Scope: dashboards.ScopeFoldersAll},
{Action: accesscontrol.ActionAlertingRuleDelete, Scope: dashboards.ScopeFoldersAll},
}),
}}
@@ -245,9 +241,6 @@ func TestIntegrationFolderServiceViaUnifiedStorage(t *testing.T) {
t.Run("Folder service tests", func(t *testing.T) {
t.Run("Given user has no permissions", func(t *testing.T) {
origNewGuardian := guardian.New
guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{})
ctx = identity.WithRequester(context.Background(), noPermUsr)
f := folder.NewFolder("Folder", "")
@@ -303,16 +296,9 @@ func TestIntegrationFolderServiceViaUnifiedStorage(t *testing.T) {
require.Error(t, err)
require.Equal(t, dashboards.ErrFolderAccessDenied, err)
})
t.Cleanup(func() {
guardian.New = origNewGuardian
})
})
t.Run("Given user has permission to save", func(t *testing.T) {
origNewGuardian := guardian.New
guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true, CanViewValue: true})
ctx = identity.WithRequester(context.Background(), usr)
f := &folder.Folder{
@@ -406,16 +392,9 @@ func TestIntegrationFolderServiceViaUnifiedStorage(t *testing.T) {
})
require.NoError(t, err)
})
t.Cleanup(func() {
guardian.New = origNewGuardian
})
})
t.Run("Given user has permission to view", func(t *testing.T) {
origNewGuardian := guardian.New
guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanViewValue: true})
t.Run("When get folder by uid should return folder", func(t *testing.T) {
actual, err := folderService.Get(ctx, &folder.GetFolderQuery{
UID: &fooFolder.UID,
@@ -507,10 +486,6 @@ func TestIntegrationFolderServiceViaUnifiedStorage(t *testing.T) {
require.Nil(t, actual)
require.ErrorIs(t, err, dashboards.ErrFolderNotFound)
})
t.Cleanup(func() {
guardian.New = origNewGuardian
})
})
t.Run("Returns root folder", func(t *testing.T) {
@@ -535,10 +510,6 @@ func TestIntegrationFolderServiceViaUnifiedStorage(t *testing.T) {
func TestSearchFoldersFromApiServer(t *testing.T) {
fakeK8sClient := new(client.MockK8sHandler)
guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{
CanSaveValue: true,
CanViewValue: true,
})
folderStore := folder.NewFakeStore()
folderStore.ExpectedFolder = &folder.Folder{
UID: "parent-uid",
@@ -546,10 +517,11 @@ func TestSearchFoldersFromApiServer(t *testing.T) {
Title: "parent title",
}
service := Service{
k8sclient: fakeK8sClient,
features: featuremgmt.WithFeatures(featuremgmt.FlagKubernetesClientDashboardsFolders),
unifiedStore: folderStore,
tracer: tracing.NewNoopTracerService(),
k8sclient: fakeK8sClient,
features: featuremgmt.WithFeatures(featuremgmt.FlagKubernetesClientDashboardsFolders),
unifiedStore: folderStore,
tracer: tracing.NewNoopTracerService(),
accessControl: actest.FakeAccessControl{ExpectedEvaluate: true},
}
user := &user.SignedInUser{OrgID: 1}
ctx := identity.WithRequester(context.Background(), user)
@@ -780,10 +752,6 @@ func TestSearchFoldersFromApiServer(t *testing.T) {
func TestGetFoldersFromApiServer(t *testing.T) {
fakeK8sClient := new(client.MockK8sHandler)
guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{
CanSaveValue: true,
CanViewValue: true,
})
folderStore := folder.NewFakeStore()
folderStore.ExpectedFolder = &folder.Folder{
UID: "parent-uid",
@@ -791,10 +759,11 @@ func TestGetFoldersFromApiServer(t *testing.T) {
Title: "parent title",
}
service := Service{
k8sclient: fakeK8sClient,
features: featuremgmt.WithFeatures(featuremgmt.FlagKubernetesClientDashboardsFolders),
unifiedStore: folderStore,
tracer: tracing.NewNoopTracerService(),
k8sclient: fakeK8sClient,
features: featuremgmt.WithFeatures(featuremgmt.FlagKubernetesClientDashboardsFolders),
unifiedStore: folderStore,
accessControl: actest.FakeAccessControl{ExpectedEvaluate: true},
tracer: tracing.NewNoopTracerService(),
}
user := &user.SignedInUser{OrgID: 1}
ctx := identity.WithRequester(context.Background(), user)
@@ -882,16 +851,13 @@ func TestDeleteFoldersFromApiServer(t *testing.T) {
unifiedStore: fakeFolderStore,
dashboardStore: dashboardStore,
publicDashboardService: publicDashboardFakeService,
accessControl: actest.FakeAccessControl{ExpectedEvaluate: true},
registry: make(map[string]folder.RegistryService),
features: featuremgmt.WithFeatures(featuremgmt.FlagKubernetesClientDashboardsFolders),
tracer: tracing.NewNoopTracerService(),
}
user := &user.SignedInUser{OrgID: 1}
ctx := identity.WithRequester(context.Background(), user)
guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{
CanSaveValue: true,
CanViewValue: true,
})
db, cfg := sqlstore.InitTestDB(t)
alertingStore := ngstore.DBstore{