diff --git a/packages/twenty-server/src/engine/core-modules/auth/services/sso-user-provisioning.service.spec.ts b/packages/twenty-server/src/engine/core-modules/auth/services/sso-user-provisioning.service.spec.ts index 8a20488c99ef1..33229906c0faf 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/services/sso-user-provisioning.service.spec.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/services/sso-user-provisioning.service.spec.ts @@ -7,12 +7,28 @@ const buildService = (overrides?: { existingUser?: unknown; workspace?: unknown; configuredSubdomain?: string; + insideLockExistingUser?: unknown; }) => { + // Find-or-create runs inside dataSource.transaction. The repository + // returned by manager.getRepository(UserEntity) is the same jest + // mock as the injected one so existing assertions on + // `userRepository.findOne/create/save` keep working unchanged. const userRepository = { findOne: jest.fn().mockResolvedValue(overrides?.existingUser ?? null), create: jest.fn((u) => u), save: jest.fn(async (u) => ({ id: 'user-id-1', ...u })), }; + + const queryFn = jest.fn().mockResolvedValue(undefined); + const dataSource = { + transaction: jest.fn(async (cb: (m: any) => Promise) => + cb({ + query: queryFn, + getRepository: jest.fn(() => userRepository), + }), + ), + }; + const workspaceRepository = { findOne: jest.fn().mockResolvedValue( overrides?.workspace ?? { @@ -37,6 +53,7 @@ const buildService = (overrides?: { workspaceRepository as any, userWorkspaceService as any, twentyConfigService as any, + dataSource as any, ); return { @@ -45,6 +62,8 @@ const buildService = (overrides?: { workspaceRepository, userWorkspaceService, twentyConfigService, + dataSource, + queryFn, }; }; @@ -137,4 +156,71 @@ describe('SsoUserProvisioningService', () => { where: { email: 'mixed@case.com' }, }); }); + + it('should acquire an advisory lock keyed by email before find+create', async () => { + // Concurrent-creation race guard: every first-login provisioning + // attempt must take a Postgres advisory lock keyed on the email + // hash so parallel requests for the same email serialise on the + // create. Other emails take different lock keys so distinct + // first-logins don't contend. + const { service, queryFn, dataSource } = buildService(); + + await service.findOrProvision('alice@askii.ai'); + + expect(dataSource.transaction).toHaveBeenCalledTimes(1); + expect(queryFn).toHaveBeenCalledWith( + expect.stringMatching(/pg_advisory_xact_lock\s*\(\s*hashtextextended/), + ['alice@askii.ai'], + ); + }); + + it('should take the advisory lock before any user lookup', async () => { + // The race protection only works if the lock is taken BEFORE the + // findOne. If a refactor accidentally moves the findOne above the + // lock, two concurrent requests could both pass the miss check + // before either takes the lock — race reopens. + const callOrder: string[] = []; + const { service, userRepository, queryFn } = buildService(); + + queryFn.mockImplementation(async () => { + callOrder.push('lock'); + }); + userRepository.findOne.mockImplementation(async () => { + callOrder.push('findOne'); + return null; + }); + + await service.findOrProvision('alice@askii.ai'); + + expect(callOrder[0]).toBe('lock'); + expect(callOrder.indexOf('findOne')).toBeGreaterThan( + callOrder.indexOf('lock'), + ); + }); + + it('should skip create when another request committed a row first', async () => { + // Race: this request waited on the advisory lock; while it waited, + // another concurrent request committed a User row for the same + // email. The lock serialises; once we acquire it, our findOne + // sees that row and we skip our own create. Without serialisation + // both racing requests would insert duplicate rows. + const winningUser = { + id: 'committed-by-other-request', + email: 'racy@askii.ai', + }; + const { service, userRepository, userWorkspaceService } = buildService({ + existingUser: winningUser, + }); + + const result = await service.findOrProvision('racy@askii.ai'); + + expect(userRepository.create).not.toHaveBeenCalled(); + expect(userRepository.save).not.toHaveBeenCalled(); + expect(result.user).toBe(winningUser); + // Membership ensure runs regardless of who won the race — the + // workspace join is idempotent on its own. + expect( + userWorkspaceService.addUserToWorkspaceIfUserNotInWorkspace, + ).toHaveBeenCalledWith(winningUser, expect.anything()); + }); }); diff --git a/packages/twenty-server/src/engine/core-modules/auth/services/sso-user-provisioning.service.ts b/packages/twenty-server/src/engine/core-modules/auth/services/sso-user-provisioning.service.ts index 83ba04752f8f1..31c26ab7051bc 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/services/sso-user-provisioning.service.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/services/sso-user-provisioning.service.ts @@ -3,7 +3,7 @@ import { InjectRepository } from '@nestjs/typeorm'; import { randomBytes } from 'crypto'; -import { Repository } from 'typeorm'; +import { DataSource, Repository } from 'typeorm'; import { AuthException, @@ -35,6 +35,9 @@ export class SsoUserProvisioningService { private readonly workspaceRepository: Repository, private readonly userWorkspaceService: UserWorkspaceService, private readonly twentyConfigService: TwentyConfigService, + // Used for the advisory-lock transaction that serialises + // concurrent first-login provisioning (see findOrCreateUser). + private readonly dataSource: DataSource, ) {} async findOrProvision(email: string): Promise { @@ -81,27 +84,56 @@ export class SsoUserProvisioningService { } private async findOrCreateUser(email: string): Promise { - const existing = await this.userRepository.findOne({ where: { email } }); - - if (existing) { - return existing; - } + // Concurrent-creation race guard. On first-ever SSO login the SPA + // fires multiple parallel API requests with no session cookie set + // yet — every request reaches the proxy-login flow, every request + // hits the findOne→save below, and before the row is committed the + // others have already passed the findOne miss. Without a + // serialisation point, N parallel requests insert N duplicate rows + // for the same email. + // + // We don't rely on the UserEntity unique-on-email constraint to + // dedupe at the DB level — that surfaces as a 500 on the losers of + // the race rather than a graceful return of the existing user. We + // serialise the find+create on a Postgres advisory lock keyed by + // the email hash. Advisory locks are per-session, transaction- + // scoped, no schema cost; different emails take different lock + // keys so concurrent first-logins for distinct users don't contend. + return await this.dataSource.transaction(async (manager) => { + // pg_advisory_xact_lock takes a bigint; hashtextextended returns a + // stable bigint hash. Released automatically when the transaction + // commits or rolls back. + await manager.query( + 'SELECT pg_advisory_xact_lock(hashtextextended($1, 0))', + [email], + ); - const unguessablePassword = randomBytes(32).toString('hex'); - const passwordHash = await hashPassword(unguessablePassword); - - // The SSO IdP already vouched for this email (the only path to the - // proxy-login route is through oauth2-proxy, which validates the - // session). Mark the user verified at creation so SSO accounts don't - // appear semi-verified to gates that read isEmailVerified. - const created = this.userRepository.create({ - email, - firstName: '', - lastName: '', - passwordHash, - isEmailVerified: true, + // Re-check inside the lock. If a concurrent request beat us to + // the create, this returns its row and we skip provisioning. + const userRepo = manager.getRepository(UserEntity); + const existing = await userRepo.findOne({ where: { email } }); + + if (existing) { + return existing; + } + + const unguessablePassword = randomBytes(32).toString('hex'); + const passwordHash = await hashPassword(unguessablePassword); + + // The SSO IdP already vouched for this email (the only path to + // the proxy-login route is through oauth2-proxy, which validates + // the session). Mark the user verified at creation so SSO + // accounts don't appear semi-verified to gates that read + // isEmailVerified. + const created = userRepo.create({ + email, + firstName: '', + lastName: '', + passwordHash, + isEmailVerified: true, + }); + + return await userRepo.save(created); }); - - return await this.userRepository.save(created); } }