From bfa4fa3c68601589068e27c564b5464b67cd80a8 Mon Sep 17 00:00:00 2001 From: Karl Persson <23356117+kalleep@users.noreply.github.com> Date: Tue, 11 Feb 2025 12:36:11 +0100 Subject: [PATCH] Authz: Refactor folder tree (#99554) * Refactor folder tree to its own structure * Make it possible to json encode the tree * Use iterations for Ancestors and Children --------- Co-authored-by: IevaVasiljeva --- pkg/services/authz/rbac/models.go | 6 - pkg/services/authz/rbac/service.go | 100 +++++---------- pkg/services/authz/rbac/service_test.go | 162 ++++++++---------------- pkg/services/authz/rbac/tree.go | 108 ++++++++++++++++ pkg/services/authz/rbac/tree_test.go | 106 ++++++++++++++++ 5 files changed, 299 insertions(+), 183 deletions(-) create mode 100644 pkg/services/authz/rbac/tree.go create mode 100644 pkg/services/authz/rbac/tree_test.go diff --git a/pkg/services/authz/rbac/models.go b/pkg/services/authz/rbac/models.go index a94311b46d5..0bc402af857 100644 --- a/pkg/services/authz/rbac/models.go +++ b/pkg/services/authz/rbac/models.go @@ -23,9 +23,3 @@ type ListRequest struct { Verb string Action string } - -type FolderNode struct { - UID string - ParentUID *string - ChildrenUIDs []string -} diff --git a/pkg/services/authz/rbac/service.go b/pkg/services/authz/rbac/service.go index 349ccd53558..429b568a091 100644 --- a/pkg/services/authz/rbac/service.go +++ b/pkg/services/authz/rbac/service.go @@ -26,7 +26,6 @@ import ( "github.com/grafana/grafana/pkg/services/accesscontrol" authzextv1 "github.com/grafana/grafana/pkg/services/authz/proto/v1" "github.com/grafana/grafana/pkg/services/authz/rbac/store" - "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/storage/legacysql" ) @@ -59,7 +58,7 @@ type Service struct { permCache *cacheWrap[map[string]bool] teamCache *cacheWrap[[]int64] basicRoleCache *cacheWrap[store.BasicRole] - folderCache *cacheWrap[map[string]FolderNode] + folderCache *cacheWrap[folderTree] } func NewService( @@ -83,7 +82,7 @@ func NewService( permCache: newCacheWrap[map[string]bool](cache, logger, shortCacheTTL), teamCache: newCacheWrap[[]int64](cache, logger, shortCacheTTL), basicRoleCache: newCacheWrap[store.BasicRole](cache, logger, shortCacheTTL), - folderCache: newCacheWrap[map[string]FolderNode](cache, logger, shortCacheTTL), + folderCache: newCacheWrap[folderTree](cache, logger, shortCacheTTL), sf: new(singleflight.Group), } } @@ -517,31 +516,26 @@ func (s *Service) checkInheritedPermissions(ctx context.Context, scopeMap map[st defer span.End() ctxLogger := s.logger.FromContext(ctx) - folderMap, err := s.buildFolderTree(ctx, req.Namespace) + tree, err := s.buildFolderTree(ctx, req.Namespace) if err != nil { ctxLogger.Error("could not build folder and dashboard tree", "error", err) return false, err } - currentUID := req.ParentFolder - for { - if node, has := folderMap[currentUID]; has { - scope := dashboards.ScopeFoldersProvider.GetResourceScopeUID(node.UID) - if scopeMap[scope] { - return true, nil - } - if node.ParentUID == nil { - break - } - currentUID = *node.ParentUID - } else { - break + if scopeMap["folders:uid:"+req.ParentFolder] { + return true, nil + } + + for n := range tree.Ancestors(req.ParentFolder) { + if scopeMap["folders:uid:"+n.UID] { + return true, nil } } + return false, nil } -func (s *Service) buildFolderTree(ctx context.Context, ns claims.NamespaceInfo) (map[string]FolderNode, error) { +func (s *Service) buildFolderTree(ctx context.Context, ns claims.NamespaceInfo) (folderTree, error) { ctx, span := s.tracer.Start(ctx, "authz_direct_db.service.buildFolderTree") defer span.End() @@ -557,41 +551,16 @@ func (s *Service) buildFolderTree(ctx context.Context, ns claims.NamespaceInfo) } span.SetAttributes(attribute.Int("num_folders", len(folders))) - folderMap := make(map[string]FolderNode, len(folders)) - for _, folder := range folders { - if node, has := folderMap[folder.UID]; !has { - folderMap[folder.UID] = FolderNode{ - UID: folder.UID, - ParentUID: folder.ParentUID, - } - } else { - node.ParentUID = folder.ParentUID - folderMap[folder.UID] = node - } - // Register that the parent has this child node - if folder.ParentUID == nil { - continue - } - if parent, has := folderMap[*folder.ParentUID]; has { - parent.ChildrenUIDs = append(parent.ChildrenUIDs, folder.UID) - folderMap[*folder.ParentUID] = parent - } else { - folderMap[*folder.ParentUID] = FolderNode{ - UID: *folder.ParentUID, - ChildrenUIDs: []string{folder.UID}, - } - } - } - - s.folderCache.Set(ctx, key, folderMap) - return folderMap, nil + tree := newFolderTree(folders) + s.folderCache.Set(ctx, key, tree) + return tree, nil }) if err != nil { - return nil, err + return folderTree{}, err } - return res.(map[string]FolderNode), nil + return res.(folderTree), nil } func (s *Service) listPermission(ctx context.Context, scopeMap map[string]bool, req *ListRequest) (*authzv1.ListResponse, error) { @@ -609,10 +578,10 @@ func (s *Service) listPermission(ctx context.Context, scopeMap map[string]bool, return nil, status.Error(codes.NotFound, "unsupported resource") } - var folderMap map[string]FolderNode + var tree folderTree if t.folderSupport { var err error - folderMap, err = s.buildFolderTree(ctx, req.Namespace) + tree, err = s.buildFolderTree(ctx, req.Namespace) if err != nil { ctxLogger.Error("could not build folder and dashboard tree", "error", err) return nil, err @@ -621,16 +590,16 @@ func (s *Service) listPermission(ctx context.Context, scopeMap map[string]bool, var res *authzv1.ListResponse if strings.HasPrefix(req.Action, "folders:") { - res = buildFolderList(scopeMap, folderMap) + res = buildFolderList(scopeMap, tree) } else { - res = buildItemList(scopeMap, folderMap, t.prefix()) + res = buildItemList(scopeMap, tree, t.prefix()) } span.SetAttributes(attribute.Int("num_folders", len(res.Folders)), attribute.Int("num_items", len(res.Items))) return res, nil } -func buildFolderList(scopes map[string]bool, tree map[string]FolderNode) *authzv1.ListResponse { +func buildFolderList(scopes map[string]bool, tree folderTree) *authzv1.ListResponse { itemSet := make(map[string]struct{}, len(scopes)) for scope := range scopes { @@ -640,7 +609,9 @@ func buildFolderList(scopes map[string]bool, tree map[string]FolderNode) *authzv } itemSet[identifier] = struct{}{} - getChildren(tree, identifier, itemSet) + for n := range tree.Children(identifier) { + itemSet[n.UID] = struct{}{} + } } itemList := make([]string, 0, len(itemSet)) @@ -651,7 +622,7 @@ func buildFolderList(scopes map[string]bool, tree map[string]FolderNode) *authzv return &authzv1.ListResponse{Items: itemList} } -func buildItemList(scopes map[string]bool, tree map[string]FolderNode, prefix string) *authzv1.ListResponse { +func buildItemList(scopes map[string]bool, tree folderTree, prefix string) *authzv1.ListResponse { folderSet := make(map[string]struct{}, len(scopes)) itemSet := make(map[string]struct{}, len(scopes)) @@ -661,7 +632,9 @@ func buildItemList(scopes map[string]bool, tree map[string]FolderNode, prefix st continue } folderSet[identifier] = struct{}{} - getChildren(tree, identifier, folderSet) + for n := range tree.Children(identifier) { + folderSet[n.UID] = struct{}{} + } } else { identifier := strings.TrimPrefix(scope, prefix) itemSet[identifier] = struct{}{} @@ -678,18 +651,3 @@ func buildItemList(scopes map[string]bool, tree map[string]FolderNode, prefix st return &authzv1.ListResponse{Folders: folderList, Items: itemList} } - -func getChildren(folderMap map[string]FolderNode, folderUID string, folderSet map[string]struct{}) { - folder, has := folderMap[folderUID] - if !has { - return - } - for _, child := range folder.ChildrenUIDs { - // We have already processed all the children of this folder - if _, ok := folderSet[child]; ok { - return - } - folderSet[child] = struct{}{} - getChildren(folderMap, child, folderSet) - } -} diff --git a/pkg/services/authz/rbac/service_test.go b/pkg/services/authz/rbac/service_test.go index a8ad76d61fe..310e7cc8cfc 100644 --- a/pkg/services/authz/rbac/service_test.go +++ b/pkg/services/authz/rbac/service_test.go @@ -25,6 +25,7 @@ func TestService_checkPermission(t *testing.T) { name string permissions []accesscontrol.Permission check CheckRequest + folders []store.Folder expected bool } @@ -146,11 +147,37 @@ func TestService_checkPermission(t *testing.T) { }, expected: false, }, + { + name: "should return true if user has permissions on folder", + permissions: []accesscontrol.Permission{ + { + Scope: "folders:uid:parent", + Kind: "folders", + Attribute: "uid", + Identifier: "parent", + }, + }, + folders: []store.Folder{ + {UID: "parent"}, + {UID: "child", ParentUID: strPtr("parent")}, + }, + check: CheckRequest{ + Action: "dashboards:read", + Group: "dashboard.grafana.app", + Resource: "dashboards", + Name: "some_dashboard", + ParentFolder: "child", + }, + expected: true, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { s := setupService() + + s.folderCache.Set(context.Background(), folderCacheKey("default"), newFolderTree(tc.folders)) + tc.check.Namespace = claims.NamespaceInfo{Value: "default", OrgID: 1} got, err := s.checkPermission(context.Background(), getScopeMap(tc.permissions), &tc.check) require.NoError(t, err) assert.Equal(t, tc.expected, got) @@ -371,90 +398,11 @@ func TestService_getUserPermissions(t *testing.T) { } } -func TestService_buildFolderTree(t *testing.T) { - type testCase struct { - name string - folders []store.Folder - cacheHit bool - expectedTree map[string]FolderNode - } - - testCases := []testCase{ - { - name: "should return folder tree from cache if available", - folders: []store.Folder{ - {UID: "folder1", ParentUID: nil}, - {UID: "folder2", ParentUID: strPtr("folder1")}, - }, - cacheHit: true, - expectedTree: map[string]FolderNode{ - "folder1": {UID: "folder1", ChildrenUIDs: []string{"folder2"}}, - "folder2": {UID: "folder2", ParentUID: strPtr("folder1")}, - }, - }, - { - name: "should return folder tree from store if not in cache", - folders: []store.Folder{ - {UID: "folder1", ParentUID: nil}, - {UID: "folder2", ParentUID: strPtr("folder1")}, - }, - cacheHit: false, - expectedTree: map[string]FolderNode{ - "folder1": {UID: "folder1", ChildrenUIDs: []string{"folder2"}}, - "folder2": {UID: "folder2", ParentUID: strPtr("folder1")}, - }, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - ctx := context.Background() - s := setupService() - - ns := claims.NamespaceInfo{Value: "stacks-12", OrgID: 1, StackID: 12} - - if tc.cacheHit { - s.folderCache.Set(ctx, folderCacheKey(ns.Value), tc.expectedTree) - } - - store := &fakeStore{folders: tc.folders} - s.store = store - s.permissionStore = store - - tree, err := s.buildFolderTree(ctx, ns) - - require.NoError(t, err) - require.Len(t, tree, len(tc.expectedTree)) - for _, folder := range tc.folders { - node, ok := tree[folder.UID] - require.True(t, ok) - // Check parent - if folder.ParentUID != nil { - require.NotNil(t, node.ParentUID) - require.Equal(t, *folder.ParentUID, *node.ParentUID) - } else { - require.Nil(t, node.ParentUID) - } - // Check children - if len(node.ChildrenUIDs) > 0 { - epectedChildren := tc.expectedTree[folder.UID].ChildrenUIDs - require.ElementsMatch(t, node.ChildrenUIDs, epectedChildren) - } - } - if tc.cacheHit { - require.Zero(t, store.calls) - } else { - require.Equal(t, 1, store.calls) - } - }) - } -} - func TestService_listPermission(t *testing.T) { type testCase struct { name string permissions []accesscontrol.Permission - folderTree map[string]FolderNode + folders []store.Folder list ListRequest expectedItems []string expectedFolders []string @@ -503,9 +451,9 @@ func TestService_listPermission(t *testing.T) { Identifier: "some_folder_2", }, }, - folderTree: map[string]FolderNode{ - "some_folder_1": {UID: "some_folder_1"}, - "some_folder_2": {UID: "some_folder_2"}, + folders: []store.Folder{ + {UID: "some_folder_1"}, + {UID: "some_folder_2"}, }, list: ListRequest{ Action: "dashboards:read", @@ -526,13 +474,13 @@ func TestService_listPermission(t *testing.T) { Identifier: "some_folder_1", }, }, - folderTree: map[string]FolderNode{ - "some_folder_parent": {UID: "some_folder_parent", ChildrenUIDs: []string{"some_folder_child"}}, - "some_folder_child": {UID: "some_folder_child", ParentUID: strPtr("some_folder_parent"), ChildrenUIDs: []string{"some_folder_subchild1", "some_folder_subchild2"}}, - "some_folder_subchild1": {UID: "some_folder_subchild1", ParentUID: strPtr("some_folder_child")}, - "some_folder_subchild2": {UID: "some_folder_subchild2", ParentUID: strPtr("some_folder_child"), ChildrenUIDs: []string{"some_folder_subsubchild"}}, - "some_folder_subsubchild": {UID: "some_folder_subsubchild", ParentUID: strPtr("some_folder_subchild2")}, - "some_folder_1": {UID: "some_folder_1", ParentUID: strPtr("some_other_folder")}, + folders: []store.Folder{ + {UID: "some_folder_parent"}, + {UID: "some_folder_child", ParentUID: strPtr("some_folder_parent")}, + {UID: "some_folder_subchild1", ParentUID: strPtr("some_folder_child")}, + {UID: "some_folder_subchild2", ParentUID: strPtr("some_folder_child")}, + {UID: "some_folder_subsubchild", ParentUID: strPtr("some_folder_subchild2")}, + {UID: "some_folder_1", ParentUID: strPtr("some_other_folder")}, }, list: ListRequest{ Action: "dashboards:read", @@ -559,9 +507,9 @@ func TestService_listPermission(t *testing.T) { Identifier: "some_folder_parent", }, }, - folderTree: map[string]FolderNode{ - "some_folder_parent": {UID: "some_folder_parent", ChildrenUIDs: []string{"some_folder_child"}}, - "some_folder_child": {UID: "some_folder_child", ParentUID: strPtr("some_folder_parent")}, + folders: []store.Folder{ + {UID: "some_folder_parent"}, + {UID: "some_folder_child", ParentUID: strPtr("some_folder_parent")}, }, list: ListRequest{ Action: "dashboards:read", @@ -589,23 +537,25 @@ func TestService_listPermission(t *testing.T) { Identifier: "some_folder_parent", }, }, - folderTree: map[string]FolderNode{ - "some_folder_parent": {UID: "some_folder_parent", ChildrenUIDs: []string{"some_folder_child"}}, - "some_folder_child": {UID: "some_folder_child", ParentUID: strPtr("some_folder_parent"), ChildrenUIDs: []string{"some_folder_subchild"}}, - "some_folder_subchild": {UID: "some_folder_subchild", ParentUID: strPtr("some_folder_child")}, + folders: []store.Folder{ + {UID: "some_folder_parent"}, + {UID: "some_folder_child", ParentUID: strPtr("some_folder_parent")}, + {UID: "some_folder_subchild", ParentUID: strPtr("some_folder_child")}, + {UID: "some_folder_child2", ParentUID: strPtr("some_folder_parent")}, }, list: ListRequest{ Action: "dashboards:read", Group: "dashboard.grafana.app", Resource: "dashboards", }, - expectedFolders: []string{"some_folder_parent", "some_folder_child", "some_folder_subchild"}, + expectedFolders: []string{"some_folder_parent", "some_folder_child", "some_folder_child2", "some_folder_subchild"}, }, { name: "return no dashboards and folders if the user doesn't have access to any resources", permissions: []accesscontrol.Permission{}, - folderTree: map[string]FolderNode{ - "some_folder_1": {UID: "some_folder_1"}, + + folders: []store.Folder{ + {UID: "some_folder_1"}, }, list: ListRequest{ Action: "dashboards:read", @@ -624,9 +574,9 @@ func TestService_listPermission(t *testing.T) { Identifier: "some_folder_parent", }, }, - folderTree: map[string]FolderNode{ - "some_folder_parent": {UID: "some_folder_parent", ChildrenUIDs: []string{"some_folder_child"}}, - "some_folder_child": {UID: "some_folder_child", ParentUID: strPtr("some_folder_parent")}, + folders: []store.Folder{ + {UID: "some_folder_parent"}, + {UID: "some_folder_child", ParentUID: strPtr("some_folder_parent")}, }, list: ListRequest{ Action: "folders:read", @@ -640,8 +590,8 @@ func TestService_listPermission(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { s := setupService() - if tc.folderTree != nil { - s.folderCache.Set(context.Background(), folderCacheKey("default"), tc.folderTree) + if tc.folders != nil { + s.folderCache.Set(context.Background(), folderCacheKey("default"), newFolderTree(tc.folders)) } tc.list.Namespace = claims.NamespaceInfo{Value: "default", OrgID: 1} @@ -667,7 +617,7 @@ func setupService() *Service { permCache: newCacheWrap[map[string]bool](cache, logger, shortCacheTTL), teamCache: newCacheWrap[[]int64](cache, logger, shortCacheTTL), basicRoleCache: newCacheWrap[store.BasicRole](cache, logger, longCacheTTL), - folderCache: newCacheWrap[map[string]FolderNode](cache, logger, shortCacheTTL), + folderCache: newCacheWrap[folderTree](cache, logger, shortCacheTTL), store: fStore, permissionStore: fStore, identityStore: &fakeIdentityStore{}, diff --git a/pkg/services/authz/rbac/tree.go b/pkg/services/authz/rbac/tree.go new file mode 100644 index 00000000000..09d28eb37a5 --- /dev/null +++ b/pkg/services/authz/rbac/tree.go @@ -0,0 +1,108 @@ +package rbac + +import ( + "iter" + + "github.com/grafana/grafana/pkg/services/authz/rbac/store" +) + +func newFolderTree(folders []store.Folder) folderTree { + t := folderTree{ + Index: make(map[string]int, len(folders)), + Nodes: make([]folderNode, 0, len(folders)), + } + + for _, f := range folders { + t.Insert(f.UID, f.ParentUID) + } + + return t +} + +type folderTree struct { + // All nodes for the folderTree. + Nodes []folderNode + // Index is a map of folderNode UID to its positons in Nodes. + Index map[string]int +} + +type folderNode struct { + // UID is the uniqiue identifier for folderNode + UID string + // Parent is the position into folderTree nodes for parent, we store -1 for nodes that don't have a parent. + Parent int + // Children is positons into folderTree nodes for all children. + Children []int +} + +func (t *folderTree) Insert(uid string, parentUID *string) int { + parent := -1 + if parentUID != nil { + // find parent + i, ok := t.Index[*parentUID] + if !ok { + // insert parent if it don't exists yet + i = t.Insert(*parentUID, nil) + } + parent = i + } + + i, ok := t.Index[uid] + if !ok { + // this node does not exist yet so we add it to the index and append the new node + i = len(t.Nodes) + t.Index[uid] = i + t.Nodes = append(t.Nodes, folderNode{ + UID: uid, + Parent: parent, + }) + } else { + // if a node is added as a parent node first, its parent will not be set, so we make sure to do it now + t.Nodes[i].Parent = parent + } + + if parent != -1 { + // update parent to include the index of new child node + t.Nodes[parent].Children = append(t.Nodes[parent].Children, i) + } + + return i +} + +// Ancestors returns an iterator that yields ancestors for uid +func (t *folderTree) Ancestors(uid string) iter.Seq[folderNode] { + current, ok := t.Index[uid] + if !ok { + return func(yield func(folderNode) bool) {} + } + + current = t.Nodes[current].Parent + return func(yield func(folderNode) bool) { + for { + if current == -1 || !yield(t.Nodes[current]) { + return + } + + current = t.Nodes[current].Parent + } + } +} + +// Children returns an iterator that yields all children for uid +func (t *folderTree) Children(uid string) iter.Seq[folderNode] { + current, ok := t.Index[uid] + if !ok { + return func(yield func(folderNode) bool) {} + } + + queue := t.Nodes[current].Children + return func(yield func(folderNode) bool) { + for len(queue) > 0 { + current, queue = queue[0], queue[1:] + if !yield(t.Nodes[current]) { + return + } + queue = append(queue, t.Nodes[current].Children...) + } + } +} diff --git a/pkg/services/authz/rbac/tree_test.go b/pkg/services/authz/rbac/tree_test.go new file mode 100644 index 00000000000..aadfae8dd07 --- /dev/null +++ b/pkg/services/authz/rbac/tree_test.go @@ -0,0 +1,106 @@ +package rbac + +import ( + "fmt" + "testing" + + "github.com/grafana/grafana/pkg/services/authz/rbac/store" + "github.com/stretchr/testify/assert" +) + +func Test_Tree(t *testing.T) { + tree := newFolderTree([]store.Folder{ + {UID: "1"}, + {UID: "11", ParentUID: strPtr("1")}, + {UID: "12", ParentUID: strPtr("1")}, + {UID: "111", ParentUID: strPtr("11")}, + {UID: "1111", ParentUID: strPtr("111")}, + {UID: "121", ParentUID: strPtr("12")}, + // not ordered insert to make sure patching works correctly + {UID: "22", ParentUID: strPtr("2")}, + {UID: "222", ParentUID: strPtr("22")}, + {UID: "21", ParentUID: strPtr("2")}, + {UID: "2"}, + }) + + verify := func(t *testing.T, expected []string, visited map[string]bool) { + assert.Len(t, visited, len(expected)) + for _, e := range expected { + assert.True(t, visited[e], fmt.Sprintf("did not visit node: %s", e)) + } + } + + t.Run("should iterate all children of folder 1", func(t *testing.T) { + visited := map[string]bool{} + for n := range tree.Children("1") { + visited[n.UID] = true + } + + expected := []string{"11", "111", "1111", "12", "121"} + verify(t, expected, visited) + }) + + t.Run("should iterate all children of folder 2", func(t *testing.T) { + visited := map[string]bool{} + + for n := range tree.Children("2") { + visited[n.UID] = true + } + + expected := []string{"21", "22", "222"} + verify(t, expected, visited) + }) + + t.Run("should iterate all children of folder 111", func(t *testing.T) { + visited := map[string]bool{} + + for n := range tree.Children("111") { + visited[n.UID] = true + } + + expected := []string{"1111"} + verify(t, expected, visited) + }) + + t.Run("should iterate all children of folder 1111", func(t *testing.T) { + visited := map[string]bool{} + + for n := range tree.Children("1111") { + visited[n.UID] = true + } + + expected := []string{} + verify(t, expected, visited) + }) + + t.Run("should iterate all acestors of folder 1111", func(t *testing.T) { + visited := map[string]bool{} + + for n := range tree.Ancestors("1111") { + visited[n.UID] = true + } + + expected := []string{"1", "11", "111"} + verify(t, expected, visited) + }) + + t.Run("should iterate all acestors of folder 11", func(t *testing.T) { + visited := map[string]bool{} + for n := range tree.Ancestors("11") { + visited[n.UID] = true + } + + expected := []string{"1"} + verify(t, expected, visited) + }) + + t.Run("should iterate all acestors of folder 222", func(t *testing.T) { + visited := map[string]bool{} + for n := range tree.Ancestors("222") { + visited[n.UID] = true + } + + expected := []string{"2", "22"} + verify(t, expected, visited) + }) +}