From 59cb420d824f1b77abc81cc70cfcf5f12e5b71b2 Mon Sep 17 00:00:00 2001 From: Antoine Moreaux Date: Mon, 20 Jan 2025 12:25:08 +0100 Subject: [PATCH] refactor(invitation): streamline invitation validation logic (#9699) Replaced `validateInvitation` with `validatePersonalInvitation` across services for consistent and specific validation handling. Removed outdated public invitation validation and improved error handling for workspace invitations. Updated tests to align with the refactored logic and added checks for edge cases. --- .../engine/core-modules/auth/auth.resolver.ts | 12 +- .../controllers/google-auth.controller.ts | 13 +- .../controllers/microsoft-auth.controller.ts | 13 +- .../auth/controllers/sso-auth.controller.ts | 11 +- .../auth/services/auth.service.spec.ts | 235 ++++++++++-------- .../auth/services/auth.service.ts | 59 +++-- .../auth/services/sign-in-up.service.spec.ts | 8 +- .../auth/services/sign-in-up.service.ts | 2 +- .../user-workspace/user-workspace.service.ts | 9 +- .../services/workspace-invitation.service.ts | 55 +--- 10 files changed, 222 insertions(+), 195 deletions(-) diff --git a/packages/twenty-server/src/engine/core-modules/auth/auth.resolver.ts b/packages/twenty-server/src/engine/core-modules/auth/auth.resolver.ts index 994662fda5e..0cf3190a738 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/auth.resolver.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/auth.resolver.ts @@ -176,10 +176,14 @@ export class AuthResolver { workspaceId: signUpInput.workspaceId, }); - const invitation = await this.authService.findInvitationForSignInUp({ - currentWorkspace, - workspacePersonalInviteToken: signUpInput.workspacePersonalInviteToken, - }); + const invitation = + currentWorkspace && signUpInput.workspacePersonalInviteToken + ? await this.authService.findInvitationForSignInUp({ + currentWorkspace, + workspacePersonalInviteToken: + signUpInput.workspacePersonalInviteToken, + }) + : undefined; const existingUser = await this.userRepository.findOne({ where: { diff --git a/packages/twenty-server/src/engine/core-modules/auth/controllers/google-auth.controller.ts b/packages/twenty-server/src/engine/core-modules/auth/controllers/google-auth.controller.ts index fe89ad55184..644f857ab8f 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/controllers/google-auth.controller.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/controllers/google-auth.controller.ts @@ -66,11 +66,14 @@ export class GoogleAuthController { }); try { - const invitation = await this.authService.findInvitationForSignInUp({ - currentWorkspace, - workspacePersonalInviteToken, - email, - }); + const invitation = + currentWorkspace && workspacePersonalInviteToken && email + ? await this.authService.findInvitationForSignInUp({ + currentWorkspace, + workspacePersonalInviteToken, + email, + }) + : undefined; const existingUser = await this.userRepository.findOne({ where: { email }, diff --git a/packages/twenty-server/src/engine/core-modules/auth/controllers/microsoft-auth.controller.ts b/packages/twenty-server/src/engine/core-modules/auth/controllers/microsoft-auth.controller.ts index 2485b1699c1..5e5a4e7086a 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/controllers/microsoft-auth.controller.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/controllers/microsoft-auth.controller.ts @@ -64,11 +64,14 @@ export class MicrosoftAuthController { }); try { - const invitation = await this.authService.findInvitationForSignInUp({ - currentWorkspace, - workspacePersonalInviteToken, - email, - }); + const invitation = + currentWorkspace && workspacePersonalInviteToken && email + ? await this.authService.findInvitationForSignInUp({ + currentWorkspace, + workspacePersonalInviteToken, + email, + }) + : undefined; const existingUser = await this.userRepository.findOne({ where: { email }, diff --git a/packages/twenty-server/src/engine/core-modules/auth/controllers/sso-auth.controller.ts b/packages/twenty-server/src/engine/core-modules/auth/controllers/sso-auth.controller.ts index b41a09b547c..5a8c35f7503 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/controllers/sso-auth.controller.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/controllers/sso-auth.controller.ts @@ -153,10 +153,13 @@ export class SSOAuthController { ); } - const invitation = await this.authService.findInvitationForSignInUp({ - currentWorkspace: identityProvider.workspace, - email: payload.email, - }); + const invitation = + payload.email && identityProvider.workspace + ? await this.authService.findInvitationForSignInUp({ + currentWorkspace: identityProvider.workspace, + email: payload.email, + }) + : undefined; const existingUser = await this.userRepository.findOne({ where: { 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 36d8ed94959..31ec9b0c8cd 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 @@ -19,6 +19,10 @@ import { WorkspaceInvitationService } from 'src/engine/core-modules/workspace-in import { SocialSsoService } from 'src/engine/core-modules/auth/services/social-sso.service'; import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity'; import { ExistingUserOrNewUser } from 'src/engine/core-modules/auth/types/signInUp.type'; +import { + AuthException, + AuthExceptionCode, +} from 'src/engine/core-modules/auth/auth.exception'; import { AuthService } from './auth.service'; @@ -29,7 +33,7 @@ const UserWorkspaceFindOneByMock = jest.fn(); const userWorkspaceServiceCheckUserWorkspaceExistsMock = jest.fn(); const workspaceInvitationGetOneWorkspaceInvitationMock = jest.fn(); -const workspaceInvitationValidateInvitationMock = jest.fn(); +const workspaceInvitationValidatePersonalInvitationMock = jest.fn(); const userWorkspaceAddUserToWorkspaceMock = jest.fn(); const environmentServiceGetMock = jest.fn(); @@ -112,7 +116,8 @@ describe('AuthService', () => { useValue: { getOneWorkspaceInvitation: workspaceInvitationGetOneWorkspaceInvitationMock, - validateInvitation: workspaceInvitationValidateInvitationMock, + validatePersonalInvitation: + workspaceInvitationValidatePersonalInvitationMock, }, }, { @@ -193,7 +198,7 @@ describe('AuthService', () => { userWorkspaceServiceCheckUserWorkspaceExistsMock.mockReturnValueOnce(false); workspaceInvitationGetOneWorkspaceInvitationMock.mockReturnValueOnce({}); - workspaceInvitationValidateInvitationMock.mockReturnValueOnce({}); + workspaceInvitationValidatePersonalInvitationMock.mockReturnValueOnce({}); userWorkspaceAddUserToWorkspaceMock.mockReturnValueOnce({}); const response = await service.challenge( @@ -216,40 +221,20 @@ describe('AuthService', () => { expect( workspaceInvitationGetOneWorkspaceInvitationMock, ).toHaveBeenCalledTimes(1); - expect(workspaceInvitationValidateInvitationMock).toHaveBeenCalledTimes(1); + expect( + workspaceInvitationValidatePersonalInvitationMock, + ).toHaveBeenCalledTimes(1); expect(userWorkspaceAddUserToWorkspaceMock).toHaveBeenCalledTimes(1); expect(UserFindOneMock).toHaveBeenCalledTimes(1); }); - it('checkAccessForSignIn - allow signin for existing user who target a workspace with right access', async () => { - const spy = jest - .spyOn(userService, 'hasUserAccessToWorkspaceOrThrow') - .mockResolvedValue(); + describe('checkAccessForSignIn', () => { + it('checkAccessForSignIn - allow signin for existing user who target a workspace with right access', async () => { + const spy = jest + .spyOn(userService, 'hasUserAccessToWorkspaceOrThrow') + .mockResolvedValue(); - await service.checkAccessForSignIn({ - userData: { - type: 'existingUser', - existingUser: { - id: 'user-id', - }, - } as ExistingUserOrNewUser['userData'], - invitation: undefined, - workspaceInviteHash: undefined, - workspace: { - id: 'workspace-id', - } as Workspace, - }); - - expect(spy).toHaveBeenCalledTimes(1); - }); - - it('checkAccessForSignIn - trigger error on existing user signin in unauthorized workspace', async () => { - const spy = jest - .spyOn(userService, 'hasUserAccessToWorkspaceOrThrow') - .mockRejectedValue(new Error('Access denied')); - - await expect( - service.checkAccessForSignIn({ + await service.checkAccessForSignIn({ userData: { type: 'existingUser', existingUser: { @@ -260,87 +245,143 @@ describe('AuthService', () => { workspaceInviteHash: undefined, workspace: { id: 'workspace-id', + isPublicInviteLinkEnabled: true, } as Workspace, - }), - ).rejects.toThrow(new Error('Access denied')); + }); - expect(spy).toHaveBeenCalledTimes(1); - }); - - it("checkAccessForSignIn - allow signup for new user who don't target a workspace", async () => { - const spy = jest - .spyOn(userService, 'hasUserAccessToWorkspaceOrThrow') - .mockResolvedValue(); - - await service.checkAccessForSignIn({ - userData: { - type: 'newUser', - newUserPayload: {}, - } as ExistingUserOrNewUser['userData'], - invitation: undefined, - workspaceInviteHash: undefined, - workspace: undefined, + expect(spy).toHaveBeenCalledTimes(1); }); - expect(spy).toHaveBeenCalledTimes(0); - }); + it('checkAccessForSignIn - trigger error on existing user signin in unauthorized workspace', async () => { + const spy = jest + .spyOn(userService, 'hasUserAccessToWorkspaceOrThrow') + .mockRejectedValue(new Error('Access denied')); - it("checkAccessForSignIn - allow signup for existing user who don't target a workspace", async () => { - const spy = jest - .spyOn(userService, 'hasUserAccessToWorkspaceOrThrow') - .mockResolvedValue(); + await expect( + service.checkAccessForSignIn({ + userData: { + type: 'existingUser', + existingUser: { + id: 'user-id', + }, + } as ExistingUserOrNewUser['userData'], + invitation: undefined, + workspaceInviteHash: undefined, + workspace: { + id: 'workspace-id', + isPublicInviteLinkEnabled: true, + } as Workspace, + }), + ).rejects.toThrow(new Error('Access denied')); - await service.checkAccessForSignIn({ - userData: { - type: 'existingUser', - existingUser: { - id: 'user-id', - }, - } as ExistingUserOrNewUser['userData'], - invitation: undefined, - workspaceInviteHash: undefined, - workspace: undefined, + expect(spy).toHaveBeenCalledTimes(1); }); - expect(spy).toHaveBeenCalledTimes(0); - }); + it('checkAccessForSignIn - trigger an error when a user attempts to sign up using a public link in a workspace where public links are disabled', async () => { + const spy = jest.spyOn(userService, 'hasUserAccessToWorkspaceOrThrow'); - it('checkAccessForSignIn - allow signup for new user who target a workspace with invitation', async () => { - const spy = jest - .spyOn(userService, 'hasUserAccessToWorkspaceOrThrow') - .mockResolvedValue(); + await expect( + service.checkAccessForSignIn({ + userData: { + type: 'existingUser', + existingUser: { + id: 'user-id', + }, + } as ExistingUserOrNewUser['userData'], + invitation: undefined, + workspaceInviteHash: 'workspaceInviteHash', + workspace: { + id: 'workspace-id', + isPublicInviteLinkEnabled: false, + } as Workspace, + }), + ).rejects.toThrow( + new AuthException( + 'Public invite link is disabled for this workspace', + AuthExceptionCode.FORBIDDEN_EXCEPTION, + ), + ); - await service.checkAccessForSignIn({ - userData: { - type: 'existingUser', - existingUser: { - id: 'user-id', - }, - } as ExistingUserOrNewUser['userData'], - invitation: {} as AppToken, - workspaceInviteHash: undefined, - workspace: {} as Workspace, + expect(spy).toHaveBeenCalledTimes(0); }); - expect(spy).toHaveBeenCalledTimes(0); - }); + it("checkAccessForSignIn - allow signup for new user who don't target a workspace", async () => { + const spy = jest + .spyOn(userService, 'hasUserAccessToWorkspaceOrThrow') + .mockResolvedValue(); - it('checkAccessForSignIn - allow signup for new user who target a workspace with workspaceInviteHash', async () => { - const spy = jest - .spyOn(userService, 'hasUserAccessToWorkspaceOrThrow') - .mockResolvedValue(); + await service.checkAccessForSignIn({ + userData: { + type: 'newUser', + newUserPayload: {}, + } as ExistingUserOrNewUser['userData'], + invitation: undefined, + workspaceInviteHash: undefined, + workspace: undefined, + }); - await service.checkAccessForSignIn({ - userData: { - type: 'newUser', - newUserPayload: {}, - } as ExistingUserOrNewUser['userData'], - invitation: undefined, - workspaceInviteHash: 'workspaceInviteHash', - workspace: {} as Workspace, + expect(spy).toHaveBeenCalledTimes(0); }); - expect(spy).toHaveBeenCalledTimes(0); + it("checkAccessForSignIn - allow signup for existing user who don't target a workspace", async () => { + const spy = jest + .spyOn(userService, 'hasUserAccessToWorkspaceOrThrow') + .mockResolvedValue(); + + await service.checkAccessForSignIn({ + userData: { + type: 'existingUser', + existingUser: { + id: 'user-id', + }, + } as ExistingUserOrNewUser['userData'], + invitation: undefined, + workspaceInviteHash: undefined, + workspace: undefined, + }); + + expect(spy).toHaveBeenCalledTimes(0); + }); + + it('checkAccessForSignIn - allow signup for new user who target a workspace with invitation', async () => { + const spy = jest + .spyOn(userService, 'hasUserAccessToWorkspaceOrThrow') + .mockResolvedValue(); + + await service.checkAccessForSignIn({ + userData: { + type: 'existingUser', + existingUser: { + id: 'user-id', + }, + } as ExistingUserOrNewUser['userData'], + invitation: {} as AppToken, + workspaceInviteHash: undefined, + workspace: {} as Workspace, + }); + + expect(spy).toHaveBeenCalledTimes(0); + }); + + it('checkAccessForSignIn - allow signup for new user who target a workspace with public invitation', async () => { + const spy = jest + .spyOn(userService, 'hasUserAccessToWorkspaceOrThrow') + .mockResolvedValue(); + + await service.checkAccessForSignIn({ + userData: { + type: 'newUser', + newUserPayload: {}, + } as ExistingUserOrNewUser['userData'], + invitation: undefined, + workspaceInviteHash: 'workspaceInviteHash', + workspace: { + isPublicInviteLinkEnabled: true, + } as Workspace, + }); + + expect(spy).toHaveBeenCalledTimes(0); + }); }); it('findWorkspaceForSignInUp - signup password auth', async () => { 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 8a1e6ed4ded..252fc04a2ca 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 @@ -98,7 +98,7 @@ export class AuthService { ); if (invitation) { - await this.workspaceInvitationService.validateInvitation({ + await this.workspaceInvitationService.validatePersonalInvitation({ workspacePersonalInviteToken: invitation.value, email: user.email, }); @@ -480,25 +480,20 @@ export class AuthService { workspacePersonalInviteToken, email, }: { - currentWorkspace?: Workspace | null; + currentWorkspace: Workspace | null; workspacePersonalInviteToken?: string; email?: string; }) { - if (!currentWorkspace) return undefined; + if (!currentWorkspace || !workspacePersonalInviteToken) return undefined; - const qr = this.appTokenRepository.createQueryBuilder('appToken'); - - qr.where('"appToken"."workspaceId" = :workspaceId', { - workspaceId: currentWorkspace.id, - }).andWhere('"appToken".type = :type', { - type: AppTokenType.InvitationToken, - }); - - if (email) { - qr.andWhere('"appToken".context->>\'email\' = :email', { - email, + const qr = this.appTokenRepository + .createQueryBuilder('appToken') + .where('"appToken"."workspaceId" = :workspaceId', { + workspaceId: currentWorkspace.id, + }) + .andWhere('"appToken".type = :type', { + type: AppTokenType.InvitationToken, }); - } if (workspacePersonalInviteToken) { qr.andWhere('"appToken".value = :personalInviteToken', { @@ -506,6 +501,12 @@ export class AuthService { }); } + if (email) { + qr.andWhere('"appToken".context->>\'email\' = :email', { + email, + }); + } + return (await qr.getOne()) ?? undefined; } @@ -567,18 +568,40 @@ export class AuthService { workspaceInviteHash?: string; } & ExistingUserOrNewUser & SignInUpBaseParams) { - const hasInvitation = invitation || workspaceInviteHash; + const hasPublicInviteLink = !!workspaceInviteHash; + const hasPersonalInvitation = !!invitation; + const isInvitedToWorkspace = hasPersonalInvitation || hasPublicInviteLink; const isTargetAnExistingWorkspace = !!workspace; const isAnExistingUser = userData.type === 'existingUser'; - if (!hasInvitation && isTargetAnExistingWorkspace && isAnExistingUser) { + if ( + hasPublicInviteLink && + !hasPersonalInvitation && + workspace && + !workspace.isPublicInviteLinkEnabled + ) { + throw new AuthException( + 'Public invite link is disabled for this workspace', + AuthExceptionCode.FORBIDDEN_EXCEPTION, + ); + } + + if ( + !isInvitedToWorkspace && + isTargetAnExistingWorkspace && + isAnExistingUser + ) { return await this.userService.hasUserAccessToWorkspaceOrThrow( userData.existingUser.id, workspace.id, ); } - if (!hasInvitation && isTargetAnExistingWorkspace && !isAnExistingUser) { + if ( + !isInvitedToWorkspace && + isTargetAnExistingWorkspace && + !isAnExistingUser + ) { throw new AuthException( 'User does not have access to this workspace', AuthExceptionCode.FORBIDDEN_EXCEPTION, 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 98dd651b208..b4cdba99ecd 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 @@ -67,7 +67,7 @@ describe('SignInUpService', () => { { provide: WorkspaceInvitationService, useValue: { - validateInvitation: jest.fn(), + validatePersonalInvitation: jest.fn(), invalidateWorkspaceInvitation: jest.fn(), }, }, @@ -145,7 +145,7 @@ describe('SignInUpService', () => { }; jest - .spyOn(workspaceInvitationService, 'validateInvitation') + .spyOn(workspaceInvitationService, 'validatePersonalInvitation') .mockResolvedValue({ isValid: true, workspace: params.workspace as Workspace, @@ -163,7 +163,9 @@ describe('SignInUpService', () => { expect(result.workspace).toEqual(params.workspace); expect(result.user).toBeDefined(); - expect(workspaceInvitationService.validateInvitation).toHaveBeenCalledWith({ + expect( + workspaceInvitationService.validatePersonalInvitation, + ).toHaveBeenCalledWith({ workspacePersonalInviteToken: 'invitationToken', email: 'test@example.com', }); 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 41ba1c67716..e0237027a7b 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 @@ -173,7 +173,7 @@ export class SignInUpService { } const invitationValidation = - await this.workspaceInvitationService.validateInvitation({ + await this.workspaceInvitationService.validatePersonalInvitation({ workspacePersonalInviteToken: params.invitation.value, email, }); 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 488c8a2a222..596bab9af62 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 @@ -125,10 +125,11 @@ export class UserWorkspaceService extends TypeOrmQueryService { } async addUserToWorkspaceByInviteToken(inviteToken: string, user: User) { - const appToken = await this.workspaceInvitationService.validateInvitation({ - workspacePersonalInviteToken: inviteToken, - email: user.email, - }); + const appToken = + await this.workspaceInvitationService.validatePersonalInvitation({ + workspacePersonalInviteToken: inviteToken, + email: user.email, + }); await this.workspaceInvitationService.invalidateWorkspaceInvitation( appToken.workspace.id, diff --git a/packages/twenty-server/src/engine/core-modules/workspace-invitation/services/workspace-invitation.service.ts b/packages/twenty-server/src/engine/core-modules/workspace-invitation/services/workspace-invitation.service.ts index 8d4594ab979..34d0d62501b 100644 --- a/packages/twenty-server/src/engine/core-modules/workspace-invitation/services/workspace-invitation.service.ts +++ b/packages/twenty-server/src/engine/core-modules/workspace-invitation/services/workspace-invitation.service.ts @@ -37,8 +37,6 @@ export class WorkspaceInvitationService { constructor( @InjectRepository(AppToken, 'core') private readonly appTokenRepository: Repository, - @InjectRepository(Workspace, 'core') - private readonly workspaceRepository: Repository, @InjectRepository(UserWorkspace, 'core') private readonly userWorkspaceRepository: Repository, private readonly environmentService: EnvironmentService, @@ -47,32 +45,7 @@ export class WorkspaceInvitationService { private readonly domainManagerService: DomainManagerService, ) {} - // VALIDATIONS METHODS - private async validatePublicInvitation(workspaceInviteHash: string) { - const workspace = await this.workspaceRepository.findOne({ - where: { - inviteHash: workspaceInviteHash, - }, - }); - - if (!workspace) { - throw new AuthException( - 'Workspace not found', - AuthExceptionCode.WORKSPACE_NOT_FOUND, - ); - } - - if (!workspace.isPublicInviteLinkEnabled) { - throw new AuthException( - 'Workspace does not allow public invites', - AuthExceptionCode.FORBIDDEN_EXCEPTION, - ); - } - - return { isValid: true, workspace }; - } - - private async validatePersonalInvitation({ + async validatePersonalInvitation({ workspacePersonalInviteToken, email, }: { @@ -109,32 +82,6 @@ export class WorkspaceInvitationService { } } - async validateInvitation({ - workspacePersonalInviteToken, - workspaceInviteHash, - email, - }: { - workspacePersonalInviteToken?: string; - workspaceInviteHash?: string; - email: string; - }) { - if (workspacePersonalInviteToken) { - return await this.validatePersonalInvitation({ - workspacePersonalInviteToken, - email, - }); - } - - if (workspaceInviteHash) { - return await this.validatePublicInvitation(workspaceInviteHash); - } - - throw new AuthException( - 'Invitation invalid', - AuthExceptionCode.FORBIDDEN_EXCEPTION, - ); - } - async findInvitationByWorkspaceSubdomainAndUserEmail({ subdomain, email,