From a5e4a533fa18d014a28f18630a450c36a67e2716 Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Wed, 30 Mar 2022 15:14:26 +0200 Subject: [PATCH] Access control: use uid for dashboard and folder scopes (#46807) * use uid:s for folder and dashboard permissions * evaluate folder and dashboard permissions based on uids * add dashboard.uid to accept list * Check for exact suffix * Check parent folder on create * update test * drop dashboard:create actions with dashboard scope * fix typo * AccessControl: test id 0 scope conversion * AccessControl: store only parent folder UID * AccessControl: extract general as a constant * FolderServices: Prevent creation of a folder uid'd general * FolderServices: Test folder creation prevention * Update pkg/services/guardian/accesscontrol_guardian.go * FolderServices: fix mock call expect * FolderServices: remove uneeded mocks Co-authored-by: jguer --- pkg/api/accesscontrol.go | 6 +- pkg/api/api.go | 14 +- pkg/api/dashboard_permission.go | 6 +- pkg/api/folder_permission.go | 2 +- pkg/models/folders.go | 1 + pkg/services/accesscontrol/filter.go | 17 +- pkg/services/accesscontrol/models.go | 3 +- .../ossaccesscontrol/permissions_services.go | 53 ++--- pkg/services/dashboards/accesscontrol.go | 31 ++- pkg/services/dashboards/accesscontrol_test.go | 56 +++-- pkg/services/dashboards/database/database.go | 15 +- .../dashboards/manager/dashboard_service.go | 4 +- .../dashboards/manager/folder_service.go | 14 +- .../dashboards/manager/folder_service_test.go | 8 + .../guardian/accesscontrol_guardian.go | 71 ++++-- .../guardian/accesscontrol_guardian_test.go | 221 +++++++++--------- .../provisioning/dashboards/file_reader.go | 4 + .../dashboards/file_reader_test.go | 20 ++ .../accesscontrol/dashboard_permissions.go | 63 ++++- .../sqlstore/permissions/dashboard.go | 12 +- .../sqlstore/permissions/dashboard_test.go | 4 +- 21 files changed, 369 insertions(+), 256 deletions(-) diff --git a/pkg/api/accesscontrol.go b/pkg/api/accesscontrol.go index 97e63a8a1ba..9ef25c8f161 100644 --- a/pkg/api/accesscontrol.go +++ b/pkg/api/accesscontrol.go @@ -308,14 +308,14 @@ func (hs *HTTPServer) declareFixedRoles() error { dashboardsCreatorRole := ac.RoleRegistration{ Role: ac.RoleDTO{ - Version: 1, + Version: 2, Name: "fixed:dashboards:creator", DisplayName: "Dashboard creator", Description: "Create dashboard in general folder.", Group: "Dashboards", Permissions: []ac.Permission{ - {Action: dashboards.ActionFoldersRead, Scope: dashboards.ScopeFoldersProvider.GetResourceScope("0")}, - {Action: ac.ActionDashboardsCreate, Scope: dashboards.ScopeFoldersProvider.GetResourceScope("0")}, + {Action: dashboards.ActionFoldersRead, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID(ac.GeneralFolderUID)}, + {Action: ac.ActionDashboardsCreate, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID(ac.GeneralFolderUID)}, }, }, Grants: []string{"Editor"}, diff --git a/pkg/api/api.go b/pkg/api/api.go index 1b3fc216d1a..de7227dbf74 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -329,18 +329,20 @@ func (hs *HTTPServer) registerRoutes() { // Folders apiRoute.Group("/folders", func(folderRoute routing.RouteRegister) { + idScope := dashboards.ScopeFoldersProvider.GetResourceScope(ac.Parameter(":id")) + uidScope := dashboards.ScopeFoldersProvider.GetResourceScopeUID(ac.Parameter(":uid")) folderRoute.Get("/", authorize(reqSignedIn, ac.EvalPermission(dashboards.ActionFoldersRead)), routing.Wrap(hs.GetFolders)) - folderRoute.Get("/id/:id", authorize(reqSignedIn, ac.EvalPermission(dashboards.ActionFoldersRead, dashboards.ScopeFoldersProvider.GetResourceScope(ac.Parameter(":id")))), routing.Wrap(hs.GetFolderByID)) + folderRoute.Get("/id/:id", authorize(reqSignedIn, ac.EvalPermission(dashboards.ActionFoldersRead, idScope)), routing.Wrap(hs.GetFolderByID)) folderRoute.Post("/", authorize(reqSignedIn, ac.EvalPermission(dashboards.ActionFoldersCreate)), routing.Wrap(hs.CreateFolder)) folderRoute.Group("/:uid", func(folderUidRoute routing.RouteRegister) { - folderUidRoute.Get("/", authorize(reqSignedIn, ac.EvalPermission(dashboards.ActionFoldersRead)), routing.Wrap(hs.GetFolderByUID)) - folderUidRoute.Put("/", authorize(reqSignedIn, ac.EvalPermission(dashboards.ActionFoldersWrite)), routing.Wrap(hs.UpdateFolder)) - folderUidRoute.Delete("/", authorize(reqSignedIn, ac.EvalPermission(dashboards.ActionFoldersDelete)), routing.Wrap(hs.DeleteFolder)) + folderUidRoute.Get("/", authorize(reqSignedIn, ac.EvalPermission(dashboards.ActionFoldersRead, uidScope)), routing.Wrap(hs.GetFolderByUID)) + folderUidRoute.Put("/", authorize(reqSignedIn, ac.EvalPermission(dashboards.ActionFoldersWrite, uidScope)), routing.Wrap(hs.UpdateFolder)) + folderUidRoute.Delete("/", authorize(reqSignedIn, ac.EvalPermission(dashboards.ActionFoldersDelete, uidScope)), routing.Wrap(hs.DeleteFolder)) folderUidRoute.Group("/permissions", func(folderPermissionRoute routing.RouteRegister) { - folderPermissionRoute.Get("/", authorize(reqSignedIn, ac.EvalPermission(dashboards.ActionFoldersPermissionsRead)), routing.Wrap(hs.GetFolderPermissionList)) - folderPermissionRoute.Post("/", authorize(reqSignedIn, ac.EvalPermission(dashboards.ActionFoldersPermissionsWrite)), routing.Wrap(hs.UpdateFolderPermissions)) + folderPermissionRoute.Get("/", authorize(reqSignedIn, ac.EvalPermission(dashboards.ActionFoldersPermissionsRead, uidScope)), routing.Wrap(hs.GetFolderPermissionList)) + folderPermissionRoute.Post("/", authorize(reqSignedIn, ac.EvalPermission(dashboards.ActionFoldersPermissionsWrite, uidScope)), routing.Wrap(hs.UpdateFolderPermissions)) }) }) }) diff --git a/pkg/api/dashboard_permission.go b/pkg/api/dashboard_permission.go index 9ce4d46db06..34ea0b797f8 100644 --- a/pkg/api/dashboard_permission.go +++ b/pkg/api/dashboard_permission.go @@ -120,7 +120,7 @@ func (hs *HTTPServer) UpdateDashboardPermissions(c *models.ReqContext) response. if err != nil { return response.Error(500, "Error while checking dashboard permissions", err) } - if err := hs.updateDashboardAccessControl(c.Req.Context(), dash.OrgId, dash.Id, false, items, old); err != nil { + if err := hs.updateDashboardAccessControl(c.Req.Context(), dash.OrgId, dash.Uid, false, items, old); err != nil { return response.Error(500, "Failed to update permissions", err) } return response.Success("Dashboard permissions updated") @@ -138,7 +138,7 @@ func (hs *HTTPServer) UpdateDashboardPermissions(c *models.ReqContext) response. } // updateDashboardAccessControl is used for api backward compatibility -func (hs *HTTPServer) updateDashboardAccessControl(ctx context.Context, orgID, dashID int64, isFolder bool, items []*models.DashboardAcl, old []*models.DashboardAclInfoDTO) error { +func (hs *HTTPServer) updateDashboardAccessControl(ctx context.Context, orgID int64, uid string, isFolder bool, items []*models.DashboardAcl, old []*models.DashboardAclInfoDTO) error { commands := []accesscontrol.SetResourcePermissionCommand{} for _, item := range items { permissions := item.Permission.String() @@ -191,7 +191,7 @@ func (hs *HTTPServer) updateDashboardAccessControl(ctx context.Context, orgID, d svc = hs.permissionServices.GetFolderService() } - _, err := svc.SetPermissions(ctx, orgID, strconv.FormatInt(dashID, 10), commands...) + _, err := svc.SetPermissions(ctx, orgID, uid, commands...) return err } diff --git a/pkg/api/folder_permission.go b/pkg/api/folder_permission.go index 554de386224..548201c7ba2 100644 --- a/pkg/api/folder_permission.go +++ b/pkg/api/folder_permission.go @@ -120,7 +120,7 @@ func (hs *HTTPServer) UpdateFolderPermissions(c *models.ReqContext) response.Res if err != nil { return response.Error(500, "Error while checking dashboard permissions", err) } - if err := hs.updateDashboardAccessControl(c.Req.Context(), c.OrgId, folder.Id, true, items, old); err != nil { + if err := hs.updateDashboardAccessControl(c.Req.Context(), c.OrgId, folder.Uid, true, items, old); err != nil { return response.Error(500, "Failed to create permission", err) } return response.Success("Dashboard permissions updated") diff --git a/pkg/models/folders.go b/pkg/models/folders.go index 1dfbe2b7f93..895cfea2a5c 100644 --- a/pkg/models/folders.go +++ b/pkg/models/folders.go @@ -12,6 +12,7 @@ var ( ErrFolderVersionMismatch = errors.New("the folder has been changed by someone else") ErrFolderTitleEmpty = errors.New("folder title cannot be empty") ErrFolderWithSameUIDExists = errors.New("a folder/dashboard with the same uid already exists") + ErrFolderInvalidUID = errors.New("invalid uid for folder provided") ErrFolderSameNameExists = errors.New("a folder or dashboard in the general folder with the same name already exists") ErrFolderFailedGenerateUniqueUid = errors.New("failed to generate unique folder ID") ErrFolderAccessDenied = errors.New("access denied to folder") diff --git a/pkg/services/accesscontrol/filter.go b/pkg/services/accesscontrol/filter.go index cdfa4e50ca1..2fc40de2796 100644 --- a/pkg/services/accesscontrol/filter.go +++ b/pkg/services/accesscontrol/filter.go @@ -9,15 +9,14 @@ import ( ) var sqlIDAcceptList = map[string]struct{}{ - "org_user.user_id": {}, - "role.id": {}, - "t.id": {}, - "team.id": {}, - "u.id": {}, - "\"user\".\"id\"": {}, // For Postgres - "`user`.`id`": {}, // For MySQL and SQLite - "dashboard.id": {}, - "dashboard.folder_id": {}, + "org_user.user_id": {}, + "role.id": {}, + "t.id": {}, + "team.id": {}, + "u.id": {}, + "\"user\".\"id\"": {}, // For Postgres + "`user`.`id`": {}, // For MySQL and SQLite + "dashboard.uid": {}, } var ( diff --git a/pkg/services/accesscontrol/models.go b/pkg/services/accesscontrol/models.go index 90718fd12b3..676cdcb4c75 100644 --- a/pkg/services/accesscontrol/models.go +++ b/pkg/services/accesscontrol/models.go @@ -241,7 +241,8 @@ type SetResourcePermissionCommand struct { } const ( - GlobalOrgID = 0 + GlobalOrgID = 0 + GeneralFolderUID = "general" // Permission actions diff --git a/pkg/services/accesscontrol/ossaccesscontrol/permissions_services.go b/pkg/services/accesscontrol/ossaccesscontrol/permissions_services.go index 3ff02acbdea..03a3dee7243 100644 --- a/pkg/services/accesscontrol/ossaccesscontrol/permissions_services.go +++ b/pkg/services/accesscontrol/ossaccesscontrol/permissions_services.go @@ -23,11 +23,11 @@ func ProvidePermissionsServices( if err != nil { return nil, err } - folderPermissions, err := provideFolderService(cfg, router, sql, ac, store) + folderPermissions, err := ProvideFolderPermissions(cfg, router, sql, ac, store) if err != nil { return nil, err } - dashboardPermissions, err := provideDashboardService(cfg, router, sql, ac, store) + dashboardPermissions, err := ProvideDashboardPermissions(cfg, router, sql, ac, store) if err != nil { return nil, err } @@ -141,20 +141,13 @@ func ProvideTeamPermissions( var DashboardViewActions = []string{accesscontrol.ActionDashboardsRead} var DashboardEditActions = append(DashboardViewActions, []string{accesscontrol.ActionDashboardsWrite, accesscontrol.ActionDashboardsDelete}...) var DashboardAdminActions = append(DashboardEditActions, []string{accesscontrol.ActionDashboardsPermissionsRead, accesscontrol.ActionDashboardsPermissionsWrite}...) -var FolderViewActions = []string{dashboards.ActionFoldersRead} -var FolderEditActions = append(FolderViewActions, []string{dashboards.ActionFoldersWrite, dashboards.ActionFoldersDelete, accesscontrol.ActionDashboardsCreate}...) -var FolderAdminActions = append(FolderEditActions, []string{dashboards.ActionFoldersPermissionsRead, dashboards.ActionFoldersPermissionsWrite}...) -func provideDashboardService( +func ProvideDashboardPermissions( cfg *setting.Cfg, router routing.RouteRegister, sql *sqlstore.SQLStore, ac accesscontrol.AccessControl, store resourcepermissions.Store, ) (*resourcepermissions.Service, error) { getDashboard := func(ctx context.Context, orgID int64, resourceID string) (*models.Dashboard, error) { - id, err := strconv.ParseInt(resourceID, 10, 64) - if err != nil { - return nil, err - } - query := &models.GetDashboardQuery{Id: id, OrgId: orgID} + query := &models.GetDashboardQuery{Uid: resourceID, OrgId: orgID} if err := sql.GetDashboard(ctx, query); err != nil { return nil, err } @@ -163,7 +156,7 @@ func provideDashboardService( options := resourcepermissions.Options{ Resource: "dashboards", - ResourceAttribute: "id", + ResourceAttribute: "uid", ResourceValidator: func(ctx context.Context, orgID int64, resourceID string) error { dashboard, err := getDashboard(ctx, orgID, resourceID) if err != nil { @@ -176,23 +169,13 @@ func provideDashboardService( return nil }, - UidSolver: func(ctx context.Context, orgID int64, uid string) (int64, error) { - query := &models.GetDashboardQuery{ - Uid: uid, - OrgId: orgID, - } - if err := sql.GetDashboard(ctx, query); err != nil { - return 0, err - } - return query.Result.Id, nil - }, InheritedScopesSolver: func(ctx context.Context, orgID int64, resourceID string) ([]string, error) { dashboard, err := getDashboard(ctx, orgID, resourceID) if err != nil { return nil, err } if dashboard.FolderId > 0 { - return []string{accesscontrol.GetResourceScope("folders", strconv.FormatInt(dashboard.FolderId, 10))}, nil + return []string{dashboards.ScopeFoldersProvider.GetResourceScopeUID(dashboard.Uid)}, nil } return []string{}, nil }, @@ -214,19 +197,19 @@ func provideDashboardService( return resourcepermissions.New(options, cfg, router, ac, store, sql) } -func provideFolderService( +var FolderViewActions = []string{dashboards.ActionFoldersRead} +var FolderEditActions = append(FolderViewActions, []string{dashboards.ActionFoldersWrite, dashboards.ActionFoldersDelete, accesscontrol.ActionDashboardsCreate}...) +var FolderAdminActions = append(FolderEditActions, []string{dashboards.ActionFoldersPermissionsRead, dashboards.ActionFoldersPermissionsWrite}...) + +func ProvideFolderPermissions( cfg *setting.Cfg, router routing.RouteRegister, sql *sqlstore.SQLStore, accesscontrol accesscontrol.AccessControl, store resourcepermissions.Store, ) (*resourcepermissions.Service, error) { options := resourcepermissions.Options{ Resource: "folders", - ResourceAttribute: "id", + ResourceAttribute: "uid", ResourceValidator: func(ctx context.Context, orgID int64, resourceID string) error { - id, err := strconv.ParseInt(resourceID, 10, 64) - if err != nil { - return err - } - query := &models.GetDashboardQuery{Id: id, OrgId: orgID} + query := &models.GetDashboardQuery{Uid: resourceID, OrgId: orgID} if err := sql.GetDashboard(ctx, query); err != nil { return err } @@ -237,16 +220,6 @@ func provideFolderService( return nil }, - UidSolver: func(ctx context.Context, orgID int64, uid string) (int64, error) { - query := &models.GetDashboardQuery{ - Uid: uid, - OrgId: orgID, - } - if err := sql.GetDashboard(ctx, query); err != nil { - return 0, err - } - return query.Result.Id, nil - }, Assignments: resourcepermissions.Assignments{ Users: true, Teams: true, diff --git a/pkg/services/dashboards/accesscontrol.go b/pkg/services/dashboards/accesscontrol.go index 0244747506e..d362fc270f9 100644 --- a/pkg/services/dashboards/accesscontrol.go +++ b/pkg/services/dashboards/accesscontrol.go @@ -9,6 +9,9 @@ import ( ) const ( + ScopeFoldersRoot = "folders" + ScopeFoldersPrefix = "folders:uid:" + ActionFoldersCreate = "folders:create" ActionFoldersRead = "folders:read" ActionFoldersWrite = "folders:write" @@ -16,7 +19,8 @@ const ( ActionFoldersPermissionsRead = "folders.permissions:read" ActionFoldersPermissionsWrite = "folders.permissions:write" - ScopeFoldersRoot = "folders" + ScopeDashboardsRoot = "dashboards" + ScopeDashboardsPrefix = "dashboards:uid:" ) var ( @@ -24,7 +28,7 @@ var ( ScopeFoldersProvider = ac.NewScopeProvider(ScopeFoldersRoot) ) -// NewNameScopeResolver provides an AttributeScopeResolver that is able to convert a scope prefixed with "folders:name:" into an id based scope. +// NewNameScopeResolver provides an AttributeScopeResolver that is able to convert a scope prefixed with "folders:name:" into an uid based scope. func NewNameScopeResolver(db Store) (string, ac.AttributeScopeResolveFunc) { prefix := ScopeFoldersProvider.GetResourceScopeName("") resolver := func(ctx context.Context, orgID int64, scope string) (string, error) { @@ -39,27 +43,34 @@ func NewNameScopeResolver(db Store) (string, ac.AttributeScopeResolveFunc) { if err != nil { return "", err } - return ScopeFoldersProvider.GetResourceScope(strconv.FormatInt(folder.Id, 10)), nil + return ScopeFoldersProvider.GetResourceScopeUID(folder.Uid), nil } return prefix, resolver } -// NewUidScopeResolver provides an AttributeScopeResolver that is able to convert a scope prefixed with "folders:uid:" into an id based scope. -func NewUidScopeResolver(db Store) (string, ac.AttributeScopeResolveFunc) { - prefix := ScopeFoldersProvider.GetResourceScopeUID("") +// NewIDScopeResolver provides an AttributeScopeResolver that is able to convert a scope prefixed with "folders:id:" into an uid based scope. +func NewIDScopeResolver(db Store) (string, ac.AttributeScopeResolveFunc) { + prefix := ScopeFoldersProvider.GetResourceScope("") resolver := func(ctx context.Context, orgID int64, scope string) (string, error) { if !strings.HasPrefix(scope, prefix) { return "", ac.ErrInvalidScope } - uid := scope[len(prefix):] - if len(uid) == 0 { + + id, err := strconv.ParseInt(scope[len(prefix):], 10, 64) + if err != nil { return "", ac.ErrInvalidScope } - folder, err := db.GetFolderByUID(ctx, orgID, uid) + + if id == 0 { + return ScopeFoldersProvider.GetResourceScopeUID(ac.GeneralFolderUID), nil + } + + folder, err := db.GetFolderByID(ctx, orgID, id) if err != nil { return "", err } - return ScopeFoldersProvider.GetResourceScope(strconv.FormatInt(folder.Id, 10)), nil + + return ScopeFoldersProvider.GetResourceScopeUID(folder.Uid), nil } return prefix, resolver } diff --git a/pkg/services/dashboards/accesscontrol_test.go b/pkg/services/dashboards/accesscontrol_test.go index 69c96f57599..04816092bed 100644 --- a/pkg/services/dashboards/accesscontrol_test.go +++ b/pkg/services/dashboards/accesscontrol_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "math/rand" + "strconv" "testing" "github.com/stretchr/testify/mock" @@ -20,7 +21,7 @@ func TestNewNameScopeResolver(t *testing.T) { require.Equal(t, "folders:name:", prefix) }) - t.Run("resolver should convert to id scope", func(t *testing.T) { + t.Run("resolver should convert to uid scope", func(t *testing.T) { dashboardStore := &FakeDashboardStore{} _, resolver := NewNameScopeResolver(dashboardStore) @@ -30,6 +31,7 @@ func TestNewNameScopeResolver(t *testing.T) { db := models.NewFolder(title) db.Id = rand.Int63() + db.Uid = util.GenerateShortUID() dashboardStore.On("GetFolderByTitle", mock.Anything, mock.Anything, mock.Anything).Return(db, nil).Once() scope := "folders:name:" + title @@ -37,7 +39,7 @@ func TestNewNameScopeResolver(t *testing.T) { resolvedScope, err := resolver(context.Background(), orgId, scope) require.NoError(t, err) - require.Equal(t, fmt.Sprintf("folders:id:%v", db.Id), resolvedScope) + require.Equal(t, fmt.Sprintf("folders:uid:%v", db.Uid), resolvedScope) dashboardStore.AssertCalled(t, "GetFolderByTitle", mock.Anything, orgId, title) }) @@ -71,56 +73,70 @@ func TestNewNameScopeResolver(t *testing.T) { }) } -func TestNewUidScopeResolver(t *testing.T) { +func TestNewIDScopeResolver(t *testing.T) { t.Run("prefix should be expected", func(t *testing.T) { - prefix, _ := NewUidScopeResolver(&FakeDashboardStore{}) - require.Equal(t, "folders:uid:", prefix) + prefix, _ := NewIDScopeResolver(&FakeDashboardStore{}) + require.Equal(t, "folders:id:", prefix) }) - t.Run("resolver should convert to id scope", func(t *testing.T) { + t.Run("resolver should convert to uid scope", func(t *testing.T) { dashboardStore := &FakeDashboardStore{} - _, resolver := NewUidScopeResolver(dashboardStore) + _, resolver := NewIDScopeResolver(dashboardStore) orgId := rand.Int63() uid := util.GenerateShortUID() - db := &models.Folder{Id: rand.Int63()} - dashboardStore.On("GetFolderByUID", mock.Anything, mock.Anything, mock.Anything).Return(db, nil).Once() + db := &models.Folder{Id: rand.Int63(), Uid: uid} + dashboardStore.On("GetFolderByID", mock.Anything, mock.Anything, mock.Anything).Return(db, nil).Once() - scope := "folders:uid:" + uid + scope := "folders:id:" + strconv.FormatInt(db.Id, 10) resolvedScope, err := resolver(context.Background(), orgId, scope) require.NoError(t, err) - require.Equal(t, fmt.Sprintf("folders:id:%v", db.Id), resolvedScope) + require.Equal(t, fmt.Sprintf("folders:uid:%v", db.Uid), resolvedScope) - dashboardStore.AssertCalled(t, "GetFolderByUID", mock.Anything, orgId, uid) + dashboardStore.AssertCalled(t, "GetFolderByID", mock.Anything, orgId, db.Id) }) t.Run("resolver should fail if input scope is not expected", func(t *testing.T) { dashboardStore := &FakeDashboardStore{} - _, resolver := NewUidScopeResolver(dashboardStore) + _, resolver := NewIDScopeResolver(dashboardStore) - _, err := resolver(context.Background(), rand.Int63(), "folders:id:123") + _, err := resolver(context.Background(), rand.Int63(), "folders:uid:123") require.ErrorIs(t, err, ac.ErrInvalidScope) }) + + t.Run("resolver should convert id 0 to general uid scope", func(t *testing.T) { + var ( + dashboardStore = &FakeDashboardStore{} + orgId = rand.Int63() + scope = "folders:id:0" + _, resolver = NewIDScopeResolver(dashboardStore) + ) + + resolvedScope, err := resolver(context.Background(), orgId, scope) + require.NoError(t, err) + + require.Equal(t, "folders:uid:general", resolvedScope) + }) + t.Run("resolver should fail if resource of input scope is empty", func(t *testing.T) { dashboardStore := &FakeDashboardStore{} - _, resolver := NewUidScopeResolver(dashboardStore) + _, resolver := NewIDScopeResolver(dashboardStore) - _, err := resolver(context.Background(), rand.Int63(), "folders:uid:") + _, err := resolver(context.Background(), rand.Int63(), "folders:id:") require.ErrorIs(t, err, ac.ErrInvalidScope) }) t.Run("returns 'not found' if folder does not exist", func(t *testing.T) { dashboardStore := &FakeDashboardStore{} - _, resolver := NewUidScopeResolver(dashboardStore) + _, resolver := NewIDScopeResolver(dashboardStore) orgId := rand.Int63() - dashboardStore.On("GetFolderByUID", mock.Anything, mock.Anything, mock.Anything).Return(nil, models.ErrDashboardNotFound).Once() - - scope := "folders:uid:" + util.GenerateShortUID() + dashboardStore.On("GetFolderByID", mock.Anything, mock.Anything, mock.Anything).Return(nil, models.ErrDashboardNotFound).Once() + scope := "folders:id:10" resolvedScope, err := resolver(context.Background(), orgId, scope) require.ErrorIs(t, err, models.ErrDashboardNotFound) require.Empty(t, resolvedScope) diff --git a/pkg/services/dashboards/database/database.go b/pkg/services/dashboards/database/database.go index a59e04b2e8c..c3d5285f30b 100644 --- a/pkg/services/dashboards/database/database.go +++ b/pkg/services/dashboards/database/database.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "strconv" "time" "github.com/grafana/grafana/pkg/infra/log" @@ -488,8 +487,7 @@ func saveDashboard(sess *sqlstore.DBSession, cmd *models.SaveDashboardCommand) e } // delete existing tags - _, err = sess.Exec("DELETE FROM dashboard_tag WHERE dashboard_id=?", dash.Id) - if err != nil { + if _, err = sess.Exec("DELETE FROM dashboard_tag WHERE dashboard_id=?", dash.Id); err != nil { return err } @@ -724,9 +722,10 @@ func (d *DashboardStore) deleteDashboard(cmd *models.DeleteDashboardCommand, ses deletes = append(deletes, "DELETE FROM dashboard WHERE folder_id = ?") var dashIds []struct { - Id int64 + Id int64 + Uid string } - err := sess.SQL("SELECT id FROM dashboard WHERE folder_id = ?", dashboard.Id).Find(&dashIds) + err := sess.SQL("SELECT id, uid FROM dashboard WHERE folder_id = ?", dashboard.Id).Find(&dashIds) if err != nil { return err } @@ -738,14 +737,14 @@ func (d *DashboardStore) deleteDashboard(cmd *models.DeleteDashboardCommand, ses } // remove all access control permission with folder scope - _, err = sess.Exec("DELETE FROM permission WHERE scope = ?", dashboards.ScopeFoldersProvider.GetResourceScope(strconv.FormatInt(dashboard.Id, 10))) + _, err = sess.Exec("DELETE FROM permission WHERE scope = ?", dashboards.ScopeFoldersProvider.GetResourceScopeUID(dashboard.Uid)) if err != nil { return err } for _, dash := range dashIds { // remove all access control permission with child dashboard scopes - _, err = sess.Exec("DELETE FROM permission WHERE scope = ?", ac.Scope("dashboards", "id", strconv.FormatInt(dash.Id, 10))) + _, err = sess.Exec("DELETE FROM permission WHERE scope = ?", ac.GetResourceScopeUID("dashboards", dash.Uid)) if err != nil { return err } @@ -792,7 +791,7 @@ func (d *DashboardStore) deleteDashboard(cmd *models.DeleteDashboardCommand, ses } } } else { - _, err = sess.Exec("DELETE FROM permission WHERE scope = ?", ac.Scope("dashboards", "id", strconv.FormatInt(dashboard.Id, 10))) + _, err = sess.Exec("DELETE FROM permission WHERE scope = ?", ac.GetResourceScopeUID("dashboards", dashboard.Uid)) if err != nil { return err } diff --git a/pkg/services/dashboards/manager/dashboard_service.go b/pkg/services/dashboards/manager/dashboard_service.go index 95273db95bf..d23bf6fce8a 100644 --- a/pkg/services/dashboards/manager/dashboard_service.go +++ b/pkg/services/dashboards/manager/dashboard_service.go @@ -3,7 +3,6 @@ package service import ( "context" "fmt" - "strconv" "strings" "time" @@ -449,7 +448,6 @@ func (dr *DashboardServiceImpl) GetDashboardsByPluginID(ctx context.Context, que func (dr *DashboardServiceImpl) setDefaultPermissions(ctx context.Context, dto *m.SaveDashboardDTO, dash *models.Dashboard, provisioned bool) error { inFolder := dash.FolderId > 0 if dr.features.IsEnabled(featuremgmt.FlagAccesscontrol) { - resourceID := strconv.FormatInt(dash.Id, 10) var permissions []accesscontrol.SetResourcePermissionCommand if !provisioned { permissions = append(permissions, accesscontrol.SetResourcePermissionCommand{ @@ -469,7 +467,7 @@ func (dr *DashboardServiceImpl) setDefaultPermissions(ctx context.Context, dto * svc = dr.folderPermissions } - _, err := svc.SetPermissions(ctx, dto.OrgId, resourceID, permissions...) + _, err := svc.SetPermissions(ctx, dto.OrgId, dash.Uid, permissions...) if err != nil { return err } diff --git a/pkg/services/dashboards/manager/folder_service.go b/pkg/services/dashboards/manager/folder_service.go index 3d54bee5868..77b31cf69ad 100644 --- a/pkg/services/dashboards/manager/folder_service.go +++ b/pkg/services/dashboards/manager/folder_service.go @@ -3,7 +3,6 @@ package service import ( "context" "errors" - "strconv" "strings" "github.com/grafana/grafana/pkg/infra/log" @@ -34,7 +33,7 @@ func ProvideFolderService( ac accesscontrol.AccessControl, sqlStore sqlstore.Store, ) *FolderServiceImpl { ac.RegisterAttributeScopeResolver(dashboards.NewNameScopeResolver(dashboardStore)) - ac.RegisterAttributeScopeResolver(dashboards.NewUidScopeResolver(dashboardStore)) + ac.RegisterAttributeScopeResolver(dashboards.NewIDScopeResolver(dashboardStore)) return &FolderServiceImpl{ cfg: cfg, @@ -134,7 +133,13 @@ func (f *FolderServiceImpl) GetFolderByTitle(ctx context.Context, user *models.S func (f *FolderServiceImpl) CreateFolder(ctx context.Context, user *models.SignedInUser, orgID int64, title, uid string) (*models.Folder, error) { dashFolder := models.NewDashboardFolder(title) dashFolder.OrgId = orgID - dashFolder.SetUid(strings.TrimSpace(uid)) + + trimmedUID := strings.TrimSpace(uid) + if trimmedUID == accesscontrol.GeneralFolderUID { + return nil, models.ErrFolderInvalidUID + } + + dashFolder.SetUid(trimmedUID) userID := user.UserId if userID == 0 { userID = -1 @@ -167,8 +172,7 @@ func (f *FolderServiceImpl) CreateFolder(ctx context.Context, user *models.Signe var permissionErr error if f.features.IsEnabled(featuremgmt.FlagAccesscontrol) { - resourceID := strconv.FormatInt(folder.Id, 10) - _, permissionErr = f.permissions.SetPermissions(ctx, orgID, resourceID, []accesscontrol.SetResourcePermissionCommand{ + _, permissionErr = f.permissions.SetPermissions(ctx, orgID, folder.Uid, []accesscontrol.SetResourcePermissionCommand{ {UserID: userID, Permission: models.PERMISSION_ADMIN.String()}, {BuiltinRole: string(models.ROLE_EDITOR), Permission: models.PERMISSION_EDIT.String()}, {BuiltinRole: string(models.ROLE_VIEWER), Permission: models.PERMISSION_VIEW.String()}, diff --git a/pkg/services/dashboards/manager/folder_service_test.go b/pkg/services/dashboards/manager/folder_service_test.go index 4388ff45283..69243876a5a 100644 --- a/pkg/services/dashboards/manager/folder_service_test.go +++ b/pkg/services/dashboards/manager/folder_service_test.go @@ -138,6 +138,14 @@ func TestFolderService(t *testing.T) { require.Equal(t, f, actualFolder) }) + t.Run("When creating folder should return error if uid is general", func(t *testing.T) { + dash := models.NewDashboardFolder("Test-Folder") + dash.Id = rand.Int63() + + _, err := service.CreateFolder(context.Background(), user, orgID, dash.Title, "general") + require.ErrorIs(t, err, models.ErrFolderInvalidUID) + }) + t.Run("When updating folder should not return access denied error", func(t *testing.T) { dashboardFolder := models.NewDashboardFolder("Folder") dashboardFolder.Id = rand.Int63() diff --git a/pkg/services/guardian/accesscontrol_guardian.go b/pkg/services/guardian/accesscontrol_guardian.go index f7c80b4ed53..80f4d71e9de 100644 --- a/pkg/services/guardian/accesscontrol_guardian.go +++ b/pkg/services/guardian/accesscontrol_guardian.go @@ -40,6 +40,7 @@ type AccessControlDashboardGuardian struct { log log.Logger dashboardID int64 dashboard *models.Dashboard + parentFolderUID string user *models.SignedInUser store sqlstore.Store ac accesscontrol.AccessControl @@ -52,12 +53,12 @@ func (a *AccessControlDashboardGuardian) CanSave() (bool, error) { } if a.dashboard.IsFolder { - return a.evaluate(accesscontrol.EvalPermission(dashboards.ActionFoldersWrite, folderScope(a.dashboardID))) + return a.evaluate(accesscontrol.EvalPermission(dashboards.ActionFoldersWrite, folderScope(a.dashboard.Uid))) } return a.evaluate(accesscontrol.EvalAny( - accesscontrol.EvalPermission(accesscontrol.ActionDashboardsWrite, dashboardScope(a.dashboard.Id)), - accesscontrol.EvalPermission(accesscontrol.ActionDashboardsWrite, folderScope(a.dashboard.FolderId)), + accesscontrol.EvalPermission(accesscontrol.ActionDashboardsWrite, dashboardScope(a.dashboard.Uid)), + accesscontrol.EvalPermission(accesscontrol.ActionDashboardsWrite, folderScope(a.parentFolderUID)), )) } @@ -65,18 +66,17 @@ func (a *AccessControlDashboardGuardian) CanEdit() (bool, error) { if err := a.loadDashboard(); err != nil { return false, err } - if setting.ViewersCanEdit { return a.CanView() } if a.dashboard.IsFolder { - return a.evaluate(accesscontrol.EvalPermission(dashboards.ActionFoldersWrite, folderScope(a.dashboardID))) + return a.evaluate(accesscontrol.EvalPermission(dashboards.ActionFoldersWrite, folderScope(a.dashboard.Uid))) } return a.evaluate(accesscontrol.EvalAny( - accesscontrol.EvalPermission(accesscontrol.ActionDashboardsWrite, dashboardScope(a.dashboard.Id)), - accesscontrol.EvalPermission(accesscontrol.ActionDashboardsWrite, folderScope(a.dashboard.FolderId)), + accesscontrol.EvalPermission(accesscontrol.ActionDashboardsWrite, dashboardScope(a.dashboard.Uid)), + accesscontrol.EvalPermission(accesscontrol.ActionDashboardsWrite, folderScope(a.parentFolderUID)), )) } @@ -86,12 +86,12 @@ func (a *AccessControlDashboardGuardian) CanView() (bool, error) { } if a.dashboard.IsFolder { - return a.evaluate(accesscontrol.EvalPermission(dashboards.ActionFoldersRead, folderScope(a.dashboardID))) + return a.evaluate(accesscontrol.EvalPermission(dashboards.ActionFoldersRead, folderScope(a.dashboard.Uid))) } return a.evaluate(accesscontrol.EvalAny( - accesscontrol.EvalPermission(accesscontrol.ActionDashboardsRead, dashboardScope(a.dashboard.Id)), - accesscontrol.EvalPermission(accesscontrol.ActionDashboardsRead, folderScope(a.dashboard.FolderId)), + accesscontrol.EvalPermission(accesscontrol.ActionDashboardsRead, dashboardScope(a.dashboard.Uid)), + accesscontrol.EvalPermission(accesscontrol.ActionDashboardsRead, folderScope(a.parentFolderUID)), )) } @@ -102,19 +102,19 @@ func (a *AccessControlDashboardGuardian) CanAdmin() (bool, error) { if a.dashboard.IsFolder { return a.evaluate(accesscontrol.EvalAll( - accesscontrol.EvalPermission(dashboards.ActionFoldersPermissionsRead, folderScope(a.dashboard.Id)), - accesscontrol.EvalPermission(dashboards.ActionFoldersPermissionsWrite, folderScope(a.dashboard.Id)), + accesscontrol.EvalPermission(dashboards.ActionFoldersPermissionsRead, folderScope(a.dashboard.Uid)), + accesscontrol.EvalPermission(dashboards.ActionFoldersPermissionsWrite, folderScope(a.dashboard.Uid)), )) } return a.evaluate(accesscontrol.EvalAny( accesscontrol.EvalAll( - accesscontrol.EvalPermission(accesscontrol.ActionDashboardsPermissionsRead, dashboardScope(a.dashboard.Id)), - accesscontrol.EvalPermission(accesscontrol.ActionDashboardsPermissionsWrite, dashboardScope(a.dashboard.Id)), + accesscontrol.EvalPermission(accesscontrol.ActionDashboardsPermissionsRead, dashboardScope(a.dashboard.Uid)), + accesscontrol.EvalPermission(accesscontrol.ActionDashboardsPermissionsWrite, dashboardScope(a.dashboard.Uid)), ), accesscontrol.EvalAll( - accesscontrol.EvalPermission(accesscontrol.ActionDashboardsPermissionsRead, folderScope(a.dashboard.FolderId)), - accesscontrol.EvalPermission(accesscontrol.ActionDashboardsPermissionsWrite, folderScope(a.dashboard.FolderId)), + accesscontrol.EvalPermission(accesscontrol.ActionDashboardsPermissionsRead, folderScope(a.parentFolderUID)), + accesscontrol.EvalPermission(accesscontrol.ActionDashboardsPermissionsWrite, folderScope(a.parentFolderUID)), ), )) } @@ -125,12 +125,12 @@ func (a *AccessControlDashboardGuardian) CanDelete() (bool, error) { } if a.dashboard.IsFolder { - return a.evaluate(accesscontrol.EvalPermission(dashboards.ActionFoldersDelete, folderScope(a.dashboard.Id))) + return a.evaluate(accesscontrol.EvalPermission(dashboards.ActionFoldersDelete, folderScope(a.dashboard.Uid))) } return a.evaluate(accesscontrol.EvalAny( - accesscontrol.EvalPermission(accesscontrol.ActionDashboardsDelete, dashboardScope(a.dashboard.Id)), - accesscontrol.EvalPermission(accesscontrol.ActionDashboardsDelete, folderScope(a.dashboard.FolderId)), + accesscontrol.EvalPermission(accesscontrol.ActionDashboardsDelete, dashboardScope(a.dashboard.Uid)), + accesscontrol.EvalPermission(accesscontrol.ActionDashboardsDelete, folderScope(a.parentFolderUID)), )) } @@ -138,8 +138,11 @@ func (a *AccessControlDashboardGuardian) CanCreate(folderID int64, isFolder bool if isFolder { return a.evaluate(accesscontrol.EvalPermission(dashboards.ActionFoldersCreate)) } - - return a.evaluate(accesscontrol.EvalPermission(accesscontrol.ActionDashboardsCreate, folderScope(folderID))) + folder, err := a.loadParentFolder(folderID) + if err != nil { + return false, err + } + return a.evaluate(accesscontrol.EvalPermission(accesscontrol.ActionDashboardsCreate, folderScope(folder.Uid))) } func (a *AccessControlDashboardGuardian) evaluate(evaluator accesscontrol.Evaluator) (bool, error) { @@ -258,15 +261,33 @@ func (a *AccessControlDashboardGuardian) loadDashboard() error { if err := a.store.GetDashboard(a.ctx, query); err != nil { return err } + if !query.Result.IsFolder { + folder, err := a.loadParentFolder(query.Result.FolderId) + if err != nil { + return err + } + a.parentFolderUID = folder.Uid + } a.dashboard = query.Result } return nil } -func dashboardScope(dashboardID int64) string { - return accesscontrol.Scope("dashboards", "id", strconv.FormatInt(dashboardID, 10)) +func (a *AccessControlDashboardGuardian) loadParentFolder(folderID int64) (*models.Dashboard, error) { + if folderID == 0 { + return &models.Dashboard{Uid: accesscontrol.GeneralFolderUID}, nil + } + folderQuery := &models.GetDashboardQuery{Id: folderID, OrgId: a.user.OrgId} + if err := a.store.GetDashboard(a.ctx, folderQuery); err != nil { + return nil, err + } + return folderQuery.Result, nil } -func folderScope(folderID int64) string { - return dashboards.ScopeFoldersProvider.GetResourceScope(strconv.FormatInt(folderID, 10)) +func dashboardScope(uid string) string { + return accesscontrol.GetResourceScopeUID("dashboards", uid) +} + +func folderScope(uid string) string { + return dashboards.ScopeFoldersProvider.GetResourceScopeUID(uid) } diff --git a/pkg/services/guardian/accesscontrol_guardian_test.go b/pkg/services/guardian/accesscontrol_guardian_test.go index 5104b8da443..b38672d8fcc 100644 --- a/pkg/services/guardian/accesscontrol_guardian_test.go +++ b/pkg/services/guardian/accesscontrol_guardian_test.go @@ -10,7 +10,6 @@ import ( "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/api/routing" - "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/database" @@ -24,7 +23,7 @@ import ( type accessControlGuardianTestCase struct { desc string - dashboardID int64 + dashUID string permissions []*accesscontrol.Permission viewersCanEdit bool expected bool @@ -33,8 +32,8 @@ type accessControlGuardianTestCase struct { func TestAccessControlDashboardGuardian_CanSave(t *testing.T) { tests := []accessControlGuardianTestCase{ { - desc: "should be able to save with dashboard wildcard scope", - dashboardID: 1, + desc: "should be able to save with dashboard wildcard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsWrite, @@ -44,8 +43,8 @@ func TestAccessControlDashboardGuardian_CanSave(t *testing.T) { expected: true, }, { - desc: "should be able to save with folder wildcard scope", - dashboardID: 1, + desc: "should be able to save with folder wildcard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsWrite, @@ -55,45 +54,45 @@ func TestAccessControlDashboardGuardian_CanSave(t *testing.T) { expected: true, }, { - desc: "should be able to save with dashboard scope", - dashboardID: 1, + desc: "should be able to save with dashboard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsWrite, - Scope: "dashboards:id:1", + Scope: "dashboards:uid:1", }, }, expected: true, }, { - desc: "should be able to save with folder scope", - dashboardID: 1, + desc: "should be able to save with folder scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsWrite, - Scope: "folders:id:0", + Scope: "folders:uid:general", }, }, expected: true, }, { - desc: "should not be able to save with incorrect dashboard scope", - dashboardID: 1, + desc: "should not be able to save with incorrect dashboard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsWrite, - Scope: "dashboards:id:10", + Scope: "dashboards:uid:10", }, }, expected: false, }, { - desc: "should not be able to save with incorrect folder scope", - dashboardID: 1, + desc: "should not be able to save with incorrect folder scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsWrite, - Scope: "folders:id:10", + Scope: "folders:uid:100", }, }, expected: false, @@ -102,7 +101,7 @@ func TestAccessControlDashboardGuardian_CanSave(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - guardian := setupAccessControlGuardianTest(t, tt.dashboardID, tt.permissions) + guardian, _ := setupAccessControlGuardianTest(t, tt.dashUID, tt.permissions) can, err := guardian.CanSave() require.NoError(t, err) @@ -110,12 +109,11 @@ func TestAccessControlDashboardGuardian_CanSave(t *testing.T) { }) } } - func TestAccessControlDashboardGuardian_CanEdit(t *testing.T) { tests := []accessControlGuardianTestCase{ { - desc: "should be able to edit with dashboard wildcard scope", - dashboardID: 1, + desc: "should be able to edit with dashboard wildcard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsWrite, @@ -125,8 +123,8 @@ func TestAccessControlDashboardGuardian_CanEdit(t *testing.T) { expected: true, }, { - desc: "should be able to edit with folder wildcard scope", - dashboardID: 1, + desc: "should be able to edit with folder wildcard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsWrite, @@ -136,56 +134,56 @@ func TestAccessControlDashboardGuardian_CanEdit(t *testing.T) { expected: true, }, { - desc: "should be able to edit with dashboard scope", - dashboardID: 1, + desc: "should be able to edit with dashboard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsWrite, - Scope: "dashboards:id:1", + Scope: "dashboards:uid:1", }, }, expected: true, }, { - desc: "should be able to edit with folder scope", - dashboardID: 1, + desc: "should be able to edit with folder scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsWrite, - Scope: "folders:id:0", + Scope: "folders:uid:general", }, }, expected: true, }, { - desc: "should not be able to edit with incorrect dashboard scope", - dashboardID: 1, + desc: "should not be able to edit with incorrect dashboard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsWrite, - Scope: "dashboards:id:10", + Scope: "dashboards:uid:10", }, }, expected: false, }, { - desc: "should not be able to edit with incorrect folder scope", - dashboardID: 1, + desc: "should not be able to edit with incorrect folder scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsWrite, - Scope: "folders:id:10", + Scope: "folders:uid:10", }, }, expected: false, }, { - desc: "should be able to edit with read action when viewer_can_edit is true", - dashboardID: 1, + desc: "should be able to edit with read action when viewer_can_edit is true", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsRead, - Scope: "dashboards:id:1", + Scope: "dashboards:uid:1", }, }, viewersCanEdit: true, @@ -195,25 +193,23 @@ func TestAccessControlDashboardGuardian_CanEdit(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - guardian := setupAccessControlGuardianTest(t, tt.dashboardID, tt.permissions) + guardian, _ := setupAccessControlGuardianTest(t, tt.dashUID, tt.permissions) if tt.viewersCanEdit { setting.ViewersCanEdit = true defer func() { setting.ViewersCanEdit = false }() } - can, err := guardian.CanEdit() require.NoError(t, err) assert.Equal(t, tt.expected, can) }) } } - func TestAccessControlDashboardGuardian_CanView(t *testing.T) { tests := []accessControlGuardianTestCase{ { - desc: "should be able to view with dashboard wildcard scope", - dashboardID: 1, + desc: "should be able to view with dashboard wildcard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsRead, @@ -223,8 +219,8 @@ func TestAccessControlDashboardGuardian_CanView(t *testing.T) { expected: true, }, { - desc: "should be able to view with folder wildcard scope", - dashboardID: 1, + desc: "should be able to view with folder wildcard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsRead, @@ -234,45 +230,45 @@ func TestAccessControlDashboardGuardian_CanView(t *testing.T) { expected: true, }, { - desc: "should be able to view with dashboard scope", - dashboardID: 1, + desc: "should be able to view with dashboard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsRead, - Scope: "dashboards:id:1", + Scope: "dashboards:uid:1", }, }, expected: true, }, { - desc: "should be able to view with folder scope", - dashboardID: 1, + desc: "should be able to view with folder scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsRead, - Scope: "folders:id:0", + Scope: "folders:uid:general", }, }, expected: true, }, { - desc: "should not be able to view with incorrect dashboard scope", - dashboardID: 1, + desc: "should not be able to view with incorrect dashboard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsRead, - Scope: "dashboards:id:10", + Scope: "dashboards:uid:10", }, }, expected: false, }, { - desc: "should not be able to view with incorrect folder scope", - dashboardID: 1, + desc: "should not be able to view with incorrect folder scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsRead, - Scope: "folders:id:10", + Scope: "folders:uid:10", }, }, expected: false, @@ -281,7 +277,7 @@ func TestAccessControlDashboardGuardian_CanView(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - guardian := setupAccessControlGuardianTest(t, tt.dashboardID, tt.permissions) + guardian, _ := setupAccessControlGuardianTest(t, tt.dashUID, tt.permissions) can, err := guardian.CanView() require.NoError(t, err) @@ -289,12 +285,11 @@ func TestAccessControlDashboardGuardian_CanView(t *testing.T) { }) } } - func TestAccessControlDashboardGuardian_CanAdmin(t *testing.T) { tests := []accessControlGuardianTestCase{ { - desc: "should be able to admin with dashboard wildcard scope", - dashboardID: 1, + desc: "should be able to admin with dashboard wildcard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsPermissionsRead, @@ -308,8 +303,8 @@ func TestAccessControlDashboardGuardian_CanAdmin(t *testing.T) { expected: true, }, { - desc: "should be able to admin with folder wildcard scope", - dashboardID: 1, + desc: "should be able to admin with folder wildcard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsPermissionsRead, @@ -323,61 +318,61 @@ func TestAccessControlDashboardGuardian_CanAdmin(t *testing.T) { expected: true, }, { - desc: "should be able to admin with dashboard scope", - dashboardID: 1, + desc: "should be able to admin with dashboard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsPermissionsRead, - Scope: "dashboards:id:1", + Scope: "dashboards:uid:1", }, { Action: accesscontrol.ActionDashboardsPermissionsWrite, - Scope: "dashboards:id:1", + Scope: "dashboards:uid:1", }, }, expected: true, }, { - desc: "should be able to admin with folder scope", - dashboardID: 1, + desc: "should be able to admin with folder scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsPermissionsRead, - Scope: "folders:id:0", + Scope: "folders:uid:general", }, { Action: accesscontrol.ActionDashboardsPermissionsWrite, - Scope: "folders:id:0", + Scope: "folders:uid:general", }, }, expected: true, }, { - desc: "should not be able to admin with incorrect dashboard scope", - dashboardID: 1, + desc: "should not be able to admin with incorrect dashboard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsPermissionsRead, - Scope: "dashboards:id:10", + Scope: "dashboards:uid:10", }, { Action: accesscontrol.ActionDashboardsPermissionsWrite, - Scope: "dashboards:id:10", + Scope: "dashboards:uid:10", }, }, expected: false, }, { - desc: "should not be able to admin with incorrect folder scope", - dashboardID: 1, + desc: "should not be able to admin with incorrect folder scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsPermissionsRead, - Scope: "folders:id:10", + Scope: "folders:uid:10", }, { Action: accesscontrol.ActionDashboardsPermissionsWrite, - Scope: "folders:id:10", + Scope: "folders:uid:10", }, }, expected: false, @@ -386,7 +381,7 @@ func TestAccessControlDashboardGuardian_CanAdmin(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - guardian := setupAccessControlGuardianTest(t, tt.dashboardID, tt.permissions) + guardian, _ := setupAccessControlGuardianTest(t, tt.dashUID, tt.permissions) can, err := guardian.CanAdmin() require.NoError(t, err) @@ -394,12 +389,11 @@ func TestAccessControlDashboardGuardian_CanAdmin(t *testing.T) { }) } } - func TestAccessControlDashboardGuardian_CanDelete(t *testing.T) { tests := []accessControlGuardianTestCase{ { - desc: "should be able to delete with dashboard wildcard scope", - dashboardID: 1, + desc: "should be able to delete with dashboard wildcard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsDelete, @@ -409,8 +403,8 @@ func TestAccessControlDashboardGuardian_CanDelete(t *testing.T) { expected: true, }, { - desc: "should be able to delete with folder wildcard scope", - dashboardID: 1, + desc: "should be able to delete with folder wildcard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsDelete, @@ -420,45 +414,45 @@ func TestAccessControlDashboardGuardian_CanDelete(t *testing.T) { expected: true, }, { - desc: "should be able to delete with dashboard scope", - dashboardID: 1, + desc: "should be able to delete with dashboard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsDelete, - Scope: "dashboards:id:1", + Scope: "dashboards:uid:1", }, }, expected: true, }, { - desc: "should be able to delete with folder scope", - dashboardID: 1, + desc: "should be able to delete with folder scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsDelete, - Scope: "folders:id:0", + Scope: "folders:uid:general", }, }, expected: true, }, { - desc: "should not be able to delete with incorrect dashboard scope", - dashboardID: 1, + desc: "should not be able to delete with incorrect dashboard scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsDelete, - Scope: "dashboards:id:10", + Scope: "dashboards:uid:10", }, }, expected: false, }, { - desc: "should not be able to delete with incorrect folder scope", - dashboardID: 1, + desc: "should not be able to delete with incorrect folder scope", + dashUID: "1", permissions: []*accesscontrol.Permission{ { Action: accesscontrol.ActionDashboardsDelete, - Scope: "folders:id:10", + Scope: "folders:uid:10", }, }, expected: false, @@ -467,7 +461,7 @@ func TestAccessControlDashboardGuardian_CanDelete(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - guardian := setupAccessControlGuardianTest(t, tt.dashboardID, tt.permissions) + guardian, _ := setupAccessControlGuardianTest(t, tt.dashUID, tt.permissions) can, err := guardian.CanDelete() require.NoError(t, err) @@ -487,18 +481,18 @@ type accessControlGuardianCanCreateTestCase struct { func TestAccessControlDashboardGuardian_CanCreate(t *testing.T) { tests := []accessControlGuardianCanCreateTestCase{ { - desc: "should be able to create dashboard in folder 0", + desc: "should be able to create dashboard in general folder", isFolder: false, folderID: 0, permissions: []*accesscontrol.Permission{ - {Action: accesscontrol.ActionDashboardsCreate, Scope: "folders:id:0"}, + {Action: accesscontrol.ActionDashboardsCreate, Scope: "folders:uid:general"}, }, expected: true, }, { desc: "should be able to create dashboard in any folder", isFolder: false, - folderID: 100, + folderID: 0, permissions: []*accesscontrol.Permission{ {Action: accesscontrol.ActionDashboardsCreate, Scope: "folders:*"}, }, @@ -507,7 +501,7 @@ func TestAccessControlDashboardGuardian_CanCreate(t *testing.T) { { desc: "should not be able to create dashboard without permissions", isFolder: false, - folderID: 100, + folderID: 0, permissions: []*accesscontrol.Permission{}, expected: false, }, @@ -523,7 +517,7 @@ func TestAccessControlDashboardGuardian_CanCreate(t *testing.T) { { desc: "should not be able to create folders without permissions", isFolder: true, - folderID: 100, + folderID: 0, permissions: []*accesscontrol.Permission{}, expected: false, }, @@ -531,7 +525,7 @@ func TestAccessControlDashboardGuardian_CanCreate(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - guardian := setupAccessControlGuardianTest(t, 0, tt.permissions) + guardian, _ := setupAccessControlGuardianTest(t, "0", tt.permissions) can, err := guardian.CanCreate(tt.folderID, tt.isFolder) require.NoError(t, err) @@ -563,16 +557,14 @@ func TestAccessControlDashboardGuardian_GetHiddenACL(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - guardian := setupAccessControlGuardianTest(t, 1, nil) + guardian, _ := setupAccessControlGuardianTest(t, "1", nil) mocked := accesscontrolmock.NewPermissionsServicesMock() guardian.permissionServices = mocked mocked.Dashboards.On("MapActions", mock.Anything).Return("View") mocked.Dashboards.On("GetPermissions", mock.Anything, mock.Anything, mock.Anything).Return(tt.permissions, nil) - cfg := setting.NewCfg() cfg.HiddenUsers = tt.hiddenUsers - permissions, err := guardian.GetHiddenACL(cfg) require.NoError(t, err) var hiddenUserNames []string @@ -587,21 +579,24 @@ func TestAccessControlDashboardGuardian_GetHiddenACL(t *testing.T) { } } -func setupAccessControlGuardianTest(t *testing.T, dashID int64, permissions []*accesscontrol.Permission) *AccessControlDashboardGuardian { +func setupAccessControlGuardianTest(t *testing.T, uid string, permissions []*accesscontrol.Permission) (*AccessControlDashboardGuardian, *models.Dashboard) { t.Helper() store := sqlstore.InitTestDB(t) + + toSave := models.NewDashboard(uid) + toSave.SetUid(uid) + // seed dashboard - _, err := dashdb.ProvideDashboardStore(store).SaveDashboard(models.SaveDashboardCommand{ - Dashboard: &simplejson.Json{}, + dash, err := dashdb.ProvideDashboardStore(store).SaveDashboard(models.SaveDashboardCommand{ + Dashboard: toSave.Data, UserId: 1, OrgId: 1, FolderId: 0, }) require.NoError(t, err) - ac := accesscontrolmock.New().WithPermissions(permissions) services, err := ossaccesscontrol.ProvidePermissionsServices(setting.NewCfg(), routing.NewRouteRegister(), store, ac, database.ProvideService(store)) require.NoError(t, err) - return NewAccessControlDashboardGuardian(context.Background(), dashID, &models.SignedInUser{OrgId: 1}, store, ac, services) + return NewAccessControlDashboardGuardian(context.Background(), dash.Id, &models.SignedInUser{OrgId: 1}, store, ac, services), dash } diff --git a/pkg/services/provisioning/dashboards/file_reader.go b/pkg/services/provisioning/dashboards/file_reader.go index 61c8b83e9b9..36dfdf7c541 100644 --- a/pkg/services/provisioning/dashboards/file_reader.go +++ b/pkg/services/provisioning/dashboards/file_reader.go @@ -15,6 +15,7 @@ import ( "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/infra/log" "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/util" ) @@ -311,6 +312,9 @@ func (fr *FileReader) getOrCreateFolderID(ctx context.Context, cfg *config, serv dash.Overwrite = true dash.OrgId = cfg.OrgID // set dashboard folderUid if given + if cfg.FolderUID == accesscontrol.GeneralFolderUID { + return 0, models.ErrFolderInvalidUID + } dash.Dashboard.SetUid(cfg.FolderUID) dbDash, err := service.SaveFolderForProvisionedDashboards(ctx, dash) if err != nil { diff --git a/pkg/services/provisioning/dashboards/file_reader_test.go b/pkg/services/provisioning/dashboards/file_reader_test.go index 80124825fa6..8e6b01f55d1 100644 --- a/pkg/services/provisioning/dashboards/file_reader_test.go +++ b/pkg/services/provisioning/dashboards/file_reader_test.go @@ -390,6 +390,26 @@ func TestDashboardFileReader(t *testing.T) { require.NoError(t, err) }) + t.Run("should not create dashboard folder with uid general", func(t *testing.T) { + setup() + cfg := &config{ + Name: "DefaultB", + Type: "file", + OrgID: 1, + Folder: "TEAM B", + FolderUID: "general", + Options: map[string]interface{}{ + "folder": defaultDashboards, + }, + } + + r, err := NewDashboardFileReader(cfg, logger, nil) + require.NoError(t, err) + + _, err = r.getOrCreateFolderID(context.Background(), cfg, fakeService, cfg.Folder) + require.ErrorIs(t, err, models.ErrFolderInvalidUID) + }) + t.Run("Walking the folder with dashboards", func(t *testing.T) { setup() noFiles := map[string]os.FileInfo{} diff --git a/pkg/services/sqlstore/migrations/accesscontrol/dashboard_permissions.go b/pkg/services/sqlstore/migrations/accesscontrol/dashboard_permissions.go index f203efbaaba..ea76bb15232 100644 --- a/pkg/services/sqlstore/migrations/accesscontrol/dashboard_permissions.go +++ b/pkg/services/sqlstore/migrations/accesscontrol/dashboard_permissions.go @@ -21,7 +21,6 @@ var dashboardPermissionTranslation = map[models.PermissionType][]string{ models.PERMISSION_EDIT: { ac.ActionDashboardsRead, ac.ActionDashboardsWrite, - ac.ActionDashboardsCreate, ac.ActionDashboardsDelete, }, models.PERMISSION_ADMIN: { @@ -39,6 +38,7 @@ var folderPermissionTranslation = map[models.PermissionType][]string{ dashboards.ActionFoldersRead, }...), models.PERMISSION_EDIT: append(dashboardPermissionTranslation[models.PERMISSION_EDIT], []string{ + ac.ActionDashboardsCreate, dashboards.ActionFoldersRead, dashboards.ActionFoldersWrite, dashboards.ActionFoldersCreate, @@ -56,6 +56,7 @@ var folderPermissionTranslation = map[models.PermissionType][]string{ func AddDashboardPermissionsMigrator(mg *migrator.Migrator) { mg.AddMigration("dashboard permissions", &dashboardPermissionsMigrator{}) + mg.AddMigration("dashboard permissions uid scopes", &dashboardUidPermissionMigrator{}) } var _ migrator.CodeMigration = new(dashboardPermissionsMigrator) @@ -219,3 +220,63 @@ func getRoleName(p models.DashboardAcl) string { } return fmt.Sprintf("managed:builtins:%s:permissions", strings.ToLower(string(*p.Role))) } + +var _ migrator.CodeMigration = new(dashboardUidPermissionMigrator) + +type dashboardUidPermissionMigrator struct { + migrator.MigrationBase +} + +func (d *dashboardUidPermissionMigrator) SQL(dialect migrator.Dialect) string { + return "code migration" +} + +func (d *dashboardUidPermissionMigrator) Exec(sess *xorm.Session, migrator *migrator.Migrator) error { + if err := d.migrateWildcards(sess); err != nil { + return err + } + return d.migrateIdScopes(sess) +} + +func (d *dashboardUidPermissionMigrator) migrateWildcards(sess *xorm.Session) error { + if _, err := sess.Exec("DELETE FROM permission WHERE action = 'dashboards:create' AND scope LIKE 'dashboards%'"); err != nil { + return err + } + if _, err := sess.Exec("UPDATE permission SET scope = 'dashboards:uid:*' WHERE scope = 'dashboards:id:*'"); err != nil { + return err + } + if _, err := sess.Exec("UPDATE permission SET scope = 'folders:uid:*' WHERE scope = 'folders:id:*'"); err != nil { + return err + } + return nil +} + +func (d *dashboardUidPermissionMigrator) migrateIdScopes(sess *xorm.Session) error { + type dashboard struct { + ID int64 `xorm:"id"` + UID string `xorm:"uid"` + IsFolder bool + } + var dashboards []dashboard + if err := sess.SQL("SELECT id, uid, is_folder FROM dashboard").Find(&dashboards); err != nil { + return err + } + + for _, d := range dashboards { + var idScope string + var uidScope string + + if d.IsFolder { + idScope = ac.Scope("folders", "id", strconv.FormatInt(d.ID, 10)) + uidScope = ac.Scope("folders", "uid", d.UID) + } else { + idScope = ac.Scope("dashboards", "id", strconv.FormatInt(d.ID, 10)) + uidScope = ac.Scope("dashboards", "uid", d.UID) + } + + if _, err := sess.Exec("UPDATE permission SET scope = ? WHERE scope = ?", uidScope, idScope); err != nil { + return err + } + } + return nil +} diff --git a/pkg/services/sqlstore/permissions/dashboard.go b/pkg/services/sqlstore/permissions/dashboard.go index 707fce60812..c4ff0978446 100644 --- a/pkg/services/sqlstore/permissions/dashboard.go +++ b/pkg/services/sqlstore/permissions/dashboard.go @@ -110,15 +110,16 @@ func (f AccessControlDashboardPermissionFilter) Where() (string, []interface{}) if len(f.dashboardActions) > 0 { builder.WriteString("((") - dashFilter, _ := accesscontrol.Filter(f.User, "dashboard.id", "dashboards:id:", f.dashboardActions...) + + dashFilter, _ := accesscontrol.Filter(f.User, "dashboard.uid", dashboards.ScopeDashboardsPrefix, f.dashboardActions...) builder.WriteString(dashFilter.Where) args = append(args, dashFilter.Args...) - builder.WriteString(" OR ") + builder.WriteString(" OR dashboard.folder_id IN(SELECT id FROM dashboard WHERE ") + dashFolderFilter, _ := accesscontrol.Filter(f.User, "dashboard.uid", dashboards.ScopeFoldersPrefix, f.dashboardActions...) - dashFolderFilter, _ := accesscontrol.Filter(f.User, "dashboard.folder_id", "folders:id:", f.dashboardActions...) builder.WriteString(dashFolderFilter.Where) - builder.WriteString(") AND NOT dashboard.is_folder)") + builder.WriteString(")) AND NOT dashboard.is_folder)") args = append(args, dashFolderFilter.Args...) } @@ -127,12 +128,11 @@ func (f AccessControlDashboardPermissionFilter) Where() (string, []interface{}) builder.WriteString(" OR ") } builder.WriteString("(") - folderFilter, _ := accesscontrol.Filter(f.User, "dashboard.id", "folders:id:", f.folderActions...) + folderFilter, _ := accesscontrol.Filter(f.User, "dashboard.uid", dashboards.ScopeFoldersPrefix, f.folderActions...) builder.WriteString(folderFilter.Where) builder.WriteString(" AND dashboard.is_folder)") args = append(args, folderFilter.Args...) } - builder.WriteString(")") return builder.String(), args } diff --git a/pkg/services/sqlstore/permissions/dashboard_test.go b/pkg/services/sqlstore/permissions/dashboard_test.go index ab4684246f3..e514a783ce1 100644 --- a/pkg/services/sqlstore/permissions/dashboard_test.go +++ b/pkg/services/sqlstore/permissions/dashboard_test.go @@ -108,7 +108,7 @@ func TestAccessControlDashboardPermissionFilter_Where(t *testing.T) { title: "folder and dashboard actions are defined", dashboardActions: []string{"test"}, folderActions: []string{"test"}, - expectedResult: "((( 1 = 0 OR 1 = 0) AND NOT dashboard.is_folder) OR ( 1 = 0 AND dashboard.is_folder))", + expectedResult: "((( 1 = 0 OR dashboard.folder_id IN(SELECT id FROM dashboard WHERE 1 = 0)) AND NOT dashboard.is_folder) OR ( 1 = 0 AND dashboard.is_folder))", }, { title: "folder actions are defined but not dashboard actions", @@ -120,7 +120,7 @@ func TestAccessControlDashboardPermissionFilter_Where(t *testing.T) { title: "dashboard actions are defined but not folder actions", dashboardActions: []string{"test"}, folderActions: nil, - expectedResult: "((( 1 = 0 OR 1 = 0) AND NOT dashboard.is_folder))", + expectedResult: "((( 1 = 0 OR dashboard.folder_id IN(SELECT id FROM dashboard WHERE 1 = 0)) AND NOT dashboard.is_folder))", }, { title: "dashboard actions are defined but not folder actions",