From fbb4b0bd91020e1dde27e54206df3d16f80874fe Mon Sep 17 00:00:00 2001 From: wtclaude Date: Sat, 29 Aug 2026 01:53:50 -0500 Subject: [PATCH] feat(auth): unique, changeable, verifiable email addresses (engagement Phase 1b) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Makes `users.email` unique, de-duplicates the addresses an upgrade will find, and builds the self-service change-and-verify flow that did not exist. The uniqueness index is on a generated `email_norm AS (LOWER(email)) STORED` column under `utf8mb4_bin`, NOT on `email` under a `_ci` collation as the plan specified. Every case-insensitive collation this server offers is also accent-insensitive: `josé@x.com` and `jose@x.com` compare equal, and those are two different mailboxes. The plan's index would have refused the second address forever and the de-duplication would have nulled a legitimate account's. A requested address is STAGED in `email_pending` and only a tokened link installs it, so a typo cannot silently redirect account-recovery mail. `isDuplicateUsername()` now distinguishes the two indexes. All five call sites branch on it; each answers differently on purpose, because a public form, an IdP callback, a half-completed invite and an admin screen do not owe the same person the same amount of truth. SSO reads the IdP's actual `email_verified`/`verified` claim instead of inferring verification from an address merely being present. Co-Authored-By: Claude --- client/src/App.jsx | 4 + client/src/api/client.js | 18 + .../components/security/EmailAddressPanel.jsx | 175 +++++ .../src/routes/admin/views/AccountAdmin.jsx | 5 + client/src/routes/player/PlayerAccount.jsx | 2 + client/src/routes/player/VerifyEmail.jsx | 166 ++++ server/db/schema.sql | 149 +++- server/routes.guards.json | 65 ++ server/routes.manifest.json | 28 + server/src/auth/providers/base.provider.js | 4 +- server/src/auth/providers/discord.provider.js | 9 +- .../auth/providers/genericOidc.provider.js | 4 + server/src/auth/providers/google.provider.js | 10 +- server/src/middleware/rateLimit.js | 14 + .../src/model/emailDedupe/emailDedupe.db.js | 22 + .../model/emailDedupe/emailDedupe.model.js | 18 + .../emailVerifications.db.js | 52 ++ .../emailVerifications.model.js | 76 ++ server/src/model/settings/settings.model.js | 26 + server/src/model/users/users.db.js | 41 +- server/src/model/users/users.model.js | 71 +- .../src/router/v1/admin/admin.controller.js | 121 ++- .../src/router/v1/admin/invites.controller.js | 13 + server/src/router/v1/admin/invites.router.js | 1 + server/src/router/v1/admin/users.router.js | 25 + .../src/router/v1/auth/account.controller.js | 130 ++++ server/src/router/v1/auth/auth.controller.js | 14 + .../router/v1/auth/emailVerify.controller.js | 100 +++ .../src/router/v1/auth/emailVerify.router.js | 50 ++ server/src/router/v1/auth/index.js | 2 + .../src/router/v1/auth/invite.controller.js | 14 + server/src/router/v1/auth/me.routes.js | 44 ++ server/src/router/v1/auth/password.router.js | 2 +- .../v1/auth/passwordReset.controller.js | 17 +- server/src/router/v1/auth/sso.controller.js | 60 +- server/src/utils/mailer.js | 43 +- server/swagger/swagger-output.json | 706 +++++++++++++++++- server/swagger/swagger.js | 60 ++ server/test/authMe.test.js | 6 + server/test/emailCollisionSurfaces.test.js | 194 +++++ server/test/emailUniqueness.test.js | 87 +++ server/test/emailVerification.test.js | 278 +++++++ server/test/providers.test.js | 17 +- server/test/ssoEmailVerified.test.js | 140 ++++ 44 files changed, 3024 insertions(+), 59 deletions(-) create mode 100644 client/src/components/security/EmailAddressPanel.jsx create mode 100644 client/src/routes/player/VerifyEmail.jsx create mode 100644 server/src/model/emailDedupe/emailDedupe.db.js create mode 100644 server/src/model/emailDedupe/emailDedupe.model.js create mode 100644 server/src/model/emailVerifications/emailVerifications.db.js create mode 100644 server/src/model/emailVerifications/emailVerifications.model.js create mode 100644 server/src/router/v1/auth/emailVerify.controller.js create mode 100644 server/src/router/v1/auth/emailVerify.router.js create mode 100644 server/test/emailCollisionSurfaces.test.js create mode 100644 server/test/emailUniqueness.test.js create mode 100644 server/test/emailVerification.test.js create mode 100644 server/test/ssoEmailVerified.test.js diff --git a/client/src/App.jsx b/client/src/App.jsx index 88ff3f7..860cc6d 100644 --- a/client/src/App.jsx +++ b/client/src/App.jsx @@ -54,6 +54,7 @@ import PlayerLogin from './routes/player/PlayerLogin.jsx' import PlayerRegister from './routes/player/PlayerRegister.jsx' import ForgotPassword from './routes/player/ForgotPassword.jsx' import ResetPassword from './routes/player/ResetPassword.jsx' +import VerifyEmail from './routes/player/VerifyEmail.jsx' import AcceptInvite from './routes/player/AcceptInvite.jsx' import PlayerPortalLayout, { PlayerIndex } from './routes/player/PlayerPortalLayout.jsx' import PlayerAccount from './routes/player/PlayerAccount.jsx' @@ -205,6 +206,9 @@ export default function App() { } /> } /> } /> + {/* Opened from a mailbox, so public like the reset page above — the + token is the proof, and confirming issues no session. */} + } /> } /> {/* PUBLIC, and grouped with the other tokened landings above rather than with the portal below: the person following an unsubscribe diff --git a/client/src/api/client.js b/client/src/api/client.js index ff281f8..c219872 100644 --- a/client/src/api/client.js +++ b/client/src/api/client.js @@ -116,6 +116,18 @@ export const api = { req('/auth/me/account/username', { method: 'PATCH', body: { username } }), changePassword: (newPassword, currentPassword) => req('/auth/me/account/password', { method: 'PATCH', body: { newPassword, currentPassword } }), + // Email address (engagement Phase 1b). changeEmail STAGES the address — the + // account keeps its current one until the emailed link is opened — so the UI + // must show `email_pending` as pending, never as the address in force. + changeEmail: (email, currentPassword) => + req('/auth/me/account/email', { method: 'PATCH', body: { email, currentPassword } }), + resendEmailVerification: () => req('/auth/me/account/email/resend', { method: 'POST' }), + cancelEmailChange: () => req('/auth/me/account/email/pending', { method: 'DELETE' }), + // The confirm half is public and token-gated — it is reached from a mailbox, + // often with no session, so it deliberately sits outside /auth/me. + lookupEmailVerification: (token) => req(`/auth/email/verify/${encodeURIComponent(token)}`), + confirmEmailVerification: (token) => + req(`/auth/email/verify/${encodeURIComponent(token)}`, { method: 'POST' }), totpSetup: () => req('/auth/me/account/totp/setup', { method: 'POST' }), totpEnable: (code) => req('/auth/me/account/totp/enable', { method: 'POST', body: { code } }), totpDisable: (code) => req('/auth/me/account/totp/disable', { method: 'POST', body: { code } }), @@ -312,6 +324,12 @@ export const api = { createUser: (data) => req('/admin/users', { method: 'POST', body: data }), updateUser: (id, data) => req(`/admin/users/${id}`, { method: 'PUT', body: data }), deleteUser: (id) => req(`/admin/users/${id}`, { method: 'DELETE' }), + // Accounts whose address was cleared when addresses became unique (Phase 1b). + // They can still sign in but can receive no mail until they set a new one, so + // they are the list an operator has to work through. + emailDedupeReport: () => req('/admin/users/email-dedupe-report'), + acknowledgeEmailDedupeReport: () => + req('/admin/users/email-dedupe-report/acknowledge', { method: 'POST' }), // A user's trusted devices + MFA reset (admin only). userTrustedDevices: (id) => req(`/admin/users/${id}/trusted-devices`), revokeUserTrustedDevice: (id, deviceId) => diff --git a/client/src/components/security/EmailAddressPanel.jsx b/client/src/components/security/EmailAddressPanel.jsx new file mode 100644 index 0000000..4902669 --- /dev/null +++ b/client/src/components/security/EmailAddressPanel.jsx @@ -0,0 +1,175 @@ +import { useState } from 'react' +import { api } from '../../api/client.js' + +// Self-service email address (engagement Phase 1b). Shared by the player portal +// and the admin account screen, the same way TrustedDevicesPanel and +// RecoveryCodesPanel are — /auth/me/account is one surface for every role, so its +// UI is one component too. +// +// The property this component exists to make visible: a requested address is +// STAGED, not applied. The account keeps receiving mail — password resets +// included — at the address it already has until the emailed link is opened. If +// the UI let a pending address look like the address in force, someone who +// mistyped would believe the change took and would only discover otherwise when +// they could not recover their account. +// +// `hasPassword` decides whether the current-password field appears: an address is +// where account recovery lands, so changing it is re-authenticated, with the same +// carve-out the password form makes for an SSO-only account. +export default function EmailAddressPanel({ account, reload, embedded = false }) { + const hasPassword = account.has_password !== false + const [email, setEmail] = useState('') + const [current, setCurrent] = useState('') + const [busy, setBusy] = useState(false) + const [msg, setMsg] = useState('') + const [error, setError] = useState('') + + const pending = account.email_pending + + async function save(e) { + e.preventDefault() + setMsg('') + setError('') + setBusy(true) + try { + const res = await api.changeEmail(email.trim(), hasPassword ? current : undefined) + setEmail('') + setCurrent('') + // Report an unsent mail honestly. Saying "check your inbox" about a message + // that was never sent turns a configuration problem into a user who waits. + if (res.emailed === false) { + setMsg( + res.reason === 'NOT_CONFIGURED' + ? 'Address saved, but this site cannot send email right now. Ask an administrator, then use Resend.' + : 'Address saved, but the confirmation email could not be sent. Try Resend in a moment.', + ) + } else { + setMsg( + `Confirmation sent to ${res.email_pending}. Your current address stays in use until you open that link.`, + ) + } + await reload() + } catch (err) { + if (err.status === 429) setError('Too many confirmation emails. Try again later.') + else setError(err.message || 'Could not change your email address.') + } finally { + setBusy(false) + } + } + + async function resend() { + setMsg('') + setError('') + setBusy(true) + try { + const res = await api.resendEmailVerification() + setMsg( + res.emailed === false + ? 'Could not send the confirmation email.' + : `Confirmation re-sent to ${res.email_pending}.`, + ) + } catch (err) { + setError(err.message || 'Could not resend the confirmation email.') + } finally { + setBusy(false) + } + } + + async function discard() { + setMsg('') + setError('') + setBusy(true) + try { + await api.cancelEmailChange() + setMsg('Pending address discarded.') + await reload() + } catch (err) { + setError(err.message || 'Could not discard the pending address.') + } finally { + setBusy(false) + } + } + + const wrap = embedded + ? {} + : { marginTop: 40, borderTop: '1px solid var(--line-soft)', paddingTop: 28 } + + return ( +
+

+ Email address +

+

+ {account.email ? ( + <> + Currently {account.email} + {account.email_verified ? ' (confirmed)' : ' (not yet confirmed)'}. This is where password-reset + email is sent. + + ) : ( + 'You have no email address on file, so you cannot reset your password by email.' + )} +

+ + {pending && ( +
+ {pending} is waiting to be confirmed. It is not in + use until you open the link in that email. +
+ + +
+
+ )} + +
+ + {hasPassword && ( + + )} +
+ +
+ {(msg || error) && ( +

+ {error || msg} +

+ )} +
+
+ ) +} diff --git a/client/src/routes/admin/views/AccountAdmin.jsx b/client/src/routes/admin/views/AccountAdmin.jsx index b557e3c..4c4ad6e 100644 --- a/client/src/routes/admin/views/AccountAdmin.jsx +++ b/client/src/routes/admin/views/AccountAdmin.jsx @@ -4,6 +4,7 @@ import ProviderIcon from '../../../components/ProviderIcon.jsx' import RecoveryCodesDisplay from '../../../components/security/RecoveryCodesDisplay.jsx' import TrustedDevicesPanel from '../../../components/security/TrustedDevicesPanel.jsx' import RecoveryCodesPanel from '../../../components/security/RecoveryCodesPanel.jsx' +import EmailAddressPanel from '../../../components/security/EmailAddressPanel.jsx' import { api } from '../../../api/client.js' // Link/unlink external SSO identities to this account. Linking redirects through @@ -322,6 +323,10 @@ export default function AccountAdmin() { )} + {/* The self-service address, from the same component the player portal + renders — /auth/me/account is one surface for every role. */} + {account && } + ) diff --git a/client/src/routes/player/PlayerAccount.jsx b/client/src/routes/player/PlayerAccount.jsx index 01782b2..d03645b 100644 --- a/client/src/routes/player/PlayerAccount.jsx +++ b/client/src/routes/player/PlayerAccount.jsx @@ -4,6 +4,7 @@ import { Loading, ErrorState } from '../../components/PageState.jsx' import RecoveryCodesDisplay from '../../components/security/RecoveryCodesDisplay.jsx' import TrustedDevicesPanel from '../../components/security/TrustedDevicesPanel.jsx' import RecoveryCodesPanel from '../../components/security/RecoveryCodesPanel.jsx' +import EmailAddressPanel from '../../components/security/EmailAddressPanel.jsx' import { useAuth } from '../../contexts/AuthContext.jsx' import { api } from '../../api/client.js' @@ -423,6 +424,7 @@ export default function PlayerAccount() { {account.email ? ` · ${account.email}` : ''}

+ {account.totp_enabled && ( diff --git a/client/src/routes/player/VerifyEmail.jsx b/client/src/routes/player/VerifyEmail.jsx new file mode 100644 index 0000000..877153d --- /dev/null +++ b/client/src/routes/player/VerifyEmail.jsx @@ -0,0 +1,166 @@ +import { useEffect, useState } from 'react' +import { Link, useParams } from 'react-router-dom' +import { api } from '../../api/client.js' +import PlayerShell from './PlayerShell.jsx' + +// Public, token-gated confirmation page (/account/verify-email/:token). +// +// Unauthenticated on purpose: the link arrives in a mailbox and is routinely +// opened on a device with no session. That is safe because the token IS the +// proof — opening it installs an address on the account it was minted for and +// does nothing else. No session is issued here, deliberately: proving control of +// a mailbox is not proving control of an account. +// +// Every failure the server can have — expired, already used, superseded by a +// later request, or an address another account confirmed first — comes back as +// the same 404. That is not laziness on the server's part; distinguishing them +// would let anyone test which addresses have accounts. So this page says the same +// thing for all of them, and must keep doing so. +export default function VerifyEmail() { + const { token } = useParams() + + const [link, setLink] = useState(null) // { username, email } once validated + const [loadErr, setLoadErr] = useState('') + const [error, setError] = useState('') + const [busy, setBusy] = useState(false) + const [done, setDone] = useState(false) + + useEffect(() => { + let active = true + api + .lookupEmailVerification(token) + .then((r) => active && setLink(r || {})) + .catch( + (err) => + active && + setLoadErr( + err.status === 404 + ? 'This confirmation link is invalid or has expired.' + : 'Could not load this confirmation link.', + ), + ) + return () => { + active = false + } + }, [token]) + + async function onConfirm() { + setError('') + setBusy(true) + try { + await api.confirmEmailVerification(token) + setDone(true) + } catch (err) { + if (err.status === 404) setError('This confirmation link is no longer usable. Request a new one from your account page.') + else if (err.status === 429) setError('Too many attempts. Please try again in a little while.') + else setError('Could not confirm your address right now. Please try again later.') + setBusy(false) + } + } + + // ── Invalid link ─────────────────────────────────────────────────────────── + if (loadErr) { + return ( + +

+ {loadErr} +

+

+ + Go to your account + +

+ + ) + } + if (link === null) { + return ( + +
+ +
+ + ) + } + + // ── Done ─────────────────────────────────────────────────────────────────── + if (done) { + return ( + +

+ {link.email ? ( + <> + {link.email} is now the address for + {link.username ? ( + <> + {' '} + {link.username} + + ) : ( + ' your account' + )} + . + + ) : ( + 'Your email address has been confirmed.' + )} +

+

+ You have not been signed in — confirming an address does not sign you in. +

+

+ + Sign in + +

+ + ) + } + + // ── Confirm ──────────────────────────────────────────────────────────────── + // + // A button rather than confirming on load. A mail client or scanner that + // pre-fetches links would otherwise spend the token before the person ever saw + // it, and this token is single-use. + return ( + +

+ Confirm that{' '} + {link.email ? {link.email} : 'this address'} should be + the contact and account-recovery address for + {link.username ? ( + <> + {' '} + {link.username} + + ) : ( + ' this account' + )} + . +

+ + {error && ( +

+ {error} +

+ )} + + + +

+ If you did not ask for this, close this page. Nothing changes and no account of yours is affected. +

