From 4b217c601a148dd0a27d96f4bb8b980e54856874 Mon Sep 17 00:00:00 2001 From: Gabriel MABILLE Date: Thu, 17 Jul 2025 14:34:10 +0200 Subject: [PATCH] AuthZ: Scope resolution (#107948) * AuthZ: Scope resolution * Account for PR feedback Co-authored-by: Ieva --- pkg/services/authz/rbac/cache.go | 4 + pkg/services/authz/rbac/mapper.go | 3 +- pkg/services/authz/rbac/resolver.go | 136 +++++++++++++++++++ pkg/services/authz/rbac/resolver_test.go | 163 +++++++++++++++++++++++ pkg/services/authz/rbac/service.go | 15 ++- pkg/services/authz/rbac/service_test.go | 75 +++++++++-- 6 files changed, 379 insertions(+), 17 deletions(-) create mode 100644 pkg/services/authz/rbac/resolver.go create mode 100644 pkg/services/authz/rbac/resolver_test.go diff --git a/pkg/services/authz/rbac/cache.go b/pkg/services/authz/rbac/cache.go index 1c5b30f01d0..0b8e4247382 100644 --- a/pkg/services/authz/rbac/cache.go +++ b/pkg/services/authz/rbac/cache.go @@ -45,6 +45,10 @@ func folderCacheKey(namespace string) string { return namespace + ".folders" } +func teamIDsCacheKey(namespace string) string { + return namespace + ".teams" +} + type cacheWrap[T any] interface { Get(ctx context.Context, key string) (T, bool) Set(ctx context.Context, key string, value T) diff --git a/pkg/services/authz/rbac/mapper.go b/pkg/services/authz/rbac/mapper.go index 7017f1f086c..789d3dd31a5 100644 --- a/pkg/services/authz/rbac/mapper.go +++ b/pkg/services/authz/rbac/mapper.go @@ -102,7 +102,8 @@ func NewMapperRegistry() MapperRegistry { "folders": newResourceTranslation("folders", "uid", true), }, "iam.grafana.app": { - "teams": newResourceTranslation("teams", "id", false), + // Teams is a special case. We translate user permissions from id to uid based. + "teams": newResourceTranslation("teams", "uid", false), "coreroles": newResourceTranslation("roles", "uid", false), }, "secret.grafana.app": { diff --git a/pkg/services/authz/rbac/resolver.go b/pkg/services/authz/rbac/resolver.go new file mode 100644 index 00000000000..f8978302c6c --- /dev/null +++ b/pkg/services/authz/rbac/resolver.go @@ -0,0 +1,136 @@ +package rbac + +import ( + "context" + "fmt" + "strconv" + "strings" + + "github.com/grafana/authlib/types" + "github.com/grafana/grafana/pkg/registry/apis/iam/legacy" + "github.com/grafana/grafana/pkg/services/accesscontrol" +) + +type ScopeResolverFunc func(scope string) (string, error) + +func (s *Service) fetchTeams(ctx context.Context, ns types.NamespaceInfo) (map[int64]string, error) { + key := teamIDsCacheKey(ns.Value) + res, err, _ := s.sf.Do(key, func() (any, error) { + teams, err := s.identityStore.ListTeams(ctx, ns, legacy.ListTeamQuery{}) + if err != nil { + return nil, fmt.Errorf("could not fetch teams: %w", err) + } + teamIDs := make(map[int64]string, len(teams.Teams)) + for _, team := range teams.Teams { + teamIDs[team.ID] = team.UID + } + return teamIDs, nil + }) + if err != nil { + return nil, err + } + teamIDs := res.(map[int64]string) + s.teamIDCache.Set(ctx, key, teamIDs) + return teamIDs, nil +} + +// Should return an error if we fail to build the resolver. +func (s *Service) newTeamNameResolver(ctx context.Context, ns types.NamespaceInfo) (ScopeResolverFunc, error) { + teamIDs, cacheHit := s.teamIDCache.Get(ctx, teamIDsCacheKey(ns.Value)) + if !cacheHit { + var err error + teamIDs, err = s.fetchTeams(ctx, ns) + if err != nil { + return nil, fmt.Errorf("could not build resolver: %w", err) + } + } + + return func(scope string) (string, error) { + teamIDStr := strings.TrimPrefix(scope, "teams:id:") + if teamIDStr == "" { + return "", fmt.Errorf("team ID is empty") + } + if teamIDStr == "*" { + return "teams:uid:*", nil + } + teamID, err := strconv.ParseInt(teamIDStr, 10, 64) + if err != nil { + return "", fmt.Errorf("invalid team ID %s: %w", teamIDStr, err) + } + if teamName, ok := teamIDs[teamID]; ok { + return "teams:uid:" + teamName, nil + } + + // Stale cache recovery: Try to fetch the teams again. + if cacheHit { + // Potential future improvement: if multiple threads have the same stale cache, + // they might refetch teams separately and asynchronously. We could use a more sophisticated + // approach to avoid this. Like checking if the cache has been updated meanwhile. + cacheHit = false + teamIDs, err = s.fetchTeams(ctx, ns) + if err != nil { + // Other improvement: Stop the calling loop if we fail to fetch teams. + return "", err + } + if teamName, ok := teamIDs[teamID]; ok { + return "teams:uid:" + teamName, nil + } + } + + return "", fmt.Errorf("team ID %s not found", teamIDStr) + }, nil +} + +func (s *Service) nameResolver(ctx context.Context, ns types.NamespaceInfo, scopePrefix string) (ScopeResolverFunc, error) { + if scopePrefix == "teams:id:" { + return s.newTeamNameResolver(ctx, ns) + } + // No resolver found for the given scope prefix. + return nil, nil +} + +// resolveScopeMap translates scopes like "teams:id:1" to "teams:uid:t1". +// It assumes only one scope resolver is needed for a given scope map, based on the first valid scope encountered. +func (s *Service) resolveScopeMap(ctx context.Context, ns types.NamespaceInfo, scopeMap map[string]bool) (map[string]bool, error) { + var ( + prefix string + scopeResolver ScopeResolverFunc + err error + ) + for scope := range scopeMap { + // Find the resolver based on the first scope with a valid prefix + if prefix == "" { + if len(strings.Split(scope, ":")) < 3 { + // Skip scopes that don't have at least 3 parts (e.g., "*", "teams:*") + // This is because we expect scopes to be in the format "resource:attribute:value". + continue + } + + // Initialize the scope resolver only once + prefix = accesscontrol.ScopePrefix(scope) + scopeResolver, err = s.nameResolver(ctx, ns, prefix) + if err != nil { + s.logger.FromContext(ctx).Error("failed to create scope resolver", "prefix", prefix, "error", err) + return nil, err + } + if scopeResolver == nil { + break // No resolver found for this prefix + } + } + + // Skip scopes that do not have the expected prefix + if !strings.HasPrefix(scope, prefix) { + continue + } + resolved, err := scopeResolver(scope) + if err != nil { + s.logger.FromContext(ctx).Warn("could not resolve scope name", "scope", scope, "error", err) + continue // Still want to process other scopes even if one fails. + } + if resolved != "" { + scopeMap[resolved] = true + delete(scopeMap, scope) + } + } + return scopeMap, nil +} diff --git a/pkg/services/authz/rbac/resolver_test.go b/pkg/services/authz/rbac/resolver_test.go new file mode 100644 index 00000000000..c19a3fee473 --- /dev/null +++ b/pkg/services/authz/rbac/resolver_test.go @@ -0,0 +1,163 @@ +package rbac + +import ( + "context" + "testing" + + "github.com/grafana/authlib/types" + "github.com/grafana/grafana/pkg/services/team" + "github.com/stretchr/testify/require" +) + +func TestService_resolveScopeMap(t *testing.T) { + tests := []struct { + name string + scopeMap map[string]bool + ns types.NamespaceInfo + cache map[string]map[int64]string // Namespace: team ID -> UID cache + store []team.Team + want map[string]bool + }{ + { + name: "Should resolve team IDs to team UIDs", + ns: types.NamespaceInfo{Value: "org-2", OrgID: 2}, + scopeMap: map[string]bool{ + "teams:id:1": true, + "teams:id:2": true, + }, + store: []team.Team{ + {ID: 1, UID: "t1"}, + {ID: 2, UID: "t2"}, + }, + want: map[string]bool{ + "teams:uid:t1": true, + "teams:uid:t2": true, + }, + }, + { + name: "Should use cache", + ns: types.NamespaceInfo{Value: "org-2", OrgID: 2}, + scopeMap: map[string]bool{ + "teams:id:1": true, + "teams:id:2": true, + }, + cache: map[string]map[int64]string{ + "org-2": {1: "t1", 2: "t2"}, + }, + want: map[string]bool{ + "teams:uid:t1": true, + "teams:uid:t2": true, + }, + }, + { + name: "Shouldn't use cache from another namespace", + ns: types.NamespaceInfo{Value: "org-2", OrgID: 2}, + scopeMap: map[string]bool{ + "teams:id:1": true, + }, + cache: map[string]map[int64]string{ + "org-31": {1: "org31-t1"}, + }, + store: []team.Team{{ID: 1, UID: "org2-t1"}}, + want: map[string]bool{ + "teams:uid:org2-t1": true, + }, + }, + { + name: "Should handle wildcard", + ns: types.NamespaceInfo{Value: "org-2", OrgID: 2}, + scopeMap: map[string]bool{"teams:id:*": true}, + store: []team.Team{{ID: 1, UID: "t1"}, {ID: 2, UID: "t2"}}, + want: map[string]bool{"teams:uid:*": true}, + }, + { + name: "Should skip short scopes", + ns: types.NamespaceInfo{Value: "org-2", OrgID: 2}, + scopeMap: map[string]bool{ + "teams:short": true, + "teams:*": true, + "*": true, + "teams:id:1": true, + }, + store: []team.Team{{ID: 1, UID: "t1"}}, + want: map[string]bool{ + "teams:short": true, + "teams:uid:t1": true, + "teams:*": true, + "*": true, + }, + }, + { + name: "Should gracefully handle invalid scopes", + ns: types.NamespaceInfo{Value: "org-2", OrgID: 2}, + scopeMap: map[string]bool{ + "teams:id:": true, + "teams:id:NaN": true, + "teams:id:1": true, + }, + store: []team.Team{{ID: 1, UID: "t1"}}, + want: map[string]bool{ + "teams:id:": true, + "teams:id:NaN": true, + "teams:uid:t1": true, + }, + }, + { + name: "Should recover from stale cache", + ns: types.NamespaceInfo{Value: "org-2", OrgID: 2}, + scopeMap: map[string]bool{ + "teams:id:1": true, + "teams:id:2": true, + "teams:id:3": true, + }, + cache: map[string]map[int64]string{ + "org-2": {1: "t1"}, + }, + store: []team.Team{ + {ID: 1, UID: "t1"}, + {ID: 2, UID: "t2"}, + {ID: 3, UID: "t3"}, + }, + want: map[string]bool{ + "teams:uid:t1": true, + "teams:uid:t2": true, + "teams:uid:t3": true, + }, + }, + { + name: "Should skip unknown team IDs", + ns: types.NamespaceInfo{Value: "org-2", OrgID: 2}, + scopeMap: map[string]bool{ + "teams:id:1": true, + }, + store: []team.Team{ + {ID: 2, UID: "t2"}, + }, + want: map[string]bool{ + "teams:id:1": true, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := setupService() + s.identityStore = &fakeIdentityStore{ + disableNsCheck: true, + teams: tt.store, + } + if tt.cache != nil { + for ns, cache := range tt.cache { + s.teamIDCache.Set(context.Background(), teamIDsCacheKey(ns), cache) + } + } + got, err := s.resolveScopeMap(context.Background(), tt.ns, tt.scopeMap) + require.NoError(t, err) + + require.Len(t, got, len(tt.want)) + for scope := range tt.want { + _, ok := got[scope] + require.True(t, ok) + } + }) + } +} diff --git a/pkg/services/authz/rbac/service.go b/pkg/services/authz/rbac/service.go index c37f4d7bdb2..36ee02e2790 100644 --- a/pkg/services/authz/rbac/service.go +++ b/pkg/services/authz/rbac/service.go @@ -58,9 +58,10 @@ type Service struct { idCache cacheWrap[store.UserIdentifiers] permCache cacheWrap[map[string]bool] permDenialCache cacheWrap[bool] - teamCache cacheWrap[[]int64] + userTeamCache cacheWrap[[]int64] basicRoleCache cacheWrap[store.BasicRole] folderCache cacheWrap[folderTree] + teamIDCache cacheWrap[map[int64]string] } type Settings struct { @@ -97,9 +98,10 @@ func NewService( idCache: newCacheWrap[store.UserIdentifiers](cache, logger, tracer, longCacheTTL), permCache: newCacheWrap[map[string]bool](cache, logger, tracer, settings.CacheTTL), permDenialCache: newCacheWrap[bool](cache, logger, tracer, settings.CacheTTL), - teamCache: newCacheWrap[[]int64](cache, logger, tracer, settings.CacheTTL), + userTeamCache: newCacheWrap[[]int64](cache, logger, tracer, settings.CacheTTL), basicRoleCache: newCacheWrap[store.BasicRole](cache, logger, tracer, settings.CacheTTL), folderCache: newCacheWrap[folderTree](cache, logger, tracer, settings.CacheTTL), + teamIDCache: newCacheWrap[map[int64]string](cache, logger, tracer, longCacheTTL), sf: new(singleflight.Group), } } @@ -435,6 +437,11 @@ func (s *Service) getUserPermissions(ctx context.Context, ns types.NamespaceInfo } scopeMap := getScopeMap(permissions) + scopeMap, err = s.resolveScopeMap(ctx, ns, scopeMap) + if err != nil { + return nil, fmt.Errorf("could not resolve scope map: %w", err) + } + s.permCache.Set(ctx, userPermKey, scopeMap) span.SetAttributes(attribute.Int("num_permissions_fetched", len(permissions))) @@ -516,7 +523,7 @@ func (s *Service) getUserTeams(ctx context.Context, ns types.NamespaceInfo, user teamIDs := make([]int64, 0, 50) teamsCacheKey := userTeamCacheKey(ns.Value, userIdentifiers.UID) - if cached, ok := s.teamCache.Get(ctx, teamsCacheKey); ok { + if cached, ok := s.userTeamCache.Get(ctx, teamsCacheKey); ok { return cached, nil } @@ -538,7 +545,7 @@ func (s *Service) getUserTeams(ctx context.Context, ns types.NamespaceInfo, user break } } - s.teamCache.Set(ctx, teamsCacheKey, teamIDs) + s.userTeamCache.Set(ctx, teamsCacheKey, teamIDs) span.SetAttributes(attribute.Int("num_user_teams", len(teamIDs))) return teamIDs, nil diff --git a/pkg/services/authz/rbac/service_test.go b/pkg/services/authz/rbac/service_test.go index f5eddd8e34d..6ae33e9636f 100644 --- a/pkg/services/authz/rbac/service_test.go +++ b/pkg/services/authz/rbac/service_test.go @@ -23,6 +23,7 @@ import ( "github.com/grafana/grafana/pkg/registry/apis/iam/legacy" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/authz/rbac/store" + "github.com/grafana/grafana/pkg/services/team" ) func TestService_checkPermission(t *testing.T) { @@ -336,11 +337,11 @@ func TestService_getUserTeams(t *testing.T) { ns := types.NamespaceInfo{Value: "stacks-12", OrgID: 1, StackID: 12} userIdentifiers := &store.UserIdentifiers{UID: "test-uid"} - identityStore := &fakeIdentityStore{teams: tc.teams, err: tc.expectedError, disableNsCheck: true} + identityStore := &fakeIdentityStore{userTeams: tc.teams, err: tc.expectedError, disableNsCheck: true} s.identityStore = identityStore if tc.cacheHit { - s.teamCache.Set(ctx, userTeamCacheKey(ns.Value, userIdentifiers.UID), tc.expectedTeams) + s.userTeamCache.Set(ctx, userTeamCacheKey(ns.Value, userIdentifiers.UID), tc.expectedTeams) } teams, err := s.getUserTeams(ctx, ns, userIdentifiers) @@ -460,6 +461,14 @@ func TestService_getUserPermissions(t *testing.T) { cacheHit: false, expectedPerms: map[string]bool{}, }, + { + name: "should return uid based permissions", + permissions: []accesscontrol.Permission{ + {Action: "teams:read", Scope: "teams:id:1"}, + }, + cacheHit: false, + expectedPerms: map[string]bool{"teams:uid:t1": true}, + }, } for _, tc := range testCases { @@ -483,13 +492,20 @@ func TestService_getUserPermissions(t *testing.T) { } s.store = store s.permissionStore = store - s.identityStore = &fakeIdentityStore{teams: []int64{1, 2}, disableNsCheck: true} + s.identityStore = &fakeIdentityStore{ + userTeams: []int64{1, 2}, + teams: []team.Team{ + {ID: 1, UID: "t1", OrgID: 1}, + {ID: 2, UID: "t2", OrgID: 1}, + }, + disableNsCheck: true, + } perms, err := s.getIdentityPermissions(ctx, ns, types.TypeUser, userID.UID, action) require.NoError(t, err) require.Len(t, perms, len(tc.expectedPerms)) - for _, perm := range tc.permissions { - _, ok := tc.expectedPerms[perm.Scope] + for scope := range perms { + _, ok := tc.expectedPerms[scope] require.True(t, ok) } if tc.cacheHit { @@ -872,6 +888,21 @@ func TestService_Check(t *testing.T) { permissions: []accesscontrol.Permission{{Action: "dashboards:read", Scope: "dashboards:uid:dash2"}}, expected: false, }, + { + name: "should translate from id to uid based permissions", + req: &authzv1.CheckRequest{ + Namespace: "org-12", + Subject: "user:test-uid", + Group: "iam.grafana.app", + Resource: "teams", + Verb: "get", + Name: "t1", + }, + permissions: []accesscontrol.Permission{ + {Action: "teams:read", Scope: "teams:id:1"}, + }, + expected: true, + }, } t.Run("User permission check", func(t *testing.T) { for _, tc := range testCases { @@ -885,17 +916,24 @@ func TestService_Check(t *testing.T) { } s.store = store s.permissionStore = store - s.identityStore = &fakeIdentityStore{} + s.identityStore = &fakeIdentityStore{ + teams: []team.Team{{ID: 1, UID: "t1", OrgID: 1}}, + } resp, err := s.Check(ctx, tc.req) require.NoError(t, err) - assert.Equal(t, tc.expected, resp.Allowed) + require.Equal(t, tc.expected, resp.Allowed) // Check cache id, ok := s.idCache.Get(ctx, userIdentifierCacheKey("org-12", "test-uid")) require.True(t, ok) require.Equal(t, id.UID, "test-uid") - perms, ok := s.permCache.Get(ctx, userPermCacheKey("org-12", "test-uid", "dashboards:read")) + + expAction := "dashboards:read" + if tc.req.Resource == "teams" { + expAction = "teams:read" + } + perms, ok := s.permCache.Get(ctx, userPermCacheKey("org-12", "test-uid", expAction)) require.True(t, ok) require.Len(t, perms, 1) }) @@ -1525,9 +1563,10 @@ func setupService() *Service { idCache: newCacheWrap[store.UserIdentifiers](cache, logger, tracer, longCacheTTL), permCache: newCacheWrap[map[string]bool](cache, logger, tracer, shortCacheTTL), permDenialCache: newCacheWrap[bool](cache, logger, tracer, shortCacheTTL), - teamCache: newCacheWrap[[]int64](cache, logger, tracer, shortCacheTTL), + userTeamCache: newCacheWrap[[]int64](cache, logger, tracer, shortCacheTTL), basicRoleCache: newCacheWrap[store.BasicRole](cache, logger, tracer, longCacheTTL), folderCache: newCacheWrap[folderTree](cache, logger, tracer, shortCacheTTL), + teamIDCache: newCacheWrap[map[int64]string](cache, logger, tracer, shortCacheTTL), settings: Settings{AnonOrgRole: "Viewer"}, store: fStore, permissionStore: fStore, @@ -1599,7 +1638,8 @@ func (f *fakeStore) ListFolders(ctx context.Context, namespace types.NamespaceIn type fakeIdentityStore struct { legacy.LegacyIdentityStore - teams []int64 + userTeams []int64 + teams []team.Team disableNsCheck bool err bool calls int @@ -1613,8 +1653,8 @@ func (f *fakeIdentityStore) ListUserTeams(ctx context.Context, namespace types.N if f.err { return nil, fmt.Errorf("identity store error") } - items := make([]legacy.UserTeam, 0, len(f.teams)) - for _, teamID := range f.teams { + items := make([]legacy.UserTeam, 0, len(f.userTeams)) + for _, teamID := range f.userTeams { items = append(items, legacy.UserTeam{ID: teamID}) } return &legacy.ListUserTeamsResult{ @@ -1622,3 +1662,14 @@ func (f *fakeIdentityStore) ListUserTeams(ctx context.Context, namespace types.N Continue: 0, }, nil } + +func (f *fakeIdentityStore) ListTeams(ctx context.Context, namespace types.NamespaceInfo, query legacy.ListTeamQuery) (*legacy.ListTeamResult, error) { + if ns, ok := request.NamespaceFrom(ctx); !f.disableNsCheck && (!ok || ns != namespace.Value) { + return nil, fmt.Errorf("namespace mismatch") + } + f.calls++ + if f.err { + return nil, fmt.Errorf("identity store error") + } + return &legacy.ListTeamResult{Teams: f.teams}, nil +}