From 3e777681447d311b7cde5de2e0c8f842a2280b50 Mon Sep 17 00:00:00 2001 From: Ieva Date: Tue, 21 May 2024 15:09:26 +0100 Subject: [PATCH] RBAC: Expand action sets when fetching permissions (#87967) * logic to expand action set to the underlying actions when permissions are fetched from the DB * updates needed for dependency injection * clean up some code, also deduplicate scopes when grouping scopes and actions * expand on a comment * rename a method --- pkg/api/folder_bench_test.go | 4 +- .../commands/conflict_user_command.go | 2 +- pkg/server/wire.go | 4 +- pkg/services/accesscontrol/accesscontrol.go | 24 +++- pkg/services/accesscontrol/acimpl/service.go | 60 ++++++---- .../accesscontrol/acimpl/service_test.go | 1 + pkg/services/accesscontrol/actest/fake.go | 19 ++++ pkg/services/accesscontrol/resolvers.go | 6 +- .../resourcepermissions/store.go | 44 ++++++-- .../resourcepermissions/store_test.go | 103 +++++++++++++++++- .../extsvcaccounts/service_test.go | 2 +- 11 files changed, 224 insertions(+), 45 deletions(-) diff --git a/pkg/api/folder_bench_test.go b/pkg/api/folder_bench_test.go index 053a3261d32..c761827b221 100644 --- a/pkg/api/folder_bench_test.go +++ b/pkg/api/folder_bench_test.go @@ -448,8 +448,6 @@ func setupServer(b testing.TB, sc benchScenario, features featuremgmt.FeatureTog license := licensingtest.NewFakeLicensing() license.On("FeatureEnabled", "accesscontrol.enforcement").Return(true).Maybe() - acSvc := acimpl.ProvideOSSService(sc.cfg, acdb.ProvideService(sc.db), localcache.ProvideService(), features, tracing.InitializeTracerForTest()) - quotaSrv := quotatest.New(false, nil) dashStore, err := database.ProvideDashboardStore(sc.db, sc.cfg, features, tagimpl.ProvideService(sc.db), quotaSrv) @@ -462,6 +460,8 @@ func setupServer(b testing.TB, sc benchScenario, features featuremgmt.FeatureTog cfg := setting.NewCfg() actionSets := resourcepermissions.NewActionSetService(ac) + acSvc := acimpl.ProvideOSSService(sc.cfg, acdb.ProvideService(sc.db), actionSets, localcache.ProvideService(), features, tracing.InitializeTracerForTest()) + folderPermissions, err := ossaccesscontrol.ProvideFolderPermissions( cfg, features, routing.NewRouteRegister(), sc.db, ac, license, &dashboards.FakeDashboardStore{}, folderServiceWithFlagOn, acSvc, sc.teamSvc, sc.userSvc, actionSets) require.NoError(b, err) diff --git a/pkg/cmd/grafana-cli/commands/conflict_user_command.go b/pkg/cmd/grafana-cli/commands/conflict_user_command.go index 68e022c2e47..bf09858e31a 100644 --- a/pkg/cmd/grafana-cli/commands/conflict_user_command.go +++ b/pkg/cmd/grafana-cli/commands/conflict_user_command.go @@ -89,7 +89,7 @@ func initializeConflictResolver(cmd *utils.ContextCommandLine, f Formatter, ctx if err != nil { return nil, fmt.Errorf("%v: %w", "failed to initialize tracer service", err) } - acService, err := acimpl.ProvideService(cfg, s, routing, nil, nil, features, tracer) + acService, err := acimpl.ProvideService(cfg, s, routing, nil, nil, nil, features, tracer) if err != nil { return nil, fmt.Errorf("%v: %w", "failed to get access control", err) } diff --git a/pkg/server/wire.go b/pkg/server/wire.go index f0520f9ae73..d6d3a720552 100644 --- a/pkg/server/wire.go +++ b/pkg/server/wire.go @@ -351,6 +351,9 @@ var wireBasicSet = wire.NewSet( secretsMigrations.ProvideMigrateFromPluginService, secretsMigrations.ProvideSecretMigrationProvider, wire.Bind(new(secretsMigrations.SecretMigrationProvider), new(*secretsMigrations.SecretMigrationProviderImpl)), + resourcepermissions.NewActionSetService, + wire.Bind(new(accesscontrol.ActionResolver), new(*resourcepermissions.InMemoryActionSets)), + wire.Bind(new(resourcepermissions.ActionSetService), new(*resourcepermissions.InMemoryActionSets)), acimpl.ProvideAccessControl, navtreeimpl.ProvideService, wire.Bind(new(accesscontrol.AccessControl), new(*acimpl.AccessControl)), @@ -382,7 +385,6 @@ var wireBasicSet = wire.NewSet( // Kubernetes API server grafanaapiserver.WireSet, apiregistry.WireSet, - resourcepermissions.NewActionSetService, ) var wireSet = wire.NewSet( diff --git a/pkg/services/accesscontrol/accesscontrol.go b/pkg/services/accesscontrol/accesscontrol.go index 170c3c9562f..ecd2d7aaebf 100644 --- a/pkg/services/accesscontrol/accesscontrol.go +++ b/pkg/services/accesscontrol/accesscontrol.go @@ -232,11 +232,29 @@ func BuildPermissionsMap(permissions []Permission) map[string]bool { // GroupScopesByAction will group scopes on action func GroupScopesByAction(permissions []Permission) map[string][]string { - m := make(map[string][]string) + // Use a map to deduplicate scopes. + // User can have the same permission from multiple sources (e.g. team, basic role, directly assigned etc). + // User will also have duplicate permissions if action sets are used, as we will be double writing permissions for a while. + m := make(map[string]map[string]struct{}) for i := range permissions { - m[permissions[i].Action] = append(m[permissions[i].Action], permissions[i].Scope) + if _, ok := m[permissions[i].Action]; !ok { + m[permissions[i].Action] = make(map[string]struct{}) + } + m[permissions[i].Action][permissions[i].Scope] = struct{}{} } - return m + + res := make(map[string][]string, len(m)) + for action, scopes := range m { + scopeList := make([]string, len(scopes)) + i := 0 + for scope := range scopes { + scopeList[i] = scope + i++ + } + res[action] = scopeList + } + + return res } // Reduce will reduce a list of permissions to its minimal form, grouping scopes by action diff --git a/pkg/services/accesscontrol/acimpl/service.go b/pkg/services/accesscontrol/acimpl/service.go index 23be6f849cb..371e25c79f7 100644 --- a/pkg/services/accesscontrol/acimpl/service.go +++ b/pkg/services/accesscontrol/acimpl/service.go @@ -46,8 +46,8 @@ var SharedWithMeFolderPermission = accesscontrol.Permission{ var OSSRolesPrefixes = []string{accesscontrol.ManagedRolePrefix, accesscontrol.ExternalServiceRolePrefix} func ProvideService(cfg *setting.Cfg, db db.DB, routeRegister routing.RouteRegister, cache *localcache.CacheService, - accessControl accesscontrol.AccessControl, features featuremgmt.FeatureToggles, tracer tracing.Tracer) (*Service, error) { - service := ProvideOSSService(cfg, database.ProvideService(db), cache, features, tracer) + accessControl accesscontrol.AccessControl, actionResolver accesscontrol.ActionResolver, features featuremgmt.FeatureToggles, tracer tracing.Tracer) (*Service, error) { + service := ProvideOSSService(cfg, database.ProvideService(db), actionResolver, cache, features, tracer) api.NewAccessControlAPI(routeRegister, accessControl, service, features).RegisterAPIEndpoints() if err := accesscontrol.DeclareFixedRoles(service, cfg); err != nil { @@ -65,15 +65,16 @@ func ProvideService(cfg *setting.Cfg, db db.DB, routeRegister routing.RouteRegis return service, nil } -func ProvideOSSService(cfg *setting.Cfg, store accesscontrol.Store, cache *localcache.CacheService, features featuremgmt.FeatureToggles, tracer tracing.Tracer) *Service { +func ProvideOSSService(cfg *setting.Cfg, store accesscontrol.Store, actionResolver accesscontrol.ActionResolver, cache *localcache.CacheService, features featuremgmt.FeatureToggles, tracer tracing.Tracer) *Service { s := &Service{ - cache: cache, - cfg: cfg, - features: features, - log: log.New("accesscontrol.service"), - roles: accesscontrol.BuildBasicRoleDefinitions(), - store: store, - tracer: tracer, + actionResolver: actionResolver, + cache: cache, + cfg: cfg, + features: features, + log: log.New("accesscontrol.service"), + roles: accesscontrol.BuildBasicRoleDefinitions(), + store: store, + tracer: tracer, } return s @@ -81,14 +82,15 @@ func ProvideOSSService(cfg *setting.Cfg, store accesscontrol.Store, cache *local // Service is the service implementing role based access control. type Service struct { - cache *localcache.CacheService - cfg *setting.Cfg - features featuremgmt.FeatureToggles - log log.Logger - registrations accesscontrol.RegistrationList - roles map[string]*accesscontrol.RoleDTO - store accesscontrol.Store - tracer tracing.Tracer + actionResolver accesscontrol.ActionResolver + cache *localcache.CacheService + cfg *setting.Cfg + features featuremgmt.FeatureToggles + log log.Logger + registrations accesscontrol.RegistrationList + roles map[string]*accesscontrol.RoleDTO + store accesscontrol.Store + tracer tracing.Tracer } func (s *Service) GetUsageStats(_ context.Context) map[string]any { @@ -138,6 +140,9 @@ func (s *Service) getUserPermissions(ctx context.Context, user identity.Requeste if err != nil { return nil, err } + if s.features.IsEnabled(ctx, featuremgmt.FlagAccessActionSets) { + dbPermissions = s.actionResolver.ExpandActionSets(dbPermissions) + } return append(permissions, dbPermissions...), nil } @@ -157,8 +162,11 @@ func (s *Service) getBasicRolePermissions(ctx context.Context, role string, orgI OrgID: orgID, RolePrefixes: OSSRolesPrefixes, }) - permissions = append(permissions, dbPermissions...) - return permissions, err + if s.features.IsEnabled(ctx, featuremgmt.FlagAccessActionSets) { + dbPermissions = s.actionResolver.ExpandActionSets(dbPermissions) + } + + return append(permissions, dbPermissions...), err } func (s *Service) getTeamsPermissions(ctx context.Context, teamIDs []int64, orgID int64) (map[int64][]accesscontrol.Permission, error) { @@ -170,6 +178,13 @@ func (s *Service) getTeamsPermissions(ctx context.Context, teamIDs []int64, orgI OrgID: orgID, RolePrefixes: OSSRolesPrefixes, }) + + if s.features.IsEnabled(ctx, featuremgmt.FlagAccessActionSets) { + for teamID, permissions := range teamPermissions { + teamPermissions[teamID] = s.actionResolver.ExpandActionSets(permissions) + } + } + return teamPermissions, err } @@ -199,6 +214,9 @@ func (s *Service) getUserDirectPermissions(ctx context.Context, user identity.Re return nil, err } + if s.features.IsEnabled(ctx, featuremgmt.FlagAccessActionSets) { + permissions = s.actionResolver.ExpandActionSets(permissions) + } if s.features.IsEnabled(ctx, featuremgmt.FlagNestedFolders) { permissions = append(permissions, SharedWithMeFolderPermission) } @@ -404,6 +422,7 @@ func (s *Service) DeclarePluginRoles(ctx context.Context, ID, name string, regs return nil } +// TODO potential changes needed here? // SearchUsersPermissions returns all users' permissions filtered by action prefixes func (s *Service) SearchUsersPermissions(ctx context.Context, usr identity.Requester, options accesscontrol.SearchOptions) (map[int64][]accesscontrol.Permission, error) { @@ -418,6 +437,7 @@ func (s *Service) SearchUsersPermissions(ctx context.Context, usr identity.Reque // Reroute to the user specific implementation of search permissions // because it leverages the user permission cache. + // TODO userPerms, err := s.SearchUserPermissions(ctx, usr.GetOrgID(), options) if err != nil { return nil, err diff --git a/pkg/services/accesscontrol/acimpl/service_test.go b/pkg/services/accesscontrol/acimpl/service_test.go index a021ea4d8dd..d5f45490d9e 100644 --- a/pkg/services/accesscontrol/acimpl/service_test.go +++ b/pkg/services/accesscontrol/acimpl/service_test.go @@ -64,6 +64,7 @@ func TestUsageMetrics(t *testing.T) { s := ProvideOSSService( cfg, database.ProvideService(db.InitTestDB(t)), + &actest.FakeActionResolver{}, localcache.ProvideService(), featuremgmt.WithFeatures(), tracing.InitializeTracerForTest(), diff --git a/pkg/services/accesscontrol/actest/fake.go b/pkg/services/accesscontrol/actest/fake.go index 7b8059f23f1..025d3278e38 100644 --- a/pkg/services/accesscontrol/actest/fake.go +++ b/pkg/services/accesscontrol/actest/fake.go @@ -156,3 +156,22 @@ func (f *FakePermissionsService) DeleteResourcePermissions(ctx context.Context, func (f *FakePermissionsService) MapActions(permission accesscontrol.ResourcePermission) string { return f.ExpectedMappedAction } + +type FakeActionResolver struct { + ExpectedErr error + ExpectedActionSets []string + ExpectedActions []string + ExpectedPermissions []accesscontrol.Permission +} + +func (f *FakeActionResolver) ResolveAction(action string) []string { + return f.ExpectedActionSets +} + +func (f *FakeActionResolver) ResolveActionSet(actionSet string) []string { + return f.ExpectedActions +} + +func (f *FakeActionResolver) ExpandActionSets(permissions []accesscontrol.Permission) []accesscontrol.Permission { + return f.ExpectedPermissions +} diff --git a/pkg/services/accesscontrol/resolvers.go b/pkg/services/accesscontrol/resolvers.go index ad2b7854edb..43967268f8b 100644 --- a/pkg/services/accesscontrol/resolvers.go +++ b/pkg/services/accesscontrol/resolvers.go @@ -16,7 +16,9 @@ type ScopeAttributeResolver interface { } type ActionResolver interface { - Resolve(action string) []string + ResolveAction(action string) []string + ResolveActionSet(actionSet string) []string + ExpandActionSets(permissions []Permission) []Permission } // ScopeAttributeResolverFunc is an adapter to allow functions to implement ScopeAttributeResolver interface @@ -94,7 +96,7 @@ func (s *Resolvers) GetActionSetResolver() ActionSetResolver { if s.actionResolver == nil { return []string{action} } - actionSetActions := s.actionResolver.Resolve(action) + actionSetActions := s.actionResolver.ResolveAction(action) actions := append(actionSetActions, action) s.log.Debug("Resolved action", "action", action, "resolved_actions", actions) return actions diff --git a/pkg/services/accesscontrol/resourcepermissions/store.go b/pkg/services/accesscontrol/resourcepermissions/store.go index a0e6a343071..814d7b53115 100644 --- a/pkg/services/accesscontrol/resourcepermissions/store.go +++ b/pkg/services/accesscontrol/resourcepermissions/store.go @@ -10,6 +10,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" + "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/serviceaccounts" @@ -756,7 +757,7 @@ type InMemoryActionSets struct { } // NewActionSetService returns a new instance of InMemoryActionSetService. -func NewActionSetService(a *acimpl.AccessControl) ActionSetService { +func NewActionSetService(a *acimpl.AccessControl) *InMemoryActionSets { actionSets := &InMemoryActionSets{ log: log.New("resourcepermissions.actionsets"), actionSetToActions: make(map[string][]string), @@ -766,20 +767,14 @@ func NewActionSetService(a *acimpl.AccessControl) ActionSetService { return actionSets } -func (s *InMemoryActionSets) Resolve(action string) []string { +func (s *InMemoryActionSets) ResolveAction(action string) []string { actionSets := s.actionToActionSets[action] sets := make([]string, 0, len(actionSets)) for _, actionSet := range actionSets { - setParts := strings.Split(actionSet, ":") - if len(setParts) != 2 { - s.log.Debug("skipping resolution for action set with invalid name", "action set", actionSet) - continue - } - prefix := setParts[0] // Only use action sets for folders and dashboards for now - // We need to verify that action sets for other resources do not share names with actions (eg, `datasources:query`) - if prefix != "folders" && prefix != "dashboards" { + // We need to verify that action sets for other resources do not share names with actions (eg, `datasources:read`) + if !isFolderOrDashboardAction(actionSet) { continue } sets = append(sets, actionSet) @@ -788,6 +783,35 @@ func (s *InMemoryActionSets) Resolve(action string) []string { return sets } +func (s *InMemoryActionSets) ResolveActionSet(actionSet string) []string { + // Only use action sets for folders and dashboards for now + // We need to verify that action sets for other resources do not share names with actions (eg, `datasources:read`) + if !isFolderOrDashboardAction(actionSet) { + return nil + } + return s.actionSetToActions[actionSet] +} + +func isFolderOrDashboardAction(action string) bool { + return strings.HasPrefix(action, dashboards.ScopeDashboardsRoot) || strings.HasPrefix(action, dashboards.ScopeFoldersRoot) +} + +func (s *InMemoryActionSets) ExpandActionSets(permissions []accesscontrol.Permission) []accesscontrol.Permission { + var expandedPermissions []accesscontrol.Permission + for _, permission := range permissions { + resolvedActions := s.ResolveActionSet(permission.Action) + if len(resolvedActions) == 0 { + expandedPermissions = append(expandedPermissions, permission) + continue + } + for _, action := range resolvedActions { + permission.Action = action + expandedPermissions = append(expandedPermissions, permission) + } + } + return expandedPermissions +} + // GetActionSet returns the action set for the given action. func (s *InMemoryActionSets) GetActionSet(actionName string) []string { actionSet, ok := s.actionSetToActions[actionName] diff --git a/pkg/services/accesscontrol/resourcepermissions/store_test.go b/pkg/services/accesscontrol/resourcepermissions/store_test.go index 09203f4b9d2..0399561474f 100644 --- a/pkg/services/accesscontrol/resourcepermissions/store_test.go +++ b/pkg/services/accesscontrol/resourcepermissions/store_test.go @@ -796,10 +796,6 @@ func TestStore_StoreActionSet(t *testing.T) { } func TestStore_ResolveActionSet(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } - actionSetService := NewActionSetService(acimpl.ProvideAccessControl(featuremgmt.WithFeatures())) actionSetService.StoreActionSet("folders", "edit", []string{"folders:read", "folders:write", "dashboards:read", "dashboards:write"}) actionSetService.StoreActionSet("folders", "view", []string{"folders:read", "dashboards:read"}) @@ -836,8 +832,105 @@ func TestStore_ResolveActionSet(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - actionSets := actionSetService.Resolve(tt.action) + actionSets := actionSetService.ResolveAction(tt.action) require.ElementsMatch(t, tt.expectedActionSets, actionSets) }) } } + +func TestStore_ExpandActions(t *testing.T) { + actionSetService := NewActionSetService(acimpl.ProvideAccessControl(featuremgmt.WithFeatures())) + actionSetService.StoreActionSet("folders", "edit", []string{"folders:read", "folders:write", "dashboards:read", "dashboards:write"}) + actionSetService.StoreActionSet("folders", "view", []string{"folders:read", "dashboards:read"}) + actionSetService.StoreActionSet("dashboards", "view", []string{"dashboards:read"}) + + type actionSetTest struct { + desc string + permissions []accesscontrol.Permission + expectedPermissions []accesscontrol.Permission + } + + tests := []actionSetTest{ + { + desc: "should return empty list if no permissions are passed in", + permissions: []accesscontrol.Permission{}, + expectedPermissions: []accesscontrol.Permission{}, + }, + { + desc: "should return unchanged permissions if none of actions are part of any action sets", + permissions: []accesscontrol.Permission{ + { + Action: "datasources:create", + }, + { + Action: "users:read", + Scope: "users:*", + }, + }, + expectedPermissions: []accesscontrol.Permission{ + { + Action: "datasources:create", + }, + { + Action: "users:read", + Scope: "users:*", + }, + }, + }, + { + desc: "should return unchanged permissions if none of actions are part of any action sets", + permissions: []accesscontrol.Permission{ + { + Action: "datasources:create", + }, + { + Action: "users:read", + Scope: "users:*", + }, + }, + expectedPermissions: []accesscontrol.Permission{ + { + Action: "datasources:create", + }, + { + Action: "users:read", + Scope: "users:*", + }, + }, + }, + { + desc: "should be able to expand one permission and leave others unchanged", + permissions: []accesscontrol.Permission{ + { + Action: "folders:view", + Scope: "folders:uid:1", + }, + { + Action: "users:read", + Scope: "users:*", + }, + }, + expectedPermissions: []accesscontrol.Permission{ + { + Action: "folders:read", + Scope: "folders:uid:1", + }, + { + Action: "dashboards:read", + Scope: "folders:uid:1", + }, + { + Action: "users:read", + Scope: "users:*", + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + permissions := actionSetService.ExpandActionSets(tt.permissions) + require.ElementsMatch(t, tt.expectedPermissions, permissions) + }) + } +} diff --git a/pkg/services/serviceaccounts/extsvcaccounts/service_test.go b/pkg/services/serviceaccounts/extsvcaccounts/service_test.go index 4ab2e2c204d..b86f8befab6 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/service_test.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/service_test.go @@ -43,7 +43,7 @@ func setupTestEnv(t *testing.T) *TestEnv { } logger := log.New("extsvcaccounts.test") env.S = &ExtSvcAccountsService{ - acSvc: acimpl.ProvideOSSService(cfg, env.AcStore, localcache.New(0, 0), fmgt, tracing.InitializeTracerForTest()), + acSvc: acimpl.ProvideOSSService(cfg, env.AcStore, &actest.FakeActionResolver{}, localcache.New(0, 0), fmgt, tracing.InitializeTracerForTest()), features: fmgt, logger: logger, metrics: newMetrics(nil, env.SaSvc, logger),