diff --git a/pkg/services/authn/authn.go b/pkg/services/authn/authn.go index 9ec4ac6de35..985858a7135 100644 --- a/pkg/services/authn/authn.go +++ b/pkg/services/authn/authn.go @@ -64,8 +64,11 @@ type ClientParams struct { } type FetchPermissionsParams struct { - // ActionsLookup will restrict the permissions to only these actions - ActionsLookup []string + // RestrictedActions will restrict the permissions to only these actions + RestrictedActions []string + // AllowedActions will be added to the identity permissions + AllowedActions []string + // Note: Kept for backwards compatibility, use AllowedActions instead // Roles permissions will be directly added to the identity permissions Roles []string } diff --git a/pkg/services/authn/authnimpl/registration.go b/pkg/services/authn/authnimpl/registration.go index e56c05663d3..85358a3545a 100644 --- a/pkg/services/authn/authnimpl/registration.go +++ b/pkg/services/authn/authnimpl/registration.go @@ -6,6 +6,7 @@ import ( "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/accesscontrol/permreg" "github.com/grafana/grafana/pkg/services/apikey" "github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/services/auth/gcomsso" @@ -29,7 +30,7 @@ type Registration struct{} func ProvideRegistration( cfg *setting.Cfg, authnSvc authn.Service, orgService org.Service, sessionService auth.UserTokenService, - accessControlService accesscontrol.Service, + accessControlService accesscontrol.Service, permRegistry permreg.PermissionRegistry, apikeyService apikey.Service, userService user.Service, jwtService auth.JWTVerifierService, userProtectionService login.UserProtectionService, loginAttempts loginattempt.Service, quotaService quota.Service, @@ -109,7 +110,7 @@ func ProvideRegistration( authnSvc.RegisterPostAuthHook(sync.ProvideOAuthTokenSync(oauthTokenService, sessionService, socialService, tracer).SyncOauthTokenHook, 60) authnSvc.RegisterPostAuthHook(userSync.FetchSyncedUserHook, 100) - rbacSync := sync.ProvideRBACSync(accessControlService, tracer) + rbacSync := sync.ProvideRBACSync(accessControlService, tracer, permRegistry) if features.IsEnabledGlobally(featuremgmt.FlagCloudRBACRoles) { authnSvc.RegisterPostAuthHook(rbacSync.SyncCloudRoles, 110) authnSvc.RegisterPreLogoutHook(gcomsso.ProvideGComSSOService(cfg).LogoutHook, 50) diff --git a/pkg/services/authn/authnimpl/service.go b/pkg/services/authn/authnimpl/service.go index 9d6e047f22e..e34a0463f67 100644 --- a/pkg/services/authn/authnimpl/service.go +++ b/pkg/services/authn/authnimpl/service.go @@ -150,14 +150,18 @@ func (s *Service) authenticate(ctx context.Context, c authn.Client, r *authn.Req attribute.String("identity.AuthenticatedBy", identity.GetAuthenticatedBy()), ) - if len(identity.ClientParams.FetchPermissionsParams.ActionsLookup) > 0 { - span.SetAttributes(attribute.StringSlice("identity.ClientParams.FetchPermissionsParams.ActionsLookup", identity.ClientParams.FetchPermissionsParams.ActionsLookup)) + if len(identity.ClientParams.FetchPermissionsParams.RestrictedActions) > 0 { + span.SetAttributes(attribute.StringSlice("identity.ClientParams.FetchPermissionsParams.RestrictedActions", identity.ClientParams.FetchPermissionsParams.RestrictedActions)) } if len(identity.ClientParams.FetchPermissionsParams.Roles) > 0 { span.SetAttributes(attribute.StringSlice("identity.ClientParams.FetchPermissionsParams.Roles", identity.ClientParams.FetchPermissionsParams.Roles)) } + if len(identity.ClientParams.FetchPermissionsParams.AllowedActions) > 0 { + span.SetAttributes(attribute.StringSlice("identity.ClientParams.FetchPermissionsParams.AllowedActions", identity.ClientParams.FetchPermissionsParams.AllowedActions)) + } + if err := s.runPostAuthHooks(ctx, identity, r); err != nil { s.errorLogFunc(ctx, err)("Failed to run post auth hook", "client", c.Name(), "id", identity.ID, "error", err) return nil, err diff --git a/pkg/services/authn/authnimpl/service_test.go b/pkg/services/authn/authnimpl/service_test.go index 216de8300d8..2727a8dace0 100644 --- a/pkg/services/authn/authnimpl/service_test.go +++ b/pkg/services/authn/authnimpl/service_test.go @@ -59,7 +59,7 @@ func TestService_Authenticate(t *testing.T) { Type: claims.TypeUser, ClientParams: authn.ClientParams{ FetchPermissionsParams: authn.FetchPermissionsParams{ - ActionsLookup: []string{ + RestrictedActions: []string{ "datasources:read", "datasources:query", }, @@ -76,7 +76,7 @@ func TestService_Authenticate(t *testing.T) { Type: claims.TypeUser, ClientParams: authn.ClientParams{ FetchPermissionsParams: authn.FetchPermissionsParams{ - ActionsLookup: []string{ + RestrictedActions: []string{ "datasources:read", "datasources:query", }, @@ -87,6 +87,50 @@ func TestService_Authenticate(t *testing.T) { }, }, }, + { + desc: "should succeed with authentication for client with fetch permissions params made of roles and actions", + clients: []authn.Client{ + &authntest.FakeClient{ + ExpectedTest: true, + ExpectedIdentity: &authn.Identity{ + ID: "2", + Type: claims.TypeUser, + ClientParams: authn.ClientParams{ + FetchPermissionsParams: authn.FetchPermissionsParams{ + RestrictedActions: []string{ + "datasources:read", + "datasources:query", + }, + AllowedActions: []string{ + "datasources:write", + }, + Roles: []string{ + "fixed:datasources:writer", + }, + }, + }, + }, + }, + }, + expectedIdentity: &authn.Identity{ + ID: "2", + Type: claims.TypeUser, + ClientParams: authn.ClientParams{ + FetchPermissionsParams: authn.FetchPermissionsParams{ + RestrictedActions: []string{ + "datasources:read", + "datasources:query", + }, + AllowedActions: []string{ + "datasources:write", + }, + Roles: []string{ + "fixed:datasources:writer", + }, + }, + }, + }, + }, { desc: "should succeed with authentication for second client when first test fail", clients: []authn.Client{ @@ -187,9 +231,13 @@ func TestService_Authenticate(t *testing.T) { assert.Equal(t, tt.expectedIdentity.AuthID, attr.Value.AsString()) case "identity.AuthenticatedBy": assert.Equal(t, tt.expectedIdentity.AuthenticatedBy, attr.Value.AsString()) - case "identity.ClientParams.FetchPermissionsParams.ActionsLookup": - if len(tt.expectedIdentity.ClientParams.FetchPermissionsParams.ActionsLookup) > 0 { - assert.Equal(t, tt.expectedIdentity.ClientParams.FetchPermissionsParams.ActionsLookup, attr.Value.AsStringSlice()) + case "identity.ClientParams.FetchPermissionsParams.RestrictedActions": + if len(tt.expectedIdentity.ClientParams.FetchPermissionsParams.RestrictedActions) > 0 { + assert.Equal(t, tt.expectedIdentity.ClientParams.FetchPermissionsParams.RestrictedActions, attr.Value.AsStringSlice()) + } + case "identity.ClientParams.FetchPermissionsParams.AllowedActions": + if len(tt.expectedIdentity.ClientParams.FetchPermissionsParams.AllowedActions) > 0 { + assert.Equal(t, tt.expectedIdentity.ClientParams.FetchPermissionsParams.AllowedActions, attr.Value.AsStringSlice()) } case "identity.ClientParams.FetchPermissionsParams.Roles": if len(tt.expectedIdentity.ClientParams.FetchPermissionsParams.Roles) > 0 { diff --git a/pkg/services/authn/authnimpl/sync/rbac_sync.go b/pkg/services/authn/authnimpl/sync/rbac_sync.go index f5b11c687ca..06fe649500f 100644 --- a/pkg/services/authn/authnimpl/sync/rbac_sync.go +++ b/pkg/services/authn/authnimpl/sync/rbac_sync.go @@ -11,6 +11,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/accesscontrol/permreg" "github.com/grafana/grafana/pkg/services/authn" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/org" @@ -21,18 +22,20 @@ var ( errSyncPermissionsForbidden = errutil.Forbidden("permissions.sync.forbidden") ) -func ProvideRBACSync(acService accesscontrol.Service, tracer tracing.Tracer) *RBACSync { +func ProvideRBACSync(acService accesscontrol.Service, tracer tracing.Tracer, permRegistry permreg.PermissionRegistry) *RBACSync { return &RBACSync{ - ac: acService, - log: log.New("permissions.sync"), - tracer: tracer, + ac: acService, + log: log.New("permissions.sync"), + permRegistry: permRegistry, + tracer: tracer, } } type RBACSync struct { - ac accesscontrol.Service - log log.Logger - tracer tracing.Tracer + ac accesscontrol.Service + permRegistry permreg.PermissionRegistry + log log.Logger + tracer tracing.Tracer } func (s *RBACSync) SyncPermissionsHook(ctx context.Context, ident *authn.Identity, _ *authn.Request) error { @@ -56,7 +59,7 @@ func (s *RBACSync) SyncPermissionsHook(ctx context.Context, ident *authn.Identit grouped := accesscontrol.GroupScopesByActionContext(ctx, permissions) // Restrict access to the list of actions - actionsLookup := ident.ClientParams.FetchPermissionsParams.ActionsLookup + actionsLookup := ident.ClientParams.FetchPermissionsParams.RestrictedActions if len(actionsLookup) > 0 { filtered := make(map[string][]string, len(actionsLookup)) for _, action := range actionsLookup { @@ -77,7 +80,8 @@ func (s *RBACSync) fetchPermissions(ctx context.Context, ident *authn.Identity) permissions := make([]accesscontrol.Permission, 0, 8) roles := ident.ClientParams.FetchPermissionsParams.Roles - if len(roles) > 0 { + actions := ident.ClientParams.FetchPermissionsParams.AllowedActions + if len(roles) > 0 || len(actions) > 0 { for _, role := range roles { roleDTO, err := s.ac.GetRoleByName(ctx, ident.GetOrgID(), role) if err != nil && !errors.Is(err, accesscontrol.ErrRoleNotFound) { @@ -88,7 +92,20 @@ func (s *RBACSync) fetchPermissions(ctx context.Context, ident *authn.Identity) permissions = append(permissions, roleDTO.Permissions...) } } - + for _, action := range actions { + scopes, ok := s.permRegistry.GetScopePrefixes(action) + if !ok { + s.log.Warn("Unknown action scopes", "action", action) + continue + } + if len(scopes) == 0 { + permissions = append(permissions, accesscontrol.Permission{Action: action}) + continue + } + for scope := range scopes { + permissions = append(permissions, accesscontrol.Permission{Action: action, Scope: scope + "*"}) + } + } return permissions, nil } diff --git a/pkg/services/authn/authnimpl/sync/rbac_sync_test.go b/pkg/services/authn/authnimpl/sync/rbac_sync_test.go index 08e3434d15d..c9bc0e08082 100644 --- a/pkg/services/authn/authnimpl/sync/rbac_sync_test.go +++ b/pkg/services/authn/authnimpl/sync/rbac_sync_test.go @@ -13,6 +13,7 @@ import ( "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/services/accesscontrol" acmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" + permreg "github.com/grafana/grafana/pkg/services/accesscontrol/permreg/test" "github.com/grafana/grafana/pkg/services/authn" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/org" @@ -22,34 +23,155 @@ func TestRBACSync_SyncPermission(t *testing.T) { type testCase struct { name string identity *authn.Identity - expectedPermissions []accesscontrol.Permission + expectedPermissions map[string][]string } testCases := []testCase{ { name: "enriches the identity successfully when SyncPermissions is true", identity: &authn.Identity{ID: "2", Type: claims.TypeUser, OrgID: 1, ClientParams: authn.ClientParams{SyncPermissions: true}}, - expectedPermissions: []accesscontrol.Permission{ - {Action: accesscontrol.ActionUsersRead}, + expectedPermissions: map[string][]string{ + accesscontrol.ActionUsersRead: {accesscontrol.ScopeUsersAll}, + accesscontrol.ActionUsersWrite: {accesscontrol.ScopeUsersAll}, }, }, { - name: "does not load the permissions when SyncPermissions is false", - identity: &authn.Identity{ID: "2", Type: claims.TypeUser, OrgID: 1, ClientParams: authn.ClientParams{SyncPermissions: true}}, - expectedPermissions: []accesscontrol.Permission{ - {Action: accesscontrol.ActionUsersRead}, + name: "does not load the permissions when SyncPermissions is false", + identity: &authn.Identity{ID: "2", Type: claims.TypeUser, OrgID: 1, ClientParams: authn.ClientParams{SyncPermissions: false}}, + expectedPermissions: nil, + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + s := setupTestEnv(t) + + err := s.SyncPermissionsHook(context.Background(), tt.identity, &authn.Request{}) + require.NoError(t, err) + + require.Equal(t, len(tt.expectedPermissions), len(tt.identity.Permissions[tt.identity.OrgID])) + for action, scopes := range tt.expectedPermissions { + require.ElementsMatch(t, scopes, tt.identity.Permissions[tt.identity.OrgID][action]) + } + }) + } +} + +func TestRBACSync_FetchPermissions(t *testing.T) { + type testCase struct { + name string + identity *authn.Identity + expectedPermissions map[string][]string + } + testCases := []testCase{ + { + name: "restrict permissions from store", + identity: &authn.Identity{ + ID: "2", Type: claims.TypeUser, OrgID: 1, + ClientParams: authn.ClientParams{ + SyncPermissions: true, + FetchPermissionsParams: authn.FetchPermissionsParams{ + RestrictedActions: []string{accesscontrol.ActionUsersRead}, + }, + }, + }, + expectedPermissions: map[string][]string{accesscontrol.ActionUsersRead: {accesscontrol.ScopeUsersAll}}, + }, + { + name: "fetch roles permissions", + identity: &authn.Identity{ + ID: "2", Type: claims.TypeUser, OrgID: 1, + ClientParams: authn.ClientParams{ + SyncPermissions: true, + FetchPermissionsParams: authn.FetchPermissionsParams{ + Roles: []string{"fixed:teams:reader"}, + }, + }, + }, + expectedPermissions: map[string][]string{accesscontrol.ActionTeamsRead: {accesscontrol.ScopeTeamsAll}}, + }, + { + name: "robust to missing roles", + identity: &authn.Identity{ + ID: "2", Type: claims.TypeUser, OrgID: 1, + ClientParams: authn.ClientParams{ + SyncPermissions: true, + FetchPermissionsParams: authn.FetchPermissionsParams{ + Roles: []string{"fixed:teams:reader", "fixed:unknown:role"}, + }, + }, + }, + expectedPermissions: map[string][]string{accesscontrol.ActionTeamsRead: {accesscontrol.ScopeTeamsAll}}, + }, + { + name: "fetch permissions from permissions registry", + identity: &authn.Identity{ + ID: "2", Type: claims.TypeUser, OrgID: 1, + ClientParams: authn.ClientParams{ + SyncPermissions: true, + FetchPermissionsParams: authn.FetchPermissionsParams{ + AllowedActions: []string{"dashboards:read"}, + }, + }, + }, + expectedPermissions: map[string][]string{"dashboards:read": {"dashboards:uid:*", "folders:uid:*"}}, + }, + { + name: "fetch scopeless permissions from permissions registry", + identity: &authn.Identity{ + ID: "2", Type: claims.TypeUser, OrgID: 1, + ClientParams: authn.ClientParams{ + SyncPermissions: true, + FetchPermissionsParams: authn.FetchPermissionsParams{ + AllowedActions: []string{"test-app:read"}, + }, + }, + }, + expectedPermissions: map[string][]string{"test-app:read": {""}}, + }, + { + name: "robust to unknown actions", + identity: &authn.Identity{ + ID: "2", Type: claims.TypeUser, OrgID: 1, + ClientParams: authn.ClientParams{ + SyncPermissions: true, + FetchPermissionsParams: authn.FetchPermissionsParams{ + AllowedActions: []string{"unknown:read"}, + }, + }, + }, + expectedPermissions: map[string][]string{}, + }, + { + name: "restrict permissions from roles and registry", + identity: &authn.Identity{ + ID: "2", Type: claims.TypeUser, OrgID: 1, + ClientParams: authn.ClientParams{ + SyncPermissions: true, + FetchPermissionsParams: authn.FetchPermissionsParams{ + RestrictedActions: []string{accesscontrol.ActionUsersWrite, accesscontrol.ActionTeamsWrite, "dashboards:read"}, + AllowedActions: []string{"dashboards:read"}, + Roles: []string{"fixed:teams:reader", "fixed:teams:writer"}, + }, + }, + }, + expectedPermissions: map[string][]string{ + "dashboards:read": {"dashboards:uid:*", "folders:uid:*"}, + accesscontrol.ActionTeamsWrite: {accesscontrol.ScopeTeamsAll}, }, }, } for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { - s := setupTestEnv() + s := setupTestEnv(t) err := s.SyncPermissionsHook(context.Background(), tt.identity, &authn.Request{}) require.NoError(t, err) - assert.Equal(t, 1, len(tt.identity.Permissions)) - assert.Equal(t, accesscontrol.GroupScopesByActionContext(context.Background(), tt.expectedPermissions), tt.identity.Permissions[tt.identity.OrgID]) + require.Equal(t, len(tt.expectedPermissions), len(tt.identity.Permissions[tt.identity.OrgID])) + for action, scopes := range tt.expectedPermissions { + require.ElementsMatch(t, scopes, tt.identity.Permissions[tt.identity.OrgID][action]) + } }) } } @@ -242,18 +364,36 @@ func TestRBACSync_cloudRolesToAddAndRemove(t *testing.T) { } } -func setupTestEnv() *RBACSync { +func setupTestEnv(t *testing.T) *RBACSync { acMock := &acmock.Mock{ GetUserPermissionsFunc: func(ctx context.Context, siu identity.Requester, o accesscontrol.Options) ([]accesscontrol.Permission, error) { return []accesscontrol.Permission{ - {Action: accesscontrol.ActionUsersRead}, + {Action: accesscontrol.ActionUsersRead, Scope: accesscontrol.ScopeUsersAll}, + {Action: accesscontrol.ActionUsersWrite, Scope: accesscontrol.ScopeUsersAll}, }, nil }, + GetRoleByNameFunc: func(ctx context.Context, i int64, s string) (*accesscontrol.RoleDTO, error) { + if s == "fixed:teams:reader" { + return &accesscontrol.RoleDTO{ + ID: 1, Name: "fixed:teams:reader", + Permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionTeamsRead, Scope: accesscontrol.ScopeTeamsAll}}, + }, nil + } + if s == "fixed:teams:writer" { + return &accesscontrol.RoleDTO{ + ID: 1, Name: "fixed:teams:writer", + Permissions: []accesscontrol.Permission{{Action: accesscontrol.ActionTeamsWrite, Scope: accesscontrol.ScopeTeamsAll}}, + }, nil + } + return nil, accesscontrol.ErrRoleNotFound + }, } + permRegistry := permreg.ProvidePermissionRegistry(t) s := &RBACSync{ - ac: acMock, - log: log.NewNopLogger(), - tracer: tracing.InitializeTracerForTest(), + ac: acMock, + log: log.NewNopLogger(), + tracer: tracing.InitializeTracerForTest(), + permRegistry: permRegistry, } return s } diff --git a/pkg/services/authn/clients/ext_jwt.go b/pkg/services/authn/clients/ext_jwt.go index db7285c4729..7d9b30f36c0 100644 --- a/pkg/services/authn/clients/ext_jwt.go +++ b/pkg/services/authn/clients/ext_jwt.go @@ -147,7 +147,7 @@ func (s *ExtendedJWT) authenticateAsUser( ClientParams: authn.ClientParams{ SyncPermissions: true, FetchPermissionsParams: authn.FetchPermissionsParams{ - ActionsLookup: accessTokenClaims.Rest.DelegatedPermissions, + RestrictedActions: accessTokenClaims.Rest.DelegatedPermissions, }, FetchSyncedUser: true, }}, nil @@ -168,6 +168,20 @@ func (s *ExtendedJWT) authenticateAsService(accessTokenClaims authlib.Claims[aut return nil, errExtJWTInvalidSubject.Errorf("unexpected identity: %s", accessTokenClaims.Subject) } + permissions := accessTokenClaims.Rest.Permissions + fetchPermissionsParams := authn.FetchPermissionsParams{} + if len(permissions) > 0 { + fetchPermissionsParams.Roles = make([]string, 0, len(permissions)) + fetchPermissionsParams.AllowedActions = make([]string, 0, len(permissions)) + for i := range permissions { + if strings.HasPrefix(permissions[i], "fixed:") { + fetchPermissionsParams.Roles = append(fetchPermissionsParams.Roles, permissions[i]) + } else { + fetchPermissionsParams.AllowedActions = append(fetchPermissionsParams.AllowedActions, permissions[i]) + } + } + } + return &authn.Identity{ ID: id, UID: id, @@ -179,11 +193,9 @@ func (s *ExtendedJWT) authenticateAsService(accessTokenClaims authlib.Claims[aut AuthID: accessTokenClaims.Subject, AllowedKubernetesNamespace: accessTokenClaims.Rest.Namespace, ClientParams: authn.ClientParams{ - SyncPermissions: true, - FetchPermissionsParams: authn.FetchPermissionsParams{ - Roles: accessTokenClaims.Rest.Permissions, - }, - FetchSyncedUser: false, + SyncPermissions: true, + FetchPermissionsParams: fetchPermissionsParams, + FetchSyncedUser: false, }, }, nil } diff --git a/pkg/services/authn/clients/ext_jwt_test.go b/pkg/services/authn/clients/ext_jwt_test.go index 7e1fc352117..6b61748dd03 100644 --- a/pkg/services/authn/clients/ext_jwt_test.go +++ b/pkg/services/authn/clients/ext_jwt_test.go @@ -35,7 +35,7 @@ var ( Rest: authnlib.AccessTokenClaims{ Scopes: []string{"profile", "groups"}, DelegatedPermissions: []string{"dashboards:create", "folders:read", "datasources:explore", "datasources.insights:read"}, - Permissions: []string{"fixed:folders:reader"}, + Permissions: []string{"fixed:folders:reader", "folders:read"}, Namespace: "default", // org ID of 1 is special and translates to default }, } @@ -236,7 +236,7 @@ func TestExtendedJWT_Authenticate(t *testing.T) { AuthID: "access-policy:this-uid", ClientParams: authn.ClientParams{ SyncPermissions: true, - FetchPermissionsParams: authn.FetchPermissionsParams{Roles: []string{"fixed:folders:reader"}}}, + FetchPermissionsParams: authn.FetchPermissionsParams{Roles: []string{"fixed:folders:reader"}, AllowedActions: []string{"folders:read"}}}, }, }, { @@ -275,7 +275,7 @@ func TestExtendedJWT_Authenticate(t *testing.T) { FetchSyncedUser: true, SyncPermissions: true, FetchPermissionsParams: authn.FetchPermissionsParams{ - ActionsLookup: []string{"dashboards:create", "folders:read", "datasources:explore", "datasources.insights:read"}, + RestrictedActions: []string{"dashboards:create", "folders:read", "datasources:explore", "datasources.insights:read"}, }, }, },