diff --git a/src/backend/clients/database/SqliteDatabaseClient.test.ts b/src/backend/clients/database/SqliteDatabaseClient.test.ts index c7000bb73..209da6262 100644 --- a/src/backend/clients/database/SqliteDatabaseClient.test.ts +++ b/src/backend/clients/database/SqliteDatabaseClient.test.ts @@ -27,7 +27,7 @@ import { DatabaseClientFactory } from './index.js'; import { SqliteDatabaseClient } from './SqliteDatabaseClient.js'; /** Highest schema version the migration table can reach. */ -const CURRENT_SCHEMA_VERSION = 61; +const CURRENT_SCHEMA_VERSION = 62; const SYSTEM_USER_UUID = '5d4adce0-a381-4982-9c02-6e2540026238'; const sqliteConfig = ( diff --git a/src/backend/clients/database/SqliteDatabaseClient.ts b/src/backend/clients/database/SqliteDatabaseClient.ts index 1f3642387..6877d16c4 100644 --- a/src/backend/clients/database/SqliteDatabaseClient.ts +++ b/src/backend/clients/database/SqliteDatabaseClient.ts @@ -95,6 +95,7 @@ const AVAILABLE_MIGRATIONS: [number, string[]][] = [ [58, ['0063_add_suspended_reason.sql']], [59, ['0064_abuse-moderation-events.sql']], [60, ['0065_app-feedback.sql']], + [61, ['0066_owned-email-unique.sql']], ]; export class SqliteDatabaseClient extends AbstractDatabaseClient { diff --git a/src/backend/clients/database/migrations/mysql/mysql_mig_21.sql b/src/backend/clients/database/migrations/mysql/mysql_mig_21.sql new file mode 100644 index 000000000..b6867a88e --- /dev/null +++ b/src/backend/clients/database/migrations/mysql/mysql_mig_21.sql @@ -0,0 +1,128 @@ +-- Copyright (C) 2024-present Puter Technologies Inc. +-- +-- This file is part of Puter. +-- +-- Puter is free software: you can redistribute it and/or modify +-- it under the terms of the GNU Affero General Public License as published +-- by the Free Software Foundation, either version 3 of the License, or +-- (at your option) any later version. +-- +-- This program is distributed in the hope that it will be useful, +-- but WITHOUT ANY WARRANTY; without even the implied warranty of +-- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +-- GNU Affero General Public License for more details. +-- +-- You should have received a copy of the GNU Affero General Public License +-- along with this program. If not, see . + +-- Enforce "at most one account owns an email address". Mirrors SQLite +-- migration 0066. +-- +-- `user.email` is deliberately not UNIQUE: several rows may legitimately hold +-- the same address while unconfirmed (admin-provisioned placeholders, signups +-- that were never confirmed, temp accounts on their way to becoming real). What +-- must never happen is two rows both *owning* an address — owning meaning the +-- row is confirmed, or holds a password and so can drive password recovery for +-- that inbox. +-- +-- Signup, save-account, change-email, OIDC and admin provisioning each check for +-- an owner before writing, but a check and an insert are not one operation: two +-- requests can both read "free" and both write. This index is what actually +-- holds the invariant; the application checks just produce a nicer error most of +-- the time. +-- +-- SQLite expresses that with a partial index. MySQL has none, so the predicate +-- lives in a generated column that evaluates to NULL for every row that does not +-- own its address — and NULLs do not collide in an InnoDB unique index, which is +-- exactly the "unlimited unconfirmed placeholders" behaviour we need. +-- +-- The column is VIRTUAL, not STORED, on purpose: adding a stored generated +-- column rebuilds the table, while a virtual one is a metadata-only change and +-- the index that follows builds INPLACE. On a `user` table of any size with +-- read replicas attached, that is the difference between a routine change and an +-- outage. The ALGORITHM/LOCK clauses are spelled out so a server that cannot +-- honour them refuses the statement instead of quietly copying the table. +-- +-- Matching is on the canonical address so provider aliases +-- (`foo.bar+tag@gmail.com` vs `foobar@gmail.com`) collide. `clean_email` is +-- written on every modern write path; the COALESCE covers rows old enough to +-- predate the column. Run the `clean_email` backfill before this migration or +-- alias collisions among those rows go unnoticed. +-- +-- Idempotent: both steps are guarded on INFORMATION_SCHEMA so the directory +-- replays safely. +-- +-- If the index creation fails with ER_DUP_ENTRY, the DB already contains +-- duplicate owners. Collapse them first (admin → One-off Jobs → Collapse +-- Duplicate Emails) — there is no safe automatic merge of two accounts. + +DROP PROCEDURE IF EXISTS _puter_add_owned_email; +DELIMITER // +CREATE PROCEDURE _puter_add_owned_email() +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM INFORMATION_SCHEMA.COLUMNS + WHERE TABLE_SCHEMA = DATABASE() + AND TABLE_NAME = 'user' + AND COLUMN_NAME = 'owned_email' + ) THEN + ALTER TABLE `user` + ADD COLUMN `owned_email` VARCHAR(256) + CHARACTER SET latin1 COLLATE latin1_swedish_ci + GENERATED ALWAYS AS ( + CASE + WHEN `email` IS NOT NULL + AND (`email_confirmed` = 1 OR `password` IS NOT NULL) + THEN COALESCE(`clean_email`, LOWER(`email`)) + ELSE NULL + END + ) VIRTUAL, + ALGORITHM=INSTANT; + END IF; + + IF NOT EXISTS ( + SELECT 1 FROM INFORMATION_SCHEMA.STATISTICS + WHERE TABLE_SCHEMA = DATABASE() + AND TABLE_NAME = 'user' + AND INDEX_NAME = 'idx_user_owned_email' + ) THEN + ALTER TABLE `user` + ADD UNIQUE KEY `idx_user_owned_email` (`owned_email`), + ALGORITHM=INPLACE, LOCK=NONE; + END IF; +END// +DELIMITER ; + +CALL _puter_add_owned_email(); + +DROP PROCEDURE IF EXISTS _puter_add_owned_email; + +-- One Puter account per external identity. OIDCStore.link already assumes this +-- constraint exists — it catches the unique violation to tell "re-linking the +-- same account" apart from "this sub belongs to someone else" — but the table +-- never actually had it, so two concurrent first-time logins could each create +-- an account and each link the same sub. Subsequent logins then resolved to +-- whichever row came back first. +-- +-- Dedupe `user_oidc_providers` before applying this: keep the lowest `id` per +-- (provider, provider_sub) and point it at the account the collapse job kept. + +DROP PROCEDURE IF EXISTS _puter_add_oidc_sub_unique; +DELIMITER // +CREATE PROCEDURE _puter_add_oidc_sub_unique() +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM INFORMATION_SCHEMA.STATISTICS + WHERE TABLE_SCHEMA = DATABASE() + AND TABLE_NAME = 'user_oidc_providers' + AND INDEX_NAME = 'idx_user_oidc_provider_sub' + ) THEN + ALTER TABLE `user_oidc_providers` + ADD UNIQUE KEY `idx_user_oidc_provider_sub` (`provider`, `provider_sub`); + END IF; +END// +DELIMITER ; + +CALL _puter_add_oidc_sub_unique(); + +DROP PROCEDURE IF EXISTS _puter_add_oidc_sub_unique; diff --git a/src/backend/clients/database/migrations/postgres/postgres_mig_10.sql b/src/backend/clients/database/migrations/postgres/postgres_mig_10.sql new file mode 100644 index 000000000..95c5efa52 --- /dev/null +++ b/src/backend/clients/database/migrations/postgres/postgres_mig_10.sql @@ -0,0 +1,40 @@ +-- Copyright (C) 2024-present Puter Technologies Inc. +-- +-- This file is part of Puter. +-- +-- Puter is free software: you can redistribute it and/or modify +-- it under the terms of the GNU Affero General Public License as published +-- by the Free Software Foundation, either version 3 of the License, or +-- (at your option) any later version. +-- +-- This program is distributed in the hope that it will be useful, +-- but WITHOUT ANY WARRANTY; without even the implied warranty of +-- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +-- GNU Affero General Public License for more details. +-- +-- You should have received a copy of the GNU Affero General Public License +-- along with this program. If not, see . + +-- Enforce "at most one account owns an email address". Mirrors SQLite +-- migration 0066 and MySQL migration 21; see those for the full rationale. +-- +-- In short: `user.email` is deliberately not UNIQUE because several rows may +-- hold the same address while unconfirmed. What must not happen is two rows +-- both owning it — confirmed, or holding a password and so able to drive +-- password recovery for that inbox. The application checks for an owner before +-- every write, but a check and a write are not one operation. +-- +-- If this fails, the DB already contains duplicate owners; collapse them first +-- (admin → One-off Jobs → Collapse Duplicate Emails). + +CREATE UNIQUE INDEX IF NOT EXISTS idx_user_owned_email + ON "user" (COALESCE(clean_email, LOWER(email))) + WHERE email IS NOT NULL + AND (email_confirmed = TRUE OR password IS NOT NULL); + +-- One Puter account per external identity. OIDCStore.link already assumes this +-- constraint exists — it catches the unique violation to tell "re-linking the +-- same account" apart from "this sub belongs to someone else" — but the table +-- never actually had it. +CREATE UNIQUE INDEX IF NOT EXISTS idx_user_oidc_provider_sub + ON user_oidc_providers (provider, provider_sub); diff --git a/src/backend/clients/database/migrations/sqlite/0066_owned-email-unique.sql b/src/backend/clients/database/migrations/sqlite/0066_owned-email-unique.sql new file mode 100644 index 000000000..da95a1144 --- /dev/null +++ b/src/backend/clients/database/migrations/sqlite/0066_owned-email-unique.sql @@ -0,0 +1,53 @@ +-- Copyright (C) 2024-present Puter Technologies Inc. +-- +-- This file is part of Puter. +-- +-- Puter is free software: you can redistribute it and/or modify +-- it under the terms of the GNU Affero General Public License as published +-- by the Free Software Foundation, either version 3 of the License, or +-- (at your option) any later version. +-- +-- This program is distributed in the hope that it will be useful, +-- but WITHOUT ANY WARRANTY; without even the implied warranty of +-- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +-- GNU Affero General Public License for more details. +-- +-- You should have received a copy of the GNU Affero General Public License +-- along with this program. If not, see . + +-- Enforce "at most one account owns an email address". +-- +-- `user.email` is deliberately not UNIQUE: several rows may legitimately hold +-- the same address while unconfirmed (admin-provisioned placeholders, signups +-- that were never confirmed, temp accounts on their way to becoming real). What +-- must never happen is two rows both *owning* an address — owning meaning the +-- row is confirmed, or holds a password and so can drive password recovery for +-- that inbox. +-- +-- Signup, save-account, change-email, OIDC and admin provisioning each check for +-- an owner before writing, but a check and an insert are not one operation: two +-- requests can both read "free" and both write. This index is what actually +-- holds the invariant; the application checks just produce a nicer error most of +-- the time. +-- +-- Matching is on the canonical address so provider aliases +-- (`foo.bar+tag@gmail.com` vs `foobar@gmail.com`) collide. `clean_email` is +-- written on every modern write path; the COALESCE covers rows old enough to +-- predate the column. +-- +-- If this CREATE fails, the DB already contains duplicate owners. Collapse them +-- first (admin → One-off Jobs → Collapse Duplicate Emails) — there is no safe +-- automatic merge of two accounts. +CREATE UNIQUE INDEX IF NOT EXISTS idx_user_owned_email + ON user(COALESCE(clean_email, lower(email))) + WHERE email IS NOT NULL + AND (email_confirmed = 1 OR password IS NOT NULL); + +-- One Puter account per external identity. OIDCStore.link already assumes this +-- constraint exists — it catches the unique violation to tell "re-linking the +-- same account" apart from "this sub belongs to someone else" — but the table +-- never actually had it, so two concurrent first-time logins could each create +-- an account and each link the same sub. Subsequent logins then resolved to +-- whichever row came back first. +CREATE UNIQUE INDEX IF NOT EXISTS idx_user_oidc_provider_sub + ON user_oidc_providers(provider, provider_sub); diff --git a/src/backend/controllers/auth/AuthController.test.ts b/src/backend/controllers/auth/AuthController.test.ts index b9f698da4..03e3dc5ad 100644 --- a/src/backend/controllers/auth/AuthController.test.ts +++ b/src/backend/controllers/auth/AuthController.test.ts @@ -346,6 +346,188 @@ describe('puter.signup.validate event', () => { // ── Signup flow ───────────────────────────────────────────────────── +// ── Concurrent claims on one address ──────────────────────────────── +// +// The duplicate checks these flows run are not atomic with the writes that +// follow them — the validate hook and bcrypt sit in between, and both are slow +// enough for a second request to pass the same check. These tests fire the +// requests together and assert the address still ends up on exactly one row. + +describe('concurrent claims on one email address', () => { + const uniq = () => Math.random().toString(36).slice(2, 10); + + const countOwners = async (email: string): Promise => { + // `email_confirmed` is TINYINT on MySQL and BOOLEAN on Postgres, so the + // literal has to come from the client rather than be hardcoded. + const isTrue = server.clients.db.booleanLiteral(true); + const rows = (await server.clients.db.read( + `SELECT COUNT(*) AS n FROM \`user\` WHERE \`email\` = ? AND (\`email_confirmed\` = ${isTrue} OR \`password\` IS NOT NULL)`, + [email], + )) as Array<{ n: number }>; + return Number(rows[0]?.n ?? 0); + }; + + it('lets only one of two simultaneous signups take the address', async () => { + const email = `race-${uniq()}@test.local`; + const signup = (username: string) => + controller.handleSignup( + makeReq({ username, email, password: 'correct-horse-battery' }), + makeRes(), + ); + + // Distinct usernames on purpose: the UNIQUE on `username` would + // otherwise be what rejects the second request, and the email race + // would go untested. + const results = await Promise.allSettled([ + signup(`r_a_${uniq()}`), + signup(`r_b_${uniq()}`), + ]); + + expect(results.filter((r) => r.status === 'fulfilled')).toHaveLength(1); + expect(await countOwners(email)).toBe(1); + + const rejected = results.find((r) => r.status === 'rejected') as + | PromiseRejectedResult + | undefined; + expect(rejected?.reason).toMatchObject({ statusCode: 400 }); + }); + + it('lets only one of many simultaneous signups take the address', async () => { + const email = `race-many-${uniq()}@test.local`; + const results = await Promise.allSettled( + Array.from({ length: 5 }, () => + controller.handleSignup( + makeReq({ + username: `r_m_${uniq()}`, + email, + password: 'correct-horse-battery', + }), + makeRes(), + ), + ), + ); + + expect(results.filter((r) => r.status === 'fulfilled')).toHaveLength(1); + expect(await countOwners(email)).toBe(1); + }); + + it('rejects a signup racing an admin-provisioned placeholder claim', async () => { + const email = `race-pseudo-${uniq()}@test.local`; + // The placeholder shape signup is allowed to convert: unconfirmed, + // no password. Two signups both see it as claimable. + await server.stores.user.create({ + username: `r_p_${uniq()}`, + uuid: uuidv4(), + password: null, + email, + clean_email: email, + }); + + const results = await Promise.allSettled([ + controller.handleSignup( + makeReq({ + username: `r_p_a_${uniq()}`, + email, + password: 'correct-horse-battery', + }), + makeRes(), + ), + controller.handleSignup( + makeReq({ + username: `r_p_b_${uniq()}`, + email, + password: 'correct-horse-battery', + }), + makeRes(), + ), + ]); + + expect(results.filter((r) => r.status === 'fulfilled')).toHaveLength(1); + expect(await countOwners(email)).toBe(1); + }); + + it('reports a duplicate address as a 400, not a constraint error', async () => { + const email = `dupe-${uniq()}@test.local`; + await controller.handleSignup( + makeReq({ + username: `d_a_${uniq()}`, + email, + password: 'correct-horse-battery', + }), + makeRes(), + ); + + // The message has to be the one the pre-check produces — a user who + // loses the race should not be able to tell. + await expect( + controller.handleSignup( + makeReq({ + username: `d_b_${uniq()}`, + email, + password: 'correct-horse-battery', + }), + makeRes(), + ), + ).rejects.toMatchObject({ + statusCode: 400, + message: + 'This email already exists in our database. Please use another one.', + }); + }); + + it('refuses to give a placeholder row a password for a taken address', async () => { + const email = `recover-${uniq()}@test.local`; + await controller.handleSignup( + makeReq({ + username: `rec_own_${uniq()}`, + email, + password: 'correct-horse-battery', + }), + makeRes(), + ); + + // An unconfirmed, password-less row is allowed to sit on the same + // address — but password recovery accepts a username, so it can be + // driven for this row rather than for the account that owns the + // address. Setting a password here would make it a second account able + // to recover that inbox. + const placeholderName = `rec_ph_${uniq()}`; + const placeholder = await server.stores.user.create({ + username: placeholderName, + uuid: uuidv4(), + password: null, + email, + clean_email: email, + }); + const token = uuidv4(); + await server.stores.user.update(placeholder.id, { + pass_recovery_token: token, + }); + + const jwt = server.services.token.sign( + 'otp', + { + token, + user_uid: placeholder.uuid, + email, + purpose: 'pass-recovery', + }, + { expiresIn: '1h' }, + ); + + await expect( + controller.handleSetPassUsingToken( + makeReq({ token: jwt, password: 'another-strong-password' }), + makeRes(), + ), + ).rejects.toMatchObject({ + statusCode: 400, + message: + 'This email is already in use. Recover the account that uses it instead.', + }); + }); +}); + describe('AuthController.handleSignup', () => { const uniq = () => Math.random().toString(36).slice(2, 10); @@ -3847,6 +4029,151 @@ describe('AuthController.handleConfirmEmail', () => { }); expect(after!.email_confirmed).toBeFalsy(); }); + + it('strips a rival placeholder before confirming, not after', async () => { + const { user, actor } = await makeUserAndActor(); + const email = user.email as string; + + // A placeholder sitting on the same address. Confirming first and + // demoting second would momentarily leave two rows owning it, which + // the unique index rejects — turning a legitimate confirmation into a + // 500. + const rival = await server.stores.user.create({ + username: `rival_${Math.random().toString(36).slice(2, 10)}`, + uuid: uuidv4(), + password: null, + email, + clean_email: email, + }); + + const refreshed = await server.stores.user.getById(user.id, { + force: true, + }); + const res = makeRes(); + await controller.handleConfirmEmail( + makeReq({ code: refreshed!.email_confirm_code! }, { actor }), + res, + ); + + expect(res.body).toMatchObject({ email_confirmed: true }); + const confirmed = await server.stores.user.getById(user.id, { + force: true, + }); + expect(confirmed!.email_confirmed).toBe(true); + const strippedRival = await server.stores.user.getById(rival.id, { + force: true, + }); + expect(strippedRival!.email).toBeNull(); + }); + + it('refuses when another account already confirmed the address', async () => { + const email = `owned-${uniq()}@test.local`; + + // Confirmed with no password is the shape an identity provider + // creates, and it is what the old rival check (which also demanded a + // password) let through. Demoting it would take the address off an + // account that proved it owns the inbox. + const owner = await server.stores.user.create({ + username: `owner_${uniq()}`, + uuid: uuidv4(), + password: null, + email, + clean_email: email, + email_confirmed: true, + }); + + // A second, unconfirmed row on the same address holding a confirm + // code — the legacy duplicate this path used to resolve in its favour. + const claimant = await server.stores.user.create({ + username: `claim_${uniq()}`, + uuid: uuidv4(), + password: null, + email, + clean_email: email, + email_confirm_code: '123456', + }); + const actor = { + user: { + id: claimant.id, + uuid: claimant.uuid, + username: claimant.username, + email, + email_confirmed: false, + }, + } as Actor; + + await expect( + controller.handleConfirmEmail( + makeReq({ code: '123456' }, { actor }), + makeRes(), + ), + ).rejects.toMatchObject({ statusCode: 400 }); + + const untouched = await server.stores.user.getById(owner.id, { + force: true, + }); + expect(untouched!.email).toBe(email); + expect(untouched!.email_confirmed).toBe(true); + const stillUnconfirmed = await server.stores.user.getById(claimant.id, { + force: true, + }); + expect(stillUnconfirmed!.email_confirmed).toBeFalsy(); + }); +}); + +describe('AuthController.handleSaveAccount address conflicts', () => { + it('refuses to promote a temp account onto a taken address', async () => { + const email = `save-${Math.random().toString(36).slice(2, 10)}@test.local`; + await controller.handleSignup( + makeReq({ + username: `save_own_${Math.random().toString(36).slice(2, 10)}`, + email, + password: 'correct-horse-battery', + }), + makeRes(), + ); + + const tempRes = makeRes(); + await controller.handleSignup(makeReq({ is_temp: true }), tempRes); + const tempUser = ( + tempRes.body as { user: { username: string; uuid: string } } + ).user; + const tempRow = await server.stores.user.getByUuid(tempUser.uuid); + const actor = { + user: { + id: tempRow!.id, + uuid: tempRow!.uuid, + username: tempRow!.username, + email: null, + email_confirmed: false, + }, + } as Actor; + + await expect( + controller.handleSaveAccount( + makeReq( + { + username: `save_new_${Math.random().toString(36).slice(2, 10)}`, + email, + password: 'another-strong-password', + }, + { actor }, + ), + makeRes(), + ), + ).rejects.toMatchObject({ + statusCode: 400, + legacyCode: 'email_already_in_use', + }); + + // The temp row must be left alone — a failed promotion that already + // wrote the username would strand the account half-converted. + const untouched = await server.stores.user.getById(tempRow!.id, { + force: true, + }); + expect(untouched!.email).toBeNull(); + expect(untouched!.password).toBeNull(); + }); }); // ── Password recovery flow ────────────────────────────────────────── diff --git a/src/backend/controllers/auth/AuthController.ts b/src/backend/controllers/auth/AuthController.ts index 28224c67b..0d359c31a 100644 --- a/src/backend/controllers/auth/AuthController.ts +++ b/src/backend/controllers/auth/AuthController.ts @@ -52,6 +52,8 @@ import { createSecret as otpCreateSecret, verify as verifyOtp, } from '../../services/auth/OTPUtil.js'; +import type { UserRow } from '../../stores/user/UserStore.js'; +import { isOwnedEmailConflict } from '../../stores/user/UserStore.js'; import { sessionCookieFlags } from '../../util/cookieFlags.js'; import { cleanEmail, isBlockedEmail } from '../../util/email.js'; import { generate_identifier } from '../../util/identifier.js'; @@ -775,11 +777,9 @@ export class AuthController extends PuterController { if (this.config.disable_user_signup) { let claimable = false; if (!is_temp) { - const existing = - (await this.stores.user.getByEmail(body.email)) ?? - (await this.stores.user.getByCleanEmail( - cleanEmail(body.email), - )); + const existing = await this.stores.user.findEmailOwner( + body.email, + ); claimable = Boolean( existing && !existing.email_confirmed && @@ -817,28 +817,18 @@ export class AuthController extends PuterController { // password to an OIDC account, the owner logs in via OIDC and // uses the authenticated change-password flow. // - // Match on both raw `email` and canonical `clean_email` so + // Matching runs against both raw `email` and canonical `clean_email` so // gmail-style aliases (`foo.bar+tag@gmail.com` vs // `foobar@gmail.com`) collapse to the same account. - let pseudo_user = null; - if (!is_temp) { - const canonical = cleanEmail(body.email); - const existing = - (await this.stores.user.getByEmail(body.email)) ?? - (await this.stores.user.getByCleanEmail(canonical)); - if (existing) { - // Confirmed account (regardless of credential type) → reject. - if (existing.email_confirmed || existing.password !== null) { - throw new HttpError( - 400, - 'This email already exists in our database. Please use another one.', - { legacyCode: 'bad_request' }, - ); - } - // Password-null AND unconfirmed → treat as pseudo. - pseudo_user = existing; - } - } + // + // This is the cheap early check: it keeps an obvious duplicate from + // paying for the validate hook and a bcrypt round. It is NOT the + // guarantee — everything between here and the insert widens the window, + // so the check runs again against the primary immediately before the + // write, and the unique index catches whatever still slips through. + let pseudo_user = is_temp + ? null + : await this.#resolveSignupEmailClaim(body.email); // Extension-level validation gate. Abuse-prevention extensions // inspect the incoming signup and can: @@ -945,24 +935,54 @@ export class AuthController extends PuterController { .slice(0, 19) .replace('T', ' '); + // Re-run the claim against the primary now that the slow work is done. + // The check above ran before the validate hook (network round-trips to + // the abuse listeners) and before bcrypt — hundreds of milliseconds in + // which a concurrent signup can take the address, or claim the very + // placeholder row we were about to convert. + if (!is_temp) { + pseudo_user = await this.#resolveSignupEmailClaim(body.email, { + force: true, + }); + } + let user; if (pseudo_user) { // -- Pseudo-user claim (convert the placeholder row) -- - await this.stores.user.update(pseudo_user.id, { - username: body.username, - password: password_hash, - uuid: user_uuid, - email_confirm_code, - email_confirm_token, - email_confirmed: 0, - requires_email_confirmation: 1, - last_activity_ts: signupSqlTs, - ...(validateEvent.reputation != null - ? { reputation: validateEvent.reputation } - : {}), - requires_phone_verification: force_phone_verification ? 1 : 0, - requires_card_verification: force_card_verification ? 1 : 0, - }); + // + // Guarded, not a plain update: the address never changes hands here + // (the row already holds it), so the unique index has nothing to + // catch. Two signups that both read this row as claimable would + // otherwise both "succeed", the second overwriting the first's + // username and password on a row the first was already given a + // session for. + const claimed = await this.stores.user.claimPlaceholder( + pseudo_user.id, + { + username: body.username, + password: password_hash, + uuid: user_uuid, + email_confirm_code, + email_confirm_token, + email_confirmed: 0, + requires_email_confirmation: 1, + last_activity_ts: signupSqlTs, + ...(validateEvent.reputation != null + ? { reputation: validateEvent.reputation } + : {}), + requires_phone_verification: force_phone_verification + ? 1 + : 0, + requires_card_verification: force_card_verification ? 1 : 0, + }, + ); + if (!claimed) { + throw new HttpError( + 400, + 'This email already exists in our database. Please use another one.', + { legacyCode: 'bad_request' }, + ); + } // Move from temp group to regular user group if (this.config.default_temp_group) { @@ -994,37 +1014,51 @@ export class AuthController extends PuterController { const clientIp = req.ip || req.socket?.remoteAddress || null; const proxyIpChain = req.headers['x-forwarded-for']; - user = await this.stores.user.create({ - username: body.username, - uuid: user_uuid, - password: password_hash, - email: is_temp ? null : body.email, - clean_email: is_temp ? null : cleanEmail(body.email), - free_storage: this.config.storage_capacity ?? null, - requires_email_confirmation: - !is_temp || force_email_confirmation, - email_confirm_code, - email_confirm_token, - audit_metadata: { - ip: clientIp, - ip_fwd: proxyIpChain, - user_agent: req.headers?.['user-agent'], - origin: req.headers?.origin, - fingerprint, - }, - signup_ip: clientIp, - signup_ip_forwarded: proxyIpChain, - signup_user_agent: req.headers?.['user-agent'] ?? null, - signup_origin: (req.headers?.origin as string | null) ?? null, - signup_server: (this.config as { serverId?: string }).serverId, - referrer: req.body.referrer ?? null, - last_activity_ts: signupSqlTs, - reputation: validateEvent.reputation, - // Phone collected later in the verification dialog (null now). - phone: null, - requires_phone_verification: force_phone_verification, - requires_card_verification: force_card_verification, - } as never); + try { + user = await this.stores.user.create({ + username: body.username, + uuid: user_uuid, + password: password_hash, + email: is_temp ? null : body.email, + clean_email: is_temp ? null : cleanEmail(body.email), + free_storage: this.config.storage_capacity ?? null, + requires_email_confirmation: + !is_temp || force_email_confirmation, + email_confirm_code, + email_confirm_token, + audit_metadata: { + ip: clientIp, + ip_fwd: proxyIpChain, + user_agent: req.headers?.['user-agent'], + origin: req.headers?.origin, + fingerprint, + }, + signup_ip: clientIp, + signup_ip_forwarded: proxyIpChain, + signup_user_agent: req.headers?.['user-agent'] ?? null, + signup_origin: + (req.headers?.origin as string | null) ?? null, + signup_server: (this.config as { serverId?: string }) + .serverId, + referrer: req.body.referrer ?? null, + last_activity_ts: signupSqlTs, + reputation: validateEvent.reputation, + // Phone collected later in the verification dialog (null now). + phone: null, + requires_phone_verification: force_phone_verification, + requires_card_verification: force_card_verification, + } as never); + } catch (e) { + // Lost the race to another signup between the re-check above and + // this insert. The index is the only thing that can see that, so + // translate it into the answer the pre-check would have given. + if (!isOwnedEmailConflict(e)) throw e; + throw new HttpError( + 400, + 'This email already exists in our database. Please use another one.', + { legacyCode: 'bad_request' }, + ); + } // Add to default group const defaultGroup = is_temp @@ -1101,7 +1135,8 @@ export class AuthController extends PuterController { is_temp: user!.password === null && user!.email === null, ip: (req?.headers?.['x-forwarded-for'] as - string | undefined) || + | string + | undefined) || ( req as unknown as { connection?: { remoteAddress?: string }; @@ -1274,6 +1309,34 @@ export class AuthController extends PuterController { // after signup but before confirmation. await this.#validateEmail(user.email!); + // An account that already confirmed this address proved access to the + // inbox, and revoking it below would hand the address to whoever + // confirmed second. Refuse instead — a duplicate this old is data to + // repair, not a race to resolve. + const canonical = cleanEmail(user.email!); + const confirmedRival = await this.stores.user.findConfirmedOtherByEmail( + user.id, + user.email!, + canonical, + ); + if (confirmedRival) { + throw new HttpError( + 400, + 'This email was confirmed on a different account.', + { legacyCode: 'email_already_in_use' as never }, + ); + } + + // Revoke the address from every remaining (unconfirmed) account holding + // it, THEN confirm this one. Only one row may own an address, so + // confirming first would momentarily create a second owner — which the + // unique index rejects, turning a legitimate confirmation into a 500. + await this.stores.user.unconfirmOthersByEmail( + user.id, + user.email!, + canonical, + ); + await this.stores.user.update(user.id, { email_confirmed: 1, requires_email_confirmation: 0, @@ -1281,16 +1344,6 @@ export class AuthController extends PuterController { email_confirm_token: null, }); - // Revoke confirmation from any other accounts sharing this - // email so only the account whose owner just proved inbox - // access retains verified status. - const canonical = cleanEmail(user.email!); - await this.stores.user.unconfirmOthersByEmail( - user.id, - user.email!, - canonical, - ); - await promoteToVerifiedGroup(this.stores.group, this.config, user); try { @@ -2277,10 +2330,26 @@ export class AuthController extends PuterController { // Atomic check: only update if the recovery token still matches const password_hash = await bcrypt.hash(password, 8); - const result = await this.clients.db.write( - 'UPDATE `user` SET `password` = ?, `pass_recovery_token` = NULL, `change_email_confirm_token` = NULL WHERE `id` = ? AND `pass_recovery_token` = ?', - [password_hash, user.id, decoded.token], - ); + let result; + try { + result = await this.clients.db.write( + 'UPDATE `user` SET `password` = ?, `pass_recovery_token` = NULL, `change_email_confirm_token` = NULL WHERE `id` = ? AND `pass_recovery_token` = ?', + [password_hash, user.id, decoded.token], + ); + } catch (e) { + if (!isOwnedEmailConflict(e)) throw e; + // Recovery can be requested by username, so this row may be an + // unconfirmed placeholder that shares its address with a real + // account. Giving it a password would make it a second account able + // to drive recovery for that inbox, which is the thing the address + // constraint exists to stop. The inbox owner has an account + // already — they should be recovering that one. + throw new HttpError( + 400, + 'This email is already in use. Recover the account that uses it instead.', + { legacyCode: 'email_already_in_use' as never }, + ); + } const affected = (result as { affectedRows?: number; changes?: number }) ?.affectedRows ?? @@ -2441,10 +2510,7 @@ export class AuthController extends PuterController { // aliases — which is also why the caller has to be excluded: an // alias of your own current address resolves back to you, and // "already in use" about yourself is nonsense. - const canonical = cleanEmail(new_email); - const existing = - (await this.stores.user.getByEmail(new_email)) ?? - (await this.stores.user.getByCleanEmail(canonical)); + const existing = await this.stores.user.findEmailOwner(new_email); if ( existing && existing.id !== req.actor!.user.id && @@ -2545,7 +2611,7 @@ export class AuthController extends PuterController { } const rows = (await this.clients.db.read( - 'SELECT * FROM `user` WHERE `change_email_confirm_token` = ? LIMIT 1', + 'SELECT * FROM `user` WHERE `change_email_confirm_token` = ? ORDER BY `id` ASC LIMIT 1', [decoded.token], )) as Array>; const user = rows[0] as @@ -2566,11 +2632,12 @@ export class AuthController extends PuterController { // Re-check nobody claimed the new email meanwhile. Match raw + // canonical; block if any real account (confirmed OR - // password-holding) already owns it. + // password-holding) already owns it. Read the primary — the request + // that took the address may have landed moments ago. const canonical = cleanEmail(newEmail); - const owner = - (await this.stores.user.getByEmail(newEmail)) ?? - (await this.stores.user.getByCleanEmail(canonical)); + const owner = await this.stores.user.findEmailOwner(newEmail, { + force: true, + }); if ( owner && owner.id !== user.id && @@ -2581,15 +2648,30 @@ export class AuthController extends PuterController { }); } - await this.stores.user.update(user.id, { - email: newEmail, - clean_email: cleanEmail(newEmail), - unconfirmed_change_email: null, - change_email_confirm_token: null, - pass_recovery_token: null, - email_confirmed: 1, - requires_email_confirmation: 0, - }); + // Strip the address off any unconfirmed placeholder still holding it + // before taking it, so this row is the only owner. + await this.stores.user.unconfirmOthersByEmail( + user.id, + newEmail, + canonical, + ); + + try { + await this.stores.user.update(user.id, { + email: newEmail, + clean_email: canonical, + unconfirmed_change_email: null, + change_email_confirm_token: null, + pass_recovery_token: null, + email_confirmed: 1, + requires_email_confirmation: 0, + }); + } catch (e) { + if (!isOwnedEmailConflict(e)) throw e; + throw new HttpError(400, 'This email is already in use.', { + legacyCode: 'email_already_in_use' as never, + }); + } await this.stores.oidc.unlinkAllByUserId(user.id); @@ -2692,9 +2774,7 @@ export class AuthController extends PuterController { // reject on ANY confirmed account (OIDC accounts have // password=null but are real) — not just password-holders. const canonical = cleanEmail(email); - const existingEmail = - (await this.stores.user.getByEmail(email)) ?? - (await this.stores.user.getByCleanEmail(canonical)); + const existingEmail = await this.stores.user.findEmailOwner(email); if ( existingEmail && existingEmail.id !== user.id && @@ -2710,16 +2790,38 @@ export class AuthController extends PuterController { const email_confirm_code = String(crypto.randomInt(100000, 1000000)); const email_confirm_token = uuidv4(); - await this.stores.user.update(user.id, { - username, - email, - clean_email: cleanEmail(email), - password: password_hash, - email_confirm_code, - email_confirm_token, - email_confirmed: 0, - requires_email_confirmation: 1, + // bcrypt above is slow enough for someone else to take the address in + // the meantime, so re-check against the primary before the write. + const raced = await this.stores.user.findEmailOwner(email, { + force: true, }); + if ( + raced && + raced.id !== user.id && + (raced.email_confirmed || raced.password !== null) + ) { + throw new HttpError(400, 'This email is already in use.', { + legacyCode: 'email_already_in_use' as never, + }); + } + + try { + await this.stores.user.update(user.id, { + username, + email, + clean_email: canonical, + password: password_hash, + email_confirm_code, + email_confirm_token, + email_confirmed: 0, + requires_email_confirmation: 1, + }); + } catch (e) { + if (!isOwnedEmailConflict(e)) throw e; + throw new HttpError(400, 'This email is already in use.', { + legacyCode: 'email_already_in_use' as never, + }); + } // Rename the user's FS home so `//Desktop` etc. // become `//Desktop`. Without this cascade, any @@ -4425,57 +4527,7 @@ export class AuthController extends PuterController { // -- Private helpers ---------------------------------------------- async #cascadeDeleteUser(userId: number): Promise { - // Capture the identifiers downstream teardown needs before the row is - // gone — the marketplace extension cancels the user's Stripe - // subscriptions off `user.delete`, keyed by uuid / customer id. - let userUuid: string | undefined; - let stripeCustomerId: string | null = null; - try { - const rows = (await this.clients.db.read( - 'SELECT `uuid`, `stripe_customer_id` FROM `user` WHERE `id` = ?', - [userId], - )) as Array<{ uuid?: string; stripe_customer_id?: string | null }>; - userUuid = rows[0]?.uuid; - stripeCustomerId = rows[0]?.stripe_customer_id ?? null; - } catch (e) { - console.warn('[cascade-delete-user] identifier lookup failed:', e); - } - - try { - await this.services.fs.removeAllForUser(userId); - } catch (e) { - // Proceed with user-row delete anyway — orphaned fsentries are - // better than a resurrected account. - console.warn('[cascade-delete-user] fs cleanup failed:', e); - } - - // Sessions FK is SET NULL, so delete explicitly to avoid dangling rows. - await this.clients.db.write( - 'DELETE FROM `sessions` WHERE `user_id` = ?', - [userId], - ); - await this.clients.db.write('DELETE FROM `user` WHERE `id` = ?', [ - userId, - ]); - await this.stores.user.invalidateById(userId); - - // Fire-and-forget: let listeners purge external state tied to the - // account (Stripe subscriptions are cancelled immediately, without - // proration). Emitted after the row delete — listeners key off the - // payload, not the DB row. - try { - this.clients.event?.emit( - 'user.delete', - { - user_id: userId, - user_uuid: userUuid, - stripe_customer_id: stripeCustomerId, - }, - {}, - ); - } catch { - // ignore — event emission shouldn't block deletion - } + await this.services.userAccount.cascadeDelete(userId); } async #generateRandomUsername(): Promise { @@ -4494,6 +4546,34 @@ export class AuthController extends PuterController { return username; } + /** + * Decide whether a signup may take `email`, and hand back the placeholder + * row it should convert instead of inserting a new one. + * + * Throws when a live account already owns the address. Returns the + * unconfirmed, password-less pseudo row when one exists (admin + * pre-provisioning — signup claims it), or null when the address is free. + * + * Called twice per signup: once early, to fail fast before the validate + * hook and bcrypt, and once against the primary immediately before the + * write. + */ + async #resolveSignupEmailClaim( + email: string, + opts: { force?: boolean } = {}, + ): Promise { + const existing = await this.stores.user.findEmailOwner(email, opts); + if (!existing) return null; + if (existing.email_confirmed || existing.password !== null) { + throw new HttpError( + 400, + 'This email already exists in our database. Please use another one.', + { legacyCode: 'bad_request' }, + ); + } + return existing; + } + /** * Config-blocklist + extension-driven email validation. Config blocklist * (suffix match on cleaned email) blocks first; then the `email.validate` diff --git a/src/backend/controllers/oidc/OIDCController.ts b/src/backend/controllers/oidc/OIDCController.ts index 60df9d145..cce1700fa 100644 --- a/src/backend/controllers/oidc/OIDCController.ts +++ b/src/backend/controllers/oidc/OIDCController.ts @@ -661,6 +661,7 @@ if (window.opener) { provider: string, userinfo: { sub: string; email?: unknown; [k: string]: unknown }, referrer?: string | null, + attempt = 0, ): Promise< | { error: string; code?: string; requestCode?: string } | { @@ -679,8 +680,12 @@ if (window.opener) { const claimedEmail = typeof userinfo.email === 'string' ? userinfo.email : null; if (claimedEmail) { - const byEmail = - await this.services.oidc.findUserByEmail(claimedEmail); + // On the retry after a lost race, read the primary — the winning + // row may be younger than the replica snapshot. + const byEmail = await this.services.oidc.findUserByEmail( + claimedEmail, + { force: attempt > 0 }, + ); if (byEmail) { if (!byEmail.email_confirmed) { return { @@ -707,6 +712,18 @@ if (window.opener) { userinfo as { sub: string; email?: string }, referrer, ); + // A concurrent callback (a second tab, a provider retry) created the + // account between step 2 and the insert. Nothing went wrong for the + // user — start over and we'll find the winner at step 1 or 2. One retry + // only: a second miss means something other than a race is going on. + if (outcome.raced && attempt === 0) { + return this.#resolveOrCreateOIDCUser( + provider, + userinfo, + referrer, + attempt + 1, + ); + } if (!outcome.success || !outcome.user) { return { error: outcome.error ?? 'Account creation failed.', diff --git a/src/backend/controllers/static/StaticPagesController.ts b/src/backend/controllers/static/StaticPagesController.ts index 036cbd4f4..8565990ee 100644 --- a/src/backend/controllers/static/StaticPagesController.ts +++ b/src/backend/controllers/static/StaticPagesController.ts @@ -280,15 +280,19 @@ export class StaticPagesController extends PuterController { (user.clean_email as string | null | undefined) ?? String(user.email ?? '').toLowerCase(); - const [dupe] = (await this.clients.db.read( - `SELECT EXISTS( - SELECT 1 FROM \`user\` WHERE (\`email\` = ? OR \`clean_email\` = ?) - AND \`email_confirmed\` = ${this.clients.db.booleanLiteral(true)} - AND \`password\` IS NOT NULL - ) AS email_exists`, - [user.email, cleanEmail], - )) as Array<{ email_exists: number }>; - if (dupe?.email_exists) { + // An account that already confirmed this address proved access + // to the inbox. The strip below would take it away from them, + // so refuse here instead. Password-less accounts count: an + // identity provider verified the address for those, and they + // are exactly what the old `password IS NOT NULL` clause let + // through. + const confirmedRival = + await this.stores.user.findConfirmedOtherByEmail( + user.id as number, + user.email as string, + cleanEmail, + ); + if (confirmedRival) { res.send( err('This email was confirmed on a different account.'), ); @@ -302,6 +306,15 @@ export class StaticPagesController extends PuterController { [user.email], ); + // Take the address off every remaining row before confirming + // this one. The check above leaves only unconfirmed rows, and + // only one row may own an address once this one is confirmed. + await this.stores.user.unconfirmOthersByEmail( + user.id, + user.email as string, + cleanEmail, + ); + await this.stores.user.update(user.id, { email_confirmed: 1, requires_email_confirmation: 0, diff --git a/src/backend/services/auth/OIDCService.test.ts b/src/backend/services/auth/OIDCService.test.ts index 9b4f5b53b..4eb325fe3 100644 --- a/src/backend/services/auth/OIDCService.test.ts +++ b/src/backend/services/auth/OIDCService.test.ts @@ -300,6 +300,110 @@ describe('OIDCService.createUserFromOIDC', () => { oidcConfig.disable_user_signup = prev; } }); + + // Two callbacks for the same brand-new identity — a second tab, a provider + // retry — used to each create an account and each link the same sub, since + // neither the address nor the sub was constrained. Later sign-ins then + // resolved to whichever row came back first. + it('creates exactly one account for two simultaneous callbacks', async () => { + const email = `race-${crypto.randomBytes(4).toString('hex')}@corp.example`; + const sub = `race-sub-${crypto.randomBytes(4).toString('hex')}`; + + const results = await Promise.all([ + runWithContext({ req }, () => + oidc().createUserFromOIDC('custom-idp', { + sub, + email, + email_verified: true, + }), + ), + runWithContext({ req }, () => + oidc().createUserFromOIDC('custom-idp', { + sub, + email, + email_verified: true, + }), + ), + ]); + + expect(results.filter((r) => r.success)).toHaveLength(1); + // The loser reports a race, not an error — the caller re-resolves onto + // the winner rather than showing the user a failure. + const loser = results.find((r) => !r.success)!; + expect(loser.raced).toBe(true); + expect(loser.error).toBeUndefined(); + + const owners = (await server.clients.db.read( + 'SELECT COUNT(*) AS n FROM `user` WHERE `email` = ?', + [email], + )) as Array<{ n: number }>; + expect(Number(owners[0].n)).toBe(1); + + // And exactly one link, so getByProviderSub cannot flip between + // accounts on subsequent sign-ins. + const links = (await server.clients.db.read( + 'SELECT COUNT(*) AS n FROM `user_oidc_providers` WHERE `provider_sub` = ?', + [sub], + )) as Array<{ n: number }>; + expect(Number(links[0].n)).toBe(1); + }); + + it('reports a race rather than a failure when the address is already taken', async () => { + const email = `taken-${crypto.randomBytes(4).toString('hex')}@corp.example`; + await server.stores.user.create({ + username: `taken-${crypto.randomBytes(4).toString('hex')}`, + uuid: crypto.randomUUID(), + password: 'hashed', + email, + clean_email: email, + }); + + const result = await runWithContext({ req }, () => + oidc().createUserFromOIDC('custom-idp', { + sub: `taken-sub-${crypto.randomBytes(4).toString('hex')}`, + email, + email_verified: true, + }), + ); + + expect(result.success).toBe(false); + expect(result.raced).toBe(true); + }); + + it('leaves no orphan account behind when the identity was linked first', async () => { + // The sub is already bound to another account, so `link()` throws after + // this call has created its own user. That account can never be signed + // in to, so it must not survive. + const sub = `orphan-sub-${crypto.randomBytes(4).toString('hex')}`; + const incumbent = await server.stores.user.create({ + username: `incumbent-${crypto.randomBytes(4).toString('hex')}`, + uuid: crypto.randomUUID(), + password: null, + email: `incumbent-${crypto.randomBytes(4).toString('hex')}@corp.example`, + }); + await server.stores.oidc.link(incumbent.id, 'custom-idp', sub, null); + + const email = `orphan-${crypto.randomBytes(4).toString('hex')}@corp.example`; + const before = (await server.clients.db.read( + 'SELECT COUNT(*) AS n FROM `user`', + )) as Array<{ n: number }>; + + const result = await runWithContext({ req }, () => + oidc().createUserFromOIDC('custom-idp', { + sub, + email, + email_verified: true, + }), + ); + + expect(result.success).toBe(false); + expect(result.raced).toBe(true); + const after = (await server.clients.db.read( + 'SELECT COUNT(*) AS n FROM `user`', + )) as Array<{ n: number }>; + expect(Number(after[0].n)).toBe(Number(before[0].n)); + expect(await server.stores.user.getByEmail(email)).toBeNull(); + }); }); describe('OIDCService.linkProviderToUser', () => { diff --git a/src/backend/services/auth/OIDCService.ts b/src/backend/services/auth/OIDCService.ts index f59c681f9..f506a4062 100644 --- a/src/backend/services/auth/OIDCService.ts +++ b/src/backend/services/auth/OIDCService.ts @@ -20,6 +20,7 @@ import type { LayerInstances } from '../../types'; import type { puterServices } from '../index'; import type { UserRow } from '../../stores/user/UserStore'; +import { isOwnedEmailConflict } from '../../stores/user/UserStore.js'; import { PuterService } from '../types'; import { cleanEmail, isBlockedEmail } from '../../util/email.js'; import { generate_identifier } from '../../util/identifier.js'; @@ -399,11 +400,11 @@ export class OIDCService extends PuterService { * signed up as `foobar@gmail.com`. Primary email is preferred over a * clean_email collision. */ - async findUserByEmail(email: string): Promise { - if (!email) return null; - const direct = await this.stores.user.getByEmail(email); - if (direct) return direct; - return this.stores.user.getByCleanEmail(cleanEmail(email)); + async findUserByEmail( + email: string, + opts: { force?: boolean } = {}, + ): Promise { + return this.stores.user.findEmailOwner(email, opts); } /** @@ -451,6 +452,10 @@ export class OIDCService extends PuterService { /** * Create a new Puter user from OIDC claims and link the provider. Returns * `{ success, user, error? }`. + * + * `raced` means a concurrent callback got there first and the caller should + * re-resolve rather than surface an error — see + * `#resolveOrCreateOIDCUser`. */ async createUserFromOIDC( providerId: string, @@ -461,6 +466,7 @@ export class OIDCService extends PuterService { user?: UserRow; error?: string; code?: string; + raced?: boolean; /** * Support-correlation id for a vetoed signup (the abuse trail id). Safe * to show the user; the veto reason in `error` is not. @@ -600,45 +606,61 @@ export class OIDCService extends PuterService { Boolean(validateEvent.requires_card_verification) || Boolean(cfg.always_require_card_verification); - const created = await this.stores.user.create({ - username, - uuid: uuidv4(), - password: null, - email, - clean_email: cleanEmail(email), - free_storage: this.config.storage_capacity ?? null, - // Email is provider-verified, so the email step is always skipped; - // the phone/card gates still apply when the harness flagged them. - requires_email_confirmation: false, - requires_phone_verification: force_phone_verification, - requires_card_verification: force_card_verification, - ...(validateEvent.reputation != null - ? { reputation: validateEvent.reputation } - : {}), - audit_metadata: { - ip: clientIp, - ip_fwd: proxyIpChain, - user_agent: req?.headers?.['user-agent'], - origin: req?.headers?.origin, - }, - signup_ip: clientIp, - signup_ip_forwarded: proxyIpChain, - signup_user_agent: req?.headers?.['user-agent'] ?? null, - signup_origin: req?.headers?.origin, - signup_server: this.config.serverId, - referrer: referrer ?? null, - }); + // The caller checked this email was free before we got here, but the + // validate hook and the blocklist checks above sit in between — long + // enough for a second callback (another tab, a provider retry) to have + // created the account. Re-check against the primary, and let the unique + // index catch anything still in flight. + if (await this.stores.user.findEmailOwner(email, { force: true })) { + return { success: false, raced: true }; + } + + let created: UserRow; + try { + created = await this.stores.user.create({ + username, + uuid: uuidv4(), + password: null, + email, + clean_email: cleanEmail(email), + free_storage: this.config.storage_capacity ?? null, + // Email is provider-verified, so the email step is always + // skipped; the phone/card gates still apply when the harness + // flagged them. + requires_email_confirmation: false, + // Confirmed in the INSERT rather than a follow-up update: an + // unconfirmed, password-less row does not own its address, so + // deferring this would let two concurrent callbacks both insert + // and only collide when they confirm — too late to report as a + // race. + email_confirmed: true, + requires_phone_verification: force_phone_verification, + requires_card_verification: force_card_verification, + ...(validateEvent.reputation != null + ? { reputation: validateEvent.reputation } + : {}), + audit_metadata: { + ip: clientIp, + ip_fwd: proxyIpChain, + user_agent: req?.headers?.['user-agent'], + origin: req?.headers?.origin, + }, + signup_ip: clientIp, + signup_ip_forwarded: proxyIpChain, + signup_user_agent: req?.headers?.['user-agent'] ?? null, + signup_origin: req?.headers?.origin, + signup_server: this.config.serverId, + referrer: referrer ?? null, + }); + } catch (e) { + if (!isOwnedEmailConflict(e)) throw e; + return { success: false, raced: true }; + } if (!created) { return { success: false, error: 'User creation failed.' }; } - // Mark email as confirmed (OIDC provider already verified it). - await this.stores.user.update(created.id, { - email_confirmed: 1, - requires_email_confirmation: 0, - }); - // Default user group — OIDC users skip the temp group entirely since // the email is already verified by the IdP. const defaultGroup = this.config.default_user_group; @@ -665,7 +687,30 @@ export class OIDCService extends PuterService { // Link OIDC provider (after provisioning so a failed link doesn't // leave an orphaned user without a home folder). - await this.stores.oidc.link(created.id, providerId, claims.sub, null); + // + // A 409 here means a concurrent callback for the same identity bound the + // sub to its own new account while we were provisioning. That leaves us + // holding an account nobody can ever sign in to, so tear it down and let + // the caller re-resolve onto the winner. + try { + await this.stores.oidc.link( + created.id, + providerId, + claims.sub, + null, + ); + } catch (e) { + if ((e as { statusCode?: number })?.statusCode !== 409) throw e; + try { + await this.services.userAccount.cascadeDelete(created.id); + } catch (cleanupError) { + console.warn( + '[oidc] failed to clean up raced account:', + cleanupError, + ); + } + return { success: false, raced: true }; + } // Re-read so callers see email_confirmed / *_uuid / *_id fields // written above. diff --git a/src/backend/services/index.ts b/src/backend/services/index.ts index ce93cc308..d6e033163 100644 --- a/src/backend/services/index.ts +++ b/src/backend/services/index.ts @@ -39,6 +39,7 @@ import { DefaultUserService } from './selfhosted/DefaultUserService'; import { SocketService } from './socket/SocketService'; import { SubdomainPermissionService } from './subdomain/SubdomainPermissionService'; import type { IPuterServiceRegistry } from './types'; +import { UserAccountService } from './user/UserAccountService'; /** * Populate `IPuterServiceInstances` (declared in `./types`) with the concrete @@ -69,6 +70,7 @@ declare module './types' { defaultUser: DefaultUserService; homepage: PuterHomepageService; health: ServerHealthService; + userAccount: UserAccountService; } } @@ -91,6 +93,9 @@ export const puterServices = { token: TokenService, auth: AuthService, fs: FSService, + // Declared after `fs` — account teardown tears the user's filesystem down + // first. + userAccount: UserAccountService, // AppPermissionService + SubdomainPermissionService register permission // rewriters/implicators only; no runtime state. Placed after fsEntry so // the FS rewriter runs first for `fs:/path` → `fs:` before any diff --git a/src/backend/services/user/UserAccountService.test.ts b/src/backend/services/user/UserAccountService.test.ts new file mode 100644 index 000000000..3dd19944b --- /dev/null +++ b/src/backend/services/user/UserAccountService.test.ts @@ -0,0 +1,206 @@ +/* + * Copyright (C) 2024-present Puter Technologies Inc. + * + * This file is part of Puter. + * + * Puter is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published + * by the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; +import { v4 as uuidv4 } from 'uuid'; +import { setupTestServer } from '../../testUtil.ts'; +import { PuterServer } from '../../server.ts'; +import { generateDefaultFsentries } from '../../util/userProvisioning.ts'; + +describe('UserAccountService', () => { + let server: PuterServer; + + beforeAll(async () => { + server = await setupTestServer(); + }); + + afterAll(async () => { + await server?.shutdown(); + }); + + const seedUser = async (overrides: Record = {}) => { + const slug = Math.random().toString(36).slice(2, 10); + return server.stores.user.create({ + username: `ua_${slug}`, + uuid: uuidv4(), + password: 'hashed', + email: `ua-${slug}@test.local`, + clean_email: `ua-${slug}@test.local`, + ...overrides, + }); + }; + + describe('getUsageSignals', () => { + it('reports a freshly created account as unused', async () => { + const user = await seedUser(); + const usage = await server.services.userAccount.getUsageSignals( + user.id, + ); + expect(usage.inUse).toBe(false); + expect(usage.signals).toEqual([]); + }); + + it('still reads as unused with only the folders signup provisions', async () => { + // The whole delete decision hinges on this: a provisioned account + // nobody ever opened must not look like somebody's files. + const user = await seedUser(); + await generateDefaultFsentries( + server.clients.db, + server.stores.user, + user, + ); + const usage = await server.services.userAccount.getUsageSignals( + user.id, + ); + expect(usage.signals).not.toContain('files'); + expect(usage.inUse).toBe(false); + }); + + it('reports files once anything beyond the provisioned set exists', async () => { + const user = await seedUser(); + await generateDefaultFsentries( + server.clients.db, + server.stores.user, + user, + ); + const fresh = (await server.stores.user.getById(user.id, { + force: true, + }))!; + const now = Math.floor(Date.now() / 1000); + await server.clients.db.write( + 'INSERT INTO `fsentries` (`uuid`, `parent_uid`, `user_id`, `name`, `is_dir`, `created`, `modified`) VALUES (?, ?, ?, ?, ?, ?, ?)', + [ + uuidv4(), + fresh.desktop_uuid, + user.id, + 'notes.txt', + 0, + now, + now, + ], + ); + + const usage = await server.services.userAccount.getUsageSignals( + user.id, + ); + expect(usage.signals).toContain('files'); + expect(usage.inUse).toBe(true); + }); + + // `stripe_customer_id` is deliberately absent: it is a prod-only column + // the self-hosted schema does not carry, which is exactly why the + // service reads the row through the store instead of naming columns. + it.each([ + ['card_fingerprint', 'fp_123', 'card-verified'], + ['phone', '+14155550100', 'phone-verified'], + ])('reports %s as %s', async (column, value, signal) => { + const user = await seedUser(); + await server.stores.user.update(user.id, { [column]: value }); + const usage = await server.services.userAccount.getUsageSignals( + user.id, + ); + expect(usage.signals).toContain(signal); + expect(usage.inUse).toBe(true); + }); + + it('reports an external identity link', async () => { + const user = await seedUser(); + await server.stores.oidc.link( + user.id, + 'custom-idp', + `sub-${uuidv4()}`, + null, + ); + const usage = await server.services.userAccount.getUsageSignals( + user.id, + ); + expect(usage.signals).toContain('oidc-link'); + }); + + it('errs toward in-use when a signal query fails', async () => { + // A signal we cannot read is not a signal that is absent. Failing + // open here would mean an unreadable table makes accounts look + // deletable, and the collapse job deletes what looks unused. + const user = await seedUser(); + const original = server.clients.db.read.bind(server.clients.db); + const read = vi + .spyOn(server.clients.db, 'read') + .mockImplementation(async (sql: string, params?: unknown[]) => { + // Only the capped-count probes fail; the row read still + // works, which is the realistic "one table is unhappy" case. + if (sql.includes('FROM (SELECT 1 FROM')) { + throw new Error('table gone'); + } + return original(sql, params); + }); + try { + const usage = await server.services.userAccount.getUsageSignals( + user.id, + ); + expect(usage.inUse).toBe(true); + expect(usage.signals).toContain('sessions'); + } finally { + read.mockRestore(); + } + }); + }); + + describe('cascadeDelete', () => { + it('removes the row, its sessions and its cache entries', async () => { + const user = await seedUser(); + await server.clients.db.write( + 'INSERT INTO `sessions` (`uuid`, `user_id`, `created_at`, `last_activity`) VALUES (?, ?, ?, ?)', + [uuidv4(), user.id, Date.now(), Date.now()], + ); + // Warm the address-keyed cache entry so a stale hit would show up. + expect( + (await server.stores.user.getByEmail(user.email as string))?.id, + ).toBe(user.id); + + await server.services.userAccount.cascadeDelete(user.id); + + expect(await server.stores.user.getById(user.id)).toBeNull(); + expect( + await server.stores.user.getByEmail(user.email as string), + ).toBeNull(); + const sessions = (await server.clients.db.read( + 'SELECT COUNT(*) AS n FROM `sessions` WHERE `user_id` = ?', + [user.id], + )) as Array<{ n: number }>; + expect(Number(sessions[0].n)).toBe(0); + }); + + it('frees the address for a new account', async () => { + const user = await seedUser(); + const email = user.email as string; + await server.services.userAccount.cascadeDelete(user.id); + + // The unique index would reject this if the row survived. + await expect( + server.stores.user.create({ + username: `ua_reuse_${Math.random().toString(36).slice(2, 8)}`, + uuid: uuidv4(), + password: 'hashed', + email, + clean_email: email, + }), + ).resolves.toBeTruthy(); + }); + }); +}); diff --git a/src/backend/services/user/UserAccountService.ts b/src/backend/services/user/UserAccountService.ts new file mode 100644 index 000000000..dc886bd94 --- /dev/null +++ b/src/backend/services/user/UserAccountService.ts @@ -0,0 +1,180 @@ +/* + * Copyright (C) 2024-present Puter Technologies Inc. + * + * This file is part of Puter. + * + * Puter is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published + * by the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +import { PuterService } from '../types.js'; + +/** + * Home directory plus the seven folders `generateDefaultFsentries` creates. An + * account with no more than these has never had a file put in it. + */ +const PROVISIONED_FSENTRY_COUNT = 8; + +/** + * Account-lifecycle operations that more than one caller needs: deleting an + * account and everything hanging off it, and measuring whether an account has + * ever actually been used. + * + * Most `user_id` foreign keys are `ON DELETE SET NULL` rather than `CASCADE`, + * so "delete the row" is never the whole job — anything reaching for that + * shortcut leaves orphans behind. Go through `cascadeDelete`. + */ +export class UserAccountService extends PuterService { + /** + * Delete a user and the state that belongs to them: their files (S3 objects + * included), their sessions, and the row itself. + * + * Irreversible. Filesystem teardown failures are logged and stepped over — + * an orphaned fsentry is a smaller problem than an account that half + * survives its own deletion. + */ + async cascadeDelete(userId: number): Promise { + // Capture the identifiers downstream teardown needs before the row is + // gone — the marketplace extension cancels the user's Stripe + // subscriptions off `user.delete`, keyed by uuid / customer id. + let userUuid: string | undefined; + let stripeCustomerId: string | null = null; + try { + const rows = (await this.clients.db.read( + 'SELECT `uuid`, `stripe_customer_id` FROM `user` WHERE `id` = ?', + [userId], + )) as Array<{ uuid?: string; stripe_customer_id?: string | null }>; + userUuid = rows[0]?.uuid; + stripeCustomerId = rows[0]?.stripe_customer_id ?? null; + } catch (e) { + console.warn('[cascade-delete-user] identifier lookup failed:', e); + } + + try { + await this.services.fs.removeAllForUser(userId); + } catch (e) { + // Proceed with user-row delete anyway — orphaned fsentries are + // better than a resurrected account. + console.warn('[cascade-delete-user] fs cleanup failed:', e); + } + + // Sessions FK is SET NULL, so delete explicitly to avoid dangling rows. + await this.clients.db.write( + 'DELETE FROM `sessions` WHERE `user_id` = ?', + [userId], + ); + await this.clients.db.write('DELETE FROM `user` WHERE `id` = ?', [ + userId, + ]); + await this.stores.user.invalidateById(userId); + + // Fire-and-forget: let listeners purge external state tied to the + // account (Stripe subscriptions are cancelled immediately, without + // proration). Emitted after the row delete — listeners key off the + // payload, not the DB row. + try { + this.clients.event?.emit( + 'user.delete', + { + user_id: userId, + user_uuid: userUuid, + stripe_customer_id: stripeCustomerId, + }, + {}, + ); + } catch { + // ignore — event emission shouldn't block deletion + } + } + + /** + * Evidence that an account has been used for something — the signals that + * separate "a row a race created and nobody ever touched" from "somebody's + * account". + * + * Deliberately generous about what counts. A false "in use" costs a row + * that sticks around; a false "unused" destroys somebody's files. + * + * `fsentryCount` is compared against the folders provisioned at signup, so + * an account whose Desktop is still empty reads as untouched. + */ + async getUsageSignals(userId: number): Promise<{ + userId: number; + signals: string[]; + inUse: boolean; + lastActivityTs: string | null; + }> { + // Each of these is a capped count, not a real one: the subquery stops at + // `cap` rows, so a user with a million fsentries costs the same as one + // with nine. We only ever compare against a small threshold. + const cappedCount = async ( + table: string, + column: string, + cap: number, + ): Promise => { + const sql = + `SELECT COUNT(*) AS n FROM ` + + `(SELECT 1 FROM \`${table}\` WHERE \`${column}\` = ? LIMIT ${cap}) t`; + try { + const rows = (await this.clients.db.read(sql, [ + userId, + ])) as Array>; + return Number(rows[0]?.n ?? 0); + } catch (e) { + // A signal we cannot read is not a signal that is absent — + // report it as present so the caller errs toward keeping. + console.warn('[user-usage] signal query failed:', sql, e); + return cap; + } + }; + + // Through the store rather than a hand-written column list: several of + // the columns read below (`stripe_customer_id`, `card_fingerprint`) are + // prod-only additions that a self-hosted schema may not carry, and + // naming them in SQL turns their absence into a thrown query instead of + // an absent signal. + const [ + row, + sessionCount, + appCount, + subdomainCount, + oidcCount, + fsentryCount, + ] = await Promise.all([ + this.stores.user.getById(userId, { force: true }), + cappedCount('sessions', 'user_id', 1), + cappedCount('apps', 'owner_user_id', 1), + cappedCount('subdomains', 'user_id', 1), + cappedCount('user_oidc_providers', 'user_id', 1), + cappedCount('fsentries', 'user_id', PROVISIONED_FSENTRY_COUNT + 1), + ]); + + const signals: string[] = []; + if (sessionCount > 0) signals.push('sessions'); + if (appCount > 0) signals.push('apps'); + if (subdomainCount > 0) signals.push('subdomains'); + if (oidcCount > 0) signals.push('oidc-link'); + if (fsentryCount > PROVISIONED_FSENTRY_COUNT) signals.push('files'); + if (row?.stripe_customer_id) signals.push('stripe-customer'); + if (row?.card_fingerprint) signals.push('card-verified'); + if (row?.phone) signals.push('phone-verified'); + if (row?.last_activity_ts) signals.push('activity'); + + return { + userId, + signals, + inUse: signals.length > 0, + lastActivityTs: (row?.last_activity_ts as string | null) ?? null, + }; + } +} diff --git a/src/backend/stores/oidc/OIDCStore.js b/src/backend/stores/oidc/OIDCStore.js index 85731cc4e..ca004fabc 100644 --- a/src/backend/stores/oidc/OIDCStore.js +++ b/src/backend/stores/oidc/OIDCStore.js @@ -39,8 +39,11 @@ export class OIDCStore extends PuterStore { // -- Reads -------------------------------------------------------- async getByProviderSub(provider, providerSub) { + // Ordered so a sub that predates the UNIQUE index (two callbacks for the + // same new identity used to be able to both insert) always resolves to + // the same link, instead of bouncing the user between two accounts. const rows = await this.clients.db.read( - 'SELECT * FROM `user_oidc_providers` WHERE `provider` = ? AND `provider_sub` = ? LIMIT 1', + 'SELECT * FROM `user_oidc_providers` WHERE `provider` = ? AND `provider_sub` = ? ORDER BY `id` ASC LIMIT 1', [provider, providerSub], ); return rows[0] ?? null; diff --git a/src/backend/stores/user/UserStore.test.ts b/src/backend/stores/user/UserStore.test.ts index 4e5e4436f..eb6ca2752 100644 --- a/src/backend/stores/user/UserStore.test.ts +++ b/src/backend/stores/user/UserStore.test.ts @@ -21,6 +21,8 @@ import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { v4 as uuidv4 } from 'uuid'; import { setupTestServer } from '../../testUtil.ts'; import { PuterServer } from '../../server.ts'; +import { cleanEmail } from '../../util/email.ts'; +import { isOwnedEmailConflict } from './UserStore.ts'; describe('UserStore', () => { let server: PuterServer; @@ -109,19 +111,17 @@ describe('UserStore', () => { const shared = `shared-${Math.random().toString(36).slice(2, 10)}@test.local`; const makeClaimant = async () => { const username = `uc-${Math.random().toString(36).slice(2, 10)}`; - const claimant = await server.stores.user.create({ + return server.stores.user.create({ username, uuid: uuidv4(), password: null, email: shared, clean_email: shared, }); - await server.stores.user.update(claimant.id, { - email_confirmed: true, - }); - return claimant; }; + // Both start as unconfirmed placeholders — the only shape in which two + // rows may hold one address. The winner then confirms it. const loser = await makeClaimant(); const winner = await makeClaimant(); @@ -135,6 +135,7 @@ describe('UserStore', () => { shared, shared, ); + await server.stores.user.update(winner.id, { email_confirmed: true }); const strippedLoser = await server.stores.user.getById(loser.id); expect(strippedLoser?.email).toBeNull(); @@ -144,6 +145,217 @@ describe('UserStore', () => { ); }); + it('refuses a second account owning the same address', async () => { + const shared = `owned-${Math.random().toString(36).slice(2, 10)}@test.local`; + const makeOwner = (password: string | null) => + server.stores.user.create({ + username: `uo-${Math.random().toString(36).slice(2, 10)}`, + uuid: uuidv4(), + password, + email: shared, + clean_email: shared, + }); + + await makeOwner('hashed'); + + // A placeholder may still share the address... + await expect(makeOwner(null)).resolves.toBeTruthy(); + + // ...but a second row that could drive password recovery for that + // inbox may not, however the application-level guards behaved. + await expect(makeOwner('hashed')).rejects.toThrow( + /idx_user_owned_email/, + ); + }); + + it('resolves a shared address to the row that owns it', async () => { + const shared = `pref-${Math.random().toString(36).slice(2, 10)}@test.local`; + const placeholder = await server.stores.user.create({ + username: `up-${Math.random().toString(36).slice(2, 10)}`, + uuid: uuidv4(), + password: null, + email: shared, + clean_email: shared, + }); + const owner = await server.stores.user.create({ + username: `uw-${Math.random().toString(36).slice(2, 10)}`, + uuid: uuidv4(), + password: 'hashed', + email: shared, + clean_email: shared, + }); + + // The placeholder has the lower id, so an unordered LIMIT 1 would be + // free to return it — and login and password recovery both resolve + // through here. + expect(placeholder.id).toBeLessThan(owner.id); + expect( + (await server.stores.user.getByEmail(shared, { cached: false })) + ?.id, + ).toBe(owner.id); + expect((await server.stores.user.findEmailOwner(shared))?.id).toBe( + owner.id, + ); + }); + + it('keeps a placeholder from shadowing the owner in the cache', async () => { + const shared = `cached-${Math.random().toString(36).slice(2, 10)}@test.local`; + const owner = await server.stores.user.create({ + username: `uk-${Math.random().toString(36).slice(2, 10)}`, + uuid: uuidv4(), + password: 'hashed', + email: shared, + clean_email: shared, + }); + // Warm the address-keyed entry, so the placeholder below has something + // to overwrite. + expect((await server.stores.user.getByEmail(shared))?.id).toBe( + owner.id, + ); + + await server.stores.user.create({ + username: `ul-${Math.random().toString(36).slice(2, 10)}`, + uuid: uuidv4(), + password: null, + email: shared, + clean_email: shared, + }); + + // Cached lookups are how login and password recovery resolve an + // address, and ordering the SQL does nothing for them — the placeholder + // must never be written under the address in the first place. + expect((await server.stores.user.getByEmail(shared))?.id).toBe( + owner.id, + ); + }); + + it('lets only one of two concurrent signups claim a placeholder row', async () => { + const email = `claim-${Math.random().toString(36).slice(2, 10)}@test.local`; + const placeholder = await server.stores.user.create({ + username: `uq-${Math.random().toString(36).slice(2, 10)}`, + uuid: uuidv4(), + password: null, + email, + clean_email: email, + }); + + // The address never changes hands here — the row already holds it — so + // the unique index has nothing to catch. Only the guard stops the + // second write from overwriting the first. + const claim = (username: string) => + server.stores.user.claimPlaceholder(placeholder.id, { + username, + password: `hash-${username}`, + uuid: uuidv4(), + email_confirmed: 0, + }); + + const first = await claim('winner'); + const second = await claim('loser'); + + expect(first).toBe(true); + expect(second).toBe(false); + const fresh = await server.stores.user.getById(placeholder.id, { + force: true, + }); + expect(fresh?.username).toBe('winner'); + expect(fresh?.password).toBe('hash-winner'); + }); + + it('retires the placeholder username once the claim converts the row', async () => { + const email = `retire-${Math.random().toString(36).slice(2, 10)}@test.local`; + const oldName = `uo-${Math.random().toString(36).slice(2, 10)}`; + const newName = `un-${Math.random().toString(36).slice(2, 10)}`; + const placeholder = await server.stores.user.create({ + username: oldName, + uuid: uuidv4(), + password: null, + email, + clean_email: email, + }); + // Warm the username-keyed cache entry so a stale hit would surface. + expect((await server.stores.user.getByUsername(oldName))?.id).toBe( + placeholder.id, + ); + + expect( + await server.stores.user.claimPlaceholder(placeholder.id, { + username: newName, + password: 'hashed', + uuid: uuidv4(), + }), + ).toBe(true); + + expect(await server.stores.user.getByUsername(oldName)).toBeNull(); + expect((await server.stores.user.getByUsername(newName))?.id).toBe( + placeholder.id, + ); + }); + + it('tells an address conflict apart from other unique violations', async () => { + const username = `udup-${Math.random().toString(36).slice(2, 10)}`; + await server.stores.user.create({ + username, + uuid: uuidv4(), + password: 'hashed', + email: `${username}@test.local`, + clean_email: `${username}@test.local`, + }); + + // A duplicate username raises the same unique-violation code. Reading + // it as an address conflict would report the wrong error to the user. + let usernameError: unknown; + try { + await server.stores.user.create({ + username, + uuid: uuidv4(), + password: 'hashed', + email: `other-${username}@test.local`, + clean_email: `other-${username}@test.local`, + }); + } catch (e) { + usernameError = e; + } + expect(usernameError).toBeTruthy(); + expect(isOwnedEmailConflict(usernameError)).toBe(false); + + let emailError: unknown; + try { + await server.stores.user.create({ + username: `${username}-2`, + uuid: uuidv4(), + password: 'hashed', + email: `${username}@test.local`, + clean_email: `${username}@test.local`, + }); + } catch (e) { + emailError = e; + } + expect(isOwnedEmailConflict(emailError)).toBe(true); + }); + + it('finds an address holder through its canonical form', async () => { + const suffix = Math.random().toString(36).slice(2, 10); + const stored = `first.last.${suffix}+tag@gmail.com`; + const owner = await server.stores.user.create({ + username: `uc-${suffix}`, + uuid: uuidv4(), + password: 'hashed', + email: stored, + clean_email: cleanEmail(stored), + }); + + // A gmail alias of a stored address has to resolve to the same account, + // or signup would happily mint a second one for the same inbox. + expect( + ( + await server.stores.user.findEmailOwner( + `firstlast${suffix}@gmail.com`, + ) + )?.id, + ).toBe(owner.id); + }); + it('counts other accounts holding the same phone number', async () => { const phone = `+1415555${Math.floor(1000 + Math.random() * 9000)}`; const makeUser = async () => { diff --git a/src/backend/stores/user/UserStore.ts b/src/backend/stores/user/UserStore.ts index 6529ca571..758dbb4b3 100644 --- a/src/backend/stores/user/UserStore.ts +++ b/src/backend/stores/user/UserStore.ts @@ -17,6 +17,7 @@ * along with this program. If not, see . */ +import { cleanEmail } from '../../util/email.js'; import { PuterStore } from '../types'; // -- Types ------------------------------------------------------------ @@ -72,6 +73,36 @@ export type UserIdProperty = (typeof USER_ID_PROPERTIES)[number]; const CACHE_KEY_PREFIX = 'users'; const CACHE_TTL_SECONDS = 15 * 60; +// Tie-break for address lookups that can match more than one row. An address is +// "owned" by a confirmed account, or failing that by one holding a password; +// everything else is an unconfirmed placeholder that anyone may still claim. +// Ordering by that precedence (then by age) makes every guard, login and +// recovery resolve the same address to the same row. +const EMAIL_OWNER_ORDER = + 'ORDER BY `email_confirmed` DESC, (`password` IS NOT NULL) DESC, `id` ASC'; + +/** + * Unique index backing "at most one row owns an address". Application checks + * race — two signups can both read "address is free" and both insert — so this + * is the only thing that actually holds the invariant. Writes that can lose + * that race have to recognise the violation and turn it into the same message + * the pre-check would have produced. + */ +export const OWNED_EMAIL_INDEX = 'idx_user_owned_email'; + +export const isOwnedEmailConflict = (e: unknown): boolean => { + const err = e as { code?: string; message?: string } | null; + if (!err) return false; + const isUnique = + err.code === 'ER_DUP_ENTRY' || + err.code === '23505' || + (typeof err.code === 'string' && + err.code.startsWith('SQLITE_CONSTRAINT')); + if (!isUnique) return false; + // Other unique columns on `user` (username, uuid, referral_code) raise the + // same code and must keep their own error handling. + return (err.message ?? '').includes(OWNED_EMAIL_INDEX); +}; // Cap on placeholders per `IN (?, ?, …)` query. SQLite's default parameter // limit is 999; staying well under that keeps `getByIds` portable across // backends without splitting the cap by driver. @@ -248,16 +279,47 @@ export class UserStore extends PuterStore { * Rehydrates through `getById` so the caller gets a normalized row (and * warms the id-keyed cache for subsequent reads). */ - async getByCleanEmail(cleanEmailValue: string): Promise { + async getByCleanEmail( + cleanEmailValue: string, + opts: { force?: boolean } = {}, + ): Promise { if (!cleanEmailValue) return null; if (!isStorableAsLatin1(cleanEmailValue)) return null; - const rows = (await this.clients.db.tryHardRead( - 'SELECT `id` FROM `user` WHERE `clean_email` = ? LIMIT 1', - [cleanEmailValue], - )) as Array<{ id: number }>; + const sql = `SELECT \`id\` FROM \`user\` WHERE \`clean_email\` = ? ${EMAIL_OWNER_ORDER} LIMIT 1`; + const rows = (await (opts.force + ? this.clients.db.pread(sql, [cleanEmailValue]) + : this.clients.db.tryHardRead(sql, [cleanEmailValue]))) as Array<{ + id: number; + }>; const row = rows[0]; if (!row) return null; - return this.getById(row.id as number); + return this.getById(row.id as number, opts); + } + + /** + * Resolve whoever currently holds an address, matching the raw `email` + * column first and falling back to the canonical `clean_email` so + * gmail-style aliases (`foo.bar+tag@gmail.com` vs `foobar@gmail.com`) + * collapse to the same account. + * + * This is the single duplicate-detection lookup for every write path that + * attaches an address to a row (signup, save-account, change-email, OIDC, + * admin provisioning). Callers decide what to do with the hit: a row that + * is confirmed or holds a password owns the address and blocks the write; + * an unconfirmed password-less row is a placeholder the caller may claim. + * + * Pass `force` to read the primary. Every caller doing a last-moment + * re-check before an insert must, or it re-reads the same stale snapshot + * the first check saw. + */ + async findEmailOwner( + email: string, + opts: { force?: boolean } = {}, + ): Promise { + if (!email) return null; + const direct = await this.getByEmail(email, { force: opts.force }); + if (direct) return direct; + return this.getByCleanEmail(cleanEmail(email), opts); } /** @@ -317,7 +379,16 @@ export class UserStore extends PuterStore { // (`pread`) to bypass replica lag for hot reads (e.g., immediately // after a signup). Otherwise `tryHardRead` parallels primary + // replica and prefers whichever returns rows. - const sql = `SELECT * FROM \`user\` WHERE \`${prop}\` = ? LIMIT 1`; + // `id`, `uuid` and `username` are UNIQUE, so at most one row matches and + // the optimizer drops the ordering. `email` is not — multiple rows may + // legitimately hold the same address while unconfirmed, so without an + // explicit order the winner is whatever the storage engine hands back + // first, and login / password recovery would resolve the same address to + // a different account run to run. Prefer the row that owns the address. + const sql = + `SELECT * FROM \`user\` WHERE \`${prop}\` = ?` + + (prop === 'email' ? ` ${EMAIL_OWNER_ORDER}` : '') + + ' LIMIT 1'; const rows = force ? await this.clients.db.pread(sql, [value]) : await this.clients.db.tryHardRead(sql, [value]); @@ -353,6 +424,15 @@ export class UserStore extends PuterStore { clean_email?: string | null; free_storage?: number | null; requires_email_confirmation?: boolean; + /** + * Set this at insert time for accounts that are confirmed from birth + * (an identity provider already verified the address). Confirming in a + * follow-up `update` instead means the insert does not yet own the + * address, so two concurrent creates both succeed and only collide on + * the later update — past the point where the caller can cleanly report + * a duplicate. + */ + email_confirmed?: boolean; email_confirm_code?: string | null; email_confirm_token?: string | null; audit_metadata?: Record | null; @@ -378,6 +458,7 @@ export class UserStore extends PuterStore { uuid, free_storage, requires_email_confirmation, + email_confirmed, email_confirm_code, email_confirm_token, audit_metadata, @@ -392,7 +473,7 @@ export class UserStore extends PuterStore { phone, requires_phone_verification, requires_card_verification) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)${this.clients.db.returningIdClause()}`, + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)${this.clients.db.returningIdClause()}`, [ fields.username, fields.email, @@ -403,6 +484,7 @@ export class UserStore extends PuterStore { this.clients.db.booleanValue( Boolean(fields.requires_email_confirmation), ), + this.clients.db.booleanValue(Boolean(fields.email_confirmed)), fields.email_confirm_code ?? null, fields.email_confirm_token ?? null, fields.audit_metadata @@ -447,6 +529,44 @@ export class UserStore extends PuterStore { userId: number, patch: Record, ): Promise { + await this.#write(userId, patch); + } + + /** + * Convert an unconfirmed, password-less placeholder row — the + * admin-provisioned pre-registration a signup claims instead of inserting a + * new row. + * + * The guard is the whole point. Two signups can both read the row as + * claimable, and an unguarded `UPDATE` lets the second overwrite the first: + * the row ends up with the second signup's username and password while the + * first was already handed a session for it. The unique index cannot catch + * that — the row already existed, so nothing is inserted and no address + * changes hands. + * + * Returns false when someone claimed the row in between, which the caller + * reports as a duplicate address. + */ + async claimPlaceholder( + userId: number, + patch: Record, + ): Promise { + const unclaimed = + '`password` IS NULL AND `email_confirmed` = ' + + this.clients.db.booleanLiteral(false); + return this.#write(userId, patch, unclaimed); + } + + /** + * Shared write path for `update` / `claimPlaceholder`. `guard` is extra SQL + * ANDed into the WHERE clause; the write is reported as lost when it + * matches no row. + */ + async #write( + userId: number, + patch: Record, + guard?: string, + ): Promise { const dbPatch: Record = {}; for (const [key, value] of Object.entries(patch)) { dbPatch[key] = @@ -458,7 +578,7 @@ export class UserStore extends PuterStore { } const keys = Object.keys(dbPatch); - if (keys.length === 0) return; + if (keys.length === 0) return true; assertLatin1Writable(dbPatch); @@ -478,15 +598,31 @@ export class UserStore extends PuterStore { ? await this.getByProperty('id', userId, { force: true }) : null; - await this.clients.db.write( - `UPDATE \`user\` SET ${setClause} WHERE \`id\` = ?`, + const result = await this.clients.db.write( + `UPDATE \`user\` SET ${setClause} WHERE \`id\` = ?` + + (guard ? ` AND ${guard}` : ''), [...values, userId], ); + if (guard) { + const affected = + (result as { affectedRows?: number; changes?: number }) + ?.affectedRows ?? + (result as { affectedRows?: number; changes?: number }) + ?.changes ?? + 0; + // Nothing was written, so there are no cache keys to retire. + if (affected === 0) return false; + } + const fresh = await this.getByProperty('id', userId, { force: true }); if (before) { - const live = new Set(fresh ? this.#cacheKeysForUser(fresh) : []); + // Compare against the keys the refresh below will actually write, + // not every key the fresh row could be found by: a row that just + // stopped owning its address keeps the address in its key list but + // no longer gets cached under it, and the old value would survive. + const live = new Set(fresh ? this.#cacheKeysToWrite(fresh) : []); const retired = this.#cacheKeysForUser(before).filter( (key) => !live.has(key), ); @@ -500,6 +636,7 @@ export class UserStore extends PuterStore { } else { await this.invalidateById(userId); } + return true; } async updateMetadata( @@ -520,6 +657,32 @@ export class UserStore extends PuterStore { } } + /** + * The account other than `userId` that has already confirmed this address, + * if there is one. Matches raw + canonical, exactly like + * `unconfirmOthersByEmail`, and is meant to run immediately before it: a + * confirmed row proved access to the inbox, so it is refused rather than + * demoted. Everything that lookup leaves behind is an unconfirmed row, + * which is what `unconfirmOthersByEmail` is for. + * + * Reads the primary — the confirmation it guards is about to write. + */ + async findConfirmedOtherByEmail( + userId: number, + email: string, + cleanEmailValue: string, + ): Promise { + if (!email) return null; + const rows = (await this.clients.db.pread( + 'SELECT * FROM `user` WHERE `id` != ? AND (`email` = ? OR `clean_email` = ?) ' + + `AND \`email_confirmed\` = ${this.clients.db.booleanLiteral(true)} ` + + 'ORDER BY `id` ASC LIMIT 1', + [userId, email, cleanEmailValue], + )) as Array>; + const row = rows[0]; + return row ? this.#normalizeRow(row) : null; + } + async unconfirmOthersByEmail( userId: number, email: string, @@ -585,6 +748,24 @@ export class UserStore extends PuterStore { return keys; } + /** + * The subset of `#cacheKeysForUser` that may point _at_ this row. Same + * keys, minus the address when the row doesn't own it: several rows may + * hold one address, and `EMAIL_OWNER_ORDER` makes SQL resolve it to the + * owner. Caching a placeholder under the address would shadow that for the + * whole TTL, and login and password recovery both resolve an address + * through the cache. + * + * Invalidation deliberately keeps using the full set, so a key written + * while the row still owned the address is never orphaned. + */ + #cacheKeysToWrite(user: UserRow): string[] { + const keys = this.#cacheKeysForUser(user); + if (user.email_confirmed || user.password != null) return keys; + const addressKey = this.#cacheKey('email', user.email); + return keys.filter((key) => key !== addressKey); + } + async #readCache( prop: UserIdProperty, value: unknown, @@ -603,7 +784,7 @@ export class UserStore extends PuterStore { } async #writeCache(user: UserRow): Promise { - const keys = this.#cacheKeysForUser(user); + const keys = this.#cacheKeysToWrite(user); if (keys.length === 0) return; const serialized = JSON.stringify(user); await Promise.all( @@ -619,7 +800,7 @@ export class UserStore extends PuterStore { } async #refreshCache(user: UserRow): Promise { - const keys = this.#cacheKeysForUser(user); + const keys = this.#cacheKeysToWrite(user); if (keys.length === 0) return; await this.publishCacheKeys({ keys,