From 7b58f71b334b8dd153776af76642a4e9ded74194 Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Wed, 17 Jan 2024 10:55:47 +0100 Subject: [PATCH] AuthN: Add auth hook that can sync grafana cloud role to rbac cloud role (#80416) * AuthnSync: Rename files and structures * AuthnSync: register rbac cloud role sync if feature toggle is enabled * RBAC: Add new sync function to service interface * RBAC: add common prefix and role names for cloud fixed roles * AuthnSync+RBAC: implement rbac cloud role sync Co-authored-by: Ieva --- pkg/services/accesscontrol/accesscontrol.go | 10 ++ pkg/services/accesscontrol/acimpl/service.go | 4 + pkg/services/accesscontrol/actest/fake.go | 1 + pkg/services/accesscontrol/mock/mock.go | 8 + pkg/services/accesscontrol/roles.go | 5 + pkg/services/authn/authnimpl/service.go | 10 +- .../authn/authnimpl/sync/permission_sync.go | 44 ----- .../authnimpl/sync/permission_sync_test.go | 65 -------- .../authn/authnimpl/sync/rbac_sync.go | 93 +++++++++++ .../authn/authnimpl/sync/rbac_sync_test.go | 157 ++++++++++++++++++ 10 files changed, 286 insertions(+), 111 deletions(-) delete mode 100644 pkg/services/authn/authnimpl/sync/permission_sync.go delete mode 100644 pkg/services/authn/authnimpl/sync/permission_sync_test.go create mode 100644 pkg/services/authn/authnimpl/sync/rbac_sync.go create mode 100644 pkg/services/authn/authnimpl/sync/rbac_sync_test.go diff --git a/pkg/services/accesscontrol/accesscontrol.go b/pkg/services/accesscontrol/accesscontrol.go index a07053c003d..c27bded83fc 100644 --- a/pkg/services/accesscontrol/accesscontrol.go +++ b/pkg/services/accesscontrol/accesscontrol.go @@ -40,6 +40,8 @@ type Service interface { SaveExternalServiceRole(ctx context.Context, cmd SaveExternalServiceRoleCommand) error // DeleteExternalServiceRole removes an external service's role and its assignment. DeleteExternalServiceRole(ctx context.Context, externalServiceID string) error + // SyncUserRoles adds provided roles to user + SyncUserRoles(ctx context.Context, orgID int64, cmd SyncUserRolesCommand) error } type RoleRegistry interface { @@ -58,6 +60,14 @@ type SearchOptions struct { UserID int64 // ID for the user for which to return information, if none is specified information is returned for all users. } +type SyncUserRolesCommand struct { + UserID int64 + // name of roles the user should have + RolesToAdd []string + // name of roles the user should not have + RolesToRemove []string +} + type TeamPermissionsService interface { GetPermissions(ctx context.Context, user identity.Requester, resourceID string) ([]ResourcePermission, error) SetUserPermission(ctx context.Context, orgID int64, user User, resourceID, permission string) (*ResourcePermission, error) diff --git a/pkg/services/accesscontrol/acimpl/service.go b/pkg/services/accesscontrol/acimpl/service.go index eb43561cdb7..a7a7cc650ca 100644 --- a/pkg/services/accesscontrol/acimpl/service.go +++ b/pkg/services/accesscontrol/acimpl/service.go @@ -425,3 +425,7 @@ func (s *Service) DeleteExternalServiceRole(ctx context.Context, externalService return s.store.DeleteExternalServiceRole(ctx, slug) } + +func (*Service) SyncUserRoles(ctx context.Context, orgID int64, cmd accesscontrol.SyncUserRolesCommand) error { + return nil +} diff --git a/pkg/services/accesscontrol/actest/fake.go b/pkg/services/accesscontrol/actest/fake.go index 87113fb6e3f..fa788b96f3d 100644 --- a/pkg/services/accesscontrol/actest/fake.go +++ b/pkg/services/accesscontrol/actest/fake.go @@ -11,6 +11,7 @@ var _ accesscontrol.Service = new(FakeService) var _ accesscontrol.RoleRegistry = new(FakeService) type FakeService struct { + accesscontrol.Service ExpectedErr error ExpectedCachedPermissions bool ExpectedPermissions []accesscontrol.Permission diff --git a/pkg/services/accesscontrol/mock/mock.go b/pkg/services/accesscontrol/mock/mock.go index 90a9cda8bfe..a0bcd2f46e9 100644 --- a/pkg/services/accesscontrol/mock/mock.go +++ b/pkg/services/accesscontrol/mock/mock.go @@ -57,6 +57,7 @@ type Mock struct { SearchUserPermissionsFunc func(ctx context.Context, orgID int64, searchOptions accesscontrol.SearchOptions) ([]accesscontrol.Permission, error) SaveExternalServiceRoleFunc func(ctx context.Context, cmd accesscontrol.SaveExternalServiceRoleCommand) error DeleteExternalServiceRoleFunc func(ctx context.Context, externalServiceID string) error + SyncUserRolesFunc func(ctx context.Context, orgID int64, cmd accesscontrol.SyncUserRolesCommand) error scopeResolvers accesscontrol.Resolvers } @@ -235,3 +236,10 @@ func (m *Mock) DeleteExternalServiceRole(ctx context.Context, externalServiceID } return nil } + +func (m *Mock) SyncUserRoles(ctx context.Context, orgID int64, cmd accesscontrol.SyncUserRolesCommand) error { + if m.SyncUserRolesFunc != nil { + return m.SyncUserRolesFunc(ctx, orgID, cmd) + } + return nil +} diff --git a/pkg/services/accesscontrol/roles.go b/pkg/services/accesscontrol/roles.go index 8aaeec91e46..929ec5b8788 100644 --- a/pkg/services/accesscontrol/roles.go +++ b/pkg/services/accesscontrol/roles.go @@ -28,6 +28,11 @@ const ( BasicRoleNoneUID = "basic_none" BasicRoleNoneName = "basic:none" + + FixedCloudRolePrefix = "fixed:cloud:" + FixedCloudViewerRole = "fixed:cloud:viewer" + FixedCloudEditorRole = "fixed:cloud:editor" + FixedCloudAdminRole = "fixed:cloud:admin" ) // Roles definition diff --git a/pkg/services/authn/authnimpl/service.go b/pkg/services/authn/authnimpl/service.go index 661b03aed42..d5d42591d06 100644 --- a/pkg/services/authn/authnimpl/service.go +++ b/pkg/services/authn/authnimpl/service.go @@ -160,10 +160,16 @@ func ProvideService( s.RegisterPostAuthHook(userSyncService.SyncUserHook, 10) s.RegisterPostAuthHook(userSyncService.EnableUserHook, 20) s.RegisterPostAuthHook(orgUserSyncService.SyncOrgRolesHook, 30) - s.RegisterPostAuthHook(userSyncService.SyncLastSeenHook, 120) + s.RegisterPostAuthHook(userSyncService.SyncLastSeenHook, 130) s.RegisterPostAuthHook(sync.ProvideOAuthTokenSync(oauthTokenService, sessionService, socialService).SyncOauthTokenHook, 60) s.RegisterPostAuthHook(userSyncService.FetchSyncedUserHook, 100) - s.RegisterPostAuthHook(sync.ProvidePermissionsSync(accessControlService).SyncPermissionsHook, 110) + + rbacSync := sync.ProvideRBACSync(accessControlService) + if features.IsEnabledGlobally(featuremgmt.FlagCloudRBACRoles) { + s.RegisterPostAuthHook(rbacSync.SyncCloudRoles, 110) + } + + s.RegisterPostAuthHook(rbacSync.SyncPermissionsHook, 120) return s } diff --git a/pkg/services/authn/authnimpl/sync/permission_sync.go b/pkg/services/authn/authnimpl/sync/permission_sync.go deleted file mode 100644 index c9c9d67f742..00000000000 --- a/pkg/services/authn/authnimpl/sync/permission_sync.go +++ /dev/null @@ -1,44 +0,0 @@ -package sync - -import ( - "context" - - "github.com/grafana/grafana/pkg/infra/log" - "github.com/grafana/grafana/pkg/services/accesscontrol" - "github.com/grafana/grafana/pkg/services/authn" - "github.com/grafana/grafana/pkg/util/errutil" -) - -var ( - errSyncPermissionsForbidden = errutil.Forbidden("permissions.sync.forbidden") -) - -func ProvidePermissionsSync(acService accesscontrol.Service) *PermissionsSync { - return &PermissionsSync{ - ac: acService, - log: log.New("permissions.sync"), - } -} - -type PermissionsSync struct { - ac accesscontrol.Service - log log.Logger -} - -func (s *PermissionsSync) SyncPermissionsHook(ctx context.Context, identity *authn.Identity, _ *authn.Request) error { - if !identity.ClientParams.SyncPermissions { - return nil - } - - permissions, err := s.ac.GetUserPermissions(ctx, identity, accesscontrol.Options{ReloadCache: false}) - if err != nil { - s.log.FromContext(ctx).Error("Failed to fetch permissions from db", "error", err, "user_id", identity.ID) - return errSyncPermissionsForbidden - } - - if identity.Permissions == nil { - identity.Permissions = make(map[int64]map[string][]string) - } - identity.Permissions[identity.OrgID] = accesscontrol.GroupScopesByAction(permissions) - return nil -} diff --git a/pkg/services/authn/authnimpl/sync/permission_sync_test.go b/pkg/services/authn/authnimpl/sync/permission_sync_test.go deleted file mode 100644 index db4445536b4..00000000000 --- a/pkg/services/authn/authnimpl/sync/permission_sync_test.go +++ /dev/null @@ -1,65 +0,0 @@ -package sync - -import ( - "context" - "testing" - - "github.com/grafana/grafana/pkg/infra/log" - "github.com/grafana/grafana/pkg/services/accesscontrol" - acmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" - "github.com/grafana/grafana/pkg/services/auth/identity" - "github.com/grafana/grafana/pkg/services/authn" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestPermissionsSync_SyncPermission(t *testing.T) { - type testCase struct { - name string - identity *authn.Identity - expectedPermissions []accesscontrol.Permission - } - testCases := []testCase{ - { - name: "enriches the identity successfully when SyncPermissions is true", - identity: &authn.Identity{ID: "user:2", 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: "user:2", OrgID: 1, ClientParams: authn.ClientParams{SyncPermissions: true}}, - expectedPermissions: []accesscontrol.Permission{ - {Action: accesscontrol.ActionUsersRead}, - }, - }, - } - - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { - s := setupTestEnv() - - 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.GroupScopesByAction(tt.expectedPermissions), tt.identity.Permissions[tt.identity.OrgID]) - }) - } -} - -func setupTestEnv() *PermissionsSync { - acMock := &acmock.Mock{ - GetUserPermissionsFunc: func(ctx context.Context, siu identity.Requester, o accesscontrol.Options) ([]accesscontrol.Permission, error) { - return []accesscontrol.Permission{ - {Action: accesscontrol.ActionUsersRead}, - }, nil - }, - } - s := &PermissionsSync{ - ac: acMock, - log: log.NewNopLogger(), - } - return s -} diff --git a/pkg/services/authn/authnimpl/sync/rbac_sync.go b/pkg/services/authn/authnimpl/sync/rbac_sync.go new file mode 100644 index 00000000000..98135f5b194 --- /dev/null +++ b/pkg/services/authn/authnimpl/sync/rbac_sync.go @@ -0,0 +1,93 @@ +package sync + +import ( + "context" + + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/auth/identity" + "github.com/grafana/grafana/pkg/services/authn" + "github.com/grafana/grafana/pkg/services/login" + "github.com/grafana/grafana/pkg/services/org" + "github.com/grafana/grafana/pkg/util/errutil" +) + +var ( + errInvalidCloudRole = errutil.BadRequest("rbac.sync.invalid-cloud-role") + errSyncPermissionsForbidden = errutil.Forbidden("permissions.sync.forbidden") +) + +func ProvideRBACSync(acService accesscontrol.Service) *RBACSync { + return &RBACSync{ + ac: acService, + log: log.New("permissions.sync"), + } +} + +type RBACSync struct { + ac accesscontrol.Service + log log.Logger +} + +func (s *RBACSync) SyncPermissionsHook(ctx context.Context, ident *authn.Identity, _ *authn.Request) error { + if !ident.ClientParams.SyncPermissions { + return nil + } + + permissions, err := s.ac.GetUserPermissions(ctx, ident, accesscontrol.Options{ReloadCache: false}) + if err != nil { + s.log.FromContext(ctx).Error("Failed to fetch permissions from db", "error", err, "id", ident.ID) + return errSyncPermissionsForbidden + } + + if ident.Permissions == nil { + ident.Permissions = make(map[int64]map[string][]string) + } + ident.Permissions[ident.OrgID] = accesscontrol.GroupScopesByAction(permissions) + return nil +} + +var fixedCloudRoles = map[org.RoleType]string{ + org.RoleViewer: accesscontrol.FixedCloudViewerRole, + org.RoleEditor: accesscontrol.FixedCloudEditorRole, + org.RoleAdmin: accesscontrol.FixedCloudAdminRole, +} + +func (s *RBACSync) SyncCloudRoles(ctx context.Context, ident *authn.Identity, r *authn.Request) error { + // we only want to run this hook during login and if the module used is grafana com + if r.GetMeta(authn.MetaKeyAuthModule) != login.GrafanaComAuthModule { + return nil + } + + namespace, id := ident.GetNamespacedID() + if namespace != authn.NamespaceUser { + s.log.FromContext(ctx).Debug("Skip syncing cloud role", "id", ident.ID) + return nil + } + + userID, err := identity.IntIdentifier(namespace, id) + if err != nil { + return err + } + + rolesToAdd := make([]string, 0, 1) + rolesToRemove := make([]string, 0, 2) + + for role, fixedRole := range fixedCloudRoles { + if role == ident.GetOrgRole() { + rolesToAdd = append(rolesToAdd, fixedRole) + } else { + rolesToRemove = append(rolesToRemove, fixedRole) + } + } + + if len(rolesToAdd) != 1 { + return errInvalidCloudRole.Errorf("invalid role: %s", ident.GetOrgRole()) + } + + return s.ac.SyncUserRoles(ctx, ident.GetOrgID(), accesscontrol.SyncUserRolesCommand{ + UserID: userID, + RolesToAdd: rolesToAdd, + RolesToRemove: rolesToRemove, + }) +} diff --git a/pkg/services/authn/authnimpl/sync/rbac_sync_test.go b/pkg/services/authn/authnimpl/sync/rbac_sync_test.go new file mode 100644 index 00000000000..27436998f4d --- /dev/null +++ b/pkg/services/authn/authnimpl/sync/rbac_sync_test.go @@ -0,0 +1,157 @@ +package sync + +import ( + "context" + "testing" + + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/services/accesscontrol" + acmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" + "github.com/grafana/grafana/pkg/services/auth/identity" + "github.com/grafana/grafana/pkg/services/authn" + "github.com/grafana/grafana/pkg/services/login" + "github.com/grafana/grafana/pkg/services/org" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRBACSync_SyncPermission(t *testing.T) { + type testCase struct { + name string + identity *authn.Identity + expectedPermissions []accesscontrol.Permission + } + testCases := []testCase{ + { + name: "enriches the identity successfully when SyncPermissions is true", + identity: &authn.Identity{ID: "user:2", 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: "user:2", OrgID: 1, ClientParams: authn.ClientParams{SyncPermissions: true}}, + expectedPermissions: []accesscontrol.Permission{ + {Action: accesscontrol.ActionUsersRead}, + }, + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + s := setupTestEnv() + + 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.GroupScopesByAction(tt.expectedPermissions), tt.identity.Permissions[tt.identity.OrgID]) + }) + } +} + +func TestRBACSync_SyncCloudRoles(t *testing.T) { + type testCase struct { + desc string + module string + identity *authn.Identity + expectedErr error + expectedCalled bool + } + + tests := []testCase{ + { + desc: "should call sync when authenticated with grafana com and has viewer role", + module: login.GrafanaComAuthModule, + identity: &authn.Identity{ + ID: authn.NamespacedID(authn.NamespaceUser, 1), + OrgID: 1, + OrgRoles: map[int64]org.RoleType{1: org.RoleViewer}, + }, + expectedErr: nil, + expectedCalled: true, + }, + { + desc: "should call sync when authenticated with grafana com and has editor role", + module: login.GrafanaComAuthModule, + identity: &authn.Identity{ + ID: authn.NamespacedID(authn.NamespaceUser, 1), + OrgID: 1, + OrgRoles: map[int64]org.RoleType{1: org.RoleEditor}, + }, + expectedErr: nil, + expectedCalled: true, + }, + { + desc: "should call sync when authenticated with grafana com and has admin role", + module: login.GrafanaComAuthModule, + identity: &authn.Identity{ + ID: authn.NamespacedID(authn.NamespaceUser, 1), + OrgID: 1, + OrgRoles: map[int64]org.RoleType{1: org.RoleAdmin}, + }, + expectedErr: nil, + expectedCalled: true, + }, + { + desc: "should not call sync when authenticated with grafana com and has invalid role", + module: login.GrafanaComAuthModule, + identity: &authn.Identity{ + ID: authn.NamespacedID(authn.NamespaceUser, 1), + OrgID: 1, + OrgRoles: map[int64]org.RoleType{1: org.RoleType("something else")}, + }, + expectedErr: errInvalidCloudRole, + expectedCalled: false, + }, + { + desc: "should not call sync when not authenticated with grafana com", + module: login.LDAPAuthModule, + identity: &authn.Identity{ + ID: authn.NamespacedID(authn.NamespaceUser, 1), + OrgID: 1, + OrgRoles: map[int64]org.RoleType{1: org.RoleAdmin}, + }, + expectedErr: nil, + expectedCalled: false, + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + var called bool + s := &RBACSync{ + ac: &acmock.Mock{ + SyncUserRolesFunc: func(_ context.Context, _ int64, _ accesscontrol.SyncUserRolesCommand) error { + called = true + return nil + }, + }, + log: log.NewNopLogger(), + } + + req := &authn.Request{} + req.SetMeta(authn.MetaKeyAuthModule, tt.module) + + err := s.SyncCloudRoles(context.Background(), tt.identity, req) + assert.ErrorIs(t, err, tt.expectedErr) + assert.Equal(t, tt.expectedCalled, called) + }) + } +} + +func setupTestEnv() *RBACSync { + acMock := &acmock.Mock{ + GetUserPermissionsFunc: func(ctx context.Context, siu identity.Requester, o accesscontrol.Options) ([]accesscontrol.Permission, error) { + return []accesscontrol.Permission{ + {Action: accesscontrol.ActionUsersRead}, + }, nil + }, + } + s := &RBACSync{ + ac: acMock, + log: log.NewNopLogger(), + } + return s +}