diff --git a/website/BACKEND_DESIGN.md b/website/BACKEND_DESIGN.md index 61ca5f4..7fed015 100644 --- a/website/BACKEND_DESIGN.md +++ b/website/BACKEND_DESIGN.md @@ -81,6 +81,11 @@ server/ token is its own authority, so it bypasses player_registration password.router.js (3) /auth/password/forgot + reset/:token + emailVerify.router.js (2) /auth/email/verify/:token — public and + token-gated like password.router: the + link arrives in a mailbox, so the + REQUEST half is at /auth/me/account/email + and only the CONFIRM half is here session.router.js (2) POST /logout and GET /me — the two singletons owning no path segment, so mounted at the group root, LAST: the @@ -240,11 +245,43 @@ listed models/two utils. They're isolated in `middleware/` + one `activity` mode | col | type | notes | |---|---|---| | id | INT PK AUTO_INCREMENT | | -| username | VARCHAR(32) UNIQUE NOT NULL | | -| password_hash | VARCHAR(72) NOT NULL | bcrypt; **never** returned by the API | -| role | ENUM('admin','editor') NOT NULL DEFAULT 'admin' | room to grow | -| created_at | DATETIME DEFAULT CURRENT_TIMESTAMP | | -| last_login_at | DATETIME NULL | shown in user management | +| username | VARCHAR(32) UNIQUE NOT NULL COLLATE utf8mb4_general_ci | the `_ci` collation is the case-insensitive uniqueness backstop | +| password_hash | VARCHAR(72) **NULL** | bcrypt; **never** returned by the API. Nullable: an SSO-provisioned account has none until it sets one, and a NULL hash makes password login impossible | +| role | ENUM('admin','editor','moderator','player') NOT NULL DEFAULT 'admin' | | +| email | VARCHAR(255) NULL | 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`, never on this column** (below) | +| email_norm | VARCHAR(255) COLLATE utf8mb4_bin **GENERATED** `AS (LOWER(email)) STORED`, UNIQUE | the uniqueness key. Every `_ci` collation MariaDB offers is also **accent**-insensitive, so a UNIQUE index on `email` would refuse `jose@x.com` once `josé@x.com` existed — two different mailboxes. `LOWER()` under `_bin` folds case without folding accents. Keeping the fold in a generated column rather than in application code means no caller can bypass it. Multiple NULLs stay legal, which is what lets the de-duplication clear an address without deleting an account | +| email_verified | TINYINT(1) NOT NULL DEFAULT 0 | set only by opening a verification link (or by an invite, which proves the address by construction). SSO sets it from the IdP's actual `email_verified`/`verified` claim — **not** from the mere presence of an address, which is what it used to do | +| email_pending | VARCHAR(255) NULL | an address requested but not yet proved. It does **not** displace `email`, so a mistyped address cannot silently redirect account-recovery mail. Deliberately **not** unique: a pending address reserves nothing, and the UNIQUE index above arbitrates at confirmation time | +| status | ENUM('active','pending','disabled','banned') NOT NULL DEFAULT 'active' | lifecycle, independent of role; enforced in `requireAuth` + login | +| totp_secret / totp_enabled | VARCHAR(64) NULL / TINYINT(1) | opt-in 2FA | +| tokens_valid_after | DATETIME NULL | session-revocation cutoff; bumped on password change / "log out everywhere" | +| created_at | DATETIME DEFAULT CURRENT_TIMESTAMP | also the **tie-break for de-duplication**: oldest account keeps a shared address | +| last_login_at / last_login_ip | DATETIME NULL / VARCHAR(45) NULL | shown in user management | + +### email_verifications *(engagement Phase 1b)* +Same shape as `password_resets`, deliberately — an opaque random token whose **sha256 only** is stored, single-use, ~24h. +| col | type | notes | +|---|---|---| +| id | INT PK AUTO_INCREMENT | | +| token_hash | CHAR(64) UNIQUE NOT NULL | sha256 of the opaque token; a DB read never yields a usable link | +| user_id | INT NOT NULL FK→users(id) ON DELETE CASCADE | | +| email | VARCHAR(255) NOT NULL | **the address this token proves.** On the row, not read from the user at confirm time: a token proves control of the address it was mailed to and nothing else, so a later request for a different address cannot be confirmed by an older link | +| status | ENUM('pending','used') NOT NULL DEFAULT 'pending' | consumed atomically | +| requested_ip | VARCHAR(64) NULL | audit only | +| expires_at / created_at / used_at | DATETIME | | + +### email_dedupe_report *(engagement Phase 1b)* +Who lost an address when addresses became unique. Written by `schema.sql`'s migration in pure SQL — `ensureSchema()` runs that file statement-by-statement and there is no JS migration hook — and only ever read afterwards. +| col | type | notes | +|---|---|---| +| id | INT PK AUTO_INCREMENT | | +| user_id | INT NOT NULL, UNIQUE | the UNIQUE is what makes the migration's `INSERT IGNORE` strictly idempotent | +| username | VARCHAR(32) NOT NULL | captured at clear time | +| lost_address | VARCHAR(255) NOT NULL | the report is the only place this value survives | +| cleared_at | DATETIME DEFAULT CURRENT_TIMESTAMP | | +| acknowledged_at | DATETIME NULL | set when an admin dismisses the dashboard warning; rows are kept as the record of what the upgrade did | + +No FK to `users`, on purpose — same reasoning as `posts.announce_job_id`: a constraint re-added on every boot is a constraint that can fail a boot, and this is a historical record rather than a live relation. ### posts — one table, four categories | col | type | notes | @@ -771,13 +808,18 @@ their own router level, and `/sso/:provider/link` carries `requireAuth` per rout | POST | `/login/totp` | — (rate-limited) | `{challenge, code? \| recoveryCode?, trustDevice?, deviceName?}` | complete 2FA with a TOTP **or** single-use recovery code. `trustDevice` sets the `rg_trust` cookie so future logins skip TOTP; at the device cap the session is still issued and the body carries `{trustLimitReached, devices}`. | | POST | `/logout` | cookie | — | clear cookie (the `rg_trust` trust cookie deliberately **survives** logout) | | GET | `/me` | cookie / bearer | — | current user (no hash) or 401 — client bootstraps auth state | -| POST | `/password/forgot` | — (rate-limited) | `{email}` | email a single-use, ~1h reset link to **every active account** on the address; **always** returns the same generic 200 (no account enumeration). Email is non-unique, so several accounts may each get a link naming their username. Logs `account.password.reset.request`. | +| POST | `/password/forgot` | — (rate-limited) | `{email}` | email a single-use, ~1h reset link to the active account on the address; **always** returns the same generic 200 (no account enumeration). Addresses are unique since Phase 1b, so this matches at most one account. Reset mail is deliberately **not** gated on `email_verified` — that gate governs opt-in engagement mail, and applying it to account recovery would lock out every user carrying an address from before verification existed. Logs `account.password.reset.request`. | | GET | `/password/reset/:token` | — | — | validate a link → `{username}` for the form, else 404 (never distinguishes expired/used/never-existed) | | POST | `/password/reset/:token` | — (rate-limited) | `{password}` | consume the single-use link, rotate the hash, and revoke **all** sessions (web cutoff + mobile refresh tokens). Does **not** sign the user in — they log in fresh (so a 2FA account still passes TOTP). Logs `account.password.reset.complete`. | -| GET | `/me/account` | cookie / bearer | — | full self account (`id, username, role, email, status, totp_enabled, has_password`) | +| GET | `/email/verify/:token` | — | — | validate an email-confirmation link → `{username, email}` for the page, else 404 | +| POST | `/email/verify/:token` | — (rate-limited) | — | consume the single-use link, install `email_pending` as `email` and set `email_verified`. **Issues no session** — it proves control of a mailbox, not of an account. Unauthenticated on purpose: the link is opened from a mailbox, routinely on a device with no session, and the token is the proof. **Answers 404 for an unusable link AND for an address another account confirmed first**, deliberately — the two must be indistinguishable, or the endpoint becomes an oracle for which addresses hold accounts. Logs `account.email.verified`. | +| GET | `/me/account` | cookie / bearer | — | full self account (`id, username, role, email, email_verified, email_pending, status, totp_enabled, has_password`) | | PATCH | `/me/account/username` | cookie / bearer (rate-limited) | `{username}` | change own username; re-issues the caller's session | | PATCH | `/me/account/password` | cookie / bearer (rate-limited) | `{newPassword, currentPassword?}` | change/set own password (current required unless the account has none); revokes other sessions, keeps the caller's | | POST | `/me/account/totp/setup` · `…/enable` · `…/disable` | cookie / bearer | `{code}` on enable/disable | self 2FA enrollment (disable needs a valid current code, not a password). **enable** returns the one-time `recoveryCodes`; **disable** clears the user's trusted devices + recovery codes | +| PATCH | `/me/account/email` | cookie / bearer (rate-limited, **password step-up**) | `{email, currentPassword?}` | request an address. **Stages it in `email_pending`; `email` is untouched**, so the account keeps receiving password-reset mail at the address it already has until the emailed link is opened — a typo cannot redirect account recovery. `currentPassword` is required when the account has one (an address is where recovery lands); an SSO-provisioned account with a null hash is exempt, the same carve-out `/me/account/password` makes. Returns `{email_pending, emailed, reason}` — `emailed:false` is reported honestly rather than pretending, because the caller typed this address themselves and there is no enumeration reason to hide it. **429** past the per-user send ceiling | +| POST | `/me/account/email/resend` | cookie / bearer (rate-limited) | — | re-send the link for the staged address; **400** when nothing is pending | +| DELETE | `/me/account/email/pending` | cookie / bearer | — | abandon the staged address **and retire its outstanding links**, so a confirmation email already delivered can no longer install it | | GET | `/me/account/identities` · DELETE `…/:provider` | cookie / bearer | — | list / unlink own SSO identities | | GET | `/me/trusted-devices` | cookie / bearer | — | list own active trusted devices (never tokens) | | POST | `/me/trusted-devices` | cookie / bearer (rate-limited) | `{deviceName?}` | trust the current device; web gets an httpOnly `rg_trust` cookie, native gets `{trustToken}`. **409 `{error:'trusted_device_limit', devices}`** at the cap | diff --git a/website/ENGAGEMENT.md b/website/ENGAGEMENT.md index 4060d12..95239ae 100644 --- a/website/ENGAGEMENT.md +++ b/website/ENGAGEMENT.md @@ -1,7 +1,8 @@ # The Engagement System — findings and plan -**Status:** design of record. **Phase 1 is built** (website#165 + docs#178, with website#164 as its -prerequisite); everything from Phase 1b on is still design. The scope decisions below are +**Status:** design of record. **Phases 1, 1a and 1b are built** (Phase 1: website#165 + docs#178, with +website#164 as its prerequisite; Phase 1a: website#166 + docs#179); everything from Phase 2 on is still +design. The scope decisions below are settled; **four of the eight questions in §7.1 were answered by the org lead on 2026-08-28** — Q1, Q3, Q5 and Q7, and Q1's answer added a whole phase (**Phase 1b**, unique email addresses). Q2, Q4, Q6 and Q8 remain open and block Phases 4, 5b, 2 and 8 respectively. Per CLAUDE.md § Conventions, no implementation @@ -173,6 +174,21 @@ collision as a username collision the instant the index exists: The fix is to distinguish the constraint (read the index name off the driver error) before Phase 1b adds the index — not after. +> **Amended 2026-08-29 (Phase 1b, as built).** This table names **two** callers of +> `isDuplicateUsername()`. There are **five**, and the three it omits fail worse than the two it +> names — `invite.controller.js` accepts an invite to an address already held and fails *after* the +> invitee has clicked the link and chosen a password, while `admin.controller.js` `createUser` and +> `updateUser` had **no catch at all** and turned a duplicate address into an opaque 500 for an admin +> who could see nothing wrong with the form. (`auth/account.controller.js` changeUsername is the +> fifth and is username-only, so it was already correct.) All five are handled; each answers +> differently on purpose, because a public form, an authenticated IdP callback, a half-completed +> invite and an admin screen do not owe the same person the same amount of truth. +> +> The index name is available **only in the driver's message text** — the mariadb connector exposes +> no structured field for it — so the discrimination is a regex over `for key '…'`, with its own +> test. That message also embeds the bound parameters, so on an email collision it *contains the +> address*: a second, independent reason these errors must never be echoed to a client. + **3. SSO auto-provisioning is the source of the duplicates, and CLAUDE.md is stale about it.** CLAUDE.md states *"identities are never auto-provisioned"*. `provisionSsoPlayer` (`sso.controller.js:197`) does exactly that whenever `player_registration ∈ {sso, both}`, writing `profile.email` straight into @@ -1345,7 +1361,7 @@ generated mirror with no CI gate, so it was regenerated wholesale here rather th --- -### Phase 1b — Unique, changeable, verifiable email addresses *(decision 6)* +### Phase 1b — Unique, changeable, verifiable email addresses ✅ *(decision 6)* **Lands alone, between 1 and 2, and before any engagement mail exists.** It touches registration, SSO provisioning and the boot-time schema path — three security-sensitive surfaces — and retrofitting @@ -1364,10 +1380,9 @@ uniqueness *after* a send log and a suppression list hold rows is strictly worse and `email_verified` to `0`. Multiple `NULL`s are legal under a UNIQUE index, so nobody loses an account and nothing cascades. The affected accounts are written to an admin-visible report — *who* was cleared and *what* address they lost — because they are exactly the users who must be contacted. - Then `ALTER TABLE users ADD UNIQUE INDEX IF NOT EXISTS uq_users_email (email)`, pinned to a - case-insensitive collation for the same reason `username` was (`schema.sql:24`): `Foo@x.com` and - `foo@x.com` are one mailbox everywhere that matters, and folding must happen in the index rather than - in application code that can be bypassed. + Then a UNIQUE index — **on a generated `email_norm` column, not on `email`.** The reasoning above + (`Foo@x.com` and `foo@x.com` are one mailbox; folding belongs in the index rather than in bypassable + application code) is right, but the collation this originally named is not: see the amendment below. 3. **A self-serve change-and-verify flow**, which does not exist today (§0.6 finding 4). Set/change address, a signed time-boxed verification link, `email_verified` set only on link use. It lands on **`/auth/me/account` and nowhere else** — Phase 1a made that the single self-service surface. SSO's @@ -1393,6 +1408,78 @@ is used; `Foo@x.com` collides with `foo@x.com`; an upgraded install has the gate migration is idempotent and re-running `ensureSchema()` is a no-op; **no destructive DDL** — the de-dupe nulls a column, it never deletes a row. +#### As built (2026-08-29) + +**The index is on a generated column, because every `_ci` collation is also accent-insensitive.** +Step 2 above said to pin `email` to a case-insensitive collation "for the same reason `username` was". +Tested against the deployment's own MariaDB 11.8, that is wrong in a way that would have destroyed +data: under **both** `utf8mb4_general_ci` and the server-default `utf8mb4_uca1400_ai_ci`, +`josé@x.com` and `jose@x.com` compare EQUAL. They are different mailboxes. A UNIQUE index over either +collation refuses the second address forever, and the de-duplication below would have nulled a +legitimate account's address and reported it as a duplicate that never was. + +The accent-sensitive, case-insensitive collations that would be exactly right +(`utf8mb4_general1400_as_ci`, `utf8mb4_0900_as_ci`) are MariaDB 11.4+ only, so pinning one moves the +"a UNIQUE email can stop a boot" failure of §0.6 to a different trigger. What shipped instead: + +```sql +email VARCHAR(255) NULL, +email_norm VARCHAR(255) COLLATE utf8mb4_bin AS (LOWER(email)) STORED, +UNIQUE KEY uq_users_email_norm (email_norm) +``` + +`LOWER()` under a `_bin` collation folds case without folding accents — verified, not assumed. The +fold still lives in the schema rather than in bypassable application code, which was the point of the +original rule. Multiple NULLs remain legal, which is what lets the de-dupe clear an address without +deleting an account. No foreign key references `users.email`, so the STORED-generated-column trap from +TEAMS.md phase 2 (`ER_GENERATED_COLUMN_FUNCTION_IS_NOT_ALLOWED` on `ON DELETE SET NULL`) does not apply. + +**The de-dupe must group on that same column, and the first version did not.** Written as +`LOWER(u2.email) = LOWER(u.email)`, the comparison uses the *column's* collation — accent-insensitive +— so it over-folds even when the index does not. A seeded fixture caught it nulling `jose@x.com` as a +"duplicate" of `josé@x.com`: the exact defect the index change was made to prevent, reintroduced one +statement later. The migration therefore **adds `email_norm` before de-duplicating and groups on it**, +so the two agree by construction rather than by a hand-matched `COLLATE` clause a later edit can get +wrong. Order in `schema.sql` is load-bearing and commented as such. + +**Four decisions taken at build time**, all approved before any code: + +| | Decision | Why | +|---|---|---| +| Index folding | generated `LOWER()` column + `_bin` index | above | +| Change flow | **pending column**, live address untouched | a typo cannot silently redirect account-recovery mail. Cost: a pending address reserves nothing, so two users may both be pending on one address and the second to confirm loses — with the same generic failure | +| Re-auth | `currentPassword` required, SSO carve-out | an address is where recovery lands, so repointing it is credential-grade; mirrors `changePassword` | +| Report surface | table + dashboard warning + read route | reuses the Phase 1 G22 shape: narrow, self-clearing, silent on installs it does not concern | + +**One deviation from the text above, deliberate:** step 3 says a "signed" link. Every comparable flow +in this codebase (`user_invites`, `password_resets`, `mobile_refresh_tokens`) uses an opaque random +token with only its sha256 at rest, and `email_verifications` matches them rather than introducing a +second token mechanism for one caller. + +**`provisionSsoPlayer` now returns `{ user }` or `{ error }`** instead of the user or a bare null. Two +ways to fail need two things said to the person at the browser; the two call sites map `error` +straight onto the `sso_error` code the login pages already render. + +**Verified on a live rig**, not only in unit tests — a real MariaDB 11.8 seeded with the pre-upgrade +schema plus three accounts sharing an address, upgraded by booting the real server, with a real SMTP +send into a mail catcher (which also discharges Phase 1's outstanding "no live SMTP send"): + +- the upgrade **boots clean**; oldest kept the address, two were nulled and reported with the exact + addresses they lost; `josé@` and `jose@` both survived +- the gate seeded **`off` on the upgrade** and `on` on a fresh install +- the dashboard warning fired with the right count and cleared on acknowledge +- a change request staged the address and **left the live one receiving mail**; the link went only to + the new address; opening it from a session-less client installed the address and **set no cookie** +- a replayed link, and a second account confirming an address the first had just taken, both returned + the **byte-identical** generic 404 — the real reason logged, never returned +- `NEWMAIL@RIG.TEST` was refused at registration as a duplicate of `newmail@rig.test`, while + `néwmail@rig.test` registered successfully beside it + +**Left for later, deliberately:** nothing consumes `email_verification_required` yet — the engine that +would honour it is Phase 4 and the deliverability rules are Phase 9. It is seeded and editable now +because the fresh-vs-upgrade distinction is only knowable at the migration that adds it, and +reconstructing "was this install fresh?" afterwards is guesswork. + --- ### Phase 2 — The trigger registry and the variable contract diff --git a/website/UPGRADE_NOTES.md b/website/UPGRADE_NOTES.md index fbab6b5..e48b64f 100644 --- a/website/UPGRADE_NOTES.md +++ b/website/UPGRADE_NOTES.md @@ -13,6 +13,71 @@ action, whether it is required, and what happens if you do nothing. --- +## Email addresses are now unique, and some accounts may lose theirs + +**Required only if the dashboard says so.** Engagement Phase 1b. + +### What changed + +An account's email address is now the destination for account recovery and, in time, for notification +mail, so an address may belong to **one account only**. Until now it could repeat — SSO sign-up wrote +whatever address the provider returned, without checking — so a database that has been running with +SSO enabled may well hold duplicates. + +Users can now also **set and change their own address**, under *Account → Email address*, confirming +it by opening a link. Before this there was no way for anyone to change their own address at all. + +### What the upgrade does on first boot + +Nothing you need to trigger, and **no account is deleted**. Where several accounts share an address: + +- the **earliest-created** account keeps it — not the "verified" one, because SSO used to mark an + address verified merely for existing, so that flag cannot arbitrate anything; +- every later account has its address **cleared** (set to nothing) and is listed in a report; +- the dashboard then shows a warning naming how many accounts were affected. + +Case is folded — `Foo@x.com` and `foo@x.com` are the same mailbox — but **accents are not**: +`josé@x.com` and `jose@x.com` are correctly treated as two different addresses. + +### What you have to do + +If the warning appears, open **Admin → Users** and read the report. It names each affected account and +the address it lost. **Those users are the reason this warning exists:** they can still sign in +normally, but they can no longer receive password-reset or notification email until they set a new +address themselves, and nothing tells them that. Contact them, and point them at *Account → Email +address*. + +Dismissing the warning keeps the report — it is the record of what the upgrade did, and the only place +the lost addresses survive. + +### Verification email + +Confirmation links are sent through the same transport as everything else, so **if outbound email is +not configured, nobody can confirm an address.** The screen says so honestly rather than claiming a +mail was sent. If you have not configured SMTP yet, see the entry below first. + +### The verification setting + +A new setting, *require a confirmed address before sending notification email*, is seeded **off on an +existing deployment** and **on for a fresh install**. The asymmetry is deliberate: switching it on +retroactively would silently stop mailing every user who had already opted in, on the day you +upgraded. Nothing reads it yet — it takes effect when engagement notifications ship — so there is no +hurry to change it. + +### If you do nothing + +The de-duplication has already run; it runs on the first boot whether or not you read this. What you +lose by ignoring it is the chance to tell the affected users, who will otherwise discover it the next +time they try to reset a password. + +### Data + +Nothing is dropped and no row is deleted. `users` gains `email_norm` (a generated lowercase copy of +`email`, which carries the uniqueness index) and `email_pending`. Two tables are added: +`email_verifications` and `email_dedupe_report`. Cleared addresses survive in the report. + +--- + ## Outbound email: the Gmail connect flow is gone; configure SMTP **Required, if this deployment currently sends email.** Engagement Phase 1. diff --git a/website/api-route-inventory.json b/website/api-route-inventory.json index ed7f851..ea1427e 100644 --- a/website/api-route-inventory.json +++ b/website/api-route-inventory.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"