From 5d7433d820b6571f26506d565e89e4f914b9b91d Mon Sep 17 00:00:00 2001 From: Ieva Date: Fri, 21 Apr 2023 15:05:11 +0100 Subject: [PATCH] RBAC: Make access control metadata for folders work with nested folders (#66464) * remove metadata for single folder listing * extendTests * remove ac metadata from dash and folder search results * remove test * remove one more test * put ac metadata back for single folder API responses * extend tests * remove ac metadata from folder frontend object * undo unneeded change * PR feedback Co-authored-by: Gabriel MABILLE --------- Co-authored-by: Gabriel MABILLE --- pkg/api/dtos/folder.go | 9 +- pkg/api/folder.go | 40 +++++-- pkg/api/folder_test.go | 64 +++++------ pkg/api/search.go | 42 +------- pkg/api/search_test.go | 101 ------------------ .../core/components/Select/FolderPicker.tsx | 10 +- .../components/rule-editor/FolderAndGroup.tsx | 35 +----- .../rule-editor/RuleFolderPicker.tsx | 6 +- .../manage-dashboards/state/actions.ts | 7 +- public/app/types/acl.ts | 6 ++ 10 files changed, 87 insertions(+), 233 deletions(-) delete mode 100644 pkg/api/search_test.go diff --git a/pkg/api/dtos/folder.go b/pkg/api/dtos/folder.go index 618a64a0d45..a35167db214 100644 --- a/pkg/api/dtos/folder.go +++ b/pkg/api/dtos/folder.go @@ -27,9 +27,8 @@ type Folder struct { } type FolderSearchHit struct { - Id int64 `json:"id"` - Uid string `json:"uid"` - Title string `json:"title"` - AccessControl accesscontrol.Metadata `json:"accessControl,omitempty"` - ParentUID string `json:"parentUid,omitempty"` + Id int64 `json:"id"` + Uid string `json:"uid"` + Title string `json:"title"` + ParentUID string `json:"parentUid,omitempty"` } diff --git a/pkg/api/folder.go b/pkg/api/folder.go index ab233d61f6d..e856cc26c9d 100644 --- a/pkg/api/folder.go +++ b/pkg/api/folder.go @@ -54,10 +54,8 @@ func (hs *HTTPServer) GetFolders(c *contextmodel.ReqContext) response.Response { return apierrors.ToFolderErrorResponse(err) } - uids := make(map[string]bool, len(folders)) result := make([]dtos.FolderSearchHit, 0) for _, f := range folders { - uids[f.UID] = true result = append(result, dtos.FolderSearchHit{ Id: f.ID, Uid: f.UID, @@ -66,13 +64,6 @@ func (hs *HTTPServer) GetFolders(c *contextmodel.ReqContext) response.Response { }) } - metadata := hs.getMultiAccessControlMetadata(c, c.OrgID, dashboards.ScopeFoldersPrefix, uids) - if len(metadata) > 0 { - for i := range result { - result[i].AccessControl = metadata[result[i].Uid] - } - } - return response.JSON(http.StatusOK, result) } @@ -318,6 +309,8 @@ func (hs *HTTPServer) newToFolderDto(c *contextmodel.ReqContext, g guardian.Dash updater = hs.getUserLogin(c.Req.Context(), folder.UpdatedBy) } + acMetadata, _ := hs.getFolderACMetadata(c, folder) + return dtos.Folder{ Id: folder.ID, Uid: folder.UID, @@ -333,11 +326,38 @@ func (hs *HTTPServer) newToFolderDto(c *contextmodel.ReqContext, g guardian.Dash UpdatedBy: updater, Updated: folder.Updated, Version: folder.Version, - AccessControl: hs.getAccessControlMetadata(c, c.OrgID, dashboards.ScopeFoldersPrefix, folder.UID), + AccessControl: acMetadata, ParentUID: folder.ParentUID, } } +func (hs *HTTPServer) getFolderACMetadata(c *contextmodel.ReqContext, f *folder.Folder) (accesscontrol.Metadata, error) { + if hs.AccessControl.IsDisabled() || !c.QueryBool("accesscontrol") { + return nil, nil + } + + parents, err := hs.folderService.GetParents(c.Req.Context(), folder.GetParentsQuery{UID: f.UID, OrgID: c.OrgID}) + if err != nil { + return nil, err + } + + folderIDs := map[string]bool{f.UID: true} + for _, p := range parents { + folderIDs[p.UID] = true + } + + allMetadata := hs.getMultiAccessControlMetadata(c, c.OrgID, dashboards.ScopeFoldersPrefix, folderIDs) + metadata := allMetadata[f.UID] + + // Flatten metadata - if any parent has a permission, the child folder inherits it + for _, md := range allMetadata { + for action := range md { + metadata[action] = true + } + } + return metadata, nil +} + func (hs *HTTPServer) searchFolders(c *contextmodel.ReqContext) ([]*folder.Folder, error) { searchQuery := search.Query{ SignedInUser: c.SignedInUser, diff --git a/pkg/api/folder_test.go b/pkg/api/folder_test.go index 6640397e63e..196023bb6ec 100644 --- a/pkg/api/folder_test.go +++ b/pkg/api/folder_test.go @@ -271,6 +271,7 @@ func testDescription(description string, expectedErr error) string { func TestHTTPServer_FolderMetadata(t *testing.T) { setUpRBACGuardian(t) folderService := &foldertest.FakeService{} + features := featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders) server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = &setting.Cfg{ RBACEnabled: true, @@ -280,35 +281,7 @@ func TestHTTPServer_FolderMetadata(t *testing.T) { hs.SearchService = &mockSearchService{ ExpectedResult: model.HitList{}, } - }) - - t.Run("Should attach access control metadata to multiple folders", func(t *testing.T) { - folderService.ExpectedFolders = []*folder.Folder{{UID: "1"}, {UID: "2"}, {UID: "3"}} - - req := server.NewGetRequest("/api/folders?accesscontrol=true") - webtest.RequestWithSignedInUser(req, &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{ - 1: accesscontrol.GroupScopesByAction([]accesscontrol.Permission{ - {Action: dashboards.ActionFoldersRead, Scope: dashboards.ScopeFoldersAll}, - {Action: dashboards.ActionFoldersWrite, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID("2")}, - }), - }}) - - res, err := server.Send(req) - require.NoError(t, err) - defer func() { require.NoError(t, res.Body.Close()) }() - assert.Equal(t, http.StatusOK, res.StatusCode) - - body := []dtos.FolderSearchHit{} - require.NoError(t, json.NewDecoder(res.Body).Decode(&body)) - - for _, f := range body { - assert.True(t, f.AccessControl[dashboards.ActionFoldersRead]) - if f.Uid == "2" { - assert.True(t, f.AccessControl[dashboards.ActionFoldersWrite]) - } else { - assert.False(t, f.AccessControl[dashboards.ActionFoldersWrite]) - } - } + hs.Features = features }) t.Run("Should attach access control metadata to folder response", func(t *testing.T) { @@ -334,7 +307,38 @@ func TestHTTPServer_FolderMetadata(t *testing.T) { assert.True(t, body.AccessControl[dashboards.ActionFoldersWrite]) }) - t.Run("Should attach access control metadata to folder response", func(t *testing.T) { + t.Run("Should attach access control metadata to folder response with permissions cascading from nested folders", func(t *testing.T) { + folderService.ExpectedFolder = &folder.Folder{UID: "folderUid"} + folderService.ExpectedFolders = []*folder.Folder{{UID: "parentUid"}} + features = featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders) + defer func() { + features = featuremgmt.WithFeatures() + folderService.ExpectedFolders = nil + }() + + req := server.NewGetRequest("/api/folders/folderUid?accesscontrol=true") + webtest.RequestWithSignedInUser(req, &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{ + 1: accesscontrol.GroupScopesByAction([]accesscontrol.Permission{ + {Action: dashboards.ActionFoldersRead, Scope: dashboards.ScopeFoldersAll}, + {Action: dashboards.ActionFoldersWrite, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID("parentUid")}, + {Action: dashboards.ActionDashboardsCreate, Scope: dashboards.ScopeFoldersProvider.GetResourceScopeUID("folderUid")}, + }), + }}) + + res, err := server.Send(req) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, res.StatusCode) + defer func() { require.NoError(t, res.Body.Close()) }() + + body := dtos.Folder{} + require.NoError(t, json.NewDecoder(res.Body).Decode(&body)) + + assert.True(t, body.AccessControl[dashboards.ActionFoldersRead]) + assert.True(t, body.AccessControl[dashboards.ActionFoldersWrite]) + assert.True(t, body.AccessControl[dashboards.ActionDashboardsCreate]) + }) + + t.Run("Should not attach access control metadata to folder response", func(t *testing.T) { folderService.ExpectedFolder = &folder.Folder{UID: "folderUid"} req := server.NewGetRequest("/api/folders/folderUid") diff --git a/pkg/api/search.go b/pkg/api/search.go index 3c7cd61ae51..6602a002235 100644 --- a/pkg/api/search.go +++ b/pkg/api/search.go @@ -6,7 +6,6 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/infra/metrics" - "github.com/grafana/grafana/pkg/services/accesscontrol" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/search" @@ -88,46 +87,7 @@ func (hs *HTTPServer) Search(c *contextmodel.ReqContext) response.Response { defer c.TimeRequest(metrics.MApiDashboardSearch) - if !c.QueryBool("accesscontrol") { - return response.JSON(http.StatusOK, hits) - } - - return hs.searchHitsWithMetadata(c, hits) -} - -func (hs *HTTPServer) searchHitsWithMetadata(c *contextmodel.ReqContext, hits model.HitList) response.Response { - folderUIDs := make(map[string]bool) - dashboardUIDs := make(map[string]bool) - - for _, hit := range hits { - if hit.Type == model.DashHitFolder { - folderUIDs[hit.UID] = true - } else { - dashboardUIDs[hit.UID] = true - folderUIDs[hit.FolderUID] = true - } - } - - folderMeta := hs.getMultiAccessControlMetadata(c, c.OrgID, dashboards.ScopeFoldersPrefix, folderUIDs) - dashboardMeta := hs.getMultiAccessControlMetadata(c, c.OrgID, dashboards.ScopeDashboardsPrefix, dashboardUIDs) - - // search hit with access control metadata attached - type hitWithMeta struct { - *model.Hit - AccessControl accesscontrol.Metadata `json:"accessControl,omitempty"` - } - hitsWithMeta := make([]hitWithMeta, 0, len(hits)) - for _, hit := range hits { - var meta accesscontrol.Metadata - if hit.Type == model.DashHitFolder { - meta = folderMeta[hit.UID] - } else { - meta = accesscontrol.MergeMeta("dashboards", dashboardMeta[hit.UID], folderMeta[hit.FolderUID]) - } - hitsWithMeta = append(hitsWithMeta, hitWithMeta{hit, meta}) - } - - return response.JSON(http.StatusOK, hitsWithMeta) + return response.JSON(http.StatusOK, hits) } // swagger:route GET /search/sorting search listSortOptions diff --git a/pkg/api/search_test.go b/pkg/api/search_test.go deleted file mode 100644 index 68b5a656868..00000000000 --- a/pkg/api/search_test.go +++ /dev/null @@ -1,101 +0,0 @@ -package api - -import ( - "encoding/json" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/grafana/grafana/pkg/services/accesscontrol" - "github.com/grafana/grafana/pkg/services/dashboards" - "github.com/grafana/grafana/pkg/services/search/model" - "github.com/grafana/grafana/pkg/setting" - "github.com/grafana/grafana/pkg/web/webtest" -) - -func TestHTTPServer_Search(t *testing.T) { - type testCase struct { - desc string - includeMetadata bool - permissions []accesscontrol.Permission - expectedMetadata map[int64]map[string]struct{} - } - - type withMeta struct { - model.Hit - AccessControl accesscontrol.Metadata `json:"accessControl,omitempty"` - } - - tests := []testCase{ - { - desc: "should attach metadata to response", - includeMetadata: true, - expectedMetadata: map[int64]map[string]struct{}{ - 1: {dashboards.ActionFoldersRead: {}}, - 2: {dashboards.ActionFoldersRead: {}, dashboards.ActionFoldersWrite: {}, dashboards.ActionDashboardsWrite: {}}, - 3: {dashboards.ActionDashboardsRead: {}, dashboards.ActionDashboardsWrite: {}}, - }, - permissions: []accesscontrol.Permission{ - {Action: "folders:read", Scope: "folders:*"}, - {Action: "folders:write", Scope: "folders:uid:folder2"}, - {Action: "dashboards:read", Scope: "dashboards:*"}, - {Action: "dashboards:write", Scope: "folders:uid:folder2"}, - }, - }, - { - desc: "not attach metadata", - includeMetadata: false, - expectedMetadata: map[int64]map[string]struct{}{}, - permissions: []accesscontrol.Permission{ - {Action: "folders:read", Scope: "folders:*"}, - {Action: "folders:write", Scope: "folders:uid:folder2"}, - {Action: "dashboards:read", Scope: "dashboards:*"}, - {Action: "dashboards:write", Scope: "folders:uid:folder2"}, - }, - }, - } - - for _, tt := range tests { - t.Run(tt.desc, func(t *testing.T) { - server := SetupAPITestServer(t, func(hs *HTTPServer) { - hs.Cfg = setting.NewCfg() - hs.SearchService = &mockSearchService{ExpectedResult: model.HitList{ - {ID: 1, UID: "folder1", Title: "folder1", Type: model.DashHitFolder}, - {ID: 2, UID: "folder2", Title: "folder2", Type: model.DashHitFolder}, - {ID: 3, UID: "dash3", Title: "dash3", FolderUID: "folder2", Type: model.DashHitDB}, - }} - }) - - url := "/api/search" - if tt.includeMetadata { - url += "?accesscontrol=true" - } - - res, err := server.Send( - webtest.RequestWithSignedInUser( - server.NewGetRequest(url), userWithPermissions(1, tt.permissions), - ), - ) - require.NoError(t, err) - - var result []withMeta - require.NoError(t, json.NewDecoder(res.Body).Decode(&result)) - - for _, r := range result { - if !tt.includeMetadata { - assert.Nil(t, r.AccessControl) - continue - } - - assert.Len(t, r.AccessControl, len(tt.expectedMetadata[r.ID])) - for action := range r.AccessControl { - _, ok := tt.expectedMetadata[r.ID][action] - assert.True(t, ok) - } - } - - require.NoError(t, res.Body.Close()) - }) - } -} diff --git a/public/app/core/components/Select/FolderPicker.tsx b/public/app/core/components/Select/FolderPicker.tsx index dc615f0f326..e3d9bca1557 100644 --- a/public/app/core/components/Select/FolderPicker.tsx +++ b/public/app/core/components/Select/FolderPicker.tsx @@ -11,7 +11,7 @@ import { t } from 'app/core/internationalization'; import { contextSrv } from 'app/core/services/context_srv'; import { createFolder, getFolderByUid, searchFolders } from 'app/features/manage-dashboards/state/actions'; import { DashboardSearchHit } from 'app/features/search/types'; -import { AccessControlAction, PermissionLevelString } from 'app/types'; +import { AccessControlAction, PermissionLevelString, SearchQueryType } from 'app/types'; export type FolderPickerFilter = (hits: DashboardSearchHit[]) => DashboardSearchHit[]; @@ -40,7 +40,7 @@ export interface Props { allowEmpty?: boolean; showRoot?: boolean; onClear?: () => void; - accessControlMetadata?: boolean; + searchQueryType?: SearchQueryType; customAdd?: CustomAdd; folderWarning?: FolderWarning; @@ -73,7 +73,7 @@ export function FolderPicker(props: Props) { rootName = 'General', showRoot = true, skipInitialLoad, - accessControlMetadata, + searchQueryType, customAdd, folderWarning, } = props; @@ -89,7 +89,7 @@ export function FolderPicker(props: Props) { const getOptions = useCallback( async (query: string) => { - const searchHits = await searchFolders(query, permissionLevel, accessControlMetadata); + const searchHits = await searchFolders(query, permissionLevel, searchQueryType); const resultsAfterMapAndFilter = mapSearchHitsToOptions(searchHits, filter); const options: Array> = resultsAfterMapAndFilter; @@ -122,7 +122,7 @@ export function FolderPicker(props: Props) { permissionLevel, rootName, showRoot, - accessControlMetadata, + searchQueryType, filter, enableCreateNew, customAdd, diff --git a/public/app/features/alerting/unified/components/rule-editor/FolderAndGroup.tsx b/public/app/features/alerting/unified/components/rule-editor/FolderAndGroup.tsx index 2aabb21e8b1..2a078322785 100644 --- a/public/app/features/alerting/unified/components/rule-editor/FolderAndGroup.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/FolderAndGroup.tsx @@ -6,9 +6,7 @@ import { useFormContext } from 'react-hook-form'; import { GrafanaTheme2, SelectableValue } from '@grafana/data'; import { Stack } from '@grafana/experimental'; import { AsyncSelect, Field, InputControl, Label, LoadingPlaceholder, useStyles2 } from '@grafana/ui'; -import { FolderPickerFilter } from 'app/core/components/Select/FolderPicker'; import { contextSrv } from 'app/core/core'; -import { DashboardSearchHit } from 'app/features/search/types'; import { AccessControlAction, useDispatch } from 'app/types'; import { useCombinedRuleNamespaces } from '../../hooks/useCombinedRuleNamespaces'; @@ -20,7 +18,7 @@ import { isGrafanaRulerRule } from '../../utils/rules'; import { InfoIcon } from '../InfoIcon'; import { MINUTE } from './AlertRuleForm'; -import { containsSlashes, Folder, RuleFolderPicker } from './RuleFolderPicker'; +import { Folder, RuleFolderPicker } from './RuleFolderPicker'; import { checkForPathSeparator } from './util'; export const SLICE_GROUP_RESULTS_TO = 1000; @@ -51,35 +49,6 @@ export const useGetGroupOptionsFromFolder = (folderTitle: string) => { return { groupOptions, loading: groupfoldersForGrafana?.loading }; }; -const useRuleFolderFilter = (existingRuleForm: RuleForm | null) => { - const isSearchHitAvailable = useCallback( - (hit: DashboardSearchHit) => { - const rbacDisabledFallback = contextSrv.hasEditPermissionInFolders; - - const canCreateRuleInFolder = contextSrv.hasAccessInMetadata( - AccessControlAction.AlertingRuleCreate, - hit, - rbacDisabledFallback - ); - - const canUpdateInCurrentFolder = - existingRuleForm && - hit.folderId === existingRuleForm.id && - contextSrv.hasAccessInMetadata(AccessControlAction.AlertingRuleUpdate, hit, rbacDisabledFallback); - return canCreateRuleInFolder || canUpdateInCurrentFolder; - }, - [existingRuleForm] - ); - - return useCallback( - (folderHits) => - folderHits - .filter(isSearchHitAvailable) - .filter((value: DashboardSearchHit) => !containsSlashes(value.title ?? '')), - [isSearchHitAvailable] - ); -}; - export function FolderAndGroup({ initialFolder }: FolderAndGroupProps) { const { formState: { errors }, @@ -89,7 +58,6 @@ export function FolderAndGroup({ initialFolder }: FolderAndGroupProps) { const styles = useStyles2(getStyles); const dispatch = useDispatch(); - const folderFilter = useRuleFolderFilter(initialFolder); const folder = watch('folder'); const group = watch('group'); @@ -165,7 +133,6 @@ export function FolderAndGroup({ initialFolder }: FolderAndGroupProps) { {...field} enableCreateNew={contextSrv.hasPermission(AccessControlAction.FoldersCreate)} enableReset={true} - filter={folderFilter} onChange={({ title, uid }) => { field.onChange({ title, uid }); if (!groupIsInGroupOptions(selectedGroup.value ?? '')) { diff --git a/public/app/features/alerting/unified/components/rule-editor/RuleFolderPicker.tsx b/public/app/features/alerting/unified/components/rule-editor/RuleFolderPicker.tsx index 51e0c24c2c6..73798d29ab1 100644 --- a/public/app/features/alerting/unified/components/rule-editor/RuleFolderPicker.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/RuleFolderPicker.tsx @@ -5,7 +5,7 @@ import { GrafanaTheme2 } from '@grafana/data'; import { Stack } from '@grafana/experimental'; import { Icon, Tooltip, useStyles2 } from '@grafana/ui'; import { FolderPicker, Props as FolderPickerProps } from 'app/core/components/Select/FolderPicker'; -import { PermissionLevelString } from 'app/types'; +import { PermissionLevelString, SearchQueryType } from 'app/types'; import { FolderWarning, CustomAdd } from '../../../../../core/components/Select/FolderPicker'; @@ -54,9 +54,9 @@ export function RuleFolderPicker(props: RuleFolderPickerProps) { allowEmpty={true} initialTitle={value?.title} initialFolderUid={value?.uid} - accessControlMetadata + searchQueryType={SearchQueryType.AlertFolder} {...props} - permissionLevel={PermissionLevelString.View} + permissionLevel={PermissionLevelString.Edit} customAdd={customAdd} folderWarning={folderWarning} /> diff --git a/public/app/features/manage-dashboards/state/actions.ts b/public/app/features/manage-dashboards/state/actions.ts index 582f407fb4b..c244a5193b8 100644 --- a/public/app/features/manage-dashboards/state/actions.ts +++ b/public/app/features/manage-dashboards/state/actions.ts @@ -4,7 +4,7 @@ import { notifyApp } from 'app/core/actions'; import { createErrorNotification } from 'app/core/copy/appNotification'; import { SaveDashboardCommand } from 'app/features/dashboard/components/SaveDashboard/types'; import { dashboardWatcher } from 'app/features/live/dashboard/dashboardWatcher'; -import { DashboardDTO, FolderInfo, PermissionLevelString, ThunkResult } from 'app/types'; +import { DashboardDTO, FolderInfo, PermissionLevelString, SearchQueryType, ThunkResult } from 'app/types'; import { LibraryElementExport } from '../../dashboard/components/DashExportModal/DashboardExporter'; import { getLibraryPanel } from '../../library-panels/state/api'; @@ -316,13 +316,12 @@ export const SLICE_FOLDER_RESULTS_TO = 1000; export function searchFolders( query: any, permission?: PermissionLevelString, - withAccessControl = false + type: SearchQueryType = SearchQueryType.Folder ): Promise { return getBackendSrv().get('/api/search', { query, - type: 'dash-folder', + type: type, permission, - accesscontrol: withAccessControl, limit: SLICE_FOLDER_RESULTS_TO, }); } diff --git a/public/app/types/acl.ts b/public/app/types/acl.ts index f603bf5dd20..7643349df33 100644 --- a/public/app/types/acl.ts +++ b/public/app/types/acl.ts @@ -74,6 +74,12 @@ export enum PermissionLevelString { Admin = 'Admin', } +export enum SearchQueryType { + Folder = 'dash-folder', + Dashboard = 'dash-db', + AlertFolder = 'dash-folder-alerting', +} + export enum DataSourcePermissionLevel { Query = 1, Admin = 2,