+ + ) +} diff --git a/server/db/schema.sql b/server/db/schema.sql index f6cfb91..bfe0b9f 100644 --- a/server/db/schema.sql +++ b/server/db/schema.sql @@ -21,11 +21,30 @@ CREATE TABLE IF NOT EXISTS users ( -- (validatePassword returns false). password_hash VARCHAR(72) NULL, role ENUM('admin','editor','moderator','player') NOT NULL DEFAULT 'admin', - -- Optional contact email (players). Not unique — SSO emails may repeat. Used - -- only for display + a future self-serve reset. email_verified is wired now so - -- an eventual SMTP verification flow needs no schema change. + -- The account's ONE contact address, and the destination for password-reset + -- mail. Unique since engagement Phase 1b — but the index is on email_norm + -- below, never on this column, and the reason is not stylistic: + -- + -- Every case-insensitive (_ci) collation this server offers is ALSO + -- accent-insensitive, so a UNIQUE index on `email` would refuse + -- jose@x.com once josé@x.com exists. Those are two different mailboxes. + -- + -- LOWER() under a _bin collation folds case WITHOUT folding accents, which is + -- exactly the equivalence a mail system uses. Keeping the fold in a generated + -- column rather than in application code means it cannot be bypassed by a + -- caller that forgets to normalize. email VARCHAR(255) NULL, + -- The uniqueness key. STORED (not VIRTUAL) because a UNIQUE index over it must + -- be materialized. Multiple NULLs are legal under a UNIQUE index, which is what + -- lets the Phase 1b de-duplication null the losers without deleting an account. + email_norm VARCHAR(255) COLLATE utf8mb4_bin AS (LOWER(email)) STORED, email_verified TINYINT(1) NOT NULL DEFAULT 0, + -- An address the user has asked for but not yet proved. It does NOT displace + -- `email` until the verification link is used, so a typo cannot silently + -- redirect this account's password-reset mail. Deliberately NOT unique: a + -- pending address reserves nothing, and two users may both be pending on one + -- address — the second to verify loses, with the same generic failure. + email_pending VARCHAR(255) NULL, -- Account lifecycle, independent of role: staff can disable/ban a player -- without changing their role. active = normal; disabled = admin-locked; -- banned = moderation ban; pending = reserved for future email-verify gating. @@ -38,7 +57,12 @@ CREATE TABLE IF NOT EXISTS users ( tokens_valid_after DATETIME NULL, created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, last_login_at DATETIME NULL, - last_login_ip VARCHAR(45) NULL -- IPv6-capable, set on each login + last_login_ip VARCHAR(45) NULL, -- IPv6-capable, set on each login + -- One account per mailbox (engagement Phase 1b). On the generated column, not + -- on `email` — see the note there. Upgraded databases get this in the migration + -- block at the foot of this file, AFTER the de-duplication that makes it + -- addable; adding it here too is what gives a FRESH install the same shape. + UNIQUE KEY uq_users_email_norm (email_norm) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; CREATE TABLE IF NOT EXISTS posts ( @@ -415,6 +439,54 @@ CREATE TABLE IF NOT EXISTS password_resets ( INDEX idx_password_resets_status (status, expires_at) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; +-- Self-service email verification (engagement Phase 1b). The same shape as +-- password_resets, deliberately: an opaque random token whose sha256 is all that +-- is stored, single-use, short-lived. The design of record calls this link +-- "signed"; every comparable flow in this codebase (user_invites, +-- password_resets, mobile_refresh_tokens) uses a hashed random token instead, and +-- matching them beats introducing a second token mechanism for one caller. +-- +-- The address lives on the ROW, not just on the user: a token proves control of +-- the address it was mailed to, so if the user changes their mind and requests a +-- different address, the older token must not be able to confirm the newer one. +CREATE TABLE IF NOT EXISTS email_verifications ( + id INT AUTO_INCREMENT PRIMARY KEY, + token_hash CHAR(64) NOT NULL UNIQUE, -- sha256 hex of the opaque token + user_id INT NOT NULL, + email VARCHAR(255) NOT NULL, -- the address THIS token proves + status ENUM('pending','used') NOT NULL DEFAULT 'pending', + requested_ip VARCHAR(64) NULL, -- who asked (audit only) + expires_at DATETIME NOT NULL, + created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, + used_at DATETIME NULL, + CONSTRAINT fk_email_verifications_user FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE, + INDEX idx_email_verifications_user (user_id), + INDEX idx_email_verifications_status (status, expires_at) +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; + +-- Who lost an address to the Phase 1b de-duplication, and what they lost. +-- +-- These accounts are exactly the ones an operator must contact: they can no +-- longer receive password-reset or engagement mail until they set a new address. +-- Written by the migration below in pure SQL (ensureSchema() reads this file +-- statement-by-statement and there is no JS migration hook), surfaced as a +-- dashboard warning until acknowledged. +-- +-- No foreign key to users, on purpose: the same reasoning as posts.announce_job_id +-- — a constraint re-added on every boot is a constraint that can fail a boot, and +-- this table is a historical record rather than a live relation. +CREATE TABLE IF NOT EXISTS email_dedupe_report ( + id INT AUTO_INCREMENT PRIMARY KEY, + user_id INT NOT NULL, + username VARCHAR(32) NOT NULL, -- captured at clear time + lost_address VARCHAR(255) NOT NULL, + cleared_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, + acknowledged_at DATETIME NULL, -- set when an admin dismisses the warning + -- Makes the migration's INSERT strictly idempotent: an account cleared once is + -- never reported twice, however many times ensureSchema() runs. + UNIQUE KEY uq_edr_user (user_id) +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; + -- ── Push notifications (opt-in) ───────────────────────────────────────────── -- One row per registered push endpoint (Android/UnifiedPush v1; FCM later). The -- `endpoint` is the UnifiedPush distributor URL the app's ntfy topic was handed — @@ -1489,6 +1561,75 @@ ALTER TABLE email_config ADD COLUMN IF NOT EXISTS transport VARCHAR(32) NOT NULL ALTER TABLE email_config ADD COLUMN IF NOT EXISTS credential_enc TEXT NULL; ALTER TABLE email_config ADD COLUMN IF NOT EXISTS reply_to VARCHAR(255) NULL; +-- ── Engagement Phase 1b: one account per mailbox ─────────────────────────── +-- (ENGAGEMENT.md Phase 1b / §0.6.) ORDER IS LOAD-BEARING and every statement here +-- is idempotent — after the first successful boot each one matches zero rows. +-- +-- Why the generated column is added BEFORE the de-duplication rather than after: +-- the de-dupe must group addresses exactly the way the index will, and it cannot +-- do that with LOWER(email) = LOWER(email) in SQL, because that comparison uses +-- the COLUMN's collation, which is accent-insensitive. Grouping on email_norm — +-- the very column the UNIQUE index goes on — makes the two agree by construction +-- instead of by a hand-matched COLLATE clause someone can get wrong later. +-- (Tested: with the LOWER()=LOWER() form, jose@x.com was nulled as a "duplicate" +-- of josé@x.com. They are different mailboxes.) + +-- 1. An empty string is a value, not an absence, so two accounts holding '' would +-- collide under the index and stop the boot. Unreachable through the current +-- routes (isEmail() rejects ''), but this runs against databases whose history +-- we do not control. +UPDATE users SET email = NULL WHERE email = ''; + +-- 2. The pending-address column and the uniqueness key. No index yet — a UNIQUE +-- index here, before step 3, is precisely the ALTER that fails and takes the +-- site down with it (§0.6 finding 1). +ALTER TABLE users ADD COLUMN IF NOT EXISTS email_pending VARCHAR(255) NULL; +ALTER TABLE users ADD COLUMN IF NOT EXISTS email_norm VARCHAR(255) COLLATE utf8mb4_bin AS (LOWER(email)) STORED; + +-- 3. Record every account about to lose its address, BEFORE nulling it — the +-- report is the only place the lost value survives. Oldest-wins (§7.1 Q1): +-- the earliest-created account keeps the address, ties broken by id so the +-- outcome is deterministic. Verified status deliberately does NOT arbitrate — +-- SSO set email_verified from the mere presence of an address, so it is too +-- weak a signal to decide who keeps a mailbox (§0.6 finding 3). +INSERT IGNORE INTO email_dedupe_report (user_id, username, lost_address) +SELECT l.id, l.username, l.email FROM ( + SELECT u.id, u.username, u.email FROM users u + WHERE u.email_norm IS NOT NULL + AND u.id <> (SELECT u2.id FROM users u2 + WHERE u2.email_norm = u.email_norm + ORDER BY u2.created_at ASC, u2.id ASC LIMIT 1) +) AS l; + +-- 4. Clear the losers. NEVER deletes a row: multiple NULLs are legal under a +-- UNIQUE index, so every account survives with its login intact and simply has +-- no contact address until its owner sets one. The extra derived table is not +-- decoration — MariaDB refuses a subquery on the table being updated (error +-- 1093) without it. +UPDATE users SET email = NULL, email_verified = 0 + WHERE id IN (SELECT id FROM ( + SELECT u.id FROM users u + WHERE u.email_norm IS NOT NULL + AND u.id <> (SELECT u2.id FROM users u2 + WHERE u2.email_norm = u.email_norm + ORDER BY u2.created_at ASC, u2.id ASC LIMIT 1) + ) AS losers); + +-- 5. Now the table can hold it. +ALTER TABLE users ADD UNIQUE INDEX IF NOT EXISTS uq_users_email_norm (email_norm); + +-- 6. The verification gate: may an UNVERIFIED address receive opt-in engagement +-- mail? ON for a fresh install, OFF for an upgrade — the asymmetry is the G22 +-- lesson, not an oversight. Turning it on retroactively would silently stop +-- mailing every existing opted-in user on upgrade day, which is exactly the +-- kind of quiet breakage Phase 1 had to write a dashboard warning to undo. +-- "Fresh" is read off the users table: a database with no users has no one to +-- surprise. Both statements are INSERT IGNORE, so an operator who has since +-- changed the value keeps theirs. +INSERT IGNORE INTO settings (`key`, value) +SELECT 'email_verification_required', 'on' FROM DUAL WHERE (SELECT COUNT(*) FROM users) = 0; +INSERT IGNORE INTO settings (`key`, value) VALUES ('email_verification_required', 'off'); + -- The status a Gmail-connected deployment carries is 'connected', and after the -- upgrade that is a lie: nothing can send. Correct it once, narrowly. The WHERE -- makes this idempotent and self-limiting — it matches only a row that still holds diff --git a/server/routes.guards.json b/server/routes.guards.json index a4499fe..b931ae3 100644 --- a/server/routes.guards.json +++ b/server/routes.guards.json @@ -1020,6 +1020,24 @@ "validate" ] }, + { + "method": "GET", + "path": "/api/v1/admin/users/email-dedupe-report", + "handlers": 1, + "gates": [ + "noindex", + "requireAuth" + ] + }, + { + "method": "POST", + "path": "/api/v1/admin/users/email-dedupe-report/acknowledge", + "handlers": 1, + "gates": [ + "noindex", + "requireAuth" + ] + }, { "method": "GET", "path": "/api/v1/admin/wiki", @@ -1162,6 +1180,24 @@ "requireAuth" ] }, + { + "method": "GET", + "path": "/api/v1/auth/email/verify/:token", + "handlers": 3, + "gates": [ + "middleware", + "validate" + ] + }, + { + "method": "POST", + "path": "/api/v1/auth/email/verify/:token", + "handlers": 4, + "gates": [ + "middleware", + "validate" + ] + }, { "method": "GET", "path": "/api/v1/auth/invite/:token", @@ -1226,6 +1262,35 @@ "requireAuth" ] }, + { + "method": "PATCH", + "path": "/api/v1/auth/me/account/email", + "handlers": 5, + "gates": [ + "noindex", + "requireAuth", + "middleware", + "validate" + ] + }, + { + "method": "DELETE", + "path": "/api/v1/auth/me/account/email/pending", + "handlers": 1, + "gates": [ + "noindex", + "requireAuth" + ] + }, + { + "method": "POST", + "path": "/api/v1/auth/me/account/email/resend", + "handlers": 2, + "gates": [ + "noindex", + "requireAuth" + ] + }, { "method": "GET", "path": "/api/v1/auth/me/account/identities", diff --git a/server/routes.manifest.json b/server/routes.manifest.json index ed7f851..ea1427e 100644 --- a/server/routes.manifest.json +++ b/server/routes.manifest.json @@ -401,6 +401,14 @@ "method": "DELETE", "path": "/api/v1/admin/users/:id/trusted-devices/:deviceId" }, + { + "method": "GET", + "path": "/api/v1/admin/users/email-dedupe-report" + }, + { + "method": "POST", + "path": "/api/v1/admin/users/email-dedupe-report/acknowledge" + }, { "method": "GET", "path": "/api/v1/admin/wiki" @@ -457,6 +465,14 @@ "method": "GET", "path": "/api/v1/admin/wiki/tags" }, + { + "method": "GET", + "path": "/api/v1/auth/email/verify/:token" + }, + { + "method": "POST", + "path": "/api/v1/auth/email/verify/:token" + }, { "method": "GET", "path": "/api/v1/auth/invite/:token" @@ -485,6 +501,18 @@ "method": "GET", "path": "/api/v1/auth/me/account" }, + { + "method": "PATCH", + "path": "/api/v1/auth/me/account/email" + }, + { + "method": "DELETE", + "path": "/api/v1/auth/me/account/email/pending" + }, + { + "method": "POST", + "path": "/api/v1/auth/me/account/email/resend" + }, { "method": "GET", "path": "/api/v1/auth/me/account/identities" diff --git a/server/src/auth/providers/base.provider.js b/server/src/auth/providers/base.provider.js index 093acb8..2ceb19c 100644 --- a/server/src/auth/providers/base.provider.js +++ b/server/src/auth/providers/base.provider.js @@ -37,7 +37,7 @@ class BaseProvider { } // Complete an SSO redirect flow: exchange the callback code for a normalized - // user profile ({ subject, email, name }). + // user profile ({ subject, email, emailVerified, name }). // eslint-disable-next-line no-unused-vars async handleCallback(params) { throw new Error(`handleCallback() not implemented for provider '${this.id}'`) @@ -49,7 +49,7 @@ class BaseProvider { throw new Error(`getUserProfile() not implemented for provider '${this.id}'`) } - // Normalize a raw external profile to { subject, email, name }. + // Normalize a raw external profile to { subject, email, emailVerified, name }. // eslint-disable-next-line no-unused-vars mapUser(profile) { throw new Error(`mapUser() not implemented for provider '${this.id}'`) diff --git a/server/src/auth/providers/discord.provider.js b/server/src/auth/providers/discord.provider.js index 76f192b..e493d6c 100644 --- a/server/src/auth/providers/discord.provider.js +++ b/server/src/auth/providers/discord.provider.js @@ -23,7 +23,14 @@ class DiscordProvider extends OAuth2Provider { } normalizeProfile(p = {}) { // global_name is the new display name; fall back to the legacy username. - return { subject: p.id, email: p.email || null, name: p.global_name || p.username || null } + return { + subject: p.id, + email: p.email || null, + // Discord spells the claim `verified` rather than `email_verified`, and it + // means exactly this: the user confirmed the address with Discord. + emailVerified: p.verified === true, + name: p.global_name || p.username || null, + } } } diff --git a/server/src/auth/providers/genericOidc.provider.js b/server/src/auth/providers/genericOidc.provider.js index bc1c0ac..2995b83 100644 --- a/server/src/auth/providers/genericOidc.provider.js +++ b/server/src/auth/providers/genericOidc.provider.js @@ -30,6 +30,10 @@ class GenericOidcProvider extends OAuth2Provider { return { subject: p.sub || p.id || p.user_id || p.uid || null, email: p.email || null, + // The standard OIDC claim. An IdP that omits it has not asserted anything, + // so the address stays unverified and the user proves it the ordinary way — + // absent is treated as false, never as true. + emailVerified: p.email_verified === true || p.email_verified === 'true', name: p.name || p.preferred_username || p.username || p.email || null, } } diff --git a/server/src/auth/providers/google.provider.js b/server/src/auth/providers/google.provider.js index 5597bd4..98653c5 100644 --- a/server/src/auth/providers/google.provider.js +++ b/server/src/auth/providers/google.provider.js @@ -27,7 +27,15 @@ class GoogleProvider extends OAuth2Provider { return { access_type: 'online', prompt: 'select_account' } } normalizeProfile(p = {}) { - return { subject: p.sub, email: p.email || null, name: p.name || p.email || null } + return { + subject: p.sub, + email: p.email || null, + // Google's OIDC userinfo carries the standard `email_verified` claim. Read + // it rather than inferring verification from the mere presence of an + // address, which is what this code used to do (ENGAGEMENT.md §0.6/1b). + emailVerified: p.email_verified === true || p.email_verified === 'true', + name: p.name || p.email || null, + } } } diff --git a/server/src/middleware/rateLimit.js b/server/src/middleware/rateLimit.js index db1fa2d..ede7ce0 100644 --- a/server/src/middleware/rateLimit.js +++ b/server/src/middleware/rateLimit.js @@ -118,6 +118,19 @@ const passwordResetConfirmLimiter = makeLimiter({ message: 'Too many attempts. Please try again later.', }) +// Email-verification confirmations (engagement Phase 1b). Same reasoning as the +// password-reset confirm limiter: the token is 256-bit random, but an +// unauthenticated token-bearing endpoint should not be free to hammer. The +// REQUEST side is authenticated and limited separately — accountChangeLimiter per +// IP, plus a per-user ceiling in the model, because the mail goes to an address +// its recipient did not ask to hear from. +const emailVerifyConfirmLimiter = makeLimiter({ + windowMs: 15 * 60 * 1000, + max: 15, + label: 'email-verify-confirm', + message: 'Too many attempts. Please try again later.', +}) + // CSP violation reports. Unauthenticated by necessity (browsers send them with no // session), and every accepted report writes a log line — so an attacker who can get // a victim to load a page could otherwise use it as a log-flood amplifier. Generous @@ -149,5 +162,6 @@ module.exports = { mobileSsoExchangeLimiter, passwordResetRequestLimiter, passwordResetConfirmLimiter, + emailVerifyConfirmLimiter, cspReportLimiter, } diff --git a/server/src/model/emailDedupe/emailDedupe.db.js b/server/src/model/emailDedupe/emailDedupe.db.js new file mode 100644 index 0000000..1aadf78 --- /dev/null +++ b/server/src/model/emailDedupe/emailDedupe.db.js @@ -0,0 +1,22 @@ +const { query } = require('../../utils/db') + +const COLS = 'id, user_id, username, lost_address, cleared_at, acknowledged_at' + +// Accounts cleared by the Phase 1b de-duplication, newest first. +async function list() { + return query(`SELECT ${COLS} FROM email_dedupe_report ORDER BY cleared_at DESC, id DESC`) +} + +async function countUnacknowledged() { + const rows = await query('SELECT COUNT(*) AS n FROM email_dedupe_report WHERE acknowledged_at IS NULL') + return Number(rows[0] ? rows[0].n : 0) +} + +// Dismiss the whole report. Idempotent — an already-acknowledged row is skipped +// so a second dismissal cannot rewrite when it happened. +async function acknowledgeAll() { + const res = await query('UPDATE email_dedupe_report SET acknowledged_at = NOW() WHERE acknowledged_at IS NULL') + return res.affectedRows || 0 +} + +module.exports = { list, countUnacknowledged, acknowledgeAll } diff --git a/server/src/model/emailDedupe/emailDedupe.model.js b/server/src/model/emailDedupe/emailDedupe.model.js new file mode 100644 index 0000000..6f51b9c --- /dev/null +++ b/server/src/model/emailDedupe/emailDedupe.model.js @@ -0,0 +1,18 @@ +// The Phase 1b de-duplication report: who lost an email address when the UNIQUE +// index went on, and what they lost. +// +// The rows are written by schema.sql's migration in pure SQL — ensureSchema() +// executes that file statement-by-statement and there is no JS migration hook — +// so this model only ever READS and acknowledges. Nothing here creates a row. +// +// It matters because these accounts are exactly the ones an operator must +// contact: each can still log in, but has no contact address, so password-reset +// and engagement mail have nowhere to go until its owner sets a new one. + +const db = require('./emailDedupe.db') + +const list = () => db.list() +const countUnacknowledged = () => db.countUnacknowledged() +const acknowledgeAll = () => db.acknowledgeAll() + +module.exports = { list, countUnacknowledged, acknowledgeAll } diff --git a/server/src/model/emailVerifications/emailVerifications.db.js b/server/src/model/emailVerifications/emailVerifications.db.js new file mode 100644 index 0000000..bd3af12 --- /dev/null +++ b/server/src/model/emailVerifications/emailVerifications.db.js @@ -0,0 +1,52 @@ +const { query } = require('../../utils/db') + +const COLS = 'id, token_hash, user_id, email, status, requested_ip, expires_at, created_at, used_at' + +async function insert({ tokenHash, userId, email, requestedIp, expiresAt }) { + const res = await query( + `INSERT INTO email_verifications (token_hash, user_id, email, requested_ip, expires_at) + VALUES (?, ?, ?, ?, ?)`, + [tokenHash, userId, email, requestedIp ?? null, expiresAt], + ) + return res.insertId +} + +async function findByTokenHash(tokenHash) { + const rows = await query(`SELECT ${COLS} FROM email_verifications WHERE token_hash = ? LIMIT 1`, [tokenHash]) + return rows[0] || null +} + +// Mark used only if still pending (atomic guard against a double-use race). +// Returns rows changed (1 = we won, 0 = already used). +async function markUsed(id) { + const res = await query( + `UPDATE email_verifications SET status = 'used', used_at = NOW() + WHERE id = ? AND status = 'pending'`, + [id], + ) + return res.affectedRows || 0 +} + +// Retire every still-pending verification for a user. Called when a fresh request +// supersedes older links and after a successful verification, so an address the +// user changed their mind about can never be installed by an old email. +async function invalidatePendingForUser(userId) { + const res = await query( + `UPDATE email_verifications SET status = 'used', used_at = NOW() + WHERE user_id = ? AND status = 'pending'`, + [userId], + ) + return res.affectedRows || 0 +} + +// How many verification mails this user has asked for since `since`. Backs the +// per-user resend ceiling, which the IP rate limiter cannot provide on its own. +async function countRecentForUser(userId, since) { + const rows = await query( + 'SELECT COUNT(*) AS n FROM email_verifications WHERE user_id = ? AND created_at >= ?', + [userId, since], + ) + return Number(rows[0] ? rows[0].n : 0) +} + +module.exports = { insert, findByTokenHash, markUsed, invalidatePendingForUser, countRecentForUser } diff --git a/server/src/model/emailVerifications/emailVerifications.model.js b/server/src/model/emailVerifications/emailVerifications.model.js new file mode 100644 index 0000000..190a4cc --- /dev/null +++ b/server/src/model/emailVerifications/emailVerifications.model.js @@ -0,0 +1,76 @@ +// Self-service email verification (engagement Phase 1b). A user asks to set or +// change their address; a tokened link goes to the address they typed, and only +// opening that link installs it. The opaque token lives only in the emailed link — +// the DB stores its sha256 — so a DB read never yields a usable link. Same shape +// as password_resets and user_invites, deliberately: the design of record calls +// this link "signed", but every comparable flow here uses a hashed random token, +// and matching them beats adding a second token mechanism for one caller. +// +// The address is stored ON THE ROW rather than read from the user at confirm +// time, because a token proves control of the address it was mailed to and +// nothing else. + +const crypto = require('crypto') +const db = require('./emailVerifications.db') + +// A day, not an hour. Unlike a password reset this is not a live credential-reset +// capability — the worst a leaked token does is attach an address its holder +// already controls — and a verification mail is routinely opened on another +// device, hours later. +const DEFAULT_TTL_MINUTES = 24 * 60 + +// Per-user ceiling on verification sends, independent of the per-IP limiter: the +// mail goes to an address the RECIPIENT did not choose to hear from, so an +// attacker with one account must not be able to use it to pester a mailbox. +const MAX_SENDS_PER_WINDOW = 5 +const SEND_WINDOW_MINUTES = 60 + +function hashToken(raw) { + return crypto.createHash('sha256').update(String(raw)).digest('hex') +} + +// Create a verification for one user + address. Returns { id, token } — the +// plaintext token is returned ONCE, for the link, and is never recoverable after. +async function create({ userId, email, requestedIp, ttlMinutes = DEFAULT_TTL_MINUTES }) { + const token = crypto.randomBytes(32).toString('base64url') + const expiresAt = new Date(Date.now() + ttlMinutes * 60 * 1000) + const id = await db.insert({ tokenHash: hashToken(token), userId, email, requestedIp, expiresAt }) + return { id, token } +} + +// Resolve a pending, unexpired verification from its plaintext token, else null. +// Returns the RAW row (incl. user_id and the address it proves). +async function findValidByToken(token) { + if (!token) return null + const row = await db.findByTokenHash(hashToken(token)) + if (!row || row.status !== 'pending') return null + if (new Date(row.expires_at).getTime() < Date.now()) return null + return row +} + +// Atomically consume a pending verification (double-use-safe). True if this call +// won the race. +async function consume(id) { + return (await db.markUsed(id)) === 1 +} + +const invalidatePendingForUser = (userId) => db.invalidatePendingForUser(userId) + +// True when this user has already asked for as many verification mails as the +// window allows. +async function sendQuotaExhausted(userId) { + const since = new Date(Date.now() - SEND_WINDOW_MINUTES * 60 * 1000) + return (await db.countRecentForUser(userId, since)) >= MAX_SENDS_PER_WINDOW +} + +module.exports = { + create, + findValidByToken, + consume, + invalidatePendingForUser, + sendQuotaExhausted, + hashToken, + DEFAULT_TTL_MINUTES, + MAX_SENDS_PER_WINDOW, + SEND_WINDOW_MINUTES, +} diff --git a/server/src/model/settings/settings.model.js b/server/src/model/settings/settings.model.js index ded35a8..14ae934 100644 --- a/server/src/model/settings/settings.model.js +++ b/server/src/model/settings/settings.model.js @@ -67,6 +67,30 @@ function registrationFlags(mode) { } } +// Engagement Phase 1b — may an UNVERIFIED address receive opt-in engagement mail? +// Stored as 'on'/'off'. Seeded by schema.sql ASYMMETRICALLY on purpose: 'on' for a +// fresh install, 'off' for an upgrade. Turning it on retroactively would silently +// stop mailing every already-opted-in user on the day the operator upgraded, which +// is the G22 mistake — a safe default must not be applied backwards to a running +// system without telling anyone. +// +// Nothing CONSUMES this yet: the engine that would honour it arrives in Phase 4 +// and the deliverability rules in Phase 9. It is seeded and editable here because +// the fresh-vs-upgrade distinction is only knowable at the migration that adds it, +// and reconstructing "was this install fresh?" later is guesswork. +const EMAIL_VERIFICATION_KEY = 'email_verification_required' + +// Fail-safe direction is 'off': an unreadable or missing value must not silently +// suppress mail an operator believes is going out. The loud failure mode (mail +// reaching an unverified address) is recoverable; the quiet one is not. +async function isEmailVerificationRequired() { + try { + return String(await settingsDb.get(EMAIL_VERIFICATION_KEY)) === 'on' + } catch { + return false + } +} + // Android App Links opt-in (M9 follow-up). When on, the shard auto-serves // /.well-known/assetlinks.json and the mobile SSO bridge additionally accepts the // self-origin https:///mobile/callback redirect. Stored as the string @@ -256,6 +280,8 @@ module.exports = { REGISTRATION_KEY, REGISTRATION_MODES, getRegistrationMode, + EMAIL_VERIFICATION_KEY, + isEmailVerificationRequired, registrationFlags, MOBILE_APP_LINKS_KEY, isMobileAppLinksEnabled, diff --git a/server/src/model/users/users.db.js b/server/src/model/users/users.db.js index fd888cb..495c697 100644 --- a/server/src/model/users/users.db.js +++ b/server/src/model/users/users.db.js @@ -1,7 +1,7 @@ const { query } = require('../../utils/db') const PUBLIC_COLS = - 'id, username, role, status, email, email_verified, totp_enabled, created_at, last_login_at' + 'id, username, role, status, email, email_verified, email_pending, totp_enabled, created_at, last_login_at' // passwordHash may be null (SSO-provisioned players who have not set one yet). // email/status/emailVerified are optional so existing admin-create callers are @@ -31,17 +31,47 @@ async function findById(id) { return rows[0] || null } -// All ACTIVE accounts on an email address. Email is intentionally non-unique -// (SSO emails may repeat), so a reset request can legitimately match several +// The ACTIVE account on an email address, as a list. Unique since engagement +// Phase 1b, so this returns at most one row — the array shape is kept because the +// password-reset caller iterates and there is nothing to gain from making it care. // accounts; the caller issues one reset link per row. Case-insensitive to match // however the address was stored. Excludes disabled/banned accounts. +// Active accounts on an address. Matches on email_norm, the same generated column +// the UNIQUE index uses, so a lookup folds case exactly the way uniqueness does — +// LOWER() here and LOWER() there can never drift apart. Since Phase 1b this +// returns at most one row; it still returns an array because the password-reset +// caller iterates and there is no value in making that caller care. async function findActiveByEmail(email) { return query( - "SELECT * FROM users WHERE email = ? AND status = 'active'", + "SELECT * FROM users WHERE email_norm = LOWER(?) AND status = 'active'", [email], ) } +// Stage an address the user has asked for but not yet proved. Does not touch +// `email`, so their current address keeps receiving mail until the link is used. +async function setPendingEmail(id, email) { + const res = await query('UPDATE users SET email_pending = ? WHERE id = ?', [email, id]) + return res.affectedRows || 0 +} + +// Promote a proved address into place. Guarded on email_pending still matching, so +// a stale link (the user asked again for a different address) cannot install the +// address it was minted for. Returns rows changed — 0 means the guard rejected it. +async function promotePendingEmail(id, email) { + const res = await query( + `UPDATE users SET email = ?, email_verified = 1, email_pending = NULL + WHERE id = ? AND email_pending = ?`, + [email, id, email], + ) + return res.affectedRows || 0 +} + +async function clearPendingEmail(id) { + const res = await query('UPDATE users SET email_pending = NULL WHERE id = ?', [id]) + return res.affectedRows || 0 +} + async function listUsers() { return query(`SELECT ${PUBLIC_COLS} FROM users ORDER BY id ASC`) } @@ -112,6 +142,9 @@ module.exports = { findByUsername, findById, findActiveByEmail, + setPendingEmail, + promotePendingEmail, + clearPendingEmail, listUsers, updateUser, deleteUser, diff --git a/server/src/model/users/users.model.js b/server/src/model/users/users.model.js index cee8717..01adef8 100644 --- a/server/src/model/users/users.model.js +++ b/server/src/model/users/users.model.js @@ -18,13 +18,54 @@ async function createUser({ username, password, role = 'admin', email = null, st return sanitize(await usersDb.findById(id)) } -// True when a DB error is the unique-index violation on username (the atomic -// backstop for the uniqueness race). Callers translate this into a 409 rather -// than doing a check-then-write. -function isDuplicateUsername(err) { +// ── Telling the two unique constraints apart ─────────────────────────────── +// +// `users` has had one unique index (username) for its whole life, so a bare +// "is this a duplicate-key error" test was enough. Engagement Phase 1b adds a +// second (email, via the generated email_norm column), and the moment it exists +// an undiscriminating test starts LYING: a duplicate email would be reported to +// the user as a taken username, and SSO provisioning would retry usernames +// forever against a conflict no username can clear (§0.6 finding 2). +// +// The violated index name is available ONLY in the driver's message text — the +// mariadb connector exposes no structured field for it — so this reads it back +// out. Verified against MariaDB 11.8: +// "(conn:60, no: 1062, SQLState: 23000) Duplicate entry 'x' for key 'username'" +// +// NOTE the message also embeds the bound parameters, so on an email collision it +// contains the address. That is fine in a server log and is exactly why these +// errors must never be echoed to a client (the anti-enumeration rule below). +const EMAIL_UNIQUE_KEY = 'uq_users_email_norm' + +function isDuplicateKeyError(err) { return Boolean(err && (err.code === 'ER_DUP_ENTRY' || err.errno === 1062)) } +// The name of the unique index that was violated, or null if this is not a +// duplicate-key error (or the driver phrased it in a way we do not recognise). +function duplicateKey(err) { + if (!isDuplicateKeyError(err)) return null + const m = /for key '([^']+)'/.exec(err.sqlMessage || err.message || '') + return m ? m[1] : null +} + +// True when the collision was on the email uniqueness index. +function isDuplicateEmail(err) { + return duplicateKey(err) === EMAIL_UNIQUE_KEY +} + +// True when a DB error is a unique-index violation that is NOT the email one (the +// atomic backstop for the username-uniqueness race). Callers translate this into +// a 409 rather than doing a check-then-write. +// +// Deliberately "not email" rather than "is username": on a database whose index +// happens to carry a different name, the old permissive behaviour is preserved +// and nothing newly falls through to a 500. Only the case we can positively +// identify — email — is carved out. +function isDuplicateUsername(err) { + return isDuplicateKeyError(err) && !isDuplicateEmail(err) +} + // Returns the raw row (incl. hash) — used by login only. async function getRawByUsername(username) { return usersDb.findByUsername(username) @@ -35,7 +76,7 @@ async function getById(id) { } // Raw rows (incl. email/status) for every active account on an email address. -// Server-side only (password-reset request); email is non-unique so this may +// Server-side only (password-reset request). Unique since Phase 1b, so this // return several. Never sent to a client. async function getActiveByEmail(email) { if (!email) return [] @@ -84,6 +125,20 @@ async function update(id, { username, password, role, email, status, emailVerifi return getById(id) } +// ── Pending email address (engagement Phase 1b) ──────────────────────────── +// A requested address is staged rather than installed: `email` keeps working +// until the verification link proves the new one. See the users table comments. +const setPendingEmail = (id, email) => usersDb.setPendingEmail(id, email) +const clearPendingEmail = (id) => usersDb.clearPendingEmail(id) + +// Promote a proved address. Returns true only if it actually landed; false means +// the guard rejected it (the user has since asked for a different address, so the +// token in hand is stale). Throws the duplicate-key error if the address was +// claimed by someone else in the meantime — the caller answers that generically. +async function promotePendingEmail(id, email) { + return (await usersDb.promotePendingEmail(id, email)) === 1 +} + // Invalidate every session token this user currently holds ("log out everywhere") // by advancing their tokens_valid_after cutoff to now. async function invalidateSessions(id) { @@ -115,9 +170,15 @@ async function recordLogin(id, ip = null) { module.exports = { createUser, isDuplicateUsername, + isDuplicateEmail, + duplicateKey, + EMAIL_UNIQUE_KEY, getRawByUsername, getById, getActiveByEmail, + setPendingEmail, + clearPendingEmail, + promotePendingEmail, getRawById, validatePassword, list, diff --git a/server/src/router/v1/admin/admin.controller.js b/server/src/router/v1/admin/admin.controller.js index e542700..8447d3a 100644 --- a/server/src/router/v1/admin/admin.controller.js +++ b/server/src/router/v1/admin/admin.controller.js @@ -10,6 +10,7 @@ const recoveryCodes = require('../../../model/recoveryCodes/recoveryCodes.model' const registries = require('../../../modules/registries') const announceJobs = require('../../../model/announceJobs/announceJobs.model') const emailConfig = require('../../../model/emailConfig/emailConfig.model') +const emailDedupe = require('../../../model/emailDedupe/emailDedupe.model') const forumSettings = require('../../../model/teams/teamForumSettings.model') const pushDispatch = require('../../../utils/pushDispatch') const { cleanBody } = require('../../../utils/sanitizeHtml') @@ -89,6 +90,60 @@ async function emailWarning() { } } +// Phase 1b — the de-duplication cleared some accounts' addresses so a UNIQUE +// index could go on (ENGAGEMENT.md Phase 1b / §0.6 finding 1). Same posture as +// the email warning above: narrow, self-clearing, and silent on the installs it +// does not concern. +// +// It has to be said out loud for the same reason G22 did. Nothing broke visibly — +// those users can still log in — but they can no longer receive password-reset or +// engagement mail, and they are the only people who can fix that, so somebody has +// to tell the operator to go and ask them. +// +// Never fails the dashboard. +async function emailDedupeWarning() { + try { + const n = await emailDedupe.countUnacknowledged() + if (!n) return null + return { + code: 'EMAIL_DEDUPE', + message: + `${n} account${n === 1 ? '' : 's'} shared an email address with another account and had it ` + + 'cleared when addresses became unique. They can still sign in, but cannot receive password-reset ' + + 'or notification email until they set a new address. Review who was affected and contact them.', + href: '/admin/users', + } + } catch (err) { + log.warn('dashboard email dedupe warning check failed', { message: err.message }) + return null + } +} + +// GET /admin/users/email-dedupe-report — who was cleared, and what they lost. +async function emailDedupeReport(req, res) { + try { + return res.json(await emailDedupe.list()) + } catch (err) { + log.error('emailDedupeReport', err) + return res.status(500).json({ message: 'Internal Server Error' }) + } +} + +// POST /admin/users/email-dedupe-report/acknowledge — dismiss the warning. The +// rows stay: the report is a record of what the upgrade did, and losing it would +// leave no way to answer "why does this user have no address?" later. +async function acknowledgeEmailDedupeReport(req, res) { + try { + const n = await emailDedupe.acknowledgeAll() + await activity.log({ req, action: 'admin.email_dedupe.acknowledge', detail: { count: n } }) + log.info('email dedupe report acknowledged', { count: n, by: req.user.username }) + return res.json({ ok: true, acknowledged: n }) + } catch (err) { + log.error('acknowledgeEmailDedupeReport', err) + return res.status(500).json({ message: 'Internal Server Error' }) + } +} + async function dashboard(req, res) { try { return res.json({ @@ -101,7 +156,7 @@ async function dashboard(req, res) { posts: await posts.counts(), users: await users.count(), }, - warnings: [await emailWarning()].filter(Boolean), + warnings: [await emailWarning(), await emailDedupeWarning()].filter(Boolean), recent_activity: await activity.list({ limit: 10 }), }) } catch (err) { @@ -562,6 +617,14 @@ async function updateSettings(req, res) { // endpoint takes arbitrary keys either way, and an unrecognised value resolves // to `disabled` on read — the module's gate fails closed, which is the right // direction for "may this player mint a game account". + // Engagement Phase 1b verification gate: 'on'/'off' only, so a typo cannot land + // a value that reads as neither and silently resolves to off. + if (settings.EMAIL_VERIFICATION_KEY in updates) { + const v = updates[settings.EMAIL_VERIFICATION_KEY] + if (v !== 'on' && v !== 'off') { + return res.status(400).json({ message: 'Invalid email_verification_required value' }) + } + } // App Links toggle is a boolean stored as a 'true'/'false' string; accept a real // boolean or those two strings and normalize, reject anything else. if (settings.MOBILE_APP_LINKS_KEY in updates) { @@ -847,13 +910,27 @@ async function createUser(req, res) { if (await users.getRawByUsername(req.body.username)) { return res.status(409).json({ message: 'Username already taken' }) } - const user = await users.createUser({ - username: req.body.username, - password: req.body.password, - role: req.body.role || 'admin', - email: req.body.email || null, - status: req.body.status || 'active', - }) + let user + try { + user = await users.createUser({ + username: req.body.username, + password: req.body.password, + role: req.body.role || 'admin', + email: req.body.email || null, + status: req.body.status || 'active', + }) + } catch (err) { + // Before Phase 1b this had no catch at all, so a duplicate address became + // an opaque 500 for an admin who could see nothing wrong with the form. + // An admin may be told the real reason: they can already list every account. + if (users.isDuplicateEmail(err)) { + return res.status(409).json({ message: 'Another account already uses that email address.' }) + } + if (users.isDuplicateUsername(err)) { + return res.status(409).json({ message: 'Username already taken' }) + } + throw err + } await activity.log({ req, action: 'user.create', @@ -885,13 +962,25 @@ async function updateUser(req, res) { return res.status(400).json({ message: 'Cannot demote the last admin' }) } } - const user = await users.update(id, { - username: req.body.username, - password: req.body.password, - role: req.body.role, - email: req.body.email, - status: req.body.status, - }) + let user + try { + user = await users.update(id, { + username: req.body.username, + password: req.body.password, + role: req.body.role, + email: req.body.email, + status: req.body.status, + }) + } catch (err) { + // Same as createUser: an uncaught duplicate address was an opaque 500. + if (users.isDuplicateEmail(err)) { + return res.status(409).json({ message: 'Another account already uses that email address.' }) + } + if (users.isDuplicateUsername(err)) { + return res.status(409).json({ message: 'Username already taken' }) + } + throw err + } await activity.log({ req, action: 'user.update', detail: { id } }) // Distinct audit trail for the security-sensitive fields (role & status), // so a promotion/ban is greppable beyond the generic user.update entry. @@ -1055,6 +1144,8 @@ module.exports = { getUser, createUser, updateUser, + emailDedupeReport, + acknowledgeEmailDedupeReport, deleteUser, listUserTrustedDevices, revokeUserTrustedDevice, diff --git a/server/src/router/v1/admin/invites.controller.js b/server/src/router/v1/admin/invites.controller.js index 08feb26..9fb057e 100644 --- a/server/src/router/v1/admin/invites.controller.js +++ b/server/src/router/v1/admin/invites.controller.js @@ -7,6 +7,7 @@ // isn't configured); the DB stores only its hash. const invites = require('../../../model/invites/invites.model') +const users = require('../../../model/users/users.model') const activity = require('../../../model/activity/activity.model') const mailer = require('../../../utils/mailer') @@ -34,6 +35,18 @@ async function create(req, res) { return res.status(400).json({ message: 'A valid email and role are required.' }) } try { + // Catch a collision HERE rather than at accept time (Phase 1b decision 1). + // Uniqueness makes an invite to an already-held address unfulfillable, and + // discovering that after the invitee has clicked the link and chosen a + // password is a bad place to find out. Telling an authenticated admin that + // one of their own users holds an address is not the enumeration surface the + // public register form is — the admin can already list every account. + const existing = await users.getActiveByEmail(email) + if (existing.length) { + log.info('invite refused: address already held', { email, by: req.user.username }) + return res.status(409).json({ message: 'An account already uses that email address.' }) + } + const { invite, token } = await invites.create({ email, role, invitedBy: req.user.id }) const url = acceptUrl(token) diff --git a/server/src/router/v1/admin/invites.router.js b/server/src/router/v1/admin/invites.router.js index 5c49495..927cd19 100644 --- a/server/src/router/v1/admin/invites.router.js +++ b/server/src/router/v1/admin/invites.router.js @@ -22,6 +22,7 @@ invitesRouter.post( /* #swagger.requestBody = { required: true, content: { "application/json": { schema: { type: "object", required: ["email","role"], properties: { email: { type: "string" }, role: { type: "string" } } } } } } */ /* #swagger.responses[201] = { description: 'Invite created', content: { "application/json": { schema: { type: "object", additionalProperties: true } } } } */ /* #swagger.responses[400] = { description: 'Validation error', content: { "application/json": { schema: { $ref: "#/components/schemas/ValidationError" } } } } */ + /* #swagger.responses[409] = { description: 'An account already uses that email address. Addresses are unique, so such an invite could never be accepted; it is refused here rather than at accept time, after the invitee has clicked the link.', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ adminOnly, body('email').isEmail().isLength({ max: 255 }), body('role').isIn(['admin', 'editor', 'moderator', 'player']), diff --git a/server/src/router/v1/admin/users.router.js b/server/src/router/v1/admin/users.router.js index 2a915aa..4b9b9cc 100644 --- a/server/src/router/v1/admin/users.router.js +++ b/server/src/router/v1/admin/users.router.js @@ -30,6 +30,31 @@ usersRouter.get( /* #swagger.responses[403] = { description: 'Admin role required', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ ctrl.listUsers, ) +// The Phase 1b de-duplication report. Declared BEFORE '/:id' — Express matches in +// order, so a literal segment registered after a parameterised one is never +// reached ('email-dedupe-report' would bind as :id). +usersRouter.get( + '/email-dedupe-report', + // #swagger.tags = ['Admin · Users'] + // #swagger.summary = 'Accounts whose email was cleared by de-duplication (admin only)' + // #swagger.description = 'When email addresses became unique, accounts sharing an address kept only the earliest-created one; the rest had their address cleared. These users can still sign in but cannot receive password-reset or notification email until they set a new address, so they are the ones to contact.' + // #swagger.security = [{ "cookieAuth": [] }, { "bearerAuth": [] }] + /* #swagger.responses[200] = { description: 'The affected accounts', content: { "application/json": { schema: { type: "array", items: { $ref: "#/components/schemas/EmailDedupeEntry" } } } } } */ + /* #swagger.responses[401] = { description: 'Not authenticated', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + /* #swagger.responses[403] = { description: 'Admin role required', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + ctrl.emailDedupeReport, +) +usersRouter.post( + '/email-dedupe-report/acknowledge', + // #swagger.tags = ['Admin · Users'] + // #swagger.summary = 'Dismiss the de-duplication warning (admin only)' + // #swagger.description = 'Marks the report acknowledged so it stops appearing as a dashboard warning. The rows are kept as a record of what the upgrade did.' + // #swagger.security = [{ "cookieAuth": [] }, { "bearerAuth": [] }] + /* #swagger.responses[200] = { description: 'Acknowledged', content: { "application/json": { schema: { type: "object", properties: { ok: { type: "boolean" }, acknowledged: { type: "integer" } } } } } } */ + /* #swagger.responses[401] = { description: 'Not authenticated', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + /* #swagger.responses[403] = { description: 'Admin role required', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + ctrl.acknowledgeEmailDedupeReport, +) usersRouter.post( '/', // #swagger.tags = ['Admin · Users'] diff --git a/server/src/router/v1/auth/account.controller.js b/server/src/router/v1/auth/account.controller.js index e260571..9329020 100644 --- a/server/src/router/v1/auth/account.controller.js +++ b/server/src/router/v1/auth/account.controller.js @@ -10,6 +10,7 @@ // two surfaces were deleted. const users = require('../../../model/users/users.model') +const emailVerifications = require('../../../model/emailVerifications/emailVerifications.model') const activity = require('../../../model/activity/activity.model') const userIdentities = require('../../../model/userIdentities/userIdentities.model') const mobileSessions = require('../../../model/mobileSessions/mobileSessions.model') @@ -22,6 +23,7 @@ const usernamePolicy = require('../../../auth/usernamePolicy') const loginProtection = require('../../../middleware/loginProtection') const botScore = require('../../../middleware/botScore') const totp = require('../../../utils/totp') +const mailer = require('../../../utils/mailer') const log = require('../../../utils/logger')('account') @@ -37,6 +39,10 @@ async function getAccount(req, res) { username: req.user.username, role: req.user.role, email: req.user.email || null, + email_verified: Boolean(req.user.email_verified), + // The address awaiting its link, so the screen can say "check your inbox" + // rather than looking as though the change silently failed. + email_pending: (raw && raw.email_pending) || null, status: req.user.status || 'active', totp_enabled: Boolean(req.user.totp_enabled), has_password: Boolean(raw && raw.password_hash), @@ -133,6 +139,127 @@ async function changePassword(req, res) { } } +// ── Email address (engagement Phase 1b) ──────────────────────────────────── + +function baseUrl() { + return (process.env.APP_BASE_URL || 'http://localhost:5173').replace(/\/+$/, '') +} + +function verifyUrl(token) { + return `${baseUrl()}/account/verify-email/${token}` +} + +// Mint a verification and mail it. Shared by the change and resend paths so the +// quota, the supersede and the log line cannot drift between them. Returns a +// { ok } or { ok: false, status, message } the caller can hand straight back. +async function issueVerification(req, email) { + if (await emailVerifications.sendQuotaExhausted(req.user.id)) { + log.warn('email verification quota exhausted', { id: req.user.id, ip: req.ip }) + return { ok: false, status: 429, message: 'Too many verification emails. Try again later.' } + } + // A fresh request supersedes every older link — otherwise an address the user + // typed by mistake stays installable for a day. + await emailVerifications.invalidatePendingForUser(req.user.id) + const { token } = await emailVerifications.create({ userId: req.user.id, email, requestedIp: req.ip }) + try { + const result = await mailer.sendEmailVerification({ to: email, verifyUrl: verifyUrl(token), username: req.user.username }) + if (!result.sent) { + // Unlike a password reset there is no enumeration reason to pretend: the + // caller typed this address themselves and is entitled to know why nothing + // arrived. The pending address stays staged so a later resend works. + log.warn('verification email not sent (mail not configured)', { id: req.user.id }) + return { ok: true, emailed: false, reason: 'NOT_CONFIGURED' } + } + } catch (err) { + log.error('verification send failed', err) + return { ok: true, emailed: false, reason: 'SEND_FAILED' } + } + return { ok: true, emailed: true } +} + +// PATCH /account/email - ask to set or change the caller's own address. +// +// The address is STAGED, not installed: `email` keeps receiving mail until the +// link is used, so a typo cannot silently redirect this account's password-reset +// mail to a mailbox its owner does not control. +// +// The current password is required when the account has one. An address is where +// account recovery lands, so repointing it is a credential-grade act; an +// SSO-provisioned account with no password hash is exempt, exactly as +// changePassword already carves out. +async function changeEmail(req, res) { + const email = String(req.body.email || '').trim() + try { + const raw = await users.getRawById(req.user.id) + if (!raw) return res.status(401).json({ message: 'Unauthorized' }) + + if (raw.password_hash) { + const ok = await users.validatePassword(raw, req.body.currentPassword || '') + if (!ok) { + loginProtection.recordFailure(req.ip) + botScore.recordLoginFailure(req.ip) + log.warn('changeEmail wrong current password', { id: req.user.id, ip: req.ip }) + return res.status(400).json({ message: 'Your current password is incorrect.' }) + } + } + + if (raw.email && raw.email.toLowerCase() === email.toLowerCase()) { + return res.status(400).json({ message: 'That is already your email address.' }) + } + + // Stage it. This is also where a collision with a live address FIRST shows up + // cheaply, but it is not the guard that matters - email_pending is deliberately + // not unique, so the real arbitration happens at verification time against the + // UNIQUE index. Answering identically in both places is what keeps this from + // becoming an address-existence oracle. + await users.setPendingEmail(req.user.id, email) + + const issued = await issueVerification(req, email) + if (!issued.ok) return res.status(issued.status).json({ message: issued.message }) + + await activity.log({ req, action: 'account.email.change_requested' }) + log.info('account email change requested', { id: req.user.id }) + return res.json({ email_pending: email, emailed: Boolean(issued.emailed), reason: issued.reason || null }) + } catch (err) { + log.error('changeEmail', err) + return res.status(500).json({ message: 'Internal Server Error' }) + } +} + +// POST /account/email/resend - re-send the link for the address already staged. +async function resendEmailVerification(req, res) { + try { + const raw = await users.getRawById(req.user.id) + if (!raw) return res.status(401).json({ message: 'Unauthorized' }) + if (!raw.email_pending) { + return res.status(400).json({ message: 'There is no email address awaiting confirmation.' }) + } + const issued = await issueVerification(req, raw.email_pending) + if (!issued.ok) return res.status(issued.status).json({ message: issued.message }) + log.info('account email verification resent', { id: req.user.id }) + return res.json({ email_pending: raw.email_pending, emailed: Boolean(issued.emailed), reason: issued.reason || null }) + } catch (err) { + log.error('resendEmailVerification', err) + return res.status(500).json({ message: 'Internal Server Error' }) + } +} + +// DELETE /account/email/pending - abandon a staged address (a typo, or a change +// of mind). Retires the outstanding links too, so the abandoned address cannot be +// installed afterwards by a link already sitting in a mailbox. +async function cancelEmailChange(req, res) { + try { + await users.clearPendingEmail(req.user.id) + await emailVerifications.invalidatePendingForUser(req.user.id) + await activity.log({ req, action: 'account.email.change_cancelled' }) + log.info('account email change cancelled', { id: req.user.id }) + return res.json({ ok: true }) + } catch (err) { + log.error('cancelEmailChange', err) + return res.status(500).json({ message: 'Internal Server Error' }) + } +} + // Step 1: generate a fresh secret (stored but not yet enabled) and return the // otpauth URL + a QR data URL for the user to scan. Overwrites any pending, // not-yet-confirmed secret. Refuses if TOTP is already enabled. @@ -396,6 +523,9 @@ async function generateRecoveryCodes(req, res) { } module.exports = { + changeEmail, + resendEmailVerification, + cancelEmailChange, getAccount, changeUsername, changePassword, diff --git a/server/src/router/v1/auth/auth.controller.js b/server/src/router/v1/auth/auth.controller.js index 3adcd88..7760abb 100644 --- a/server/src/router/v1/auth/auth.controller.js +++ b/server/src/router/v1/auth/auth.controller.js @@ -132,6 +132,20 @@ async function register(req, res) { role: 'player', }) } catch (err) { + // Two unique indexes, two different answers (§0.6 finding 2). Before Phase + // 1b this branch caught both and told an email collision it was a username + // one — the single field the user had NOT collided on. + // + // The email answer is deliberately generic and deliberately NOT scored: a + // truthful "that address already has an account" makes account existence + // queryable through a public form, and treating an honest typo on a + // colleague's address as an attack would push a legitimate user toward an + // IP ban. The real reason is logged and never returned — note the driver's + // message embeds the address, which is a second reason it stays server-side. + if (users.isDuplicateEmail(err)) { + log.warn('register rejected: email already registered', { username: check.name, ip: req.ip }) + return res.status(400).json({ message: 'Registration failed. Please check your details and try again.' }) + } // The UNIQUE index is the source of truth for the uniqueness race — a // concurrent duplicate loses here and gets a clean 409. if (users.isDuplicateUsername(err)) { diff --git a/server/src/router/v1/auth/emailVerify.controller.js b/server/src/router/v1/auth/emailVerify.controller.js new file mode 100644 index 0000000..5beff09 --- /dev/null +++ b/server/src/router/v1/auth/emailVerify.controller.js @@ -0,0 +1,100 @@ +// ── Email-address verification (public, token-gated) ─────────────────────── +// +// The confirm half of the Phase 1b change-and-verify flow. The request half is +// authenticated and lives on /auth/me/account/email; this half is deliberately +// NOT, because the link is opened from a mailbox, routinely on a device that is +// not logged in — requiring a session here would strand exactly the users the +// flow exists to serve. +// +// That is safe because the token IS the proof: it is opaque, single-use, +// short-lived, stored only as a sha256, and it carries the user and the address +// it was minted for. Using it installs an address on that account and does +// nothing else — it grants no session, no access, and no way to read anything. +// Compare passwordReset.controller, which is the same posture for a strictly +// more powerful capability. +// +// GET /auth/email/verify/:token -> validate the link so the page can render +// POST /auth/email/verify/:token -> install the address + +const emailVerifications = require('../../../model/emailVerifications/emailVerifications.model') +const users = require('../../../model/users/users.model') +const activity = require('../../../model/activity/activity.model') + +const log = require('../../../utils/logger')('auth-email-verify') + +const INVALID = 'This confirmation link is invalid or has expired.' + +// GET /auth/email/verify/:token — validate a link so the page can render. 404 for +// anything not currently usable, never distinguishing expired from used from +// never-was. +async function lookup(req, res) { + try { + const row = await emailVerifications.findValidByToken(req.params.token) + if (!row) return res.status(404).json({ message: INVALID }) + const user = await users.getById(row.user_id) + if (!user) return res.status(404).json({ message: INVALID }) + // The address is echoed because the person holding this link is the person it + // was mailed to — they already know it. The username tells them which account + // they are about to attach it to. + return res.json({ username: user.username, email: row.email }) + } catch (err) { + log.error('lookup', err) + return res.status(500).json({ message: 'Internal Server Error' }) + } +} + +// POST /auth/email/verify/:token — install the address. +async function confirm(req, res) { + try { + const row = await emailVerifications.findValidByToken(req.params.token) + if (!row) return res.status(404).json({ message: INVALID }) + + // Consume first: if we lost a double-submit race, stop before touching the + // account so a spent link cannot be replayed. + const won = await emailVerifications.consume(row.id) + if (!won) return res.status(404).json({ message: INVALID }) + + let installed + try { + installed = await users.promotePendingEmail(row.user_id, row.email) + } catch (err) { + // The UNIQUE index is the arbiter, and it fires here rather than at request + // time because a pending address reserves nothing: between staging and + // confirming, someone else may have verified the same address first. + // + // ANTI-ENUMERATION: the answer is the generic INVALID, identical to an + // expired or already-used link. Saying "that address is taken" would turn + // this endpoint into an oracle for which addresses hold accounts — the same + // posture passwordReset.controller keeps. The real reason is logged, never + // returned; note the driver's message embeds the address, which is another + // reason it must not travel to a client. + if (users.isDuplicateEmail(err)) { + log.warn('email verification lost to an existing address', { userId: row.user_id }) + await users.clearPendingEmail(row.user_id).catch(() => {}) + return res.status(404).json({ message: INVALID }) + } + throw err + } + + // The guard rejected it: the user has since asked for a different address, so + // this token is stale even though it had not expired. Same generic answer. + if (!installed) { + log.info('email verification superseded by a later request', { userId: row.user_id }) + return res.status(404).json({ message: INVALID }) + } + + // Retire any other outstanding links for this user — one address is now proved + // and the others must not be installable behind the user's back. + await emailVerifications.invalidatePendingForUser(row.user_id) + + await activity.log({ req, userId: row.user_id, action: 'account.email.verified' }) + log.info('account email verified', { userId: row.user_id, ip: req.ip }) + // No session is issued: this proves control of a mailbox, not of an account. + return res.json({ ok: true, message: 'Your email address has been confirmed.' }) + } catch (err) { + log.error('confirm', err) + return res.status(500).json({ message: 'Internal Server Error' }) + } +} + +module.exports = { lookup, confirm } diff --git a/server/src/router/v1/auth/emailVerify.router.js b/server/src/router/v1/auth/emailVerify.router.js new file mode 100644 index 0000000..49c6dc9 --- /dev/null +++ b/server/src/router/v1/auth/emailVerify.router.js @@ -0,0 +1,50 @@ +// Auth · Email — the confirm half of the self-service email change. Public but +// token-gated: validate a link, then install the address it proves. +// +// Mounted at /api/v1/auth/email by auth/index.js, so the routes below emit +// GET|POST /auth/email/verify/:token. +// +// Requesting a change is a different, AUTHENTICATED route — +// PATCH /auth/me/account/email. This half is unauthenticated on purpose: the link +// is opened from a mailbox, often on a device with no session. +// +// One anti-enumeration property is load-bearing and must survive any edit here: +// every unusable link answers with the same 404, and so does a link that lost the +// address to another account. Distinguishing "already taken" from "expired" would +// make this endpoint an oracle for which addresses hold accounts. + +const express = require('express') +const { param } = require('express-validator') + +const { lookup, confirm } = require('./emailVerify.controller') +const { emailVerifyConfirmLimiter } = require('../../../middleware/rateLimit') +const validate = require('../../../middleware/validate') + +const emailRouter = express.Router() + +emailRouter.get( + '/verify/:token', + // #swagger.tags = ['Auth'] + // #swagger.summary = 'Validate an email-confirmation link' + // #swagger.description = 'Returns the target username and the address the link proves, so the confirmation page can render. 404 for anything not currently usable (never distinguishes expired from used from never-existed).' + /* #swagger.responses[200] = { description: 'Confirmation link is valid', content: { "application/json": { schema: { type: "object", properties: { username: { type: "string" }, email: { type: "string", format: "email" } } } } } } */ + /* #swagger.responses[404] = { description: 'Invalid or expired confirmation link', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + param('token').isString().isLength({ min: 8, max: 128 }), + validate, + lookup, +) +emailRouter.post( + '/verify/:token', + // #swagger.tags = ['Auth'] + // #swagger.summary = 'Confirm an email address from its link' + // #swagger.description = 'Consumes the single-use link and installs the address on the account, marking it verified. Issues no session — it proves control of a mailbox, not of an account. Answers 404 for an unusable link AND for an address another account has since verified, deliberately: the two are indistinguishable to a caller so the endpoint cannot be used to test which addresses hold accounts.' + /* #swagger.responses[200] = { description: 'Address confirmed', content: { "application/json": { schema: { $ref: "#/components/schemas/Message" } } } } */ + /* #swagger.responses[404] = { description: 'Invalid, expired, superseded, or already-used confirmation link', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + /* #swagger.responses[429] = { description: 'Too many attempts', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + emailVerifyConfirmLimiter, + param('token').isString().isLength({ min: 8, max: 128 }), + validate, + confirm, +) + +module.exports = emailRouter diff --git a/server/src/router/v1/auth/index.js b/server/src/router/v1/auth/index.js index 58b78fc..325f991 100644 --- a/server/src/router/v1/auth/index.js +++ b/server/src/router/v1/auth/index.js @@ -26,6 +26,7 @@ const loginRouter = require('./login.router') const registerRouter = require('./register.router') const inviteRouter = require('./invite.router') const passwordRouter = require('./password.router') +const emailRouter = require('./emailVerify.router') const sessionRouter = require('./session.router') const authRouter = express.Router() @@ -54,6 +55,7 @@ authRouter.use('/login', loginRouter) authRouter.use('/register', registerRouter) authRouter.use('/invite', inviteRouter) authRouter.use('/password', passwordRouter) +authRouter.use('/email', emailRouter) // The two singletons that own no path segment of their own: POST /logout and // GET /me. Mounted at the group root and **last**, because `use('/me', …)` above diff --git a/server/src/router/v1/auth/invite.controller.js b/server/src/router/v1/auth/invite.controller.js index 33c0f6e..4bc9ca8 100644 --- a/server/src/router/v1/auth/invite.controller.js +++ b/server/src/router/v1/auth/invite.controller.js @@ -55,6 +55,20 @@ async function acceptInvite(req, res) { emailVerified: true, // they proved control of the address by using the link }) } catch (err) { + // The address on the invite is already held. Admin invites are checked for + // this at CREATION (POST /admin/invites 409s), so reaching here means the + // address was claimed in the window between the invite going out and the + // invitee clicking — a race, not the ordinary case. It still has to be + // survivable: the invitee has already clicked a link and typed a password, + // and an opaque 500 at that point is the worst possible moment to fail. + if (users.isDuplicateEmail(err)) { + log.warn('invite accept rejected: address already held', { inviteId: row.id }) + return res.status(409).json({ + message: + 'This invitation cannot be completed because its email address is already in use. ' + + 'Ask an administrator for a new invitation.', + }) + } if (users.isDuplicateUsername(err)) { return res.status(409).json({ message: 'That username is already taken.' }) } diff --git a/server/src/router/v1/auth/me.routes.js b/server/src/router/v1/auth/me.routes.js index f4777e2..d3e8af8 100644 --- a/server/src/router/v1/auth/me.routes.js +++ b/server/src/router/v1/auth/me.routes.js @@ -79,6 +79,50 @@ meRouter.patch( account.changePassword, ) +// Email address (engagement Phase 1b). The change is STAGED and only a tokened +// link installs it, so these three routes never alter the address that is +// currently receiving mail. The confirm half is public and lives at +// /auth/email/verify/:token, because the link is opened from a mailbox. +meRouter.patch( + '/account/email', + // #swagger.tags = ['Auth · Me'] + // #swagger.summary = 'Request a new email address (self, any role)' + // #swagger.description = 'Stages the address and emails a confirmation link. The account keeps its current address until that link is used, so a mistyped address cannot redirect password-reset mail. Requires currentPassword when the account has a password; SSO-provisioned accounts with no password are exempt.' + // #swagger.security = [{ "cookieAuth": [] }, { "bearerAuth": [] }] + /* #swagger.requestBody = { required: true, content: { "application/json": { schema: { $ref: "#/components/schemas/ChangeEmailRequest" } } } } */ + /* #swagger.responses[200] = { description: 'Address staged; a confirmation link was sent', content: { "application/json": { schema: { $ref: "#/components/schemas/PendingEmail" } } } } */ + /* #swagger.responses[400] = { description: 'Validation error, wrong current password, or already your address', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + /* #swagger.responses[401] = { description: 'Not authenticated', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + /* #swagger.responses[429] = { description: 'Too many verification emails', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + accountChangeLimiter, + body('email').isString().trim().isEmail().isLength({ max: 255 }), + body('currentPassword').optional({ values: 'falsy' }).isString(), + validate, + account.changeEmail, +) +meRouter.post( + '/account/email/resend', + // #swagger.tags = ['Auth · Me'] + // #swagger.summary = 'Re-send the confirmation link for the pending address' + // #swagger.security = [{ "cookieAuth": [] }, { "bearerAuth": [] }] + /* #swagger.responses[200] = { description: 'Confirmation link re-sent', content: { "application/json": { schema: { $ref: "#/components/schemas/PendingEmail" } } } } */ + /* #swagger.responses[400] = { description: 'No address is awaiting confirmation', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + /* #swagger.responses[401] = { description: 'Not authenticated', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + /* #swagger.responses[429] = { description: 'Too many verification emails', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + accountChangeLimiter, + account.resendEmailVerification, +) +meRouter.delete( + '/account/email/pending', + // #swagger.tags = ['Auth · Me'] + // #swagger.summary = 'Abandon the pending email address' + // #swagger.description = 'Clears the staged address and retires its outstanding links, so a confirmation email already delivered can no longer install it.' + // #swagger.security = [{ "cookieAuth": [] }, { "bearerAuth": [] }] + /* #swagger.responses[200] = { description: 'Pending address cleared', content: { "application/json": { schema: { $ref: "#/components/schemas/OkFlag" } } } } */ + /* #swagger.responses[401] = { description: 'Not authenticated', content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } } */ + account.cancelEmailChange, +) + // TOTP self-enrollment (disable requires a valid current code; it does not take // a password). meRouter.post( diff --git a/server/src/router/v1/auth/password.router.js b/server/src/router/v1/auth/password.router.js index 5759a8e..2da8cd8 100644 --- a/server/src/router/v1/auth/password.router.js +++ b/server/src/router/v1/auth/password.router.js @@ -29,7 +29,7 @@ passwordRouter.post( '/forgot', // #swagger.tags = ['Auth'] // #swagger.summary = 'Request a password-reset link by email' - // #swagger.description = 'Emails a single-use, ~1h reset link to every active account on the address. Always returns the same generic 200 whether or not the email matches (no account enumeration). Email is non-unique, so multiple accounts may each receive a link naming their username. Rate limited per IP.' + // #swagger.description = 'Emails a single-use, ~1h reset link to the active account on the address. Always returns the same generic 200 whether or not the email matches (no account enumeration). Rate limited per IP.' /* #swagger.requestBody = { required: true, content: { "application/json": { schema: { type: "object", required: ["email"], properties: { email: { type: "string", format: "email" } } } } } } */ /* #swagger.responses[200] = { description: 'Generic acknowledgement (sent if the account exists)', content: { "application/json": { schema: { $ref: "#/components/schemas/Message" } } } } */ /* #swagger.responses[400] = { description: 'Validation error', content: { "application/json": { schema: { $ref: "#/components/schemas/ValidationError" } } } } */ diff --git a/server/src/router/v1/auth/passwordReset.controller.js b/server/src/router/v1/auth/passwordReset.controller.js index c783848..a4d400e 100644 --- a/server/src/router/v1/auth/passwordReset.controller.js +++ b/server/src/router/v1/auth/passwordReset.controller.js @@ -5,13 +5,16 @@ // 2. GET /auth/password/reset/:token → validate the link (for the form) // 3. POST /auth/password/reset/:token { password } → set the new password // -// Email is intentionally non-unique (SSO emails may repeat), so a request can -// match several accounts; each gets its own link, and the email names the -// username so the recipient knows which account it's for. The request step NEVER -// reveals whether an address exists — it always returns the same generic success -// (no user enumeration). Only the sha256 hash of each opaque token is stored, so a -// DB read never yields a usable link (same pattern as user_invites). Tokens are -// single-use + expire in ~1h. Setting a new password rotates the hash and revokes +// Addresses are unique since engagement Phase 1b, so a request matches at most one +// account; the loop below is kept because it costs nothing and the email names the +// username anyway. The request step NEVER reveals whether an address exists — it +// always returns the same generic success (no user enumeration). Only the sha256 +// hash of each opaque token is stored, so a DB read never yields a usable link +// (same pattern as user_invites). Tokens are single-use + expire in ~1h. +// +// Reset mail is deliberately NOT gated on email_verified. The verification gate +// (Phase 1b) governs opt-in ENGAGEMENT mail; applying it to account recovery would +// lock out every user carrying an address from before verification existed. Setting a new password rotates the hash and revokes // every session (web cookie cutoff + mobile refresh tokens). We do NOT auto-log-in // afterwards: the user signs in fresh, so a 2FA account still passes TOTP. diff --git a/server/src/router/v1/auth/sso.controller.js b/server/src/router/v1/auth/sso.controller.js index cb51f18..794d5ac 100644 --- a/server/src/router/v1/auth/sso.controller.js +++ b/server/src/router/v1/auth/sso.controller.js @@ -192,8 +192,12 @@ async function callback(req, res) { // Auto-provision a `player` from an SSO profile when no identity is linked yet // and registration allows SSO sign-up. Derives a unique username (reserved-name // safe) with a bounded retry against the UNIQUE index, captures the provider -// email, links the identity, and audit-logs the provision. Returns the new user, -// or null if a unique username couldn't be found. +// email, links the identity, and audit-logs the provision. +// +// Returns { user } on success, or { error } naming why it failed. It used to +// return the user or a bare null, which was enough while username was the only +// unique index; since Phase 1b there are two ways to fail and they need different +// things said to the person in front of the browser. async function provisionSsoPlayer(req, providerId, profile) { const base = usernamePolicy.deriveUsernameBase(profile) for (let attempt = 0; attempt < PROVISION_MAX_TRIES; attempt++) { @@ -203,9 +207,17 @@ async function provisionSsoPlayer(req, providerId, profile) { username: candidate, role: 'player', email: profile.email || null, - // The built-in providers only return an email the IdP has verified, so - // treat a supplied address as verified (skips the eventual re-verify). - emailVerified: Boolean(profile.email), + // Honour what the IdP actually ASSERTED, not the mere presence of an + // address. The old `Boolean(profile.email)` marked every SSO address + // verified, which made email_verified too weak a signal to mean anything + // (§0.6 finding 3). An IdP that omits the claim leaves the address + // unverified and the user proves it through the ordinary flow. + // + // Forward-only, by decision: existing rows keep the verified flag they + // were given. Retroactively demoting live users is the G22 mistake — a + // safe default applied backwards to a running system without telling + // anyone. + emailVerified: profile.emailVerified === true, }) await userIdentities.link({ userId: user.id, @@ -215,8 +227,24 @@ async function provisionSsoPlayer(req, providerId, profile) { }) await activity.log({ req, userId: user.id, action: 'auth.sso.provision', detail: { provider: providerId } }) log.info('sso player provisioned', { provider: providerId, id: user.id, username: user.username }) - return user + return { user } } catch (err) { + // An EMAIL collision can never be cleared by trying another username, so + // retrying is not merely useless — it burns every candidate and returns + // null, and the log then blames usernames for a conflict that was never + // about them (§0.6 finding 2). Stop, and say which it was. + // + // This is not the enumeration surface the register form is: the caller has + // already authenticated with the IdP, and the address is one the IdP + // asserted for them. Naming the real reason here is what makes the failure + // diagnosable instead of opaque. + if (users.isDuplicateEmail(err)) { + log.warn('sso provision: address already held by another account', { + provider: providerId, + subject: profile.subject, + }) + return { error: 'email_in_use' } + } // Username collided with a concurrent/existing account — try the next // suffix. Any other error is real; propagate it. if (users.isDuplicateUsername(err)) continue @@ -224,7 +252,7 @@ async function provisionSsoPlayer(req, providerId, profile) { } } log.error('sso provision: exhausted username candidates', { provider: providerId, base }) - return null + return { error: 'error' } } // Trusted-device skip for the SSO paths — the exact analogue of the check in @@ -267,8 +295,9 @@ async function finishLogin(req, res, providerId, kind, tx, profile) { log.warn('sso login refused: no linked account', { provider: providerId }) return res.redirect(loginError('not_linked', portal)) } - user = await provisionSsoPlayer(req, providerId, profile) - if (!user) return res.redirect(loginError('error', portal)) + const provisioned = await provisionSsoPlayer(req, providerId, profile) + if (provisioned.error) return res.redirect(loginError(provisioned.error, portal)) + user = provisioned.user } // Status gate (parity with local login): a disabled/banned account can't @@ -384,12 +413,12 @@ async function resolveMobileSsoUser(req, res, sess, providerId, profile) { res.redirect(appError(sess, 'not_linked')) return null } - const user = await provisionSsoPlayer(req, providerId, profile) - if (!user) { - res.redirect(appError(sess, 'error')) + const provisioned = await provisionSsoPlayer(req, providerId, profile) + if (provisioned.error) { + res.redirect(appError(sess, provisioned.error)) return null } - return user + return provisioned.user } async function finishMobileLogin(req, res, providerId, kind, tx, profile) { @@ -535,6 +564,11 @@ async function finishLink(req, res, providerId, tx, profile) { } module.exports = { + // Exported for tests only. The behaviour that matters is a COUNT — on an email + // conflict it must stop rather than work through every username candidate — and + // that is not observable through the route handlers without stubbing most of the + // OAuth flow to watch a loop it never reaches. + provisionSsoPlayer, listProviders, start, linkStart, diff --git a/server/src/utils/mailer.js b/server/src/utils/mailer.js index dc37ba4..6f28973 100644 --- a/server/src/utils/mailer.js +++ b/server/src/utils/mailer.js @@ -220,8 +220,7 @@ async function sendInvite({ to, acceptUrl, role, invitedByName }) { /** * Send a password-reset link. `to` is the account's email, `resetUrl` the tokened - * reset link, `username` names which account it's for (email is non-unique, so one - * address may receive a link per account). If email is not configured, returns + * reset link, `username` names which account it's for. If email is not configured, returns * { sent: false, reason: 'NOT_CONFIGURED' } — the caller still returns a generic * success to avoid leaking whether the address exists. Throws only on a send failure. */ @@ -251,6 +250,45 @@ async function sendPasswordReset({ to, resetUrl, username }) { } } +/** + * Send an email-verification link (engagement Phase 1b). `to` is the address + * being PROVED — which is by definition not yet the account's address, and may + * belong to someone who has never heard of this site. So the copy names the + * account and says plainly what to do if it was not you, and the link installs + * an address rather than granting any access. + * + * Returns { sent: false, reason: 'NOT_CONFIGURED' } when mail is unconfigured; + * the caller surfaces that honestly, because unlike a password reset there is no + * enumeration reason to pretend a mail went out to an address the CALLER typed. + */ +async function sendEmailVerification({ to, verifyUrl, username }) { + const built = await buildTransport() + if (!built) return { sent: false, reason: 'NOT_CONFIGURED' } + const { transport, config } = built + const forWhom = username ? ` “${username}”` : '' + try { + await transport.sendMail({ + from: fromHeader(config), + to, + replyTo: replyToFor(config), + subject: `Confirm your email address for ${brand.name}`, + text: + `The ${brand.name} account${forWhom} asked to use this address for contact and account recovery.\n\n` + + `Confirm it here:\n${verifyUrl}\n\n` + + `This link is single-use and expires in about a day. Until it is used, nothing changes — ` + + `the account keeps whatever address it had.\n\n` + + `If you did not ask for this, you can ignore this email. Someone may have mistyped their ` + + `own address; no account of yours is affected and this link grants no access to anything.`, + }) + await emailConfig.recordStatus({ status: 'connected', statusDetail: 'Verification send OK', lastVerifiedAt: new Date() }) + return { sent: true } + } catch (err) { + log.error('email verification send failed', err) + await emailConfig.recordStatus({ status: 'error', statusDetail: describeSendError(err, config) }) + throw err + } +} + /** * Send a Team notification — one event (`immediate` mode) or a day's worth * (`digest` mode). TEAMS.md §6.4. @@ -328,5 +366,6 @@ module.exports = { sendTest, sendInvite, sendPasswordReset, + sendEmailVerification, sendTeamNotification, } diff --git a/server/swagger/swagger-output.json b/server/swagger/swagger-output.json index ee31a2c..4198015 100644 --- a/server/swagger/swagger-output.json +++ b/server/swagger/swagger-output.json @@ -1166,6 +1166,16 @@ } } }, + "409": { + "description": "An account already uses that email address. Addresses are unique, so such an invite could never be accepted; it is refused here rather than at accept time, after the invitee has clicked the link.", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, "500": { "description": "Internal Server Error" } @@ -5453,6 +5463,121 @@ } } }, + "/api/v1/admin/users/email-dedupe-report": { + "get": { + "tags": [ + "Admin · Users" + ], + "summary": "Accounts whose email was cleared by de-duplication (admin only)", + "description": "When email addresses became unique, accounts sharing an address kept only the earliest-created one; the rest had their address cleared. These users can still sign in but cannot receive password-reset or notification email until they set a new address, so they are the ones to contact.", + "responses": { + "200": { + "description": "The affected accounts", + "content": { + "application/json": { + "schema": { + "type": "array", + "items": { + "$ref": "#/components/schemas/EmailDedupeEntry" + } + } + } + } + }, + "401": { + "description": "Not authenticated", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "403": { + "description": "Admin role required", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "500": { + "description": "Internal Server Error" + } + }, + "security": [ + { + "cookieAuth": [] + }, + { + "bearerAuth": [] + } + ] + } + }, + "/api/v1/admin/users/email-dedupe-report/acknowledge": { + "post": { + "tags": [ + "Admin · Users" + ], + "summary": "Dismiss the de-duplication warning (admin only)", + "description": "Marks the report acknowledged so it stops appearing as a dashboard warning. The rows are kept as a record of what the upgrade did.", + "responses": { + "200": { + "description": "Acknowledged", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "ok": { + "type": "boolean" + }, + "acknowledged": { + "type": "integer" + } + } + } + } + } + }, + "401": { + "description": "Not authenticated", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "403": { + "description": "Admin role required", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "500": { + "description": "Internal Server Error" + } + }, + "security": [ + { + "cookieAuth": [] + }, + { + "bearerAuth": [] + } + ] + } + }, "/api/v1/admin/users/{id}": { "put": { "tags": [ @@ -6993,6 +7118,117 @@ ] } }, + "/api/v1/auth/email/verify/{token}": { + "get": { + "tags": [ + "Auth" + ], + "summary": "Validate an email-confirmation link", + "description": "Returns the target username and the address the link proves, so the confirmation page can render. 404 for anything not currently usable (never distinguishes expired from used from never-existed).", + "parameters": [ + { + "name": "token", + "in": "path", + "required": true, + "schema": { + "type": "string" + } + } + ], + "responses": { + "200": { + "description": "Confirmation link is valid", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "username": { + "type": "string" + }, + "email": { + "type": "string", + "format": "email" + } + } + } + } + } + }, + "400": { + "description": "Bad Request" + }, + "404": { + "description": "Invalid or expired confirmation link", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "500": { + "description": "Internal Server Error" + } + } + }, + "post": { + "tags": [ + "Auth" + ], + "summary": "Confirm an email address from its link", + "description": "Consumes the single-use link and installs the address on the account, marking it verified. Issues no session — it proves control of a mailbox, not of an account. Answers 404 for an unusable link AND for an address another account has since verified, deliberately: the two are indistinguishable to a caller so the endpoint cannot be used to test which addresses hold accounts.", + "parameters": [ + { + "name": "token", + "in": "path", + "required": true, + "schema": { + "type": "string" + } + } + ], + "responses": { + "200": { + "description": "Address confirmed", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Message" + } + } + } + }, + "400": { + "description": "Bad Request" + }, + "404": { + "description": "Invalid, expired, superseded, or already-used confirmation link", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "429": { + "description": "Too many attempts", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "500": { + "description": "Internal Server Error" + } + } + } + }, "/api/v1/auth/invite/{token}": { "get": { "tags": [ @@ -7385,6 +7621,191 @@ ] } }, + "/api/v1/auth/me/account/email": { + "patch": { + "tags": [ + "Auth · Me" + ], + "summary": "Request a new email address (self, any role)", + "description": "Stages the address and emails a confirmation link. The account keeps its current address until that link is used, so a mistyped address cannot redirect password-reset mail. Requires currentPassword when the account has a password; SSO-provisioned accounts with no password are exempt.", + "responses": { + "200": { + "description": "Address staged; a confirmation link was sent", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/PendingEmail" + } + } + } + }, + "400": { + "description": "Validation error, wrong current password, or already your address", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "401": { + "description": "Not authenticated", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "403": { + "description": "Forbidden" + }, + "429": { + "description": "Too many verification emails", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "500": { + "description": "Internal Server Error" + } + }, + "security": [ + { + "cookieAuth": [] + }, + { + "bearerAuth": [] + } + ], + "requestBody": { + "required": true, + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ChangeEmailRequest" + } + } + } + } + } + }, + "/api/v1/auth/me/account/email/pending": { + "delete": { + "tags": [ + "Auth · Me" + ], + "summary": "Abandon the pending email address", + "description": "Clears the staged address and retires its outstanding links, so a confirmation email already delivered can no longer install it.", + "responses": { + "200": { + "description": "Pending address cleared", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/OkFlag" + } + } + } + }, + "401": { + "description": "Not authenticated", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "403": { + "description": "Forbidden" + }, + "500": { + "description": "Internal Server Error" + } + }, + "security": [ + { + "cookieAuth": [] + }, + { + "bearerAuth": [] + } + ] + } + }, + "/api/v1/auth/me/account/email/resend": { + "post": { + "tags": [ + "Auth · Me" + ], + "summary": "Re-send the confirmation link for the pending address", + "description": "", + "responses": { + "200": { + "description": "Confirmation link re-sent", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/PendingEmail" + } + } + } + }, + "400": { + "description": "No address is awaiting confirmation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "401": { + "description": "Not authenticated", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "403": { + "description": "Forbidden" + }, + "429": { + "description": "Too many verification emails", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "500": { + "description": "Internal Server Error" + } + }, + "security": [ + { + "cookieAuth": [] + }, + { + "bearerAuth": [] + } + ] + } + }, "/api/v1/auth/me/account/identities": { "get": { "tags": [ @@ -9056,7 +9477,7 @@ "Auth" ], "summary": "Request a password-reset link by email", - "description": "Emails a single-use, ~1h reset link to every active account on the address. Always returns the same generic 200 whether or not the email matches (no account enumeration). Email is non-unique, so multiple accounts may each receive a link naming their username. Rate limited per IP.", + "description": "Emails a single-use, ~1h reset link to the active account on the address. Always returns the same generic 200 whether or not the email matches (no account enumeration). Rate limited per IP.", "responses": { "200": { "description": "Generic acknowledgement (sent if the account exists)", @@ -15229,6 +15650,45 @@ } } }, + "email_verified": { + "type": "object", + "properties": { + "type": { + "type": "string", + "example": "boolean" + }, + "description": { + "type": "string", + "example": "Whether the address above has been proved by opening a confirmation link." + }, + "example": { + "type": "boolean", + "example": true + } + } + }, + "email_pending": { + "type": "object", + "properties": { + "type": { + "type": "string", + "example": "string" + }, + "format": { + "type": "string", + "example": "email" + }, + "nullable": { + "type": "boolean", + "example": true + }, + "description": { + "type": "string", + "example": "An address the user has requested but not yet confirmed. It does NOT replace `email` until the confirmation link is used." + }, + "example": {} + } + }, "status": { "type": "object", "properties": { @@ -15288,6 +15748,250 @@ } } }, + "ChangeEmailRequest": { + "type": "object", + "properties": { + "type": { + "type": "string", + "example": "object" + }, + "required": { + "type": "array", + "example": [ + "email" + ], + "items": { + "type": "string" + } + }, + "properties": { + "type": "object", + "properties": { + "email": { + "type": "object", + "properties": { + "type": { + "type": "string", + "example": "string" + }, + "format": { + "type": "string", + "example": "email" + }, + "maxLength": { + "type": "number", + "example": 255 + }, + "example": { + "type": "string", + "example": "new@example.com" + } + } + }, + "currentPassword": { + "type": "object", + "properties": { + "type": { + "type": "string", + "example": "string" + }, + "format": { + "type": "string", + "example": "password" + }, + "description": { + "type": "string", + "example": "Required when the account has a password. An address is where account recovery lands, so changing it is re-authenticated; an SSO-provisioned account with no password is exempt." + } + } + } + } + } + } + }, + "PendingEmail": { + "type": "object", + "properties": { + "type": { + "type": "string", + "example": "object" + }, + "description": { + "type": "string", + "example": "The address now awaiting confirmation. The account keeps its existing address until the emailed link is used." + }, + "properties": { + "type": "object", + "properties": { + "email_pending": { + "type": "object", + "properties": { + "type": { + "type": "string", + "example": "string" + }, + "format": { + "type": "string", + "example": "email" + }, + "example": { + "type": "string", + "example": "new@example.com" + } + } + }, + "emailed": { + "type": "object", + "properties": { + "type": { + "type": "string", + "example": "boolean" + }, + "description": { + "type": "string", + "example": "False when outbound email is not configured or the send failed; the address stays staged so a resend can succeed later." + }, + "example": { + "type": "boolean", + "example": true + } + } + }, + "reason": { + "type": "object", + "properties": { + "type": { + "type": "string", + "example": "string" + }, + "nullable": { + "type": "boolean", + "example": true + }, + "enum": { + "type": "array", + "example": [ + "NOT_CONFIGURED", + "SEND_FAILED", + null + ], + "items": {} + }, + "description": { + "type": "string", + "example": "Why nothing was sent, when `emailed` is false." + }, + "example": {} + } + } + } + } + } + }, + "EmailDedupeEntry": { + "type": "object", + "properties": { + "type": { + "type": "string", + "example": "object" + }, + "description": { + "type": "string", + "example": "One account whose email address was cleared when addresses became unique, because an older account already held it." + }, + "properties": { + "type": "object", + "properties": { + "id": { + "type": "object", + "properties": { + "type": { + "type": "string", + "example": "integer" + }, + "example": { + "type": "number", + "example": 3 + } + } + }, + "user_id": { + "type": "object", + "properties": { + "type": { + "type": "string", + "example": "integer" + }, + "example": { + "type": "number", + "example": 42 + } + } + }, + "username": { + "type": "object", + "properties": { + "type": { + "type": "string", + "example": "string" + }, + "example": { + "type": "string", + "example": "someplayer" + } + } + }, + "lost_address": { + "type": "object", + "properties": { + "type": { + "type": "string", + "example": "string" + }, + "format": { + "type": "string", + "example": "email" + }, + "example": { + "type": "string", + "example": "shared@example.com" + } + } + }, + "cleared_at": { + "type": "object", + "properties": { + "type": { + "type": "string", + "example": "string" + }, + "format": { + "type": "string", + "example": "date-time" + } + } + }, + "acknowledged_at": { + "type": "object", + "properties": { + "type": { + "type": "string", + "example": "string" + }, + "format": { + "type": "string", + "example": "date-time" + }, + "nullable": { + "type": "boolean", + "example": true + } + } + } + } + } + } + }, "OkFlag": { "type": "object", "properties": { diff --git a/server/swagger/swagger.js b/server/swagger/swagger.js index e0c6f18..e806dea 100644 --- a/server/swagger/swagger.js +++ b/server/swagger/swagger.js @@ -499,6 +499,19 @@ const doc = { username: { type: 'string', example: 'newplayer' }, role: { type: 'string', enum: ['admin', 'editor', 'moderator', 'player'], example: 'player' }, email: { type: 'string', format: 'email', nullable: true, example: 'player@example.com' }, + email_verified: { + type: 'boolean', + description: 'Whether the address above has been proved by opening a confirmation link.', + example: true, + }, + email_pending: { + type: 'string', + format: 'email', + nullable: true, + description: + 'An address the user has requested but not yet confirmed. It does NOT replace `email` until the confirmation link is used.', + example: null, + }, status: { type: 'string', enum: ['active', 'disabled', 'banned', 'pending'], example: 'active' }, totp_enabled: { type: 'boolean', example: false }, has_password: { @@ -508,6 +521,53 @@ const doc = { }, }, }, + // Engagement Phase 1b — the change-and-verify flow. + ChangeEmailRequest: { + type: 'object', + required: ['email'], + properties: { + email: { type: 'string', format: 'email', maxLength: 255, example: 'new@example.com' }, + currentPassword: { + type: 'string', + format: 'password', + description: + 'Required when the account has a password. An address is where account recovery lands, so changing it is re-authenticated; an SSO-provisioned account with no password is exempt.', + }, + }, + }, + PendingEmail: { + type: 'object', + description: + 'The address now awaiting confirmation. The account keeps its existing address until the emailed link is used.', + properties: { + email_pending: { type: 'string', format: 'email', example: 'new@example.com' }, + emailed: { + type: 'boolean', + description: 'False when outbound email is not configured or the send failed; the address stays staged so a resend can succeed later.', + example: true, + }, + reason: { + type: 'string', + nullable: true, + enum: ['NOT_CONFIGURED', 'SEND_FAILED', null], + description: 'Why nothing was sent, when `emailed` is false.', + example: null, + }, + }, + }, + EmailDedupeEntry: { + type: 'object', + description: + 'One account whose email address was cleared when addresses became unique, because an older account already held it.', + properties: { + id: { type: 'integer', example: 3 }, + user_id: { type: 'integer', example: 42 }, + username: { type: 'string', example: 'someplayer' }, + lost_address: { type: 'string', format: 'email', example: 'shared@example.com' }, + cleared_at: { type: 'string', format: 'date-time' }, + acknowledged_at: { type: 'string', format: 'date-time', nullable: true }, + }, + }, OkFlag: { type: 'object', properties: { ok: { type: 'boolean', example: true } }, diff --git a/server/test/authMe.test.js b/server/test/authMe.test.js index 6783799..03f9044 100644 --- a/server/test/authMe.test.js +++ b/server/test/authMe.test.js @@ -24,6 +24,12 @@ test('/auth/me/account* rejects unauthenticated callers with 401', async () => { ['GET', '/api/v1/auth/me/account/identities'], ['PATCH', '/api/v1/auth/me/account/username', { username: 'someone' }], ['PATCH', '/api/v1/auth/me/account/password', { newPassword: 'abcd1234' }], + // Engagement Phase 1b — the email change/verify request half is self-service + // and must be gated exactly like the rest. (The CONFIRM half is public by + // design and lives at /auth/email/verify/:token, tested separately.) + ['PATCH', '/api/v1/auth/me/account/email', { email: 'new@example.com' }], + ['POST', '/api/v1/auth/me/account/email/resend'], + ['DELETE', '/api/v1/auth/me/account/email/pending'], ['POST', '/api/v1/auth/me/account/totp/setup'], ['POST', '/api/v1/auth/me/account/totp/enable', { code: '123456' }], ['DELETE', '/api/v1/auth/me/account/identities/google'], diff --git a/server/test/emailCollisionSurfaces.test.js b/server/test/emailCollisionSurfaces.test.js new file mode 100644 index 0000000..7045b98 --- /dev/null +++ b/server/test/emailCollisionSurfaces.test.js @@ -0,0 +1,194 @@ +// Engagement Phase 1b — how each of the five write paths answers a duplicate +// EMAIL, now that `users` has two unique indexes. +// +// Before this phase every one of them either misreported the collision as a +// username clash or fell through to an opaque 500. The answers are deliberately +// NOT uniform, and the differences are the point: +// +// register generic 400, unscored — a public form; the truth would make +// account existence queryable, and +// scoring an honest typo would push a +// real user toward an IP ban +// SSO provision stops, names the reason — the caller already authenticated +// with the IdP; retrying usernames can +// never clear an email conflict +// invite accept 409, explains — the invitee has already clicked a +// link and typed a password +// admin create 409, names the field — an admin can already list every +// admin update 409, names the field account, so there is nothing to leak +process.env.DB_HOST = '127.0.0.1' +process.env.DB_PORT = '59999' + +const { test, after, beforeEach, afterEach } = require('node:test') +const assert = require('node:assert/strict') + +const authCtrl = require('../src/router/v1/auth/auth.controller') +const inviteCtrl = require('../src/router/v1/auth/invite.controller') +const adminCtrl = require('../src/router/v1/admin/admin.controller') +const users = require('../src/model/users/users.model') +const invites = require('../src/model/invites/invites.model') +const settings = require('../src/model/settings/settings.model') +const activity = require('../src/model/activity/activity.model') +const botScore = require('../src/middleware/botScore') +const loginProtection = require('../src/middleware/loginProtection') +const db = require('../src/utils/db') + +after(() => db.close()) + +function mockRes() { + return { + statusCode: 200, + body: null, + status(c) { + this.statusCode = c + return this + }, + json(b) { + this.body = b + return this + }, + cookie() { + return this + }, + } +} + +const patched = [] +function stub(obj, name, fn) { + patched.push([obj, name, obj[name]]) + obj[name] = fn +} +afterEach(() => { + while (patched.length) { + const [obj, name, fn] = patched.pop() + obj[name] = fn + } +}) + +// The exact error the mariadb connector raises for each index, captured from +// MariaDB 11.8. Note the address is inside the message: that is why none of these +// paths may echo it. +function dupEmailError(value = 'taken@example.com') { + const err = new Error( + `(conn:60, no: 1062, SQLState: 23000) Duplicate entry '${value}' for key 'uq_users_email_norm'\n` + + `sql: INSERT INTO users ... - parameters:['someone','${value}']`, + ) + err.code = 'ER_DUP_ENTRY' + err.errno = 1062 + err.sqlMessage = `Duplicate entry '${value}' for key 'uq_users_email_norm'` + return err +} + +function dupUsernameError() { + const err = new Error("(conn:60, no: 1062, SQLState: 23000) Duplicate entry 'someone' for key 'username'") + err.code = 'ER_DUP_ENTRY' + err.errno = 1062 + err.sqlMessage = "Duplicate entry 'someone' for key 'username'" + return err +} + +let scored + +beforeEach(() => { + scored = [] + stub(activity, 'log', async () => {}) + stub(botScore, 'recordHoneypot', (ip) => scored.push(['honeypot', ip])) + stub(botScore, 'recordLoginFailure', (ip) => scored.push(['loginFailure', ip])) + stub(loginProtection, 'recordFailure', (ip) => scored.push(['backoff', ip])) +}) + +// ── register: generic, and NOT scored ────────────────────────────────────── + +test('register answers a duplicate email generically and never says "username"', async () => { + stub(settings, 'getRegistrationMode', async () => 'password') + stub(users, 'createUser', async () => { + throw dupEmailError() + }) + const res = mockRes() + await authCtrl.register( + { body: { username: 'newperson', password: 'abcd1234', email: 'taken@example.com' }, ip: '9.9.9.9' }, + res, + ) + assert.equal(res.statusCode, 400, 'not the 409 a username clash gets - the shape itself must not distinguish') + assert.match(res.body.message, /Registration failed/i) + assert.doesNotMatch(res.body.message, /username/i, 'must not misattribute to the field they did NOT collide on') + assert.doesNotMatch(res.body.message, /email/i, 'and must not confirm the address exists') + assert.doesNotMatch(res.body.message, /taken@example\.com/, 'the address must never come back') +}) + +test('a duplicate email at register feeds NOTHING to the bot scorer or the backoff', async () => { + stub(settings, 'getRegistrationMode', async () => 'password') + stub(users, 'createUser', async () => { + throw dupEmailError() + }) + await authCtrl.register( + { body: { username: 'newperson', password: 'abcd1234', email: 'taken@example.com' }, ip: '9.9.9.9' }, + mockRes(), + ) + // A legitimate user typing a colleague's address is not an attacker. Scoring + // this would walk them toward an automatic IP ban for an honest mistake. + assert.deepEqual(scored, [], 'no bot score, no backoff') +}) + +test('register still reports a genuine username clash as 409', async () => { + stub(settings, 'getRegistrationMode', async () => 'password') + stub(users, 'createUser', async () => { + throw dupUsernameError() + }) + const res = mockRes() + await authCtrl.register({ body: { username: 'someone', password: 'abcd1234' }, ip: '9.9.9.9' }, res) + assert.equal(res.statusCode, 409) + assert.match(res.body.message, /username/i) +}) + +// ── invite accept: survivable, and distinguishable from a username clash ─── + +test('invite accept explains a duplicate email instead of blaming the username', async () => { + stub(invites, 'findValidByToken', async () => ({ id: 5, email: 'taken@example.com', role: 'player' })) + stub(users, 'createUser', async () => { + throw dupEmailError() + }) + const res = mockRes() + await inviteCtrl.acceptInvite( + { body: { username: 'newperson', password: 'abcd1234' }, params: { token: 't' }, ip: '9.9.9.9' }, + res, + ) + assert.equal(res.statusCode, 409) + assert.match(res.body.message, /email address is already in use/i) + assert.doesNotMatch(res.body.message, /username is already taken/i) +}) + +// ── admin user CRUD: was an opaque 500, now a 409 that names the field ───── + +test('admin createUser answers a duplicate email with 409, not a 500', async () => { + stub(users, 'getRawByUsername', async () => null) + stub(users, 'createUser', async () => { + throw dupEmailError() + }) + const res = mockRes() + await adminCtrl.createUser({ body: { username: 'newperson', password: 'abcd1234', email: 'taken@example.com' } }, res) + assert.equal(res.statusCode, 409, 'before Phase 1b this had no catch at all and became a 500') + assert.match(res.body.message, /email address/i) +}) + +test('admin updateUser answers a duplicate email with 409, not a 500', async () => { + stub(users, 'getById', async () => ({ id: 3, username: 'existing', role: 'player', status: 'active' })) + stub(users, 'update', async () => { + throw dupEmailError() + }) + const res = mockRes() + await adminCtrl.updateUser({ params: { id: '3' }, body: { email: 'taken@example.com' } }, res) + assert.equal(res.statusCode, 409) + assert.match(res.body.message, /email address/i) +}) + +test('admin createUser still reports a username clash as a username clash', async () => { + stub(users, 'getRawByUsername', async () => null) + stub(users, 'createUser', async () => { + throw dupUsernameError() + }) + const res = mockRes() + await adminCtrl.createUser({ body: { username: 'someone', password: 'abcd1234' } }, res) + assert.equal(res.statusCode, 409) + assert.match(res.body.message, /Username already taken/i) +}) diff --git a/server/test/emailUniqueness.test.js b/server/test/emailUniqueness.test.js new file mode 100644 index 0000000..4f03b12 --- /dev/null +++ b/server/test/emailUniqueness.test.js @@ -0,0 +1,87 @@ +// Engagement Phase 1b — telling the two unique constraints on `users` apart. +// +// This is the piece the whole phase rests on: `users` grew a second unique index, +// and until Phase 1b the duplicate-key test could not tell which one fired. Every +// call site that creates or updates a user branches on these predicates, so a +// wrong answer here means a duplicate email reported as a taken username, an SSO +// sign-up retrying usernames against a conflict no username can clear, or an +// opaque 500 on the admin user form. +// +// The error strings below are VERBATIM from MariaDB 11.8 through the mariadb Node +// connector, captured against a real duplicate insert. The key name lives only in +// the message text — the driver exposes no structured field for it — which is +// exactly why this needs its own test: it is parsing, and parsing rots silently. +process.env.DB_HOST = '127.0.0.1' +process.env.DB_PORT = '59999' + +const { test, after } = require('node:test') +const assert = require('node:assert/strict') + +const users = require('../src/model/users/users.model') +const db = require('../src/utils/db') + +after(() => db.close()) + +// Shaped exactly as the connector delivers them, including the trailing `sql:` +// section — which is also the reason these must never be echoed to a client: note +// the bound parameters, and therefore the address, are in the text. +function dupError(key, value) { + const err = new Error( + `(conn:60, no: 1062, SQLState: 23000) Duplicate entry '${value}' for key '${key}'\n` + + `sql: INSERT INTO users (username, email) VALUES (?, ?) - parameters:['someone','${value}']`, + ) + err.code = 'ER_DUP_ENTRY' + err.errno = 1062 + err.sqlState = '23000' + err.sqlMessage = `Duplicate entry '${value}' for key '${key}'` + return err +} + +test('an email collision is reported as email, not username', () => { + const err = dupError('uq_users_email_norm', 'taken@example.com') + assert.equal(users.isDuplicateEmail(err), true) + assert.equal(users.isDuplicateUsername(err), false, 'must NOT masquerade as a username collision') + assert.equal(users.duplicateKey(err), 'uq_users_email_norm') +}) + +test('a username collision is still reported as username', () => { + const err = dupError('username', 'someone') + assert.equal(users.isDuplicateUsername(err), true) + assert.equal(users.isDuplicateEmail(err), false) + assert.equal(users.duplicateKey(err), 'username') +}) + +// The permissive fallback is deliberate. Only the case we can positively identify +// — email — is carved out; anything else keeps the pre-Phase-1b behaviour so no +// call site newly falls through to a 500 on a database whose index carries an +// unexpected name. +test('an unrecognised unique index keeps the old permissive behaviour', () => { + const err = dupError('some_other_uq', 'x') + assert.equal(users.isDuplicateUsername(err), true) + assert.equal(users.isDuplicateEmail(err), false) +}) + +test('a duplicate-key error the message does not name is treated as username', () => { + const err = new Error('Duplicate entry - no key clause here') + err.code = 'ER_DUP_ENTRY' + err.errno = 1062 + assert.equal(users.duplicateKey(err), null) + assert.equal(users.isDuplicateUsername(err), true) + assert.equal(users.isDuplicateEmail(err), false) +}) + +test('non-duplicate errors are neither', () => { + for (const err of [null, undefined, new Error('boom'), { code: 'ER_NO_SUCH_TABLE' }]) { + assert.equal(users.isDuplicateUsername(err), false) + assert.equal(users.isDuplicateEmail(err), false) + assert.equal(users.duplicateKey(err), null) + } +}) + +// errno alone, with no `code`, is how some driver paths surface it. +test('errno 1062 without a code still counts', () => { + const err = new Error("Duplicate entry 'a@b.com' for key 'uq_users_email_norm'") + err.errno = 1062 + assert.equal(users.isDuplicateEmail(err), true) + assert.equal(users.isDuplicateUsername(err), false) +}) diff --git a/server/test/emailVerification.test.js b/server/test/emailVerification.test.js new file mode 100644 index 0000000..11c91be --- /dev/null +++ b/server/test/emailVerification.test.js @@ -0,0 +1,278 @@ +// Engagement Phase 1b — the change-and-verify flow, at the controller level. +// +// Every model call is monkeypatched, so no query runs. What is under test is the +// DECISION-MAKING, and three properties in particular that no single unit of the +// code enforces on its own: +// +// 1. Requesting a change never touches the live address. The account keeps +// receiving password-reset mail at the address it had until a link proves the +// new one. A regression here is silent, and only shows up when somebody +// cannot recover their account. +// 2. Confirming answers IDENTICALLY for every failure. Expired, already-used, +// superseded, and "another account verified this address first" are one 404 +// with one message. Any divergence turns the endpoint into an oracle for +// which addresses hold accounts. +// 3. Changing the address is re-authenticated, with the SSO carve-out. +process.env.DB_HOST = '127.0.0.1' +process.env.DB_PORT = '59999' + +const { test, after, beforeEach, afterEach } = require('node:test') +const assert = require('node:assert/strict') + +const account = require('../src/router/v1/auth/account.controller') +const verify = require('../src/router/v1/auth/emailVerify.controller') +const users = require('../src/model/users/users.model') +const emailVerifications = require('../src/model/emailVerifications/emailVerifications.model') +const activity = require('../src/model/activity/activity.model') +const mailer = require('../src/utils/mailer') +const db = require('../src/utils/db') + +after(() => db.close()) + +function mockRes() { + return { + statusCode: 200, + body: null, + status(c) { + this.statusCode = c + return this + }, + json(b) { + this.body = b + return this + }, + } +} + +// Restore-on-teardown patching, keyed by owner+name so two modules may each carry +// a function of the same name. +const patched = [] +function stub(obj, name, fn) { + patched.push([obj, name, obj[name]]) + obj[name] = fn +} + +let sent +let staged +let promoted + +beforeEach(() => { + sent = [] + staged = [] + promoted = [] + stub(activity, 'log', async () => {}) + stub(mailer, 'sendEmailVerification', async (args) => { + sent.push(args) + return { sent: true } + }) + stub(users, 'setPendingEmail', async (id, email) => { + staged.push([id, email]) + return 1 + }) + stub(users, 'clearPendingEmail', async () => 1) + stub(users, 'promotePendingEmail', async (id, email) => { + promoted.push([id, email]) + return true + }) + stub(users, 'validatePassword', async (_u, pw) => pw === 'correct-horse') + stub(emailVerifications, 'sendQuotaExhausted', async () => false) + stub(emailVerifications, 'invalidatePendingForUser', async () => 0) + stub(emailVerifications, 'create', async () => ({ id: 1, token: 'tok-abcdefgh' })) +}) + +afterEach(() => { + while (patched.length) { + const [obj, name, fn] = patched.pop() + obj[name] = fn + } +}) + +const reqFor = (body, user = {}) => ({ + body, + ip: '10.0.0.1', + user: { id: 7, username: 'alice', ...user }, +}) + +// ── 1. The live address is never touched by a request ────────────────────── + +test('requesting a change stages the address and leaves the live one alone', async () => { + let updateCalled = false + stub(users, 'update', async () => { + updateCalled = true + }) + stub(users, 'getRawById', async () => ({ id: 7, password_hash: 'h', email: 'old@example.com' })) + + const res = mockRes() + await account.changeEmail(reqFor({ email: 'new@example.com', currentPassword: 'correct-horse' }), res) + + assert.equal(res.statusCode, 200) + assert.deepEqual(staged, [[7, 'new@example.com']], 'the new address is STAGED') + assert.equal(updateCalled, false, 'users.update must NOT be called - the live address stands') + assert.equal(res.body.email_pending, 'new@example.com') + assert.equal(sent.length, 1, 'a verification mail goes to the address being proved') + assert.equal(sent[0].to, 'new@example.com') +}) + +test('the verification link goes to the NEW address, never the old one', async () => { + stub(users, 'getRawById', async () => ({ id: 7, password_hash: 'h', email: 'old@example.com' })) + const res = mockRes() + await account.changeEmail(reqFor({ email: 'new@example.com', currentPassword: 'correct-horse' }), res) + assert.equal(sent[0].to, 'new@example.com') + assert.notEqual(sent[0].to, 'old@example.com') +}) + +// ── 2. Re-authentication, with the SSO carve-out ─────────────────────────── + +test('a wrong current password is refused and stages nothing', async () => { + stub(users, 'getRawById', async () => ({ id: 7, password_hash: 'h', email: 'old@example.com' })) + const res = mockRes() + await account.changeEmail(reqFor({ email: 'new@example.com', currentPassword: 'wrong' }), res) + assert.equal(res.statusCode, 400) + assert.deepEqual(staged, [], 'nothing may be staged on a failed re-auth') + assert.equal(sent.length, 0, 'and no mail may go out') +}) + +test('an SSO-only account (no password hash) may change its address without one', async () => { + stub(users, 'getRawById', async () => ({ id: 7, password_hash: null, email: null })) + const res = mockRes() + await account.changeEmail(reqFor({ email: 'first@example.com' }), res) + assert.equal(res.statusCode, 200, 'the carve-out changePassword already makes, made here too') + assert.deepEqual(staged, [[7, 'first@example.com']]) +}) + +test('setting the same address again is refused', async () => { + stub(users, 'getRawById', async () => ({ id: 7, password_hash: 'h', email: 'Same@Example.com' })) + const res = mockRes() + // Compared case-folded, because the uniqueness index folds case: this IS the + // same mailbox, and staging it would mail the user a link to prove what they + // have already proved. + await account.changeEmail(reqFor({ email: 'same@example.com', currentPassword: 'correct-horse' }), res) + assert.equal(res.statusCode, 400) + assert.deepEqual(staged, []) +}) + +// ── 3. Confirming: every failure answers identically ─────────────────────── + +const INVALID = 'This confirmation link is invalid or has expired.' + +test('an unusable link 404s with the generic message', async () => { + stub(emailVerifications, 'findValidByToken', async () => null) + const res = mockRes() + await verify.confirm({ params: { token: 'nope' }, ip: '1.2.3.4' }, res) + assert.equal(res.statusCode, 404) + assert.equal(res.body.message, INVALID) +}) + +test('an address another account verified first answers the SAME 404', async () => { + stub(emailVerifications, 'findValidByToken', async () => ({ id: 1, user_id: 7, email: 'taken@example.com' })) + stub(emailVerifications, 'consume', async () => true) + stub(users, 'promotePendingEmail', async () => { + const err = new Error("Duplicate entry 'taken@example.com' for key 'uq_users_email_norm'") + err.code = 'ER_DUP_ENTRY' + err.errno = 1062 + err.sqlMessage = "Duplicate entry 'taken@example.com' for key 'uq_users_email_norm'" + throw err + }) + const res = mockRes() + await verify.confirm({ params: { token: 'tok' }, ip: '1.2.3.4' }, res) + assert.equal(res.statusCode, 404, 'not a 409 - that would be an enumeration oracle') + assert.equal(res.body.message, INVALID, 'byte-identical to an expired link') +}) + +test('a superseded link answers the SAME 404', async () => { + stub(emailVerifications, 'findValidByToken', async () => ({ id: 1, user_id: 7, email: 'stale@example.com' })) + stub(emailVerifications, 'consume', async () => true) + stub(users, 'promotePendingEmail', async () => false) // the guard rejected it + const res = mockRes() + await verify.confirm({ params: { token: 'tok' }, ip: '1.2.3.4' }, res) + assert.equal(res.statusCode, 404) + assert.equal(res.body.message, INVALID) +}) + +test('a link that lost the double-use race answers the SAME 404', async () => { + stub(emailVerifications, 'findValidByToken', async () => ({ id: 1, user_id: 7, email: 'a@example.com' })) + stub(emailVerifications, 'consume', async () => false) // someone else consumed it first + const res = mockRes() + await verify.confirm({ params: { token: 'tok' }, ip: '1.2.3.4' }, res) + assert.equal(res.statusCode, 404) + assert.equal(res.body.message, INVALID) + assert.deepEqual(promoted, [], 'and must not touch the account') +}) + +test('a good link installs the address and issues no session', async () => { + stub(emailVerifications, 'findValidByToken', async () => ({ id: 1, user_id: 7, email: 'good@example.com' })) + stub(emailVerifications, 'consume', async () => true) + const res = mockRes() + await verify.confirm({ params: { token: 'tok' }, ip: '1.2.3.4' }, res) + assert.equal(res.statusCode, 200) + assert.equal(res.body.ok, true) + assert.deepEqual(promoted, [[7, 'good@example.com']]) + // The response carries no token, cookie or user — proving control of a mailbox + // is not proving control of an account. + assert.equal(res.body.token, undefined) + assert.equal(res.body.user, undefined) +}) + +// ── 4. The send ceiling, and honest reporting when mail is off ───────────── + +test('the per-user send ceiling refuses with 429 and sends nothing', async () => { + stub(users, 'getRawById', async () => ({ id: 7, password_hash: null, email: null })) + stub(emailVerifications, 'sendQuotaExhausted', async () => true) + const res = mockRes() + await account.changeEmail(reqFor({ email: 'new@example.com' }), res) + assert.equal(res.statusCode, 429) + assert.equal(sent.length, 0) +}) + +test('unconfigured mail is reported honestly, and the address stays staged', async () => { + stub(users, 'getRawById', async () => ({ id: 7, password_hash: null, email: null })) + stub(mailer, 'sendEmailVerification', async () => ({ sent: false, reason: 'NOT_CONFIGURED' })) + const res = mockRes() + await account.changeEmail(reqFor({ email: 'new@example.com' }), res) + assert.equal(res.statusCode, 200) + assert.equal(res.body.emailed, false) + assert.equal(res.body.reason, 'NOT_CONFIGURED') + assert.deepEqual(staged, [[7, 'new@example.com']], 'staged, so a later resend can work') +}) + +test('resending with nothing pending is a 400, not a mail', async () => { + stub(users, 'getRawById', async () => ({ id: 7, password_hash: 'h', email: 'a@example.com', email_pending: null })) + const res = mockRes() + await account.resendEmailVerification(reqFor({}), res) + assert.equal(res.statusCode, 400) + assert.equal(sent.length, 0) +}) + +test('resending re-sends to the pending address', async () => { + stub(users, 'getRawById', async () => ({ + id: 7, + password_hash: 'h', + email: 'a@example.com', + email_pending: 'p@example.com', + })) + const res = mockRes() + await account.resendEmailVerification(reqFor({}), res) + assert.equal(res.statusCode, 200) + assert.equal(sent.length, 1) + assert.equal(sent[0].to, 'p@example.com') +}) + +test('cancelling clears the pending address AND retires its outstanding links', async () => { + let cleared = false + let retired = false + stub(users, 'clearPendingEmail', async () => { + cleared = true + return 1 + }) + stub(emailVerifications, 'invalidatePendingForUser', async () => { + retired = true + return 1 + }) + const res = mockRes() + await account.cancelEmailChange(reqFor({}), res) + assert.equal(res.statusCode, 200) + assert.equal(cleared, true) + // Both halves matter: clearing the column alone would leave a link already + // sitting in a mailbox able to install the address the user just abandoned. + assert.equal(retired, true, 'outstanding links must be retired too') +}) diff --git a/server/test/providers.test.js b/server/test/providers.test.js index 23ecc90..8356567 100644 --- a/server/test/providers.test.js +++ b/server/test/providers.test.js @@ -50,7 +50,16 @@ test('Google handleCallback exchanges code and normalizes the profile', async () }) const p = new GoogleProvider({ id: 'google', clientId: 'gid', clientSecret: 'gsecret' }) const profile = await p.handleCallback({ code: 'C', redirectUri: 'https://app/cb', codeVerifier: 'V' }) - assert.deepEqual(profile, { subject: '11550', email: 'alice@example.com', name: 'Alice' }) + // emailVerified is false because this userinfo document carries no + // `email_verified` claim. Before engagement Phase 1b the presence of an address + // was itself treated as verification, which is the bug that made the flag + // meaningless — see ssoEmailVerified.test.js. + assert.deepEqual(profile, { + subject: '11550', + email: 'alice@example.com', + emailVerified: false, + name: 'Alice', + }) }) test('Discord authorize URL + profile mapping (global_name → name, id → subject)', async () => { @@ -64,7 +73,8 @@ test('Discord authorize URL + profile mapping (global_name → name, id → subj 'discord.com/api/users/@me': { id: '99', username: 'bob', global_name: 'Bob', email: 'bob@x.io' }, }) const profile = await p.handleCallback({ code: 'C', redirectUri: 'https://app/cb' }) - assert.deepEqual(profile, { subject: '99', email: 'bob@x.io', name: 'Bob' }) + // Discord spells the claim `verified`, and this fixture does not send it. + assert.deepEqual(profile, { subject: '99', email: 'bob@x.io', emailVerified: false, name: 'Bob' }) }) test('Generic OIDC provider uses configured endpoints and OIDC profile fields', async () => { @@ -82,7 +92,8 @@ test('Generic OIDC provider uses configured endpoints and OIDC profile fields', 'idp.example/userinfo': { sub: 'abc', email: 'c@d.e', preferred_username: 'carol' }, }) const profile = await p.handleCallback({ code: 'C', redirectUri: 'https://app/cb', codeVerifier: 'V' }) - assert.deepEqual(profile, { subject: 'abc', email: 'c@d.e', name: 'carol' }) + // An IdP that omits the claim has asserted nothing: absent is false, never true. + assert.deepEqual(profile, { subject: 'abc', email: 'c@d.e', emailVerified: false, name: 'carol' }) }) test('handleCallback throws when the token exchange fails', async () => { diff --git a/server/test/ssoEmailVerified.test.js b/server/test/ssoEmailVerified.test.js new file mode 100644 index 0000000..0ad6983 --- /dev/null +++ b/server/test/ssoEmailVerified.test.js @@ -0,0 +1,140 @@ +// Engagement Phase 1b — what SSO does with an email address. +// +// Two corrections, both of them things the old code got wrong quietly: +// +// 1. `emailVerified: Boolean(profile.email)` marked EVERY SSO address verified, +// because an address was present. That made `email_verified` mean "we have an +// address", which is not a fact about anything, and is why the de-duplication +// resolves duplicates oldest-wins rather than verified-wins (§0.6 finding 3). +// Now each provider reports the claim its IdP actually asserted. +// 2. Provisioning retried usernames on ANY duplicate-key error. Once email is +// unique that loop can never clear an email conflict — it burns every +// candidate and returns "could not find a username", blaming usernames for a +// conflict that was never about them (§0.6 finding 2). +process.env.DB_HOST = '127.0.0.1' +process.env.DB_PORT = '59999' + +const { test, after, afterEach } = require('node:test') +const assert = require('node:assert/strict') + +const GoogleProvider = require('../src/auth/providers/google.provider') +const DiscordProvider = require('../src/auth/providers/discord.provider') +const GenericOidcProvider = require('../src/auth/providers/genericOidc.provider') +const sso = require('../src/router/v1/auth/sso.controller') +const users = require('../src/model/users/users.model') +const userIdentities = require('../src/model/userIdentities/userIdentities.model') +const activity = require('../src/model/activity/activity.model') +const db = require('../src/utils/db') + +after(() => db.close()) + +// ── 1. Each provider reads its own spelling of the claim ─────────────────── + +test('Google reads the standard email_verified claim', () => { + const p = new GoogleProvider({ id: 'google' }) + assert.equal(p.normalizeProfile({ sub: '1', email: 'a@b.com', email_verified: true }).emailVerified, true) + assert.equal(p.normalizeProfile({ sub: '1', email: 'a@b.com', email_verified: false }).emailVerified, false) + // Present-but-unasserted is NOT verified. This is the whole bug. + assert.equal(p.normalizeProfile({ sub: '1', email: 'a@b.com' }).emailVerified, false) +}) + +test('Discord reads `verified`, which is how Discord spells it', () => { + const p = new DiscordProvider({ id: 'discord' }) + assert.equal(p.normalizeProfile({ id: '1', email: 'a@b.com', verified: true }).emailVerified, true) + assert.equal(p.normalizeProfile({ id: '1', email: 'a@b.com', verified: false }).emailVerified, false) + assert.equal(p.normalizeProfile({ id: '1', email: 'a@b.com' }).emailVerified, false) +}) + +test('a generic OIDC provider that omits the claim leaves the address unverified', () => { + const p = new GenericOidcProvider({ id: 'custom' }) + assert.equal(p.normalizeProfile({ sub: '1', email: 'a@b.com', email_verified: true }).emailVerified, true) + // An IdP that asserts nothing has asserted nothing. Absent is false, never true. + assert.equal(p.normalizeProfile({ sub: '1', email: 'a@b.com' }).emailVerified, false) +}) + +// Some IdPs stringify booleans in the userinfo document. +test('the string "true" counts, anything else does not', () => { + const p = new GenericOidcProvider({ id: 'custom' }) + assert.equal(p.normalizeProfile({ sub: '1', email: 'a@b.com', email_verified: 'true' }).emailVerified, true) + assert.equal(p.normalizeProfile({ sub: '1', email: 'a@b.com', email_verified: 'yes' }).emailVerified, false) + assert.equal(p.normalizeProfile({ sub: '1', email: 'a@b.com', email_verified: 1 }).emailVerified, false) +}) + +test('every provider still returns the fields the rest of the flow reads', () => { + const cases = [ + [new GoogleProvider({ id: 'google' }), { sub: 'g1', email: 'a@b.com', name: 'A' }], + [new DiscordProvider({ id: 'discord' }), { id: 'd1', email: 'a@b.com', global_name: 'A' }], + [new GenericOidcProvider({ id: 'custom' }), { sub: 'c1', email: 'a@b.com', name: 'A' }], + ] + for (const [provider, raw] of cases) { + const out = provider.normalizeProfile(raw) + assert.ok(out.subject, `${provider.id} must still derive a subject`) + assert.equal(out.email, 'a@b.com') + assert.equal(typeof out.emailVerified, 'boolean', `${provider.id} must report a boolean, never undefined`) + assert.ok('name' in out) + } +}) + +// ── 2. Provisioning stops on an email conflict instead of burning candidates ─ + +const patched = [] +function stub(obj, name, fn) { + patched.push([obj, name, obj[name]]) + obj[name] = fn +} +afterEach(() => { + while (patched.length) { + const [obj, name, fn] = patched.pop() + obj[name] = fn + } +}) + +function dupError(key, value) { + const err = new Error(`Duplicate entry '${value}' for key '${key}'`) + err.code = 'ER_DUP_ENTRY' + err.errno = 1062 + err.sqlMessage = `Duplicate entry '${value}' for key '${key}'` + return err +} + +const req = { ip: '1.2.3.4' } +const profile = { subject: 'idp-1', email: 'taken@example.com', name: 'Someone', emailVerified: true } + +test('an email conflict stops provisioning at the FIRST attempt', async () => { + let attempts = 0 + stub(users, 'createUser', async () => { + attempts += 1 + throw dupError('uq_users_email_norm', 'taken@example.com') + }) + const out = await sso.provisionSsoPlayer(req, 'google', profile) + // PROVISION_MAX_TRIES is 25. Retrying usernames cannot clear an EMAIL conflict, + // so 25 attempts would be 24 pointless writes ending in a log line blaming + // usernames for something they had nothing to do with. + assert.equal(attempts, 1, 'must not retry a conflict no username change can resolve') + assert.equal(out.error, 'email_in_use', 'and must say which conflict it was') + assert.equal(out.user, undefined) +}) + +test('a username conflict still retries the next candidate', async () => { + let attempts = 0 + stub(users, 'createUser', async () => { + attempts += 1 + if (attempts < 3) throw dupError('username', 'someone') + return { id: 42, username: `someone${attempts}`, role: 'player' } + }) + stub(userIdentities, 'link', async () => {}) + stub(activity, 'log', async () => {}) + const out = await sso.provisionSsoPlayer(req, 'google', profile) + assert.equal(attempts, 3, 'the bounded username retry is unchanged') + assert.equal(out.user.id, 42) + assert.equal(out.error, undefined) +}) + +test('exhausting username candidates reports a generic error, not an email one', async () => { + stub(users, 'createUser', async () => { + throw dupError('username', 'someone') + }) + const out = await sso.provisionSsoPlayer(req, 'google', profile) + assert.equal(out.error, 'error') + assert.equal(out.user, undefined) +})