From 8b7188e85b4dcc0360148ae1c71ce2d60a52380b Mon Sep 17 00:00:00 2001 From: Marie <51697796+ijreilly@users.noreply.github.com> Date: Thu, 20 Mar 2025 11:37:26 +0100 Subject: [PATCH] [fix] [permissions] Fix role assignment at sign-up (#11045) In [a previous PR](https://github.com/twentyhq/twenty/pull/11023) I fixed the issue where users where re-assigned the default role when signin up using SSO. The "fix" actually introduced another error, calling `assignRoleToUserWorkspace` only when a user is signing up to twenty, forgetting about the case where an existing twenty user signs up to a different workspace. They should still be assigned the role in that case. To fix this and improve clarity, I moved assignRoleToUserWorkspace to addUserToWorkspace and renamed addUserToWorkspace to addUserToWorkspaceIfUserNotInWorkspace since this is at each sign in but does nothing if user is already in the workspace. I think ideally we should refactor this part to improve readability and understandability, maybe with different flows for each case: signIn and signUp, to twenty or to a workspace --- .../auth/services/auth.service.spec.ts | 5 +- .../auth/services/auth.service.ts | 7 +- .../auth/services/sign-in-up.service.spec.ts | 115 +++--------------- .../auth/services/sign-in-up.service.ts | 58 +++++---- .../user-workspace/user-workspace.module.ts | 4 +- .../user-workspace/user-workspace.service.ts | 44 +++++-- .../permissions/permissions.exception.ts | 2 + ...sion-graphql-api-exception-handler.util.ts | 1 + 8 files changed, 99 insertions(+), 137 deletions(-) diff --git a/packages/twenty-server/src/engine/core-modules/auth/services/auth.service.spec.ts b/packages/twenty-server/src/engine/core-modules/auth/services/auth.service.spec.ts index 563b0ec3d..d794fa434 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/services/auth.service.spec.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/services/auth.service.spec.ts @@ -9,8 +9,8 @@ import { AuthException, AuthExceptionCode, } from 'src/engine/core-modules/auth/auth.exception'; -import { SignInUpService } from 'src/engine/core-modules/auth/services/sign-in-up.service'; import { AuthSsoService } from 'src/engine/core-modules/auth/services/auth-sso.service'; +import { SignInUpService } from 'src/engine/core-modules/auth/services/sign-in-up.service'; import { AccessTokenService } from 'src/engine/core-modules/auth/token/services/access-token.service'; import { RefreshTokenService } from 'src/engine/core-modules/auth/token/services/refresh-token.service'; import { ExistingUserOrNewUser } from 'src/engine/core-modules/auth/types/signInUp.type'; @@ -101,7 +101,8 @@ describe('AuthService', () => { useValue: { checkUserWorkspaceExists: userWorkspaceServiceCheckUserWorkspaceExistsMock, - addUserToWorkspace: userWorkspaceAddUserToWorkspaceMock, + addUserToWorkspaceIfUserNotInWorkspace: + userWorkspaceAddUserToWorkspaceMock, }, }, { diff --git a/packages/twenty-server/src/engine/core-modules/auth/services/auth.service.ts b/packages/twenty-server/src/engine/core-modules/auth/services/auth.service.ts index ee9c3e47b..3e58f7b54 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/services/auth.service.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/services/auth.service.ts @@ -37,8 +37,8 @@ import { UserNotExists, } from 'src/engine/core-modules/auth/dto/user-exists.entity'; import { WorkspaceInviteHashValid } from 'src/engine/core-modules/auth/dto/workspace-invite-hash-valid.entity'; -import { SignInUpService } from 'src/engine/core-modules/auth/services/sign-in-up.service'; import { AuthSsoService } from 'src/engine/core-modules/auth/services/auth-sso.service'; +import { SignInUpService } from 'src/engine/core-modules/auth/services/sign-in-up.service'; import { AccessTokenService } from 'src/engine/core-modules/auth/token/services/access-token.service'; import { RefreshTokenService } from 'src/engine/core-modules/auth/token/services/refresh-token.service'; import { @@ -106,7 +106,10 @@ export class AuthService { workspacePersonalInviteToken: invitation.value, email: user.email, }); - await this.userWorkspaceService.addUserToWorkspace(user, workspace); + await this.userWorkspaceService.addUserToWorkspaceIfUserNotInWorkspace( + user, + workspace, + ); return; } diff --git a/packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.spec.ts b/packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.spec.ts index a4dd783ad..05a306902 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.spec.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.spec.ts @@ -44,8 +44,6 @@ describe('SignInUpService', () => { let userWorkspaceService: UserWorkspaceService; let environmentService: EnvironmentService; let domainManagerService: DomainManagerService; - let userRoleService: UserRoleService; - let featureFlagService: FeatureFlagService; beforeEach(async () => { const module: TestingModule = await Test.createTestingModule({ @@ -83,7 +81,7 @@ describe('SignInUpService', () => { { provide: UserWorkspaceService, useValue: { - addUserToWorkspace: jest.fn(), + addUserToWorkspaceIfUserNotInWorkspace: jest.fn(), checkUserWorkspaceExists: jest.fn(), create: jest.fn(), }, @@ -148,8 +146,6 @@ describe('SignInUpService', () => { environmentService = module.get(EnvironmentService); domainManagerService = module.get(DomainManagerService); - userRoleService = module.get(UserRoleService); - featureFlagService = module.get(FeatureFlagService); }); it('should handle signInUp with valid personal invitation', async () => { @@ -179,10 +175,9 @@ describe('SignInUpService', () => { .spyOn(workspaceInvitationService, 'invalidateWorkspaceInvitation') .mockResolvedValue(undefined); - jest.spyOn(userWorkspaceService, 'addUserToWorkspace').mockResolvedValue({ - user: {} as User, - userWorkspace: {} as UserWorkspace, - }); + jest + .spyOn(userWorkspaceService, 'addUserToWorkspaceIfUserNotInWorkspace') + .mockResolvedValue(undefined); const result = await service.signInUp(params); @@ -200,6 +195,9 @@ describe('SignInUpService', () => { (params.workspace as Workspace).id, 'test@example.com', ); + expect( + userWorkspaceService.addUserToWorkspaceIfUserNotInWorkspace, + ).toHaveBeenCalled(); }); it('should handle signInUp on existing workspace without invitation', async () => { @@ -217,16 +215,17 @@ describe('SignInUpService', () => { }, }; - jest.spyOn(userWorkspaceService, 'addUserToWorkspace').mockResolvedValue({ - user: {} as User, - userWorkspace: {} as UserWorkspace, - }); + jest + .spyOn(userWorkspaceService, 'addUserToWorkspaceIfUserNotInWorkspace') + .mockResolvedValue(undefined); const result = await service.signInUp(params); expect(result.workspace).toEqual(params.workspace); expect(result.user).toBeDefined(); - expect(userWorkspaceService.addUserToWorkspace).toHaveBeenCalled(); + expect( + userWorkspaceService.addUserToWorkspaceIfUserNotInWorkspace, + ).toHaveBeenCalled(); }); it('should handle signUp on new workspace for a new user', async () => { @@ -294,10 +293,9 @@ describe('SignInUpService', () => { }; jest.spyOn(environmentService, 'get').mockReturnValue(false); - jest.spyOn(userWorkspaceService, 'addUserToWorkspace').mockResolvedValue({ - user: {} as User, - userWorkspace: {} as UserWorkspace, - }); + jest + .spyOn(userWorkspaceService, 'addUserToWorkspaceIfUserNotInWorkspace') + .mockResolvedValue(undefined); jest .spyOn(userWorkspaceService, 'checkUserWorkspaceExists') .mockResolvedValue({} as UserWorkspace); @@ -306,7 +304,9 @@ describe('SignInUpService', () => { expect(result.workspace).toEqual(params.workspace); expect(result.user).toBeDefined(); - expect(userWorkspaceService.addUserToWorkspace).toHaveBeenCalled(); + expect( + userWorkspaceService.addUserToWorkspaceIfUserNotInWorkspace, + ).toHaveBeenCalled(); }); it('should throw - handle signUp on workspace in pending state', async () => { @@ -365,81 +365,4 @@ describe('SignInUpService', () => { expect(WorkspaceRepository.create).toHaveBeenCalled(); expect(WorkspaceRepository.save).toHaveBeenCalled(); }); - - it('should not assign default role when permissions are enabled and user exists', async () => { - const params: SignInUpBaseParams & - ExistingUserOrPartialUserWithPicture & - AuthProviderWithPasswordType = { - workspace: { - id: 'workspaceId', - defaultRoleId: 'defaultRoleId', - activationStatus: WorkspaceActivationStatus.ACTIVE, - } as Workspace, - authParams: { provider: 'password', password: 'validPassword' }, - userData: { - type: 'existingUser', - existingUser: { email: 'test@example.com' } as User, - }, - }; - - const mockUserWorkspace = { id: 'userWorkspaceId' }; - - jest.spyOn(featureFlagService, 'isFeatureEnabled').mockResolvedValue(true); - jest.spyOn(userWorkspaceService, 'addUserToWorkspace').mockResolvedValue({ - user: {} as User, - userWorkspace: mockUserWorkspace as UserWorkspace, - }); - - await service.signInUp(params); - - expect(params.workspace).toBeDefined(); - expect(userRoleService.assignRoleToUserWorkspace).not.toHaveBeenCalled(); - }); - - it('should assign default role when permissions are enabled and user does not exist', async () => { - const params: SignInUpBaseParams & - ExistingUserOrPartialUserWithPicture & - AuthProviderWithPasswordType = { - workspace: { - id: 'workspaceId', - defaultRoleId: 'defaultRoleId', - activationStatus: WorkspaceActivationStatus.ACTIVE, - } as Workspace, - authParams: { provider: 'password', password: 'validPassword' }, - userData: { - type: 'newUserWithPicture', - newUserWithPicture: { - email: 'newuser@example.com', - picture: 'pictureUrl', - }, - }, - }; - - const mockUserWorkspace = { id: 'userWorkspaceId' }; - - jest.spyOn(featureFlagService, 'isFeatureEnabled').mockResolvedValue(true); - - jest.spyOn(fileUploadService, 'uploadImage').mockResolvedValue({ - id: '', - mimeType: '', - paths: ['path/to/image'], - }); - jest.spyOn(UserRepository, 'create').mockReturnValue({} as User); - jest - .spyOn(UserRepository, 'save') - .mockResolvedValue({ id: 'newUserId' } as User); - jest.spyOn(userWorkspaceService, 'addUserToWorkspace').mockResolvedValue({ - user: {} as User, - userWorkspace: mockUserWorkspace as UserWorkspace, - }); - - await service.signInUp(params); - - expect(params.workspace).toBeDefined(); - expect(userRoleService.assignRoleToUserWorkspace).toHaveBeenCalledWith({ - workspaceId: params.workspace!.id, - userWorkspaceId: mockUserWorkspace.id, - roleId: params.workspace!.defaultRoleId, - }); - }); }); diff --git a/packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts b/packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts index 2be28fd83..31ed89bc2 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts @@ -226,37 +226,45 @@ export class SignInUpService { ) { await this.throwIfWorkspaceIsNotReadyForSignInUp(params.workspace, params); - const currentUser = - params.userData.type === 'newUserWithPicture' - ? await this.saveNewUser( - params.userData.newUserWithPicture, - params.workspace.id, - { canAccessFullAdminPanel: false, canImpersonate: false }, - ) - : params.userData.existingUser; + const isNewUser = params.userData.type === 'newUserWithPicture'; - const { user: updatedUser, userWorkspace } = - await this.userWorkspaceService.addUserToWorkspace( - currentUser, + if (isNewUser) { + const userData = params.userData as { + type: 'newUserWithPicture'; + newUserWithPicture: PartialUserWithPicture; + }; + + const user = await this.saveNewUser( + userData.newUserWithPicture, + params.workspace.id, + { + canAccessFullAdminPanel: false, + canImpersonate: false, + }, + ); + + await this.activateOnboardingForUser(user, params.workspace); + + await this.userWorkspaceService.addUserToWorkspaceIfUserNotInWorkspace( + user, params.workspace, ); - const user = Object.assign(currentUser, updatedUser); - - const isSignUp = params.userData.type === 'newUserWithPicture'; - - if (isSignUp) { - await this.activateOnboardingForUser(user, params.workspace); - - if (params.workspace.defaultRoleId) { - await this.userRoleService.assignRoleToUserWorkspace({ - workspaceId: params.workspace.id, - userWorkspaceId: userWorkspace.id, - roleId: params.workspace.defaultRoleId, - }); - } + return user; } + const userData = params.userData as { + type: 'existingUser'; + existingUser: User; + }; + + const user = userData.existingUser; + + await this.userWorkspaceService.addUserToWorkspaceIfUserNotInWorkspace( + user, + params.workspace, + ); + return user; } diff --git a/packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.module.ts b/packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.module.ts index 71bec0a35..269209815 100644 --- a/packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.module.ts +++ b/packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.module.ts @@ -4,6 +4,7 @@ import { NestjsQueryGraphQLModule } from '@ptc-org/nestjs-query-graphql'; import { NestjsQueryTypeOrmModule } from '@ptc-org/nestjs-query-typeorm'; import { TypeORMModule } from 'src/database/typeorm/typeorm.module'; +import { DomainManagerModule } from 'src/engine/core-modules/domain-manager/domain-manager.module'; import { TwoFactorMethod } from 'src/engine/core-modules/two-factor-method/two-factor-method.entity'; import { UserWorkspace } from 'src/engine/core-modules/user-workspace/user-workspace.entity'; import { UserWorkspaceResolver } from 'src/engine/core-modules/user-workspace/user-workspace.resolver'; @@ -13,9 +14,9 @@ import { WorkspaceInvitationModule } from 'src/engine/core-modules/workspace-inv import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity'; import { DataSourceModule } from 'src/engine/metadata-modules/data-source/data-source.module'; import { ObjectMetadataEntity } from 'src/engine/metadata-modules/object-metadata/object-metadata.entity'; +import { UserRoleModule } from 'src/engine/metadata-modules/user-role/user-role.module'; import { TwentyORMModule } from 'src/engine/twenty-orm/twenty-orm.module'; import { WorkspaceDataSourceModule } from 'src/engine/workspace-datasource/workspace-datasource.module'; -import { DomainManagerModule } from 'src/engine/core-modules/domain-manager/domain-manager.module'; @Module({ imports: [ @@ -32,6 +33,7 @@ import { DomainManagerModule } from 'src/engine/core-modules/domain-manager/doma WorkspaceInvitationModule, DomainManagerModule, TwentyORMModule, + UserRoleModule, ], services: [UserWorkspaceService], }), diff --git a/packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.service.ts b/packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.service.ts index 8627326ad..ea824da88 100644 --- a/packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.service.ts +++ b/packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.service.ts @@ -21,6 +21,12 @@ import { WorkspaceInvitationService } from 'src/engine/core-modules/workspace-in import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity'; import { DataSourceService } from 'src/engine/metadata-modules/data-source/data-source.service'; import { ObjectMetadataEntity } from 'src/engine/metadata-modules/object-metadata/object-metadata.entity'; +import { + PermissionsException, + PermissionsExceptionCode, + PermissionsExceptionMessage, +} from 'src/engine/metadata-modules/permissions/permissions.exception'; +import { UserRoleService } from 'src/engine/metadata-modules/user-role/user-role.service'; import { TwentyORMGlobalManager } from 'src/engine/twenty-orm/twenty-orm-global.manager'; import { WorkspaceEventEmitter } from 'src/engine/workspace-event-emitter/workspace-event-emitter'; import { WorkspaceMemberWorkspaceEntity } from 'src/modules/workspace-member/standard-objects/workspace-member.workspace-entity'; @@ -30,6 +36,8 @@ export class UserWorkspaceService extends TypeOrmQueryService { constructor( @InjectRepository(UserWorkspace, 'core') private readonly userWorkspaceRepository: Repository, + @InjectRepository(Workspace, 'core') + private readonly workspaceRepository: Repository, @InjectRepository(User, 'core') private readonly userRepository: Repository, @InjectRepository(ObjectMetadataEntity, 'metadata') @@ -40,6 +48,7 @@ export class UserWorkspaceService extends TypeOrmQueryService { private readonly workspaceEventEmitter: WorkspaceEventEmitter, private readonly domainManagerService: DomainManagerService, private readonly twentyORMGlobalManager: TwentyORMGlobalManager, + private readonly userRoleService: UserRoleService, ) { super(userWorkspaceRepository); } @@ -112,7 +121,10 @@ export class UserWorkspaceService extends TypeOrmQueryService { }); } - async addUserToWorkspace(user: User, workspace: Workspace) { + async addUserToWorkspaceIfUserNotInWorkspace( + user: User, + workspace: Workspace, + ) { let userWorkspace = await this.checkUserWorkspaceExists( user.id, workspace.id, @@ -122,17 +134,27 @@ export class UserWorkspaceService extends TypeOrmQueryService { userWorkspace = await this.create(user.id, workspace.id); await this.createWorkspaceMember(workspace.id, user); + + const defaultRoleId = workspace.defaultRoleId; + + if (!isDefined(defaultRoleId)) { + throw new PermissionsException( + PermissionsExceptionMessage.DEFAULT_ROLE_NOT_FOUND, + PermissionsExceptionCode.DEFAULT_ROLE_NOT_FOUND, + ); + } + + await this.userRoleService.assignRoleToUserWorkspace({ + workspaceId: workspace.id, + userWorkspaceId: userWorkspace.id, + roleId: defaultRoleId, + }); + + await this.workspaceInvitationService.invalidateWorkspaceInvitation( + workspace.id, + user.email, + ); } - - await this.workspaceInvitationService.invalidateWorkspaceInvitation( - workspace.id, - user.email, - ); - - return { - user, - userWorkspace, - }; } public async getUserCount(workspaceId: string): Promise { diff --git a/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.exception.ts b/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.exception.ts index c0fa16584..35677a7c2 100644 --- a/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.exception.ts +++ b/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.exception.ts @@ -24,6 +24,7 @@ export enum PermissionsExceptionCode { INVALID_ARG = 'INVALID_ARG', PERMISSIONS_V2_NOT_ENABLED = 'PERMISSIONS_V2_NOT_ENABLED', ROLE_LABEL_ALREADY_EXISTS = 'ROLE_LABEL_ALREADY_EXISTS', + DEFAULT_ROLE_NOT_FOUND = 'DEFAULT_ROLE_NOT_FOUND', } export enum PermissionsExceptionMessage { @@ -43,4 +44,5 @@ export enum PermissionsExceptionMessage { NO_ROLE_FOUND_FOR_USER_WORKSPACE = 'No role found for userWorkspace', PERMISSIONS_V2_NOT_ENABLED = 'Permissions V2 is not enabled', ROLE_LABEL_ALREADY_EXISTS = 'A role with this label already exists', + DEFAULT_ROLE_NOT_FOUND = 'Default role not found', } diff --git a/packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-graphql-api-exception-handler.util.ts b/packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-graphql-api-exception-handler.util.ts index 0d96d4619..86f9d3179 100644 --- a/packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-graphql-api-exception-handler.util.ts +++ b/packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-graphql-api-exception-handler.util.ts @@ -25,6 +25,7 @@ export const permissionGraphqlApiExceptionHandler = ( case PermissionsExceptionCode.ROLE_NOT_FOUND: case PermissionsExceptionCode.USER_WORKSPACE_NOT_FOUND: throw new NotFoundError(error.message); + case PermissionsExceptionCode.DEFAULT_ROLE_NOT_FOUND: default: throw new InternalServerError(error.message); }