diff --git a/packages/core/src/user/identity.test.ts b/packages/core/src/user/identity.test.ts index 6c70472a..3aa6ab73 100644 --- a/packages/core/src/user/identity.test.ts +++ b/packages/core/src/user/identity.test.ts @@ -221,3 +221,73 @@ describe('Identity.linkSteam', () => { expect(resolved.userID).toBe(legacy.userID); }); }); + +/** + * The same call, several times at once, against a real database. + * + * Every one of these holds a rule that is enforced across two statements — a + * lookup and then a write — which means the rule is only as good as whatever + * stops the two from interleaving. Run one at a time they all pass whether or + * not that protection exists, which is exactly why they are written this way. + */ +describe('the same thing happening twice at once', () => { + beforeEach(cleanup); + + test('the cap holds when the links arrive together', async () => { + const { userID } = await Identity.fromVerifiedEmail({ email: email(8) }); + track(userID); + + const wanted = Identity.MAX_STEAM_ACCOUNTS + 2; + const results = await Promise.allSettled( + Array.from({ length: wanted }, (_, i) => + Identity.linkSteam({ userId: userID, steamId: steamID(60 + i) }) + ) + ); + + expect(await Identity.listSteam(userID)).toHaveLength(Identity.MAX_STEAM_ACCOUNTS); + expect(results.filter((r) => r.status === 'fulfilled')).toHaveLength( + Identity.MAX_STEAM_ACCOUNTS + ); + for (const rejected of results.filter((r) => r.status === 'rejected')) { + expect((rejected as PromiseRejectedResult).reason.code).toBe('invalid_state'); + } + }); + + test('several sign-ins for one new address make one account', async () => { + const address = email(9); + + const results = await Promise.all([ + Identity.fromVerifiedEmail({ email: address }), + Identity.fromVerifiedEmail({ email: address }), + Identity.fromVerifiedEmail({ email: address }) + ]); + results.forEach((r) => track(r.userID)); + + expect(new Set(results.map((r) => r.userID)).size).toBe(1); + expect(results.filter((r) => r.created)).toHaveLength(1); + + const rows = await sql` + select count(*)::int as n from "user" + where email = ${address} and time_deleted is null + `; + expect(rows[0]!.n).toBe(1); + }); + + test('two accounts claiming one address get an answer rather than a driver error', async () => { + const first = await legacySteamUser(70); + const second = await legacySteamUser(71); + const address = email(10); + + const results = await Promise.allSettled([ + Identity.claimWithEmail({ userId: first.userID, email: address }), + Identity.claimWithEmail({ userId: second.userID, email: address }) + ]); + + const rejected = results.filter((r) => r.status === 'rejected') as PromiseRejectedResult[]; + expect(rejected).toHaveLength(1); + // The point of the assertion: a sentence a screen can render, and not + // whatever text the driver puts on a constraint violation. + expect(rejected[0]!.reason.type).toBe('already_exists'); + expect(rejected[0]!.reason.message).toMatch(/another account/); + }); +}); diff --git a/packages/core/src/user/identity.ts b/packages/core/src/user/identity.ts index a07c07be..15b94be2 100644 --- a/packages/core/src/user/identity.ts +++ b/packages/core/src/user/identity.ts @@ -6,11 +6,38 @@ import { ErrorCodes, VisibleError } from '../error.js'; import { fn } from '../fn.js'; import { Identifier } from '../id.js'; import { User } from './index.js'; +import { UserTable } from './user.sql.js'; import { LinkedAccount } from './linked-account.js'; import { LinkedAccountTable } from './linked-account.sql.js'; const STEAM_ID_RE = /^\d{17}$/; +/** The partial unique index on a live account's address, named by the migration. */ +const EMAIL_UNIQUE = 'user_email_unique'; + +/** + * Whether a failure is the database refusing a duplicate. + * + * Every read-then-write below has a window between the read and the write, and + * the index is what actually closes it. Recognising the refusal is how the + * loser of a race turns a raw driver error into the answer it was asking for — + * so the constraint is the mechanism and this is how the code hears from it. + */ +function isUniqueViolation(err: unknown, constraint: string): boolean { + // Walked rather than read off the top, because the query builder wraps what + // the driver threw: the outer error carries the SQL and the parameters, and + // the code and the constraint name are on the cause underneath it. + for (let e: unknown = err, depth = 0; e && depth < 8; depth++) { + if (typeof e !== 'object') break; + const candidate = e as { code?: unknown; constraint_name?: unknown; cause?: unknown }; + if (String(candidate.code) === '23505' && candidate.constraint_name === constraint) { + return true; + } + e = candidate.cause; + } + return false; +} + /** * Email is the root of an account, so two spellings of one address must not be * two accounts. Case and surrounding whitespace are the two ways the same @@ -51,27 +78,48 @@ export namespace Identity { z.object({ email: Email, name: z.string().optional() }), async (input) => { const email = input.email; - return Database.transaction(async () => { - const existing = await User.fromEmail(email); - if (existing) { - // An address that was attached but never confirmed is - // confirmed now: getting here means a code was redeemed. - if (!existing.emailVerified) { - await User.setEmail({ id: existing.id, email, emailVerified: true }); - } - return { userID: existing.id, created: false }; - } - const userID = Identifier.ascending('user'); - await User.create({ - id: userID, - name: input.name?.trim() || email.split('@')[0]!, - email, - emailVerified: true, - image: null + async function attempt() { + return Database.transaction(async () => { + const existing = await User.fromEmail(email); + if (existing) { + // An address that was attached but never confirmed is + // confirmed now: getting here means a code was redeemed. + if (!existing.emailVerified) { + await User.setEmail({ id: existing.id, email, emailVerified: true }); + } + return { userID: existing.id, created: false }; + } + + const userID = Identifier.ascending('user'); + await User.create({ + id: userID, + name: input.name?.trim() || email.split('@')[0]!, + email, + emailVerified: true, + image: null + }); + return { userID, created: true }; }); - return { userID, created: true }; - }); + } + + try { + return await attempt(); + } catch (err) { + if (!isUniqueViolation(err, EMAIL_UNIQUE)) throw err; + + // Somebody else finished the same sign-in first. + // + // Two people redeeming a code for one address is one person + // with two tabs, and the answer they both want is the account + // that now exists. The lookup and the insert cannot be made one + // statement here — the row is built from an id this process + // generates — so the index arbitrates and the loser reads back + // what the winner wrote. Retried once and not in a loop: a + // second refusal means the row is gone again, which is a + // deletion racing a sign-in and not something to spin on. + return await attempt(); + } } ); @@ -86,29 +134,43 @@ export namespace Identity { z.object({ userId: z.string(), email: Email }), async (input) => { const email = input.email; - return Database.transaction(async () => { - const holder = await User.fromEmail(email); - if (holder && holder.id !== input.userId) { - throw new VisibleError( - 'already_exists', - ErrorCodes.Validation.ALREADY_EXISTS, - 'That email address already belongs to another account' - ); - } - const updated = await User.setEmail({ - id: input.userId, - email, - emailVerified: true + try { + return await Database.transaction(async () => { + const holder = await User.fromEmail(email); + if (holder && holder.id !== input.userId) { + throw new VisibleError( + 'already_exists', + ErrorCodes.Validation.ALREADY_EXISTS, + 'That email address already belongs to another account' + ); + } + const updated = await User.setEmail({ + id: input.userId, + email, + emailVerified: true + }); + if (!updated) { + throw new VisibleError( + 'not_found', + ErrorCodes.NotFound.RESOURCE_NOT_FOUND, + 'No such account' + ); + } + return updated; }); - if (!updated) { - throw new VisibleError( - 'not_found', - ErrorCodes.NotFound.RESOURCE_NOT_FOUND, - 'No such account' - ); - } - return updated; - }); + } catch (err) { + // The check above and the update below are two statements, so + // two accounts claiming one address can both find it free. The + // index refuses the second, and the person deserves the same + // sentence they would have got a moment earlier rather than a + // driver's error text. + if (!isUniqueViolation(err, EMAIL_UNIQUE)) throw err; + throw new VisibleError( + 'already_exists', + ErrorCodes.Validation.ALREADY_EXISTS, + 'That email address already belongs to another account' + ); + } } ); @@ -143,8 +205,8 @@ export namespace Identity { * Hang a Steam account off a user, up to {@link MAX_STEAM_ACCOUNTS}. * * The count and the insert are one transaction because they are one - * decision: read the four, then write the fifth, and two concurrent calls - * each see four. + * decision, and the transaction takes the account's own row first so that + * two callers cannot each count four and each write a fifth. */ export const linkSteam = fn( z.object({ @@ -154,6 +216,30 @@ export namespace Identity { }), async (input) => { return Database.transaction(async (tx) => { + // Take the account's own row first, and hold it. + // + // The cap is a count, and a count only means something if it + // is taken while nothing can change it. Locking the + // connections instead locks nothing at all when there are + // none: there are no gap locks under read committed, so + // `for update` over an empty result set is an empty set of + // locks, and several simultaneous first-time links all read + // zero and all insert. The account's own row is the one thing + // every caller for it is guaranteed to contend on, so it is + // what serializes them. + const [owner] = await tx + .select({ id: UserTable.id }) + .from(UserTable) + .where(and(eq(UserTable.id, input.userId), isNull(UserTable.timeDeleted))) + .for('update'); + if (!owner) { + throw new VisibleError( + 'not_found', + ErrorCodes.NotFound.RESOURCE_NOT_FOUND, + 'No such account' + ); + } + const existing = await LinkedAccount.findByProvider({ provider: 'steam', providerAccountId: input.steamId @@ -172,8 +258,8 @@ export namespace Identity { return existing.id; } - // `for update` on the rows already there, so a second caller - // holding at the cap waits rather than counting alongside. + // Counted under the lock taken above, so what is counted is + // what is still there at the insert. const held = await tx .select({ id: LinkedAccountTable.id }) .from(LinkedAccountTable) @@ -183,8 +269,7 @@ export namespace Identity { eq(LinkedAccountTable.provider, 'steam'), isNull(LinkedAccountTable.timeDeleted) ) - ) - .for('update'); + ); if (held.length >= MAX_STEAM_ACCOUNTS) { throw new VisibleError(