From 89da0bf1786775d1db99f8df1f4aaa144ee2028f Mon Sep 17 00:00:00 2001 From: Todd Treece <360020+toddtreece@users.noreply.github.com> Date: Fri, 10 Oct 2025 09:44:02 -0400 Subject: [PATCH] Access Control: Fix plugin async install role registration (#112123) --- pkg/services/accesscontrol/acimpl/service.go | 86 +++++-- .../acimpl/service_bench_test.go | 16 +- .../accesscontrol/acimpl/service_test.go | 213 +++++++++++++++++- 3 files changed, 276 insertions(+), 39 deletions(-) diff --git a/pkg/services/accesscontrol/acimpl/service.go b/pkg/services/accesscontrol/acimpl/service.go index 6afca8dd946..3c1deeb1935 100644 --- a/pkg/services/accesscontrol/acimpl/service.go +++ b/pkg/services/accesscontrol/acimpl/service.go @@ -6,6 +6,7 @@ import ( "slices" "strconv" "strings" + "sync" "time" "github.com/prometheus/client_golang/prometheus" @@ -108,9 +109,11 @@ type Service struct { features featuremgmt.FeatureToggles log log.Logger registrations accesscontrol.RegistrationList + rolesMu sync.RWMutex roles map[string]*accesscontrol.RoleDTO store accesscontrol.Store permRegistry permreg.PermissionRegistry + isInitialized bool } func (s *Service) GetUsageStats(_ context.Context) map[string]any { @@ -139,11 +142,13 @@ func (s *Service) getUserPermissions(ctx context.Context, user identity.Requeste defer span.End() permissions := make([]accesscontrol.Permission, 0) + s.rolesMu.RLock() for _, builtin := range accesscontrol.GetOrgRoles(user) { if basicRole, ok := s.roles[builtin]; ok { permissions = append(permissions, basicRole.Permissions...) } } + s.rolesMu.RUnlock() permissions = append(permissions, SharedWithMeFolderPermission) // we don't care about the error here, if this fails we get 0 and no @@ -170,9 +175,11 @@ func (s *Service) getBasicRolePermissions(ctx context.Context, role string, orgI defer span.End() var permissions []accesscontrol.Permission + s.rolesMu.RLock() if basicRole, ok := s.roles[role]; ok { permissions = basicRole.Permissions } + s.rolesMu.RUnlock() // Fetch managed role permissions assigned to basic roles dbPermissions, err := s.store.GetBasicRolesPermissions(ctx, accesscontrol.GetUserPermissionsQuery{ @@ -423,33 +430,43 @@ func (s *Service) RegisterFixedRoles(ctx context.Context) error { _, span := tracer.Start(ctx, "accesscontrol.acimpl.RegisterFixedRoles") defer span.End() - s.registrations.Range(func(registration accesscontrol.RoleRegistration) bool { - for br := range accesscontrol.BuiltInRolesWithParents(registration.Grants) { - if basicRole, ok := s.roles[br]; ok { - for _, p := range registration.Role.Permissions { - if registration.Role.IsPlugin() && p.Action == pluginaccesscontrol.ActionAppAccess { - s.log.Debug("Plugin is attempting to grant access permission, but this permission is already granted by default and will be ignored", - "role", registration.Role.Name, "permission", p.Action, "scope", p.Scope) - continue - } - perm := accesscontrol.Permission{ - Action: p.Action, - Scope: p.Scope, - } + s.rolesMu.Lock() + defer s.rolesMu.Unlock() - perm.Kind, perm.Attribute, perm.Identifier = accesscontrol.SplitScope(perm.Scope) - basicRole.Permissions = append(basicRole.Permissions, perm) - } - } else { - s.log.Error("Unknown builtin role", "builtInRole", br) - } - } + s.registrations.Range(func(registration accesscontrol.RoleRegistration) bool { + s.registerRolesLocked(registration) return true }) + s.isInitialized = true return nil } +// registerRolesLocked processes a single role registration and adds permissions to basic roles. +// Must be called with s.rolesMu locked. +func (s *Service) registerRolesLocked(registration accesscontrol.RoleRegistration) { + for br := range accesscontrol.BuiltInRolesWithParents(registration.Grants) { + if basicRole, ok := s.roles[br]; ok { + for _, p := range registration.Role.Permissions { + if registration.Role.IsPlugin() && p.Action == pluginaccesscontrol.ActionAppAccess { + s.log.Debug("Plugin is attempting to grant access permission, but this permission is already granted by default and will be ignored", + "role", registration.Role.Name, "permission", p.Action, "scope", p.Scope) + continue + } + perm := accesscontrol.Permission{ + Action: p.Action, + Scope: p.Scope, + } + + perm.Kind, perm.Attribute, perm.Identifier = accesscontrol.SplitScope(perm.Scope) + basicRole.Permissions = append(basicRole.Permissions, perm) + } + } else { + s.log.Error("Unknown builtin role", "builtInRole", br) + } + } +} + // DeclarePluginRoles allow the caller to declare, to the service, plugin roles and their assignments // to organization roles ("Viewer", "Editor", "Admin") or "Grafana Admin" func (s *Service) DeclarePluginRoles(ctx context.Context, ID, name string, regs []plugins.RoleRegistration) error { @@ -476,6 +493,16 @@ func (s *Service) DeclarePluginRoles(ctx context.Context, ID, name string, regs s.log.Debug("Registering plugin role", "role", r.Role.Name) s.registrations.Append(r) + + s.rolesMu.RLock() + initialized := s.isInitialized + s.rolesMu.RUnlock() + if initialized { + s.rolesMu.Lock() + s.registerRolesLocked(r) + s.rolesMu.Unlock() + s.cache.Flush() + } } return nil @@ -513,6 +540,7 @@ func (s *Service) SearchUsersPermissions(ctx context.Context, usr identity.Reque // Filter ram permissions basicPermissions := map[string][]accesscontrol.Permission{} + s.rolesMu.RLock() for role, basicRole := range s.roles { for i := range basicRole.Permissions { if PermissionMatchesSearchOptions(basicRole.Permissions[i], &options) { @@ -520,6 +548,7 @@ func (s *Service) SearchUsersPermissions(ctx context.Context, usr identity.Reque } } } + s.rolesMu.RUnlock() usersRoles, err := s.store.GetUsersBasicRoles(ctx, nil, usr.GetOrgID()) if err != nil { @@ -630,6 +659,7 @@ func (s *Service) searchUserPermissions(ctx context.Context, orgID int64, search return nil, fmt.Errorf("found no basic roles for user %d in organisation %d", searchOptions.UserID, orgID) } permissions := make([]accesscontrol.Permission, 0) + s.rolesMu.RLock() for _, builtin := range roles { if basicRole, ok := s.roles[builtin]; ok { for _, permission := range basicRole.Permissions { @@ -639,6 +669,7 @@ func (s *Service) searchUserPermissions(ctx context.Context, orgID int64, search } } } + s.rolesMu.RUnlock() searchOptions.ActionSets = s.actionResolver.ResolveAction(searchOptions.Action) searchOptions.ActionSets = append(searchOptions.ActionSets, @@ -741,7 +772,15 @@ func (s *Service) SyncUserRoles(ctx context.Context, orgID int64, cmd accesscont } func (s *Service) GetStaticRoles(ctx context.Context) map[string]*accesscontrol.RoleDTO { - return s.roles + s.rolesMu.RLock() + defer s.rolesMu.RUnlock() + + // Return a copy to avoid external modifications + rolesCopy := make(map[string]*accesscontrol.RoleDTO, len(s.roles)) + for k, v := range s.roles { + rolesCopy[k] = v + } + return rolesCopy } func (s *Service) GetRoleByName(ctx context.Context, orgID int64, roleName string) (*accesscontrol.RoleDTO, error) { @@ -749,7 +788,10 @@ func (s *Service) GetRoleByName(ctx context.Context, orgID int64, roleName strin defer span.End() err := accesscontrol.ErrRoleNotFound - if _, ok := s.roles[roleName]; ok { + s.rolesMu.RLock() + _, isBasicRole := s.roles[roleName] + s.rolesMu.RUnlock() + if isBasicRole { return nil, err } diff --git a/pkg/services/accesscontrol/acimpl/service_bench_test.go b/pkg/services/accesscontrol/acimpl/service_bench_test.go index 8749f418837..9fb41a34f96 100644 --- a/pkg/services/accesscontrol/acimpl/service_bench_test.go +++ b/pkg/services/accesscontrol/acimpl/service_bench_test.go @@ -14,6 +14,8 @@ import ( "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/actest" "github.com/grafana/grafana/pkg/services/accesscontrol/database" + "github.com/grafana/grafana/pkg/services/accesscontrol/permreg" + "github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" @@ -28,12 +30,14 @@ func setupBenchEnv(b *testing.B, usersCount, resourceCount int) (accesscontrol.S sqlStore := db.InitTestDB(b) store := database.ProvideService(sqlStore) acService := &Service{ - cfg: setting.NewCfg(), - log: log.New("accesscontrol-test"), - registrations: accesscontrol.RegistrationList{}, - store: store, - roles: accesscontrol.BuildBasicRoleDefinitions(), - cache: localcache.New(1*time.Second, 1*time.Second), + cfg: setting.NewCfg(), + log: log.New("accesscontrol-test"), + registrations: accesscontrol.RegistrationList{}, + store: store, + roles: accesscontrol.BuildBasicRoleDefinitions(), + cache: localcache.New(1*time.Second, 1*time.Second), + permRegistry: permreg.ProvidePermissionRegistry(), + actionResolver: resourcepermissions.NewActionSetService(), } // Prepare default permissions diff --git a/pkg/services/accesscontrol/acimpl/service_test.go b/pkg/services/accesscontrol/acimpl/service_test.go index bb81fb700e2..8d6e85b5fcc 100644 --- a/pkg/services/accesscontrol/acimpl/service_test.go +++ b/pkg/services/accesscontrol/acimpl/service_test.go @@ -31,7 +31,7 @@ func TestMain(m *testing.M) { testsuite.Run(m) } -func setupTestEnv(t testing.TB) *Service { +func setupTestEnv(t testing.TB, registerRoles bool) *Service { t.Helper() cfg := setting.NewCfg() @@ -46,7 +46,11 @@ func setupTestEnv(t testing.TB) *Service { permRegistry: permreg.ProvidePermissionRegistry(), actionResolver: resourcepermissions.NewActionSetService(), } - require.NoError(t, ac.RegisterFixedRoles(context.Background())) + + if registerRoles { + require.NoError(t, ac.RegisterFixedRoles(context.Background())) + } + return ac } @@ -145,7 +149,7 @@ func TestIntegrationService_DeclareFixedRoles(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ac := setupTestEnv(t) + ac := setupTestEnv(t, true) // Reset the registations ac.registrations = accesscontrol.RegistrationList{} @@ -260,7 +264,7 @@ func TestIntegrationService_DeclarePluginRoles(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ac := setupTestEnv(t) + ac := setupTestEnv(t, true) // Reset the registations ac.registrations = accesscontrol.RegistrationList{} @@ -361,7 +365,7 @@ func TestIntegrationService_RegisterFixedRoles(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ac := setupTestEnv(t) + ac := setupTestEnv(t, true) ac.registrations.Append(tt.registrations...) @@ -389,6 +393,193 @@ func TestIntegrationService_RegisterFixedRoles(t *testing.T) { } } +func TestIntegrationService_DeclarePluginRoles_DynamicRegistration(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + + tests := []struct { + name string + postRegister bool + pluginID string + registrations []plugins.RoleRegistration + wantGrantedRoles []string + wantPermissions []string + wantPermCount int + }{ + { + name: "should register plugin roles before initialization", + postRegister: false, + pluginID: "test-app", + registrations: []plugins.RoleRegistration{ + { + Role: plugins.Role{ + Name: "Reader", + Description: "Test reader role", + Permissions: []plugins.Permission{ + {Action: "test-app:read", Scope: "test-app:*"}, + }, + }, + Grants: []string{string(identity.RoleViewer)}, + }, + }, + wantGrantedRoles: []string{string(identity.RoleViewer)}, + wantPermissions: []string{"test-app:read"}, + wantPermCount: 1, + }, + { + name: "should register plugin roles dynamically after initialization", + postRegister: true, + pluginID: "dynamic-app", + registrations: []plugins.RoleRegistration{ + { + Role: plugins.Role{ + Name: "DynamicReader", + Description: "Dynamic reader role", + Permissions: []plugins.Permission{ + {Action: "dynamic-app:read", Scope: "dynamic-app:*"}, + {Action: "dynamic-app:query", Scope: "dynamic-app:*"}, + }, + }, + Grants: []string{string(identity.RoleViewer)}, + }, + }, + wantGrantedRoles: []string{string(identity.RoleViewer)}, + wantPermissions: []string{"dynamic-app:read", "dynamic-app:query"}, + wantPermCount: 2, + }, + { + name: "should grant to multiple roles dynamically", + postRegister: true, + pluginID: "multi-app", + registrations: []plugins.RoleRegistration{ + { + Role: plugins.Role{ + Name: "MultiReader", + Description: "Multi reader role", + Permissions: []plugins.Permission{ + {Action: "multi-app:read", Scope: "multi-app:*"}, + }, + }, + Grants: []string{string(identity.RoleViewer), string(identity.RoleEditor)}, + }, + }, + wantGrantedRoles: []string{string(identity.RoleViewer), string(identity.RoleEditor)}, + wantPermissions: []string{"multi-app:read"}, + wantPermCount: 1, + }, + { + name: "should register multiple permissions dynamically", + postRegister: true, + pluginID: "batch-app", + registrations: []plugins.RoleRegistration{ + { + Role: plugins.Role{ + Name: "BatchReader", + Permissions: []plugins.Permission{ + {Action: "batch-app:read"}, + {Action: "batch-app:write"}, + {Action: "batch-app:delete"}, + }, + }, + Grants: []string{string(identity.RoleEditor)}, + }, + }, + wantGrantedRoles: []string{string(identity.RoleEditor)}, + wantPermissions: []string{"batch-app:read", "batch-app:write", "batch-app:delete"}, + wantPermCount: 3, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ac := setupTestEnv(t, tt.postRegister) + initialPermCounts := make(map[string]int) + for _, roleName := range tt.wantGrantedRoles { + if role, ok := ac.roles[roleName]; ok { + initialPermCounts[roleName] = len(role.Permissions) + } + } + + err := ac.DeclarePluginRoles(context.Background(), tt.pluginID, tt.pluginID, tt.registrations) + require.NoError(t, err) + + if !tt.postRegister { + err = ac.RegisterFixedRoles(context.Background()) + require.NoError(t, err) + } + + // Verify permissions were added to expected roles + for _, roleName := range tt.wantGrantedRoles { + role, ok := ac.roles[roleName] + require.True(t, ok, "Role %s should exist", roleName) + + expectedCount := initialPermCounts[roleName] + tt.wantPermCount + assert.Equal(t, expectedCount, len(role.Permissions)) + + actions := make([]string, 0, len(role.Permissions)) + for _, perm := range role.Permissions { + actions = append(actions, perm.Action) + } + + for _, wantAction := range tt.wantPermissions { + assert.Contains(t, actions, wantAction, "Role %s should have permission %s", roleName, wantAction) + } + } + }) + } +} + +func TestIntegrationService_DeclarePluginRoles_UserPermissions(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + + t.Run("dynamic registration should be reflected in user permissions", func(t *testing.T) { + ac := setupTestEnv(t, true) + + // Create a test user with Viewer role + testUser := &user.SignedInUser{ + UserID: 1, + OrgID: 1, + OrgRole: identity.RoleViewer, + } + + // Get initial permissions + initialPerms, err := ac.getUserPermissions(context.Background(), testUser, accesscontrol.Options{}) + require.NoError(t, err) + initialCount := len(initialPerms) + + // Register plugin roles dynamically + err = ac.DeclarePluginRoles(context.Background(), "perm-test-app", "Perm Test App", []plugins.RoleRegistration{ + { + Role: plugins.Role{ + Name: "PermTester", + Description: "Permission test role", + Permissions: []plugins.Permission{ + {Action: "perm-test-app:test", Scope: "perm-test-app:*"}, + }, + }, + Grants: []string{string(identity.RoleViewer)}, + }, + }) + require.NoError(t, err) + + updatedPerms, err := ac.getUserPermissions(context.Background(), testUser, accesscontrol.Options{}) + require.NoError(t, err) + + require.Greater(t, len(updatedPerms), initialCount) + hasNewPerm := false + hasNewScope := false + for _, perm := range updatedPerms { + if perm.Action == "perm-test-app:test" { + hasNewPerm = true + } + if perm.Scope == "perm-test-app:*" { + hasNewScope = true + } + } + assert.True(t, hasNewPerm) + assert.True(t, hasNewScope) + }) +} + func TestIntegrationService_SearchUsersPermissions(t *testing.T) { testutil.SkipIntegrationTestInShortMode(t) @@ -585,7 +776,7 @@ func TestIntegrationService_SearchUsersPermissions(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ac := setupTestEnv(t) + ac := setupTestEnv(t, true) ac.roles = tt.ramRoles ac.store = actest.FakeStore{ @@ -817,7 +1008,7 @@ func TestIntegrationService_SearchUserPermissions(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ac := setupTestEnv(t) + ac := setupTestEnv(t, true) if tt.withActionSets { actionSetSvc := resourcepermissions.NewActionSetService() for set, actions := range tt.actionSets { @@ -913,7 +1104,7 @@ func TestIntegrationService_SaveExternalServiceRole(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx := context.Background() - ac := setupTestEnv(t) + ac := setupTestEnv(t, true) ac.cfg.ManagedServiceAccountsEnabled = true ac.features = featuremgmt.WithFeatures(featuremgmt.FlagExternalServiceAccounts) for _, r := range tt.runs { @@ -969,7 +1160,7 @@ func TestIntegrationService_DeleteExternalServiceRole(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx := context.Background() - ac := setupTestEnv(t) + ac := setupTestEnv(t, true) ac.cfg.ManagedServiceAccountsEnabled = true ac.features = featuremgmt.WithFeatures(featuremgmt.FlagExternalServiceAccounts) @@ -1006,7 +1197,7 @@ func TestIntegrationService_GetRoleByName(t *testing.T) { t.Run("when the role does not exists, it returns an error", func(t *testing.T) { t.Parallel() - ac := setupTestEnv(t) + ac := setupTestEnv(t, true) ac.registrations = accesscontrol.RegistrationList{} role, err := ac.GetRoleByName(ctx, 0, "not-found-role") @@ -1019,7 +1210,7 @@ func TestIntegrationService_GetRoleByName(t *testing.T) { roleName := "fixed:test:test" - ac := setupTestEnv(t) + ac := setupTestEnv(t, true) ac.registrations = accesscontrol.RegistrationList{} ac.registrations.Append(accesscontrol.RoleRegistration{ Role: accesscontrol.RoleDTO{Name: roleName},