From d105407c708754834097f962e92ed692b2fb8d45 Mon Sep 17 00:00:00 2001 From: awais786 Date: Sat, 16 May 2026 19:41:20 +0500 Subject: [PATCH] fix(sso): serialise first-login user provisioning behind an advisory lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On first-ever SSO login the SPA fires multiple parallel API requests with no session cookie set yet, so 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. The losers of the race throw a unique-constraint 500 instead of returning the winner's row. Real production occurrence (in Outline, same class of bug): 5 duplicate users for one Cognito identity within ~800ms. Twenty's UserEntity DOES have a unique constraint on email, but the current code does NOT catch the resulting DB error. So even though we won't store duplicates, the losing requests fail with a 500 — and on retry, both the request that succeeded and any subsequent ones go through the same race window. Fix: serialise the find+create on a Postgres advisory lock keyed by the email hash. Different emails take different lock keys; concurrent first-logins for distinct users don't contend. Mechanism: - dataSource.transaction wraps the lookup + create - SELECT pg_advisory_xact_lock(hashtextextended($1, 0)) inside the transaction makes concurrent requests for the same email block on the lock; the second one waits until the first commits - Re-check findOne inside the lock so the waiting request sees the just-created row and skips its own create - Lock is transaction-scoped (xact variant), released automatically on commit/rollback — no leak risk Tests: - should-acquire-an-advisory-lock-keyed-by-email-before-find-create pins the lock pattern (call shape + parameter) - should-take-the-advisory-lock-before-any-user-lookup pins the ordering invariant (lock first, then findOne) — guards against a refactor that moves the findOne above the lock - should-skip-create-when-another-request-committed-a-row-first simulates the race outcome: lock serialises, findOne under the lock sees the winning row, we return it without creating This matches the openspec contract "Concurrent creation races SHALL fall back to read" requirement — the advisory lock IS the runtime serialisation point; reads under the lock observe the winner. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../sso-user-provisioning.service.spec.ts | 86 +++++++++++++++++++ .../services/sso-user-provisioning.service.ts | 74 +++++++++++----- 2 files changed, 139 insertions(+), 21 deletions(-) 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); } }