diff --git a/README.md b/README.md index 444af6d..afbdcd9 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ BeStore is a single-brand online store in one Next.js application: a storefront ## Current state -The application foundation, database model, shared design system, and admin sign-in with required TOTP are implemented. The storefront currently shows a coming-soon page. Product browsing and the core buy loop, card payment, shipping, catalog and order management, customer accounts, and launch work remain planned. The [scope](docs/scope/scope.md) is the source of truth for feature status and the [specifications](docs/specs/) explain completed decisions. +The application foundation, database model, shared design system, and admin sign-in (email and password) are implemented. The storefront currently shows a coming-soon page. Product browsing and the core buy loop, card payment, shipping, catalog and order management, customer accounts, and launch work remain planned. The [scope](docs/scope/scope.md) is the source of truth for feature status and the [specifications](docs/specs/) explain completed decisions. ### Interface previews diff --git a/app/admin/(auth)/forgot-password/page.tsx b/app/admin/(auth)/forgot-password/page.tsx deleted file mode 100644 index e0f3ffb..0000000 --- a/app/admin/(auth)/forgot-password/page.tsx +++ /dev/null @@ -1,33 +0,0 @@ -import type { Metadata } from "next"; -import Link from "next/link"; - -import { AuthCard } from "@/features/admin-auth/components/auth-frame"; -import { ForgotPasswordForm } from "@/features/admin-auth/components/forgot-password-form"; -import { redirectIfSignedIn } from "@/features/admin-auth/require-admin"; - -// Reads the session or search params at the top level; allowed to block until converted -// to Suspense (spec 0005, Caching model). -export const instant = false; - -export const metadata: Metadata = { title: "Reset your password" }; - -export default async function ForgotPasswordPage() { - await redirectIfSignedIn(null); - - return ( - - Back to sign in - - } - > - - - ); -} diff --git a/app/admin/(auth)/mfa/page.tsx b/app/admin/(auth)/mfa/page.tsx deleted file mode 100644 index ddf4db1..0000000 --- a/app/admin/(auth)/mfa/page.tsx +++ /dev/null @@ -1,80 +0,0 @@ -import type { Metadata } from "next"; -import { redirect } from "next/navigation"; - -import { FormNotice } from "@/components/form-notice"; -import { AuthCard } from "@/features/admin-auth/components/auth-frame"; -import { SignOutButton } from "@/features/admin-auth/components/sign-out-button"; -import { TotpCodeForm } from "@/features/admin-auth/components/totp-code-form"; -import { TotpSetup } from "@/features/admin-auth/components/totp-setup"; -import { authMessages } from "@/features/admin-auth/messages"; -import { mfaViewFor } from "@/features/admin-auth/mfa-view"; -import { adminMetadata, requireAdminSession } from "@/features/admin-auth/require-admin"; -import { safeAdminPath } from "@/features/admin-auth/safe-admin-path"; - -// Reads the session or search params at the top level; allowed to block until converted -// to Suspense (spec 0005, Caching model). -export const instant = false; - -export function generateMetadata(): Promise { - return adminMetadata({ title: "Authenticator code" }, { allowAal1: true }); -} - -export default async function MfaPage({ searchParams }: PageProps<"/admin/mfa">) { - const { next } = await searchParams; - const nextPath = typeof next === "string" ? next : null; - const session = await requireAdminSession({ allowAal1: true }); - if (session.aal === "aal2") redirect(safeAdminPath(nextPath)); - - const view = await mfaViewFor(session); - const footer = ( - <> -

Signed in as {session.admin.email}

- - - ); - - switch (view) { - case "verify": - return ( - - - - ); - case "setup": - case "setup-pending": - return ( - - - - ); - case "not-set-up": - return ( - - - - ); - case "rate_limited": - case "unavailable": - return ( - - - - ); - } -} diff --git a/app/admin/(auth)/reset-password/page.tsx b/app/admin/(auth)/reset-password/page.tsx deleted file mode 100644 index 831de77..0000000 --- a/app/admin/(auth)/reset-password/page.tsx +++ /dev/null @@ -1,27 +0,0 @@ -import type { Metadata } from "next"; - -import { AuthCard } from "@/features/admin-auth/components/auth-frame"; -import { ResetPasswordForm } from "@/features/admin-auth/components/reset-password-form"; -import { adminMetadata, requireAdminSession } from "@/features/admin-auth/require-admin"; - -// Reads the session or search params at the top level; allowed to block until converted -// to Suspense (spec 0005, Caching model). -export const instant = false; - -export function generateMetadata(): Promise { - return adminMetadata({ title: "Choose a new password" }); -} - -// Needs aal2: the TOTP step comes before the new password, so the inbox alone is not enough. -export default async function ResetPasswordPage() { - const { admin } = await requireAdminSession(); - - return ( - - - - ); -} diff --git a/app/admin/(auth)/sign-in/page.tsx b/app/admin/(auth)/sign-in/page.tsx index 33f8ec0..5b38ee6 100644 --- a/app/admin/(auth)/sign-in/page.tsx +++ b/app/admin/(auth)/sign-in/page.tsx @@ -17,10 +17,7 @@ export default async function SignInPage({ searchParams }: PageProps<"/admin/sig await redirectIfSignedIn(nextPath); return ( - + ); diff --git a/app/auth/confirm/page.tsx b/app/auth/confirm/page.tsx deleted file mode 100644 index f7189ce..0000000 --- a/app/auth/confirm/page.tsx +++ /dev/null @@ -1,35 +0,0 @@ -import type { Metadata } from "next"; -import { redirect } from "next/navigation"; - -import { AuthCard, AuthFrame } from "@/features/admin-auth/components/auth-frame"; -import { ConfirmResetLinkForm } from "@/features/admin-auth/components/confirm-reset-link-form"; -import { signInPath } from "@/features/admin-auth/safe-admin-path"; - -// Reads the session or search params at the top level; allowed to block until converted -// to Suspense (spec 0005, Caching model). -export const instant = false; - -export const metadata: Metadata = { - title: "Reset your password", - robots: { index: false, follow: false }, -}; - -// Opening the link verifies nothing: mail scanners open links before people do, and a GET -// that verified would spend the one time token. Only Continue (a POST) spends it (AC-9). -export default async function ConfirmPage({ searchParams }: PageProps<"/auth/confirm">) { - const { token_hash: tokenHash, type } = await searchParams; - if (type !== "recovery" || typeof tokenHash !== "string" || tokenHash === "") { - redirect(signInPath({ reason: "invalid_link" })); - } - - return ( - - - - - - ); -} diff --git a/docs/reviews/2026-10-27-feat-defer-analytics.md b/docs/reviews/2026-10-27-feat-defer-analytics.md new file mode 100644 index 0000000..7f0429d --- /dev/null +++ b/docs/reviews/2026-10-27-feat-defer-analytics.md @@ -0,0 +1,34 @@ +# Review, feat/defer-analytics, 2026-10-27 + +**Reviewed by**: Sonnet 5.5 (author on a different model) +**Scope**: 112 files, branch vs main (merge base 7e00d5b, plus uncommitted docs edits) +**Verdict**: Approve with nits + +## Summary +Spec 0019 removes admin TOTP/MFA, the admin email reset and `/auth/confirm`, and replaces them with a password only guard that also requires a `password` entry in `amr`, plus `pnpm admin:reset-password`. The security model holds up: every admin page, layout, action and the one admin route (sign-out, deliberately open) is guarded, the proxy and `requireAdmin()` share `decideAdminAccess`, and an email link session is turned away both in the proxy (real redirect) and in the guard. The only findings are stale nested docs and small cleanups. + +## Minor +### 🟡 Stale account AGENTS.md still describes the admin reset and TOTP, `src/features/account/AGENTS.md:15,28` +**Problem**: Line 15 says the hook "picks the template (admin reset first)"; line 28 says a store runner "always gets the admin reset email" and mentions a verified TOTP factor and `auth.mfa_factors` reads. After this change the hook skips store runners' recovery email and `runsTheStore` reads `admin_users` only. +**Why it matters**: This file is loaded as agent context; it now contradicts the code and spec 0019 AC-8, AC-10. +**Suggested fix**: Reword both lines (skip, `admin_users` only, no `auth.mfa_factors`). Likely /sync's job, but it should land with this branch. + +### 🟡 Double admin lookup in the reset script, `scripts/admin/reset-password.ts:14-17` +**Problem**: `findAdminId` is called to fail early, then `resetAdminPassword` looks the admin up again. +**Why it matters**: Harmless, but two reads of the same row and a small chance of divergence; `resetAdminPassword` could accept an id or the script could just call it after prompting. +**Suggested fix**: Keep the early refusal but pass the found id into the reset step. + +## Nits +- ⚪ `src/features/admin-auth/require-admin.ts:20`, if `getClaims()` fails after `getUser()` succeeded, `amr` is null and the user is sent to sign in with `reason=expired`; fail closed is right, the "expired" message is slightly misleading. +- ⚪ `src/features/admin-auth/proxy.ts:107`, gate lookup failure still fails open (pre-existing; page guard still refuses), worth keeping the comment current. +- ⚪ `src/features/admin-auth/sign-out.ts:6-8`, import order is inconsistent (`endLocalSession` import after local imports); cosmetic. +- ⚪ `tests/e2e/admin/denials.spec.ts:31`, `/admin/mfa` assertion is fine but duplicates `sign-in.spec.ts:105` removed paths check. + +## Strengths +- Password only rule is pinned where it matters: `tests/stack/admin-amr.stack.test.ts` proves a password sign in has a `password` amr and a verified recovery link does not, and `admin-scripts.stack.test.ts` pins that the reset ends every session. +- `decideAdminAccess` is pure, fails closed on unreadable `amr`, handles both `amr` shapes; the proxy can only deny earlier, never grant, and gives a real 3xx for password-less sessions. +- Defense in depth for admin inbox takeover: hook skips recovery for store runners (200, same timing), `confirmEmail` signs out and refuses, `admin:create` overwrites a planted password and confirms email, 14 char admin password. +- Clean removal: no remaining MFA/TOTP/`otpauth`/`/auth/confirm` references in code, config, emails or runbooks beyond intentional removed-path tests; runbook updated with compromise procedure and Firewall rule widened to `/account/sign-in`. + +## Test coverage +Good. Unit tests cover access verdicts, both `amr` shapes, proxy decisions and gate, `requireAdmin`, hook skip, confirm refusal, password schema, CLI helpers and script cores. Stack tests pin Supabase behaviors the design relies on; e2e covers sign in, removed paths, denials, a recovery-link admin session being refused, and a11y of the sign-in page. No gaps found in new logic. diff --git a/docs/runbooks/admin-accounts.md b/docs/runbooks/admin-accounts.md index 08078a3..9959903 100644 --- a/docs/runbooks/admin-accounts.md +++ b/docs/runbooks/admin-accounts.md @@ -1,6 +1,6 @@ # Runbook: admin accounts and admin sign in -Spec: [0004](../specs/0004-admin-sign-in/index.md). Admins sign in with email, password and a TOTP code from an authenticator app. There is no admin UI for accounts: the three `admin:*` scripts below are the whole lifecycle. +Specs: [0004](../specs/0004-admin-sign-in/index.md), with [0019](../specs/0019-remove-admin-mfa/index.md). Admins sign in with email and password only; there is no authenticator step. There is no admin UI for accounts: the three `admin:*` scripts below are the whole lifecycle, and an admin password changes only through them. These are recommendations for running things safely. Each step says why, so you can judge when to skip one. @@ -11,18 +11,18 @@ The production auth settings live in the Supabase dashboard, not in code, so the ### Supabase project: Authentication settings - **Sign ups**: on since customer accounts (spec 0012); see [customer-accounts.md](customer-accounts.md). Signing up never makes an admin: only `admin:create` adds an `admin_users` row. Keep the Email provider itself on (turning it off also blocks password sign in). -- **Passwords**: minimum length 8, required characters "lowercase, uppercase, digits and symbols". `passwordSchema` in `src/lib/auth/password.ts` mirrors this; keep the two in step. -- **MFA**: TOTP enroll and verify both on. +- **Passwords**: minimum length 8, required characters "lowercase, uppercase, digits and symbols". `passwordSchema` in `src/lib/auth/password.ts` mirrors this; keep the two in step. Admin passwords set by the scripts must also be at least 14 characters (`adminPasswordSchema`, spec 0019 AC-16). +- **MFA**: TOTP enroll and verify both **off** (spec 0019, AC-11). If any admin still has a factor from before spec 0019, delete it first (Authentication, Users, the admin, remove the factor; or `DELETE FROM auth.mfa_factors;` in the SQL editor), then turn TOTP off. Locally, `supabase/config.toml` already has it off; run the same `DELETE` against the local database (or reset the local stack) to clear old dev factors. - **Site URL**: the production domain, the same value as `NEXT_PUBLIC_SITE_URL` (for example `https://shop.example.com`). Reset links always point here, so a reset started from a preview deploy lands on production. Test password reset locally, not on previews. - **Redirect URLs**: `https:///**`. - **JWT signing keys**: switch to asymmetric keys, so `getClaims()` in the proxy verifies tokens locally without a network call. -- **Auth emails**: since spec 0012, Supabase hands every auth email to the app's Send Email Hook, which sends the admin reset email (`emails/admin-reset-password.tsx`, link `/auth/confirm?token_hash=...&type=recovery`) to anyone with an `admin_users` row or an authenticator, whatever page asked. The hook setup is in [customer-accounts.md](customer-accounts.md). Until it is done, only the scripts can recover an admin. +- **Auth emails**: since spec 0012, Supabase hands every auth email to the app's Send Email Hook. Since spec 0019 the hook sends **nothing** for a password reset of anyone with an `admin_users` row (it answers 200 and logs `auth_email.skipped`), so an inbox alone can never change an admin password. The hook setup is in [customer-accounts.md](customer-accounts.md). **Check it after setup**: request a reset on `/account/forgot-password` for an admin email and confirm no email arrives. With the hook off, Supabase's default recovery email would reach the admin, and its link could set a new password through the Auth API directly. -### Vercel Firewall rule (AC-15) +### Vercel Firewall rule (spec 0019, AC-14): required before the first admin exists -Supabase sees every sign in coming from Vercel's servers, so its per IP limit is shared by all admins. The Firewall rule is the real per IP guard. +The password is the only factor, and Supabase sees every sign in coming from Vercel's servers, so its per IP limit is shared by everyone. The Firewall rule is the real per IP guard. An admin password works on both sign in pages, and both sessions open `/admin`, so the rule covers both. -- Rule: rate limit, condition "Request path is one of `/admin/sign-in`, `/admin/mfa`, `/admin/forgot-password`" and "Method is `POST`". +- Rule: rate limit, condition "Request path is one of `/admin/sign-in`, `/account/sign-in`" and "Method is `POST`". - Limit: 10 requests per 60 seconds, keyed by IP. - Action: deny, for 10 minutes. - Apply it to the production environment and publish the change. @@ -45,9 +45,9 @@ The file holds the service role key, which bypasses every Auth rule. Keep it on pnpm admin:create --email --name "" ``` -It asks twice for a password (hidden, never a flag, so it stays out of shell history) that meets the policy above. If an auth user with that email already exists (for example a customer), it keeps that user and their password, confirms the email, and adds or re-enables the admin row. +It asks twice for a password (hidden, never a flag, so it stays out of shell history) of at least 14 characters that meets the policy above. Use a password manager to generate it. If an auth user with that email already exists (for example a customer), it keeps that user, sets the password you typed, confirms the email, and adds or re-enables the admin row. The password is always set, so a password someone planted through public sign up never becomes an admin password (spec 0019, AC-15). -**The enrollment window.** Until the new admin signs in for the first time, whoever holds the password can set up the authenticator. So hand the password over directly (in person or through a password manager share, never plain email or chat), and ask them to sign in soon. At the first sign in they scan a QR code and type a code, and from then on every sign in needs it. +Hand the password over directly (in person or through a password manager share, never plain email or chat). ### Disable an admin @@ -55,24 +55,30 @@ It asks twice for a password (hidden, never a flag, so it stays out of shell his pnpm admin:disable --email ``` -Their next request gets a 404. Their session cookie becomes useless and dies at the 12 hour cap. To re-enable them, run `admin:create` again with the same email (it keeps their password). +Their next request gets a 404. Their session cookie becomes useless and dies at the 12 hour cap. To re-enable them, run `admin:create` again with the same email (it asks for a password and sets it). -### Lost authenticator +### Forgotten password + +There is no self service reset for admins: the sign in page has no "Forgot password?" link, and the customer reset sends nothing to an admin. Reset it from the terminal: ```bash -pnpm admin:reset-mfa --email +pnpm admin:reset-password --email ``` -This deletes their TOTP factors and every session, so every device is signed out. At their next sign in they set up the authenticator again, which reopens the enrollment window above: confirm who you are talking to before you run it, and ask them to sign in right away. +It finds the admin by email (enabled or disabled), asks twice for a new password of at least 14 characters, and sets it. Supabase's own auth audit log records the change. Confirm who you are talking to before you run it. -### Forgotten password +Changing the password through Supabase's admin API also **signs the admin out on every device**: Supabase deletes all of that user's sessions (the stack test in `tests/stack/admin-scripts.stack.test.ts` pins this). + +### Compromised admin account -The admin uses "Forgot password?" on the sign in page. The link asks for their authenticator code before it lets them set a new password, so an inbox alone is not enough. +1. `pnpm admin:disable --email `: every request from that account gets a 404 at once. +2. `pnpm admin:reset-password --email `: the old password stops working, and every session made with it ends. +3. `pnpm admin:create --email --name ""` when you are ready to re-enable them (it sets a password again). -An admin who never set up an authenticator cannot reset this way (a reset link may verify a factor, never create one). They have not finished their first sign in yet, so the simplest fix is to agree a new first password with them. There is no script for that yet; if it comes up, it is worth a small `admin:set-password` script rather than a dashboard workaround. +Because the password change ends every session, there is no need to wait for the 12 hour cap before re-enabling. If a future Supabase version stops ending sessions on a password change (the stack test would fail), wait 12 hours after step 2 before step 3. ## Where to look when something goes wrong -- Vercel logs: every auth event is one JSON line named `auth.*` (for example `auth.sign_in.failed` with a `reason`). Admins show as an id; unknown emails as a SHA-256 hash; never a password, code or token. +- Vercel logs: every auth event is one JSON line named `auth.*` (for example `auth.sign_in.failed` with a `reason`). Admins show as an id; unknown emails as a SHA-256 hash; never a password, code or token. An admin who signs in through `/account/sign-in` writes `account.*` events instead, not `auth.sign_in.*`. +- `auth_email.skipped` with `reason: "runs_the_store"`: someone asked for a password reset for an admin email. Expected if the admin tried it; worth a look if they did not. - Supabase: Authentication, Logs, for Supabase's own audit trail. -- "Too many incorrect codes": 5 wrong codes for one account in 15 minutes lock the code step for that account, from any device. It unlocks on its own after 15 minutes. If the admin did not type those codes, their password has leaked: have them reset it. diff --git a/docs/runbooks/customer-accounts.md b/docs/runbooks/customer-accounts.md index 5225b25..1fd6ff2 100644 --- a/docs/runbooks/customer-accounts.md +++ b/docs/runbooks/customer-accounts.md @@ -1,6 +1,6 @@ # Runbook: customer accounts in production -Spec: [0012](../specs/0012-customer-accounts/index.md). Customers sign up with their email, confirm it from a link, then choose a password. Every auth email (confirm sign up, reset password, the admin reset) leaves through the app's Send Email Hook and Resend, never through Supabase's own mailer. +Spec: [0012](../specs/0012-customer-accounts/index.md). Customers sign up with their email, confirm it from a link, then choose a password. Every auth email (confirm sign up, reset password) leaves through the app's Send Email Hook and Resend, never through Supabase's own mailer. These are recommendations for running things safely. Each step says why, so you can judge when to skip one. They are applied at deploy (scope feature 19); nothing here is needed before then. @@ -37,7 +37,7 @@ Supabase sees every sign in, sign up and reset coming from Vercel's servers, so ## Where to look when something goes wrong -- **No auth email arrives** (sign up, reset, or an admin reset): the hook failed or never ran. The visitor still saw "Check your email", because Supabase runs after the reply. Look in Vercel logs for `auth_email.failed` (an error, with Resend's error name) and `auth_email.bad_signature` (the secret differs between Supabase and Vercel). Supabase's own Auth logs show `hook_failed` or `hook_timeout`. Resend or the app being down stops every sign up and reset, admins included, until it recovers. +- **No auth email arrives** (sign up or reset): for an admin email that is expected, the hook skips it on purpose and logs `auth_email.skipped` (spec 0019, AC-8). Otherwise the hook failed or never ran. The visitor still saw "Check your email", because Supabase runs after the reply. Look in Vercel logs for `auth_email.failed` (an error, with Resend's error name) and `auth_email.bad_signature` (the secret differs between Supabase and Vercel). Supabase's own Auth logs show `hook_failed` or `hook_timeout`. Resend or the app being down stops every sign up and reset until it recovers. - **`account.email_conflict`** (an error): someone's account setup found another live account holding the same email. It should not happen; compare the two `customers` rows and their `auth.users` rows before changing anything. - **`account.delete_incomplete`** (an error): a customer's login was deleted but the database commit failed after it. The leftover `customers` row has no login and is removed when that email signs up again; it can also be deleted by hand. - Every account log line carries the customer id, order ids, counts and reason codes, never an email, name, phone, address, password, token, token hash or idempotency key. diff --git a/docs/scope/scope.md b/docs/scope/scope.md index 82a607e..bb8cfff 100644 --- a/docs/scope/scope.md +++ b/docs/scope/scope.md @@ -32,7 +32,7 @@ _These are recommendations to keep your build orderly, not requirements. Skip an | 23 | Native controls in GET forms | Slice 9 follow up | done | | 16 | Legal pages & cookie consent | Launch | done | | 17 | SEO for storefront pages | Launch | done | -| 18 | Analytics & error tracking | Launch | planned | +| 24 | Remove admin 2FA | Launch | in-progress | | 19 | Production deploy | Launch | planned | ## Foundations @@ -101,6 +101,7 @@ spec [0004](../specs/0004-admin-sign-in/index.md) · code in [src/features/admin - [x] Test it: `/test admin sign in` - [x] Review it (fresh model): `/check review admin sign in` - [x] Document it: `/document admin sign in` +_Note: the TOTP step, the admin email reset and `admin:reset-mfa` were removed by feature 24 (spec [0019](../specs/0019-remove-admin-mfa/index.md)); admins sign in with a password only._ ### 6. Core buy loop · done The narrow end to end path: an admin creates a product with variants, price, stock, and one image; the storefront shows a product list and a product page; a customer picks a variant, adds it to a cart, and reaches checkout as a guest. @@ -337,17 +338,27 @@ spec [0018](../specs/0018-storefront-seo/index.md) · code in [src/lib/seo/](../ - [x] Verify it: `/check verify SEO for storefront pages` (the Rich Results Test waits for the first preview deploy, a spec 0018 follow-up) - [x] Test it: `/test SEO for storefront pages` -### 18. Analytics & error tracking · needs a decision -See how customers move through the store and know when something breaks. -**Done when:** the funnel (view product, add to cart, start checkout, pay) is tracked with consent respected; front end and server errors, including failed payments, raise alerts. -- [ ] Design it (spec): `/architect analytics & error tracking` +### 24. Remove admin 2FA · in-progress · GA +Admins sign in with email and password only; the TOTP step goes. An admin password changes only from the terminal, so an inbox alone never opens `/admin`. +**Done when:** an admin reaches `/admin` with just a password; no MFA or admin email reset surface remains; `pnpm admin:reset-password` works; every existing denial still holds. +spec [0019](../specs/0019-remove-admin-mfa/index.md) · supersedes parts of spec [0004](../specs/0004-admin-sign-in/index.md) and [0012](../specs/0012-customer-accounts/index.md) · code in [src/features/admin-auth/](../../src/features/admin-auth/), [scripts/admin/](../../scripts/admin/), [docs/runbooks/admin-accounts.md](../runbooks/admin-accounts.md) +- [x] Design it (spec): `/architect remove admin 2FA` +- [x] Build it: `/develop remove admin 2FA` + - [x] Terminal reset replaces the email reset: `admin:reset-password`, 14 character admin passwords, `admin:create` sets the password, hook skips store runners, `/auth/confirm` and admin reset pages gone (AC-4, AC-7 to AC-10, AC-15, AC-16) + - [x] Password only guard: `password` amr rule, proxy redirect, MFA page, actions, lockout, `admin:reset-mfa` and `otpauth` gone (AC-1 to AC-6, AC-11, AC-12, AC-13) + - [x] Config and records: TOTP off, runbook, spec 0004 and 0012 notes, README, Firewall item (AC-11, AC-14) +- [x] Verify it: `/check verify remove admin 2FA` +- [x] Test it: `/test remove admin 2FA` +- [x] Review it (fresh model): `/check review remove admin 2FA` +- [ ] Document it: `/document remove admin 2FA` ### 19. Production deploy The last step: ship to Vercel and the production Supabase project, and turn on the production only settings every earlier slice left for deploy time. **Done when:** the production app runs on Vercel against production Supabase, and each production only item below is applied and checked there. - [ ] Build it: `/develop production deploy` - - [ ] Admin sign in, from the runbook [docs/runbooks/admin-accounts.md](../runbooks/admin-accounts.md): production Supabase auth settings, Resend SMTP, recovery template, redirect URLs, asymmetric JWT keys, the first real admin (spec 0004 AC-10, AC-11) - - [ ] Vercel Firewall rate limit on `POST` to `/admin/sign-in`, `/admin/mfa`, `/admin/forgot-password`: 10 per minute per IP, deny 10 minutes (spec 0004 AC-15) + - [ ] Admin sign in, from the runbook [docs/runbooks/admin-accounts.md](../runbooks/admin-accounts.md): production Supabase auth settings, Resend SMTP, redirect URLs, asymmetric JWT keys, the first real admin (spec 0004 AC-10, AC-11) + - [ ] **Required before the first production admin exists**: Vercel Firewall rate limit on `POST` to `/admin/sign-in` and `/account/sign-in`: 10 per minute per IP, deny 10 minutes (spec 0019 AC-14, replacing spec 0004 AC-15) + - [ ] Admin TOTP off in production: delete any existing factors, then turn TOTP enroll and verify off; check the Send Email Hook is on (a reset request for an admin email sends nothing) (spec 0019 AC-8, AC-11) - [ ] Vercel Firewall rate limit on `POST /checkout` (promo code Apply and Pay), generous enough for real Pay clicks (spec 0014 Follow-up) - [ ] Verify it: `/check verify production deploy` @@ -370,6 +381,7 @@ Out of scope for the current build pass, kept so the plan stays honest. - **Customer email change**: Supabase secure email change, keeping `customers.email` and order linking in step · needs a decision · from spec 0012 - **Order data retention and export**: set the accounting retention period, extend the spec 0008 purge to settled orders past it, and offer account holders a data export · needs a decision · GA · from spec 0012 - **Admin customers page**: search customers and see their orders, if support needs it · needs a decision · from spec 0012 +- **Analytics & error tracking (feature 18)**: the consent respected funnel (view product, add to cart, start checkout, pay) and alerts on front end, server and payment errors · needs a decision ## Legend diff --git a/docs/specs/0004-admin-sign-in/index.md b/docs/specs/0004-admin-sign-in/index.md index be6cc01..87c8cea 100644 --- a/docs/specs/0004-admin-sign-in/index.md +++ b/docs/specs/0004-admin-sign-in/index.md @@ -1,7 +1,9 @@ # 0004. Admin sign in with required TOTP **Date**: 2026-09-25 (amended 2026-10-15 by [spec 0012](../0012-customer-accounts/index.md): public sign up, the recovery template, the proxy matcher) -**Status**: Accepted +**Status**: Accepted, partly superseded by [spec 0019](../0019-remove-admin-mfa/index.md) (2026-10-27) + +> **Superseded by spec 0019**: admins sign in with email and password only; TOTP and the admin email reset are gone, and an admin password changes only through `pnpm admin:reset-password`. Superseded: AC-2 in part, AC-4, AC-5, AC-9, AC-10 in part (admin passwords need 14 characters), AC-11 in part (`admin:reset-mfa` is gone), AC-13 in part, AC-14 in part (`/admin/sign-in` is the only auth page), AC-15 in part (the Firewall rule now covers both sign in paths), AC-16, AC-17 in part. Everything else still holds. ## Summary diff --git a/docs/specs/0012-customer-accounts/index.md b/docs/specs/0012-customer-accounts/index.md index 1b50e93..d383c70 100644 --- a/docs/specs/0012-customer-accounts/index.md +++ b/docs/specs/0012-customer-accounts/index.md @@ -1,7 +1,9 @@ # 0012. Customer accounts **Date**: 2026-10-14 (amended the same day after a cross check: AC-1 to AC-5, AC-7 to AC-11, AC-13 to AC-15, AC-17, AC-19, AC-20, AC-23, AC-24, AC-26; amended 2026-10-14 again: AC-15 now names where the order's `customer_name` comes from; amended 2026-10-15 after the code review: AC-2, AC-3, AC-5, AC-7, AC-13, AC-24, the template choice, and the leftover account cart cookie) -**Status**: Accepted +**Status**: Accepted, partly superseded by [spec 0019](../0019-remove-admin-mfa/index.md) (2026-10-27) + +> **Superseded by spec 0019**: the admin branch of AC-8 and AC-19. The Send Email Hook no longer sends an admin reset email (a recovery email for a store runner is skipped), `/auth/confirm` is gone, and a store runner is an `admin_users` row in any state, with no `auth.mfa_factors` check. Everything else still holds. ## Summary @@ -219,7 +221,7 @@ Reasoning and options: see [rationale.md](rationale.md). | checkout page | prefill | pending order for this cart (spec 0007), else default `customer_addresses` row | | address book | country | the store's country from `src/lib/shipping` (spec 0007) | | `signUpFromOrder` | email, eligibility | the order found by `stripe_checkout_session_id`: `email`, `status`, `customer_id`; the visitor's session | -| AC-20 | open order count, admin refusal | count of the customer's `orders` with status `paid`; `admin_users` row by id or a verified `auth.mfa_factors` row | +| AC-20 | open order count, admin refusal | count of the customer's `orders` with status `paid`; `admin_users` row by id (the `auth.mfa_factors` check is gone, spec 0019) | | AC-21 | Account or Guest | `orders.customer_id` null or not | **Key invariants**: diff --git a/docs/specs/0019-remove-admin-mfa/index.md b/docs/specs/0019-remove-admin-mfa/index.md new file mode 100644 index 0000000..fc81564 --- /dev/null +++ b/docs/specs/0019-remove-admin-mfa/index.md @@ -0,0 +1,213 @@ +# 0019. Remove admin two factor sign in + +**Date**: 2026-10-27 +**Status**: In Progress + +## Summary + +Admins now sign in to `/admin` with their email and password only. The authenticator step (TOTP, a 6 digit code from a phone app) is removed completely, with all its pages, actions, scripts and tests. Because the password becomes the only lock, the password must never be changeable from an inbox alone: the admin self service password reset goes too, and an admin's password is reset only from the terminal with a new `pnpm admin:reset-password` script. The 12 hour session cap, the 404 for anyone who is not an active admin, and `admin:create` / `admin:disable` stay exactly as spec [0004](../0004-admin-sign-in/index.md) built them. + +This spec supersedes the TOTP and admin reset parts of spec 0004 (AC-2 in part, AC-4, AC-5, AC-9, AC-10 in part, AC-11 in part, AC-13 in part, AC-14 in part, AC-15 in part, AC-16, AC-17 in part) and the admin branch of spec [0012](../0012-customer-accounts/index.md) AC-8 and AC-19. Everything else in those specs still holds. + +## Requirements + +**User stories**: +- As an admin, I want to sign in with my email and password only, so that I reach the panel without an authenticator app. +- As the store owner, I want an admin's password to be changeable only from the terminal, so that someone who takes over an admin's inbox still cannot get into `/admin`. +- As the store owner, I want every existing denial (non admins, disabled admins, the 12 hour cap) to keep working, so that removing a factor does not open anything else. + +**Acceptance criteria**: +- **AC-1**: An active admin who submits the correct email and password on `/admin/sign-in` lands straight on the `next` path when it passes spec 0004 AC-12, else on `/admin`, which shows "Signed in as ". There is no `/admin/mfa` step. A wrong email, a wrong password, or a non admin's valid credentials still show "Email or password is incorrect." and leave no session (spec 0004 AC-3, unchanged). +- **AC-2**: `decideAdminAccess` no longer checks the assurance level (`aal1` or `aal2`, Supabase's record of how many factors a session passed) and has no `mfa` verdict. Instead an admin session must have been opened by a password: its `amr` list (the sign in methods recorded in the token) must contain a `password` entry. A session for an admin user without one (opened by an email link: `recovery`, `otp`, or a sign up confirm) gets the `sign-in` verdict on every `/admin` page and admin action, so it is redirected to `/admin/sign-in` like a visitor with no session. The proxy answers that redirect itself on page loads (a real 3xx, not a 200 streamed shell); an admin action redirects from its own guard. Both `amr` shapes Supabase uses (a plain string, or an object with `method`) are read. +- **AC-3**: The 12 hour cap from the earliest `amr` timestamp, the 404 for a user with no `admin_users` row or with `disabled_at` set, and sign out on this device only all behave exactly as spec 0004 AC-6, AC-7 and AC-8 say. +- **AC-4**: `/admin/mfa`, `/admin/forgot-password` and `/admin/reset-password` no longer exist. They behave like any unknown admin path: a visitor with no session is redirected to `/admin/sign-in?next=`, and an active admin gets the 404 page. `/auth/confirm` no longer exists and answers 404, and the proxy no longer runs on `/auth/:path*`. +- **AC-5**: `/admin/sign-in` has no "Forgot password?" link and no hint about resets. Its `reason` messages are only `expired` and `signed_out`; any other `reason` value (for example an old `too_many_codes` or `invalid_link` bookmark) shows no message. +- **AC-6**: A signed in visitor who opens `/admin/sign-in` is sent on only when `decideAdminAccess` allows them: an active admin with a password session goes to the validated `next` or `/admin`. Anyone else sees the sign in page. +- **AC-7**: `pnpm admin:reset-password --email [--env-file ]` finds the admin by lower cased email in `admin_users` (enabled or disabled, the same lookup as `admin:disable`), prompts twice for a hidden password, checks it with `adminPasswordSchema` (AC-16, naming each missing part), refuses "The passwords do not match." on a mismatch, and sets it with `auth.admin.updateUserById(admin.id, { password })`. Its strings: without `--email`, "Usage: pnpm admin:reset-password --email [--env-file ]"; no row, "No admin has that email." (nothing changes); Supabase refusing, "Could not set the password: "; success, "Password changed for ." then "Every session they had is signed out." (Supabase's admin password change deletes every session of the user; the stack test pins it). It writes no log line: Supabase's own auth audit log records the admin API update. +- **AC-8**: The Send Email Hook sends nothing for a `recovery` email to a user who runs the store (`runsTheStore`, AC-10): it answers 200 so Supabase does not retry or surface an error, and logs `auth_email.skipped` with the user id and `reason: "runs_the_store"`. The customer `/account/forgot-password` page still shows its usual neutral reply for that email, at the same speed. The `admin_reset` email kind and `emails/admin-reset-password.tsx` are removed. +- **AC-9**: A recovery link for a user who runs the store that still reaches `/account/confirm` (an old link, or one made outside the app) changes no password: after `verifyOtp` the action ends the session it just made on this device (`signOut({ scope: 'local' })`) and redirects to `/account/confirm?failed=1&type=recovery`. A sign up confirm for a store runner keeps their password as today (spec 0012). +- **AC-10**: `runsTheStore(userId)` is true only when an `admin_users` row exists for that id, in any state. It no longer reads `auth.mfa_factors`. Its other callers (customer set password, change password, delete account) keep refusing a store runner as today. +- **AC-11**: `supabase/config.toml` sets `[auth.mfa.totp] enroll_enabled = false` and `verify_enabled = false`. The `admin:reset-mfa` script and package script, the `otpauth` dev dependency, the TOTP e2e helpers, and every MFA unit, e2e and stack test (`tests/stack/mfa-challenges.stack.test.ts` included) are deleted, and the test auth stub no longer creates `auth.mfa_factors`. The runbook records the same TOTP setting for the production Supabase project and a one time step to delete existing factors. +- **AC-12**: The `auth.mfa.enrolled`, `auth.mfa.succeeded`, `auth.mfa.failed`, `auth.mfa.locked`, `auth.reset.requested` and `auth.password.changed` events are no longer emitted. Every other `auth.*` event and the logger's redaction (including the `code` key) are unchanged; comments that justify the `code` redaction by TOTP codes are reworded (auth codes in general). +- **AC-13**: `/admin/sign-in`, now the only admin auth page, still works by keyboard alone and axe reports zero violations on desktop and phone (spec 0004 AC-14, narrowed to this page). +- **AC-14**: The scope's production deploy item for the Vercel Firewall rate limit covers `POST /admin/sign-in` and `POST /account/sign-in` (an admin's password works on both, and both sessions open `/admin`), 10 per minute per IP, deny 10 minutes, and is marked required before the first production admin exists. Spec 0004 AC-15 is replaced by this rule. +- **AC-15**: `pnpm admin:create` for an email that already has an auth user (for example a customer) no longer keeps that user's password: it prompts for a new one like a new user (AC-16) and sets it together with `email_confirm: true`, so a password planted through public sign up on an unconfirmed user can never become an admin password. Its last line becomes "They sign in with the password you set." (the authenticator line goes). +- **AC-16**: Admin passwords set by the scripts must pass `adminPasswordSchema`: the shared `passwordSchema` rule plus at least 14 characters, named as its own missing part ("At least 14 characters"). Customer passwords keep the spec 0004 AC-10 rule. Customer password change and set password already refuse a store runner (AC-10), so the scripts are the only way an admin password is set. + +## Decision + +**Chosen option**: Option 3: remove TOTP completely and move admin password resets to the terminal. + +The guard accepts a password session at any assurance level, every MFA surface is deleted, and the only way to change an admin password without knowing it is `pnpm admin:reset-password`, which needs the production service role key. + +**Implementation skills**: `supabase` (`supabase/agent-skills`, `.agents/skills/supabase/`) · `prisma-client-api` (`prisma/skills`, `.agents/skills/prisma-client-api/`) + +**Decisions made in the grilling session** (fixed inputs, 2026-10-27): +- Remove TOTP completely; no flag, no optional per admin factor. +- The password as the only lock is an accepted risk (see Consequences). +- Keep the 12 hour cap, `admin:create`, `admin:disable`, the 404 denials, the proxy first gate and the logger's `code` redaction. +- Replace the admin email reset with `pnpm admin:reset-password`; it does not end sessions itself (no write to `auth.sessions`); `admin:disable` is the immediate cut off. Built result: Supabase's password change ends every session on its own (see AC-7). +- Remove the sign in page's reset link with no replacement hint. +- `runsTheStore` checks `admin_users` only; a store runner gets no reset email from either flow. +- Turn TOTP off in `config.toml` and in the production project; delete existing factors once. +- The Firewall rule covers admin sign in only and becomes required before launch (widened to `/account/sign-in` after the cross check, AC-14). +- Work on the current branch `feat/defer-analytics`, one Conventional Commit per milestone. + +**Decisions made while writing** (each with its runner up): +- **Admin sessions must come from a password (AC-2)**: once `aal2` is gone, any session for an admin user would open the panel, including one made by verifying an email link (`verifyOtp` signs the user in). Requiring a `password` entry in `amr` keeps "an inbox alone never opens `/admin`" true even if a recovery or confirm link for an admin is ever made. It replaces `isResetSession`, which only existed to stop enrollment. Runner up: rely on the hook never sending admin links, which leaves the panel one mistake away from an inbox takeover. +- **One `amr` source in the guard (AC-2)**: after `getUser()` confirms the user, the guard reads `amr` from `getClaims()` (as the proxy already does) instead of `mfa.getAuthenticatorAssuranceLevel()`, so no `auth.mfa` API stays in use. A pure `hasPasswordMethod(amr)` beside `sessionStartedAtSeconds` reads both entry shapes. A stack test pins the assumption the whole rule rests on: `signInWithPassword` gives a `password` entry, and a recovery link verified with `verifyOtp` gives none. Runner up: keeping `getAuthenticatorAssuranceLevel()`, a second reader of the same token from an API this change otherwise drops. +- **The proxy redirects password less sessions (AC-2)**: `decideAdminGate` today continues on `sign-in`, which under Cache Components lets the page's own `redirect()` answer 200. It now redirects page loads to `signInPath({ next })` on a `sign-in` verdict (expiry is still decided earlier by `decideProxy`). Runner up: leaving it to the page guard, which breaks the status code rule in `AGENTS.md`. +- **A stronger admin password (AC-16)**: with one factor, length is the cheapest real defense against guessing that slips past rate limits (the Supabase API is callable directly with the public anon key). 14 characters with a password manager costs the operator nothing. Runner up: keeping 8, which spec 0004 accepted only because TOTP sat behind it. +- **`admin:create` sets the password for an existing user (AC-15)**: public sign up lets anyone create an unconfirmed user with a password they know; `admin:create` confirming that user would hand the stranger an admin password. Runner up: refusing unconfirmed users only, which still keeps a customer's weaker password as the admin password. +- **The hook skips instead of refusing (AC-8)**: answering 200 without sending means Supabase reports success to `resetPasswordForEmail`, so the customer page's reply and timing do not change and nothing reveals that the email belongs to an admin. The skip is decided before the kind: `AuthEmailKind` becomes `"confirm_signup" | "customer_reset"`, and `authEmailLink` loses the `/auth/confirm` path. If `runsTheStore` throws, the hook still answers 500 as today (no send). `usedActionType` already refuses every other email type (magic link, invite, email change) with 400, which stays. Runner up: answering 4xx, which Supabase turns into an error on the reset call (visible in logs, and a timing difference). +- **Delete `/auth/confirm` instead of refusing there (AC-4)**: it only ever served the admin reset. With no admin reset emails, it has no legitimate visitor, and an old link should 404 like any dead path. The proxy matcher drops `/auth/:path*`. `robots.ts` keeps disallowing `/auth` (harmless, and one less change). Runner up: keep the route and redirect every link to the invalid link message, which keeps dead code alive. +- **Sign out after a refused recovery confirm (AC-9)**: `verifyOtp` already set a session cookie before the store runner check runs. AC-2 would block it from `/admin` anyway, but leaving a stray signed in session from a refused link is wrong on its face, so the action ends it on this device. Runner up: leave the session, relying on AC-2 alone. +- **`admin:reset-password` touches no `auth` table directly**: it finds the admin by `admin_users.email` (as `disableAdmin` does, so the two scripts cannot disagree about who is an admin) and sets the password with `auth.admin.updateUserById(admin.id, { password })`, so Supabase's own password policy applies too. The stack test pins that this call ends every session the user had (observed while building: Supabase deletes them), and the runbook states it. With `admin:reset-mfa` gone, the app no longer writes to the `auth` schema anywhere. Runner up: also deleting `auth.sessions` rows to sign out every device, which the grilling session declined and which turned out to be unneeded (Supabase already does it). +- **Old `reason` keys are dropped, not mapped (AC-5)**: `messages.ts` maps keys to fixed strings and ignores unknown keys, so removing `too_many_codes`, `invalid_link`, `invalid_code`, `mfa_not_set_up`, `reset_sent`, `same_password` and `mismatch` from the admin table needs no extra handling. `mismatch` and the password rule parts move to the script's own output if the script needs them; the shared `passwordSchema` stays where it is. +- **`safeAdminPath` keeps refusing the removed paths**: its auth page list becomes `/admin/sign-in` only. `/admin/mfa`, `/admin/forgot-password` and `/admin/reset-password` then pass as ordinary admin paths and 404 after sign in (AC-4), which is harmless and keeps the rule simple. Runner up: keeping them in the deny list forever, which keeps dead names in live code. + +## Rationale + +Reasoning and options: see [rationale.md](rationale.md). + +## Feature design + +**Data model sketch**: no schema change and no migration. What this feature reads: + +| Table | Owner | Used for | +|---|---|---| +| `admin_users` | app, spec 0002 | admin authority (unchanged) and `runsTheStore` (now its only input) | +| `auth.users` | Supabase | read only lookup by lower cased email in `admin:reset-password` | +| `auth.mfa_factors`, `auth.mfa_challenges` | Supabase | no longer read by the app; existing rows deleted once by hand (runbook) | + +**State transitions** (one admin session): + +``` +no session ──password ok + active admin──▶ password session (panel open) + ├──sign out──────────▶ no session (this device) + ├──12h since sign in──▶ no session (proxy signs out) + ├──admin:reset-password──▶ no session (every device) + └──disabled_at set────▶ 404 on every request +email link session (recovery, otp, confirm) for an admin user ──▶ treated as no session (AC-2) +``` + +**`decideAdminAccess` verdicts** (pure, unit tested; checked in this order): + +| Input state | Verdict | +|---|---| +| no verified user | `sign-in` | +| session age over 12 hours | `sign-in` with reason `expired` | +| no `admin_users` row, or `disabled_at` set | `not-found` | +| `amr` has no `password` entry | `sign-in` | +| otherwise | `allow` with the admin | + +`AccessInput` loses `aal` and `allowAal1`; `AccessVerdict` loses `mfa`; `AdminSession` loses `aal` and `fromResetLink`. `requireAdminSession` and `requireAdmin` remain (or fold into one, a build detail). `redirectIfSignedIn` acts only on `allow`. `adminMetadata` loses its `allowAal1` option (its `layout` option stays). The proxy's `decideAdminGate` drops the `mfa` redirect and `aal1AdminPages`; `publicAdminPages` becomes `["/admin/sign-in"]`. + +**API surface** (what changes; everything else in spec 0004 stays): + +| Surface | Kind | Key inputs | Key outputs | Auth | Key errors | +|---|---|---|---|---|---| +| `signIn` | action | email, password, next | redirect to `safeAdminPath(next)` | public | unchanged (spec 0004) | +| `/admin/sign-in` | page | `next?`, `reason?` (`expired`, `signed_out`) | form, no reset link | public; `redirectIfSignedIn` | none | +| `requireAdmin()` | server function | none | `{ id, email, name }` | the check | `redirect()` to sign in, or `notFound()` | +| `handleSendEmailHook` | route `POST /api/auth/send-email` | signed payload | 200, sends the email or skips a store runner's recovery | Standard Webhooks signature | unchanged | +| `confirmEmail` | action | token_hash, type | redirect | public | store runner recovery → sign out, `?failed=1&type=recovery` | +| `pnpm admin:reset-password` | script | `--email`, `--env-file?`, hidden password twice | "Password changed for ." plus the sessions note | service role key + DB URL | unknown admin, weak password, mismatch, Supabase error | +| removed | | `/admin/mfa`, `enrollTotp`, `verifyTotp`, `/admin/forgot-password`, `requestPasswordReset` (admin), `/auth/confirm`, `/admin/reset-password`, `resetPassword`, `admin:reset-mfa` | | | | + +**Value sourcing**: + +| Action | Value produced / displayed | Source | +|---|---|---| +| `requireAdmin`, proxy gate | session opened by a password | `hasPasswordMethod(getClaims().claims.amr)`: an entry that is `"password"` or has `method === "password"` | +| `requireAdmin`, proxy | session start, 12 hour cap, active admin | unchanged from spec 0004 | +| `signIn` | redirect target | `next` input through `safeAdminPath()`, else `/admin` | +| sign in page | reason message | `reason` param mapped to `expired` or `signed_out`; anything else shows nothing | +| `admin:reset-password` | which admin, may reset | `admin_users` by lower cased `--email`, any `disabled_at` | +| `admin:reset-password` | new password | hidden prompt, twice, `adminPasswordSchema` | +| `admin:create` (existing user) | new password | hidden prompt, twice, `adminPasswordSchema` | +| Send Email Hook | send or skip | `type === "recovery"` and `runsTheStore(user.id)` | +| `confirmEmail` | refuse a store runner's reset | `runsTheStore(data.user.id)` after `verifyOtp` | +| `runsTheStore` | true or false | `EXISTS` on `admin_users` by id | + +**Key invariants**: +- An admin page or action runs only for a password session of an active admin, younger than 12 hours. No other session state opens the panel. +- An admin password changes only through `pnpm admin:reset-password` (service role key) or by someone who already knows it. No email, link or page can change it. +- A store runner never receives a password reset email from any flow. +- Admin authority still comes only from a live `admin_users` row, checked on every request; the proxy can only deny earlier, never grant. +- The app writes nothing to Supabase's `auth` schema and reads only `auth.users` (scripts). It no longer reads `auth.mfa_*`. +- Passwords are never logged or passed as a flag (prompt only, so they stay out of shell history). + +**Security model**: +- One factor: the password, at least 14 characters (AC-16). Brute force protection is Supabase's shared `sign_in_sign_ups` rate limit until the Vercel Firewall rule on `POST /admin/sign-in` and `POST /account/sign-in` is live (AC-14); the rule must exist before the first production admin. Supabase's Auth API is also callable directly with the public anon key, where only Supabase's own limits apply; the password length is the defense there. +- Inbox takeover: closed by AC-2, AC-8 and AC-9 together (no admin reset email, no admin link that opens the panel, no password change from a link). This rests on the production Send Email Hook being on: with it off, Supabase's default recovery email would reach the admin, and its link could set a new password through the Auth API directly (AC-2 blocks only that link's session). The deploy runbook checks it: a recovery request for an admin email must produce no email. +- Planted passwords: closed by AC-15 (`admin:create` always sets the password). +- Password theft: an attacker with the password has full panel access until the password is changed. Response: `admin:disable` (immediate 404), then `admin:reset-password`, which makes the old password useless and ends every session the admin had on every device (Supabase deletes them; the stack test pins it), then re-enable with `admin:create` (which sets a password again, AC-15) as soon as you are ready. No 12 hour wait is needed. If a future Supabase version stops ending sessions on a password change, that stack test fails, and the response falls back to waiting 12 hours after the reset before re-enabling. +- Enumeration: the sign in reply and the customer reset reply still do not reveal whether an email is an admin. +- Compliance: unchanged from spec 0004 (admin emails and names logged only as id or hash). + +**Configuration required** (no new env vars): +- `supabase/config.toml`: `[auth.mfa.totp] enroll_enabled = false`, `verify_enabled = false`. +- Production Supabase project (dashboard, at deploy): TOTP enroll and verify off, after deleting existing factors. +- `package.json`: add `admin:reset-password`, remove `admin:reset-mfa`, remove the `otpauth` dev dependency. +- `next.config.ts`: the comment that mentions TOTP codes printed to the terminal is reworded. + +**Critical test scenarios**: +- Happy path: e2e, a seeded admin signs in with email and password and lands on `/admin` with their name, then on a `next` path, verifies **AC-1** +- Failure case: unit, `decideAdminAccess` allows an admin at `aal1` with a `password` amr entry, and returns `sign-in` for amr lists with only `recovery`, only `otp`, or no entries, verifies **AC-2** +- Failure case: unit, the 12 hour boundary (exactly 12h, 12h plus 1s) and disabled or missing rows still give `sign-in` (expired) and `not-found`, verifies **AC-3** +- Failure case: e2e, `/admin/mfa`, `/admin/forgot-password`, `/admin/reset-password` redirect a signed out visitor to sign in and 404 for an admin; `/auth/confirm` answers 404, verifies **AC-4** +- Failure case: e2e plus axe, the sign in page has no reset link, ignores `reason=too_many_codes`, and has zero violations on desktop and phone, verifies **AC-5**, **AC-13** +- Failure case: unit, a signed in admin opening `/admin/sign-in` is redirected on; an email link session is not, verifies **AC-6** +- Failure case: stack test beside `admin-scripts.stack.test.ts`, `admin:reset-password` changes the password of an admin (enabled and disabled), refuses an email with no `admin_users` row (also a customer only email), rejects a 13 character password, and shows the admin's other sessions end (the old refresh token fails, no `auth.sessions` row is left), verifies **AC-7**, **AC-16** +- Failure case: stack test, `admin:create` on an existing unconfirmed user with a planted password: the planted password no longer signs in, the new one does, verifies **AC-15** +- Failure case: stack test, `signInWithPassword` gives a `password` amr entry and a `verifyOtp` recovery session gives none, and `decideAdminAccess` / the proxy gate redirect the latter to sign in (proxy unit test for the 3xx), verifies **AC-2** +- Failure case: unit plus e2e, the hook answers 200 and sends nothing for a store runner's recovery (Mailpit stays empty), still sends a customer's reset, and the customer forgot page shows the same reply, verifies **AC-8** +- Failure case: unit, `confirmEmail` with a store runner's recovery signs out locally and redirects to `?failed=1&type=recovery`, verifies **AC-9** +- Failure case: db test, `runsTheStore` is true for an enabled or disabled admin row and false for a customer, with no `auth.mfa_factors` table in the stub, verifies **AC-10** +- Auth/permission: a signed in non admin and a disabled admin still get 404 on `/admin` and on an admin action, verifies **AC-3** + +## Build plan + +Tracer Bullet, ordered so every commit typechecks on its own (the pre commit hook runs `pnpm typecheck`) and no commit ever has an admin email reset without TOTP behind it: the reset path goes first, while TOTP still guards the panel, then the guard drops TOTP, then config and records. Nothing deploys yet. + +1. **Spec** (this file): `docs(specs): ...`. satisfies all ACs as the record +2. **Terminal reset replaces the email reset** (TOTP still on): add `adminPasswordSchema` and `scripts/admin/reset-password.ts` with the `admin:reset-password` package script and its stack test; `admin:create` sets the password for an existing user, new last line; delete `app/admin/(auth)/forgot-password`, `app/admin/(auth)/reset-password`, `app/auth/confirm`, the admin `password-reset` action, `confirm-reset-link-form`, the sign in page's reset link, and their tests; drop `/auth/:path*` from the proxy matcher and the `/auth` branch in `proxy.ts`; `runsTheStore` on `admin_users` only, and drop `auth.mfa_factors` from `tests/db/auth-stub.ts`; the hook skips a store runner's recovery and logs `auth_email.skipped`; `AuthEmailKind` without `admin_reset`, `emails/admin-reset-password.tsx` and its `render-auth` case removed; `confirmEmail` signs out and redirects with `failed` for a store runner's recovery, `adminResetPath` removed; rewrite `tests/e2e/account/password-reset.spec.ts` (no email for a store runner), remove `tests/e2e/admin/password-reset.spec.ts`, update `tests/e2e/seo/seo.spec.ts` for the removed pages; reword the comment in `account/actions/password-reset.ts`. satisfies **AC-4** (reset pages, `/auth/confirm`), **AC-7**, **AC-8**, **AC-9**, **AC-10**, **AC-15**, **AC-16** +3. **Password only guard and sign in**: `hasPasswordMethod` and `decideAdminAccess` without `aal` and `mfa`, with the `password` rule (replacing `isResetSession`); the guard reads `amr` from `getClaims()`; `requireAdminSession` / `adminMetadata` / `redirectIfSignedIn` without `allowAal1` and the `mfa` branch; proxy `decideAdminGate` redirects page loads on `sign-in`, drops the `mfa` redirect, `publicAdminPages` = sign in only, `aal1AdminPages` and `mfaPath` removed; `signIn` redirects to `safeAdminPath(next)`; `safeAdminPath` auth page list = sign in only; delete `app/admin/(auth)/mfa`, `actions/mfa.ts`, `factors.ts`, `mfa-lockout.ts`, `mfa-view.ts`, `totp-setup.tsx`, `totp-code-form.tsx`, `scripts/admin/reset-mfa.ts` and `admin:reset-mfa`, the MFA log events and messages, and their tests; delete `tests/stack/mfa-challenges.stack.test.ts`, the TOTP helpers in `tests/e2e/admin/support.ts` and the `otpauth` dependency; add the amr stack test; update the unit tests for the new verdict table and the e2e sign in, denials and a11y specs. satisfies **AC-1**, **AC-2**, **AC-3**, **AC-4** (`/admin/mfa`), **AC-5**, **AC-6**, **AC-11** (code part), **AC-12**, **AC-13** +4. **Config and records**: `supabase/config.toml` TOTP off; reword the `code` redaction comments (`next.config.ts`, `src/features/dashboard/log.ts`, the logger); `docs/runbooks/admin-accounts.md` (sign in without TOTP, `admin:reset-password`, the 14 character rule, the compromised account steps with the 12 hour wait or the observed session result, delete existing factors once then turn TOTP off, the hook check, the Firewall rule on both sign in paths); spec 0004 and 0012 status lines and superseded AC notes; `README.md`; the scope (feature 5 note, feature 19 items, this feature's row). satisfies **AC-11**, **AC-14** + +## Migration plan + +**Strategy**: big bang (one branch, one merge), acceptable because nothing is deployed yet and there is one operator. +**Phases**: +1. Merge milestones 2 to 4 together. +2. Locally: `pnpm db:start` picks up the TOTP setting on the next start; delete leftover dev factors with `DELETE FROM auth.mfa_factors;` against the local database (or reset the local stack). +3. At production deploy (scope feature 19): delete existing factors in the Supabase dashboard (Authentication, Users, each admin, remove factor) or with SQL, then turn TOTP enroll and verify off, then apply the Firewall rule before creating the first admin. + +**Rollback**: revert the merge. Deleted factors cannot be restored, so after a rollback each admin enrolls again at their next sign in (spec 0004 AC-4). +**Risks**: forgetting the Firewall rule leaves sign in limited only by Supabase's shared rate limit; the runbook and the scope item mark it as required before launch. + +## Consequences + +**Positive**: +- Sign in is one step, with no authenticator app to install, lose or recover. +- A large, fragile surface goes away: the MFA page, the lockout that read Supabase's internal `auth.mfa_challenges`, the only write the app ever made to `auth.sessions`, the TOTP timing logic in e2e, and the `otpauth` dependency. +- The inbox takeover path is closed harder than before: no admin reset email exists at all. + +**Negative / tradeoffs**: +- A leaked, phished or guessed admin password is now enough to open the panel (orders, refunds, customer personal data, prices, discounts). This is the accepted risk; a strong unique password and a password manager are the mitigation the runbook asks for. +- Until the Vercel Firewall rule is live, brute force protection on sign in is only Supabase's rate limit, shared by every caller from Vercel's servers. +- An admin who forgets their password needs you at a terminal with production credentials; there is no self service recovery. +- `admin:reset-password` signs the admin out on every device, so a reset alone cuts off a stolen session. This rests on Supabase behavior the app does not control; the stack test is the alarm if it changes. A compromised account still gets `admin:disable` first (immediate, even before you pick a new password), then a reset, then `admin:create` to re-enable. +- If 2FA is wanted again later (more admins, a compliance need), it is rebuilt from history, not toggled back on. +- An admin who signs in through `/account/sign-in` gets a working `/admin` session, but no `auth.sign_in.*` event is written for it (the account flow logs `account.*` events). The admin sign in audit trail misses those sign ins; Supabase's own auth audit log still has them. + +**Neutral**: +- Many specs, tests and docs mention TOTP (specs 0005, 0010, 0011, 0012, 0015, 0016, reviews). Closed specs are history and are not rewritten; only 0004 and 0012 get status notes, and `/sync` updates the AGENTS.md files. +- `robots.ts` still disallows `/auth`, which now has no pages. + +## Follow-up + +- [ ] Scope feature 19 (Production deploy): delete existing factors, turn TOTP off in the production project, apply the Firewall rule on `POST /admin/sign-in` and `POST /account/sign-in`, and check that the Send Email Hook is on (a recovery request for an admin email produces no email), all before the first admin exists. +- [ ] `/sync`: update `src/features/account/AGENTS.md` (`runsTheStore`, the hook skip, `confirmEmail`, its line on the admin reset), the root `AGENTS.md` context line for admin auth if any mentions TOTP, and `emails/AGENTS.md` (the admin reset template is gone). +- [ ] If more admins join or a compliance need appears, decide 2FA again in a new spec (passkeys would be the first option to weigh). diff --git a/docs/specs/0019-remove-admin-mfa/rationale.md b/docs/specs/0019-remove-admin-mfa/rationale.md new file mode 100644 index 0000000..b75e26b --- /dev/null +++ b/docs/specs/0019-remove-admin-mfa/rationale.md @@ -0,0 +1,65 @@ +# 0019. Remove admin two factor sign in: rationale + +## Context + +Spec [0004](../0004-admin-sign-in/index.md) requires every admin to pass a TOTP step (a 6 digit code from an authenticator app) after their password, every sign in. The store has one operator, who also builds it. The step adds daily friction, and its supporting machinery is large: an MFA page with enroll and verify views, a per factor wrong code lockout that reads Supabase's internal `auth.mfa_challenges` table, a recovery script that writes to `auth.sessions`, TOTP timing logic in every admin e2e test, and an `otpauth` dependency. + +The TOTP step also carries a second, less visible job. Admin password resets go through email, and spec 0004 put the new password behind the TOTP step so that someone who controls only the admin's inbox cannot take over the panel. Spec [0012](../0012-customer-accounts/index.md) kept that promise when customer accounts arrived: any user who runs the store gets the admin reset email, which leads to the TOTP step. Removing TOTP without replacing that job would make an inbox takeover equal to a panel takeover. + +A third detail matters: Supabase signs a user in when it verifies an email link (`verifyOtp`). With TOTP gone, the guard no longer distinguishes how a session was opened, so any session for an admin user would open the panel unless something else checks it. + +The store is not deployed yet, so no production admin, factor or session exists; the change can land in one step. + +## Options considered + +### Option 1: Keep TOTP, make it optional per environment + +A flag such as `ADMIN_MFA_REQUIRED`, off in local development and on in production. + +**Pros**: +- Production keeps two factors; local work loses the friction. + +**Cons**: +- Every guard and test path doubles (with and without the step), and the code that is off locally is the code least exercised before production. +- Keeps all of the machinery this change wants gone. + +### Option 2: Keep TOTP, make it optional per admin + +Admins may enroll a factor; the guard requires `aal2` only for admins who have one. + +**Pros**: +- Security conscious admins keep the second factor. + +**Cons**: +- Keeps the whole MFA surface plus a new branch in the guard. +- An attacker with the password of an admin who never enrolled walks in, so the store's real security level is the weakest admin's. + +### Option 3: Remove TOTP completely, reset admin passwords from the terminal + +Delete every MFA surface, accept password sessions, and replace the email reset with a CLI script. + +**Pros**: +- The smallest system: one sign in step, no MFA code, no internal `auth` table access beyond a read only user lookup. +- Closes the inbox takeover path completely (no admin reset email exists), instead of guarding it. + +**Cons**: +- The password becomes the only factor. +- Admins lose self service password recovery; the operator must run a script with production credentials. + +### Option 4: Remove TOTP, keep the email reset as is + +Delete MFA but leave `/admin/forgot-password` working without the TOTP step. + +**Pros**: +- Admins keep self service recovery. + +**Cons**: +- An inbox takeover becomes a panel takeover, the exact failure spec 0004 designed against. + +## Rationale + +The engineer decided on full removal and explicitly accepted the password as the only factor (grilling session, 2026-10-27). Given that, Option 3 is the version of removal that loses the least. Options 1 and 2 keep the machinery the change exists to remove, and Option 2's security is only as strong as the least careful admin. Option 4 quietly reopens the inbox takeover path, which matters more once the password is the only lock: a password reset by email would make the inbox worth exactly as much as the password. + +With one operator who already runs `admin:create` and `admin:disable` from the terminal, losing self service recovery costs almost nothing, and holding the production service role key becomes a practical second factor for password changes. The `password` amr rule closes the remaining gap left by removing `aal2`: a session made by verifying any email link can never open the panel, even if a future change starts sending admin links again. + +Not ending sessions on reset was chosen in the grilling session: it avoids the app's only write to Supabase's internal `auth` schema, the 12 hour cap bounds any old session, and `admin:disable` cuts access on the very next request when an account is compromised. While building, the stack test showed that Supabase's admin password change deletes every session of the user anyway. So the script still writes nothing to `auth`, yet a reset now signs the admin out everywhere, and the 12 hour wait before re-enabling a compromised account is no longer needed. diff --git a/docs/specs/0019-remove-admin-mfa/verify.md b/docs/specs/0019-remove-admin-mfa/verify.md new file mode 100644 index 0000000..d132d5b --- /dev/null +++ b/docs/specs/0019-remove-admin-mfa/verify.md @@ -0,0 +1,39 @@ +# Verify: Remove admin 2FA · spec 0019 · updated 2026-10-27 +_Steps derived from spec 0019 acceptance criteria. `/check verify` runs these; `/test` locks the durable ones._ + +Run the app on `localhost:3000` against the local Supabase stack, with `EMAIL_TRANSPORT=mailpit`. Restart the stack once (`pnpm db:stop && pnpm db:start`) so it picks up TOTP off from `supabase/config.toml`. + +## UI / manual +- [x] `pnpm admin:create` a new admin, open `/admin/sign-in?next=%2Fadmin%2Forders`, sign in with email and password → lands on `/admin/orders`, "Signed in as " shows, no code step → AC-1 +- [x] Sign in with a wrong password, an unknown email, and a customer's valid credentials → each shows "Email or password is incorrect." and leaves no `sb-` cookie → AC-1 +- [x] Make a recovery token for an admin with `auth.admin.generateLink`, open a session from it (`verifyOtp`), then `curl -I` an admin page with that cookie → a real 3xx to `/admin/sign-in?next=...`, not a 200 → AC-2 +- [x] An admin session older than 12 hours → `/admin/sign-in?reason=expired`; a disabled admin and a signed in customer → 404 on `/admin` and on an admin action; sign out ends only this device → AC-3 +- [x] Signed out, open `/admin/mfa`, `/admin/forgot-password`, `/admin/reset-password` → each redirects to `/admin/sign-in?next=`; signed in as an admin → each answers 404; `/auth/confirm` answers 404 → AC-4 +- [x] `/admin/sign-in` shows no "Forgot password?" link and no reset hint; `?reason=too_many_codes` and `?reason=invalid_link` show no message; `?reason=expired` and `?reason=signed_out` show theirs → AC-5 +- [x] Signed in admin opens `/admin/sign-in?next=%2Fadmin%2Forders` → sent to `/admin/orders`; the recovery link session from the AC-2 step opens `/admin/sign-in` → stays on it → AC-6 +- [x] `/account/forgot-password` with an admin email → the usual neutral reply, Mailpit gets nothing, logs show `auth_email.skipped` with `reason: "runs_the_store"`; with a customer email → the reset email still arrives → AC-8 +- [x] Open `/account/confirm?token_hash=&type=recovery`, press Continue → `/account/confirm?failed=1&type=recovery`, the admin's password is unchanged, and `/account` sends you to sign in (the session was ended) → AC-9 +- [x] Signed in as a store runner (enabled or disabled admin row) on `/account/settings` → change password and delete account are refused → AC-10 +- [x] `/admin/sign-in` by keyboard alone (Tab from Email to Password to Sign in, Enter) reaches `/admin`; axe shows zero violations on desktop and phone, empty and with errors → AC-13 + +## Commands +- [x] `pnpm admin:reset-password` with no flag → "Usage: pnpm admin:reset-password --email [--env-file ]" → AC-7 +- [x] `pnpm admin:reset-password --email ` → "No admin has that email." before any prompt, nothing changes → AC-7 +- [x] `pnpm admin:reset-password --email ` with a 13 character password → "The password needs: - At least 14 characters"; with two different passwords → "The passwords do not match." → AC-7, AC-16 +- [x] `pnpm admin:reset-password --email ` (upper case, also for a disabled admin) with a valid 14+ character password twice → "Password changed for ." then "Every session they had is signed out."; the old password no longer signs in, the new one does, and an open admin tab is sent to sign in on its next request → AC-7 +- [x] `pnpm admin:create --email ` → asks for a new password (14+ characters), and the customer's old password no longer signs in; last line "They sign in with the password you set." → AC-15, AC-16 +- [x] `grep -rn "auth.mfa\|auth.reset.requested\|auth.password.changed" src` → nothing; the logger still redacts `code` → AC-12 +- [x] `grep -n "enroll_enabled\|verify_enabled" supabase/config.toml` under `[auth.mfa.totp]` → both `false`; `package.json` has no `admin:reset-mfa` and no `otpauth`; `tests/stack/mfa-challenges.stack.test.ts` is gone → AC-11 +- [x] `pnpm test:stack` → `admin-amr.stack.test.ts` passes (password sign in has a `password` amr entry, a recovery session has none) and `admin-scripts.stack.test.ts` passes → AC-2, AC-7, AC-15 +- [x] `docs/runbooks/admin-accounts.md` and scope feature 19 name the Firewall rule on `POST /admin/sign-in` and `POST /account/sign-in`, 10 per minute per IP, deny 10 minutes, required before the first production admin → AC-14 + +## Value sourcing +- [x] Session opened by a password: an `amr` entry as a plain string `"password"` and as `{ method: "password" }` both allow; `recovery`, `otp` or an empty list send to sign in (unit `access.test.ts`, plus the AC-2 step above) +- [x] Redirect target: `next` through `safeAdminPath`; `//evil.example/admin` lands on `/admin` +- [x] Reason message: only `expired` and `signed_out` map to text +- [x] Which admin may be reset: by lower cased email in `admin_users`, any `disabled_at` (the upper case and disabled cases above) +- [x] Send or skip: `type === "recovery"` and an `admin_users` row → skip; a sign up confirm for a store runner still sends +- [x] `runsTheStore`: true for an enabled or disabled admin row, false for a customer, with no `auth.mfa_factors` table in the db test stub + +## Acceptance-criteria coverage +- AC-1 sign in steps · AC-2 recovery session 3xx and stack test · AC-3 expiry, denials, sign out · AC-4 removed pages · AC-5 sign in page · AC-6 skip sign in page · AC-7 reset script commands · AC-8 forgot page with an admin email · AC-9 old recovery link · AC-10 account settings refusals · AC-11 config and deletions · AC-12 log events · AC-13 keyboard and axe · AC-14 runbook and scope Firewall item · AC-15 create on an existing user · AC-16 the 14 character rule diff --git a/emails/AGENTS.md b/emails/AGENTS.md index a91546c..7a1034b 100644 --- a/emails/AGENTS.md +++ b/emails/AGENTS.md @@ -10,7 +10,7 @@ React Email templates for the customer emails (order confirmation, shipped, refu - Sending happens after the work it reports is committed, and must be safe to retry: an email for one event goes out once. - Build the Resend idempotency key from the email type plus the order or event id (e.g. `order_confirmation:`, spec 0011 AC-2), so different emails for one order never share a key. - Resend keeps idempotency keys for only 24 hours. Retries later than that must also check a durable record of sent emails in our DB. -- Auth emails (`confirm-signup.tsx`, `reset-password.tsx`, `admin-reset-password.tsx`) are not sent by Supabase templates: Supabase calls the Send Email Hook at `POST /api/auth/send-email` (`src/features/account/send-email-hook.ts`, signed with `SEND_EMAIL_HOOK_SECRET`), which picks the template, admin reset first, and sends it through the app's transport within 4 seconds (spec 0012, AC-8). +- Auth emails (`confirm-signup.tsx`, `reset-password.tsx`) are not sent by Supabase templates: Supabase calls the Send Email Hook at `POST /api/auth/send-email` (`src/features/account/send-email-hook.ts`, signed with `SEND_EMAIL_HOOK_SECRET`), which picks the template and sends it through the app's transport within 4 seconds (spec 0012, AC-8). A store runner gets no reset email at all; their password changes only from the terminal (spec 0019, AC-8). - `EMAIL_TRANSPORT` is `log` (unit tests), `mailpit` (local dev, e2e, CI; read the mail at `MAILPIT_URL`) or `resend`, the only one allowed when `VERCEL_ENV` is production. - `RESEND_API_KEY` and `EMAIL_FROM` join the Zod schema in `src/lib/env.ts` when this area is built. diff --git a/emails/admin-reset-password.tsx b/emails/admin-reset-password.tsx deleted file mode 100644 index e73c4b7..0000000 --- a/emails/admin-reset-password.tsx +++ /dev/null @@ -1,35 +0,0 @@ -import { AuthLinkEmail } from "./components/auth-link-email"; -import type { EmailFrame } from "./components/layout"; -import { fixtureFrame } from "./fixtures"; - -// spec 0004, AC-9, sent by the Send Email Hook since spec 0012 (AC-8): the wording of the old -// Supabase template. The link always goes to /auth/confirm, where the reset waits behind the -// authenticator, whatever redirect the request asked for. - -export type AdminResetPasswordProps = { - readonly frame: EmailFrame; - readonly resetUrl: string; -}; - -export const adminResetPasswordSubject = "Reset your admin password"; - -export function AdminResetPasswordEmail(props: AdminResetPasswordProps) { - return ( - - ); -} - -AdminResetPasswordEmail.PreviewProps = { - frame: { ...fixtureFrame, greetingName: null }, - resetUrl: "http://localhost:3000/auth/confirm?token_hash=pkce_example&type=recovery", -} satisfies AdminResetPasswordProps; - -// React Email's preview server (`pnpm email:dev`) finds a template by its default export. -export default AdminResetPasswordEmail; diff --git a/emails/reset-password.tsx b/emails/reset-password.tsx index cae3f45..0cf5109 100644 --- a/emails/reset-password.tsx +++ b/emails/reset-password.tsx @@ -3,7 +3,7 @@ import type { EmailFrame } from "./components/layout"; import { fixtureFrame } from "./fixtures"; // spec 0012, AC-8 and AC-9: a customer's password reset, sent by the Send Email Hook. Users who -// run the store get admin-reset-password.tsx instead. +// run the store get no reset email at all (spec 0019, AC-8). export type ResetPasswordProps = { readonly frame: EmailFrame; diff --git a/next.config.ts b/next.config.ts index a64596c..5fac912 100644 --- a/next.config.ts +++ b/next.config.ts @@ -7,7 +7,7 @@ const nextConfig: NextConfig = { // Static catalog shell, request data behind Suspense (spec 0005, Caching model). cacheComponents: true, // Next logs every Server Function call with its arguments in development by default, which - // prints admin passwords and TOTP codes to the terminal (spec 0004, AC-13). + // prints passwords and auth codes to the terminal (spec 0004, AC-13). logging: { serverFunctions: false }, images: productImagesConfig(process.env.NEXT_PUBLIC_SUPABASE_URL), // The legal texts are bundled as strings, never read with fs at runtime (spec 0017). Scoped to diff --git a/package.json b/package.json index 80a89e9..7f84bff 100644 --- a/package.json +++ b/package.json @@ -24,7 +24,7 @@ "email:dev": "email dev --dir emails --port 3001", "admin:create": "tsx scripts/admin/create.ts", "admin:disable": "tsx scripts/admin/disable.ts", - "admin:reset-mfa": "tsx scripts/admin/reset-mfa.ts" + "admin:reset-password": "tsx scripts/admin/reset-password.ts" }, "dependencies": { "@base-ui/react": "^1.8.0", @@ -74,7 +74,6 @@ "eslint-config-next": "16.3.6", "eslint-config-prettier": "^10.1.8", "lint-staged": "^17.5.1", - "otpauth": "9.5.2", "prettier": "^3.9.9", "prettier-plugin-tailwindcss": "^0.8.1", "prisma": "7.10.0", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 59d9b0f..8c18bbd 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -144,9 +144,6 @@ importers: lint-staged: specifier: ^17.5.1 version: 17.5.1 - otpauth: - specifier: 9.5.2 - version: 9.5.2 prettier: specifier: ^3.9.9 version: 3.9.9 @@ -1058,10 +1055,6 @@ packages: resolution: {integrity: sha512-jCs9ldd7NwzpgXDIf6P3+NrHh9/sD6CQdxHyjQI+h/6rDNo88ypBxxz45UDuZHz9r3tNz7N/VInSVoVdtXEI4A==} engines: {node: ^14.21.3 || >=16} - '@noble/hashes@2.4.0': - resolution: {integrity: sha512-X5XaVWZIBCT7HHZGm5I7ZQXDwLG+bGXuSrMQAW+7Zvl87h1kmc1ZB1VSRJcpUfoUrGQp4Fkoxm5kZ+Ms+aW+eA==} - engines: {node: '>= 20.19.0'} - '@nodelib/fs.scandir@2.1.5': resolution: {integrity: sha512-vq24Bq3ym5HEQm2NKCr3yXDwjc7vTsEThRDnkp2DK9p1uqLR+DHurm/NOTo0KG7HYHU7eppKZj3MyqYuMBf62g==} engines: {node: '>= 8'} @@ -3957,9 +3950,6 @@ packages: resolution: {integrity: sha512-weP+BZ8MVNnlCm8c0Qdc1WSWq4Qn7I+9CJGm7Qali6g44e/PUzbjNqJX5NJ9ljlNMosfJvg1fKEGILklK9cwnw==} engines: {node: '>=18'} - otpauth@9.5.2: - resolution: {integrity: sha512-GQ5emWR/x1tcExT62IBT0UfO95wZzJZyxYOJOGVeQF47SYEN9vmh0vISvDZaNMuFJRG+IaWCKtfm+t9Bfoal6w==} - own-keys@1.0.2: resolution: {integrity: sha512-19YVAg7T+WTrxggPukVq7DjTv6+PJ867TmhCvBsYwmbFCsZd344rq2Ld1p0wo8f8Qrrhgp82c6FJRqdXWtSEhg==} engines: {node: '>= 0.4'} @@ -5890,8 +5880,6 @@ snapshots: '@noble/hashes@1.8.0': {} - '@noble/hashes@2.4.0': {} - '@nodelib/fs.scandir@2.1.5': dependencies: '@nodelib/fs.stat': 2.0.5 @@ -8995,10 +8983,6 @@ snapshots: string-width: 7.2.0 strip-ansi: 7.2.0 - otpauth@9.5.2: - dependencies: - '@noble/hashes': 2.4.0 - own-keys@1.0.2: dependencies: call-bound: 1.0.4 diff --git a/proxy.ts b/proxy.ts index 0f49ff8..70ce68d 100644 --- a/proxy.ts +++ b/proxy.ts @@ -8,7 +8,7 @@ import { adminAuthProxy } from "@/features/admin-auth/proxy"; // action requireCustomer() (spec 0012, AC-13). export function proxy(request: NextRequest) { const { pathname } = request.nextUrl; - if (pathname.startsWith("/admin") || pathname.startsWith("/auth")) { + if (pathname.startsWith("/admin")) { return adminAuthProxy(request); } return accountProxy(request); @@ -16,5 +16,5 @@ export function proxy(request: NextRequest) { // Catalog pages never reach Supabase and stay cached (spec 0004, AC-1; spec 0012, AC-22). export const config = { - matcher: ["/admin/:path*", "/auth/:path*", "/account/:path*", "/checkout/:path*"], + matcher: ["/admin/:path*", "/account/:path*", "/checkout/:path*"], }; diff --git a/scripts/admin/cli.test.ts b/scripts/admin/cli.test.ts new file mode 100644 index 0000000..bb3f449 --- /dev/null +++ b/scripts/admin/cli.test.ts @@ -0,0 +1,69 @@ +import { describe, expect, it, vi } from "vitest"; + +import { promptAdminPassword } from "./cli"; + +// spec 0019, AC-7 and AC-16: the password prompt shared by admin:create and admin:reset-password. +// The stack test runs admin:reset-password end to end; this pins the messages and the order of +// prompts. + +const STRONG = "Garden-gnome-4242"; + +function prompter(answers: readonly string[]) { + const queue = [...answers]; + const hidden = vi.fn(async () => { + const next = queue.shift(); + if (next === undefined) throw new Error("No input."); + return next; + }); + return { hidden, close: vi.fn() }; +} + +describe("promptAdminPassword", () => { + it("returns a password that passes the admin rule and was typed the same twice", async () => { + const prompt = prompter([STRONG, STRONG]); + + await expect(promptAdminPassword(prompt)).resolves.toBe(STRONG); + expect(prompt.hidden.mock.calls).toEqual([["Password: "], ["Password again: "]]); + }); + + // covers: AC-16 + it("refuses 13 characters by naming the 14 character rule, before asking again", async () => { + const prompt = prompter(["Garden-gnome4"]); + + await expect(promptAdminPassword(prompt)).rejects.toThrow( + "The password needs:\n- At least 14 characters", + ); + expect(prompt.hidden).toHaveBeenCalledOnce(); + }); + + it("names every missing part of a weak password, one per line", async () => { + const prompt = prompter(["short"]); + + const error = await promptAdminPassword(prompt).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toBe( + [ + "The password needs:", + "- At least 8 characters", + "- An upper case letter", + "- A digit", + "- A symbol", + "- At least 14 characters", + ].join("\n"), + ); + }); + + // covers: AC-7 + it("refuses two different passwords", async () => { + const prompt = prompter([STRONG, `${STRONG}x`]); + + await expect(promptAdminPassword(prompt)).rejects.toThrow("The passwords do not match."); + }); + + it("fails cleanly when input ends before the second password", async () => { + const prompt = prompter([STRONG]); + + await expect(promptAdminPassword(prompt)).rejects.toThrow("No input."); + }); +}); diff --git a/scripts/admin/cli.ts b/scripts/admin/cli.ts index f87b1bf..75773e7 100644 --- a/scripts/admin/cli.ts +++ b/scripts/admin/cli.ts @@ -2,6 +2,8 @@ import { createInterface } from "node:readline/promises"; import { Writable } from "node:stream"; import { parseArgs } from "node:util"; +import { adminPasswordSchema } from "@/lib/auth/password"; + // Shared by the admin:* scripts: flags, a hidden prompt, and a clean exit on failure. export function readFlags(names: readonly K[]) { @@ -45,6 +47,25 @@ export function createPrompter() { }; } +type Prompter = ReturnType; + +// Asks twice and checks the admin rule (spec 0019, AC-16), naming each missing part. +export async function promptAdminPassword(prompt: Prompter): Promise { + const password = await prompt.hidden("Password: "); + const check = adminPasswordSchema.safeParse(password); + if (!check.success) { + throw new Error( + `The password needs:\n${check.error.issues.map((i) => `- ${i.message}`).join("\n")}`, + ); + } + if ((await prompt.hidden("Password again: ")) !== password) { + throw new Error("The passwords do not match."); + } + return password; +} + +// The scripts' edge: the lib answers an expected refusal as a value, and the script throws it +// here to print it and exit 1, like any other failure. export function run(main: () => Promise) { main().catch((error: unknown) => { console.error(error instanceof Error ? error.message : error); diff --git a/scripts/admin/create.ts b/scripts/admin/create.ts index 3ab3202..613979c 100644 --- a/scripts/admin/create.ts +++ b/scripts/admin/create.ts @@ -1,6 +1,5 @@ -import { passwordSchema } from "../../src/lib/auth/password"; -import { createPrompter, readFlags, run } from "./cli"; -import { connect, createAdmin, findAuthUserId } from "./lib"; +import { createPrompter, promptAdminPassword, readFlags, run } from "./cli"; +import { connect, createAdmin } from "./lib"; // pnpm admin:create --email --name [--env-file ] run(async () => { @@ -11,25 +10,10 @@ run(async () => { const deps = connect(flags["env-file"]); const prompt = createPrompter(); try { - const existing = await findAuthUserId(deps.db, flags.email); - let password: string | null = null; - if (existing) { - console.log("A user with this email already exists; they keep their current password."); - } else { - password = await prompt.hidden("Password: "); - const check = passwordSchema.safeParse(password); - if (!check.success) { - throw new Error( - `The password needs:\n${check.error.issues.map((i) => `- ${i.message}`).join("\n")}`, - ); - } - if ((await prompt.hidden("Password again: ")) !== password) { - throw new Error("The passwords do not match."); - } - } + const password = await promptAdminPassword(prompt); const { id } = await createAdmin(deps, { email: flags.email, name: flags.name, password }); console.log(`Admin ready: ${id}`); - console.log("They set up their authenticator app at their first sign in."); + console.log("They sign in with the password you set."); } finally { prompt.close(); await deps.close(); diff --git a/scripts/admin/disable.ts b/scripts/admin/disable.ts index ac1096f..6a9e21a 100644 --- a/scripts/admin/disable.ts +++ b/scripts/admin/disable.ts @@ -1,5 +1,5 @@ import { readFlags, run } from "./cli"; -import { connect, disableAdmin } from "./lib"; +import { connect, disableAdmin, noAdminMessage } from "./lib"; // pnpm admin:disable --email [--env-file ] run(async () => { @@ -9,6 +9,7 @@ run(async () => { const deps = connect(flags["env-file"]); try { const id = await disableAdmin(deps, flags.email); + if (id === null) throw new Error(noAdminMessage); console.log(`Disabled admin ${id}. Their next request gets a 404.`); } finally { await deps.close(); diff --git a/scripts/admin/lib.test.ts b/scripts/admin/lib.test.ts new file mode 100644 index 0000000..1bec5aa --- /dev/null +++ b/scripts/admin/lib.test.ts @@ -0,0 +1,61 @@ +import { describe, expect, it, vi } from "vitest"; + +import { type AdminDeps, findAdminId, setAdminPassword } from "./lib"; + +// spec 0019, AC-7: the admin:reset-password steps with the database and Supabase faked at the +// edge. The script itself runs against the local stack in admin-scripts.stack.test.ts; this +// covers the Supabase refusal, which the stack cannot be made to produce on demand. + +const ADMIN_ID = "11111111-1111-1111-1111-111111111111"; + +function deps(admin: { id: string } | null, updateError: { message: string } | null = null) { + const findUnique = vi.fn(async () => admin); + const updateUserById = vi.fn(async () => ({ data: {}, error: updateError })); + const fake = { + db: { adminUser: { findUnique } }, + supabase: { auth: { admin: { updateUserById } } }, + } as unknown as AdminDeps; + return { fake, findUnique, updateUserById }; +} + +describe("findAdminId", () => { + it("looks the admin up by lower cased email", async () => { + const { fake, findUnique } = deps({ id: ADMIN_ID }); + + await expect(findAdminId(fake.db, " Ada@Example.COM ")).resolves.toBe(ADMIN_ID); + expect(findUnique).toHaveBeenCalledWith({ where: { email: "ada@example.com" } }); + }); + + it("answers null for an email with no admin row", async () => { + const { fake } = deps(null); + + await expect(findAdminId(fake.db, "ada@example.com")).resolves.toBeNull(); + }); +}); + +describe("setAdminPassword", () => { + it("sets the password on that id", async () => { + const { fake, updateUserById } = deps({ id: ADMIN_ID }); + + await setAdminPassword(fake, ADMIN_ID, "Garden-gnome-4242"); + expect(updateUserById).toHaveBeenCalledWith(ADMIN_ID, { password: "Garden-gnome-4242" }); + }); + + it("passes on Supabase's reason when it refuses the password", async () => { + const { fake } = deps({ id: ADMIN_ID }, { message: "Password is known to be weak" }); + + await expect(setAdminPassword(fake, ADMIN_ID, "Garden-gnome-4242")).rejects.toThrow( + "Could not set the password: Password is known to be weak", + ); + }); + + it("never puts the password in its error", async () => { + const { fake } = deps({ id: ADMIN_ID }, { message: "refused" }); + + const error = await setAdminPassword(fake, ADMIN_ID, "Garden-gnome-4242").catch( + (e: unknown) => e, + ); + + expect((error as Error).message).not.toContain("Garden-gnome-4242"); + }); +}); diff --git a/scripts/admin/lib.ts b/scripts/admin/lib.ts index 4c2f5c5..f059841 100644 --- a/scripts/admin/lib.ts +++ b/scripts/admin/lib.ts @@ -3,7 +3,7 @@ import { createClient, type SupabaseClient } from "@supabase/supabase-js"; import { config } from "dotenv"; import { z } from "zod"; -import { PrismaClient } from "../../src/generated/prisma/client"; +import { PrismaClient } from "@/generated/prisma/client"; // The admin account scripts (spec 0004): no admin UI exists, so these are the whole account // lifecycle. They never import server-only app modules; they build their own clients here. @@ -50,8 +50,10 @@ export async function findAuthUserId(db: PrismaClient, email: string): Promise, email: string): Promise { +// Said by every script that takes an admin's email (spec 0019, AC-7). +export const noAdminMessage = "No admin has that email."; + +// Null when no admin row has the email: an expected answer, which each script reports itself. +export async function findAdminId(db: PrismaClient, email: string): Promise { const admin = await db.adminUser.findUnique({ where: { email: normalizeEmail(email) } }); - if (!admin) throw new Error("No admin has that email."); - // Takes effect on their next request: requireAdmin() reads this row every time. - await db.adminUser.update({ where: { id: admin.id }, data: { disabledAt: new Date() } }); - return admin.id; + return admin?.id ?? null; } -export async function resetAdminMfa( - { db, supabase }: AdminDeps, +// Null when no admin has the email. Takes effect on their next request: requireAdmin() reads +// this row every time. +export async function disableAdmin( + { db }: Pick, email: string, -): Promise<{ readonly id: string; readonly factorsRemoved: number }> { - const admin = await db.adminUser.findUnique({ where: { email: normalizeEmail(email) } }); - if (!admin) throw new Error("No admin has that email."); - - const { data, error } = await supabase.auth.admin.mfa.listFactors({ userId: admin.id }); - if (error) throw new Error(`Could not list factors: ${error.message}`); - for (const factor of data.factors) { - const { error: deleteError } = await supabase.auth.admin.mfa.deleteFactor({ - id: factor.id, - userId: admin.id, - }); - if (deleteError) throw new Error(`Could not delete a factor: ${deleteError.message}`); - } +): Promise { + const id = await findAdminId(db, email); + if (id === null) return null; + await db.adminUser.update({ where: { id }, data: { disabledAt: new Date() } }); + return id; +} - // Deleting the sessions revokes every refresh token (they cascade), and requireAdmin()'s - // getUser() finds no session, so every device is out on its next request. The only write to - // the auth schema anywhere in the app. - await db.$executeRaw`DELETE FROM auth.sessions WHERE user_id = ${admin.id}::uuid`; - return { id: admin.id, factorsRemoved: data.factors.length }; +// The only way to change an admin password without knowing it (spec 0019, AC-7). The id comes +// from findAdminId, the same lookup as disableAdmin, so the two scripts never disagree about who +// is an admin, enabled or not; the admin API applies Supabase's own password policy and writes +// its auth audit log. +export async function setAdminPassword( + { supabase }: Pick, + id: string, + password: string, +): Promise { + const { error } = await supabase.auth.admin.updateUserById(id, { password }); + if (error) throw new Error(`Could not set the password: ${error.message}`); } diff --git a/scripts/admin/reset-mfa.ts b/scripts/admin/reset-mfa.ts deleted file mode 100644 index 87fa44e..0000000 --- a/scripts/admin/reset-mfa.ts +++ /dev/null @@ -1,20 +0,0 @@ -import { readFlags, run } from "./cli"; -import { connect, resetAdminMfa } from "./lib"; - -// pnpm admin:reset-mfa --email [--env-file ] -run(async () => { - const flags = readFlags(["email"]); - if (!flags.email) { - throw new Error("Usage: pnpm admin:reset-mfa --email [--env-file ]"); - } - const deps = connect(flags["env-file"]); - try { - const { id, factorsRemoved } = await resetAdminMfa(deps, flags.email); - console.log( - `Removed ${factorsRemoved} factor(s) and every session for admin ${id}. ` + - "They set up their authenticator again at their next sign in.", - ); - } finally { - await deps.close(); - } -}); diff --git a/scripts/admin/reset-password.ts b/scripts/admin/reset-password.ts new file mode 100644 index 0000000..8b97f0a --- /dev/null +++ b/scripts/admin/reset-password.ts @@ -0,0 +1,26 @@ +import { createPrompter, promptAdminPassword, readFlags, run } from "./cli"; +import { connect, findAdminId, noAdminMessage, setAdminPassword } from "./lib"; + +// pnpm admin:reset-password --email [--env-file ] +run(async () => { + const flags = readFlags(["email"]); + if (!flags.email) { + throw new Error("Usage: pnpm admin:reset-password --email [--env-file ]"); + } + const deps = connect(flags["env-file"]); + const prompt = createPrompter(); + try { + // Refuse an unknown email before asking for a password. + const id = await findAdminId(deps.db, flags.email); + if (id === null) throw new Error(noAdminMessage); + const password = await promptAdminPassword(prompt); + await setAdminPassword(deps, id, password); + console.log(`Password changed for ${flags.email}.`); + // Supabase's admin password change deletes every session of the user (pinned by the stack + // test), so this also cuts off anyone holding a stolen session. + console.log("Every session they had is signed out."); + } finally { + prompt.close(); + await deps.close(); + } +}); diff --git a/src/features/account/AGENTS.md b/src/features/account/AGENTS.md index d51178b..9b1134d 100644 --- a/src/features/account/AGENTS.md +++ b/src/features/account/AGENTS.md @@ -12,10 +12,10 @@ Optional customer accounts under `/account`: sign up, confirm, choose a password - `proxy.ts`, `proxy-decision.ts`: the first gate, called from the root `proxy.ts` for `/account` and `/checkout`. Refreshes the session, redirects signed out visitors to sign in, sets `Cache-Control: no-store` on account pages. - `guards.ts`: `redirectIfSignedIn` for the sign in and sign up pages. - `queries.ts`, `order-view.ts`: order history and detail; `order-sign-up.ts`: when the confirmation page may offer sign up. -- `send-email-hook.ts`: `handleSendEmailHook`, behind `POST /api/auth/send-email` (AC-8). Checks the Standard Webhooks signature, picks the template (admin reset first) and sends it within 4 seconds; the pure parts live in `src/lib/email/auth-hook.ts`. +- `send-email-hook.ts`: `handleSendEmailHook`, behind `POST /api/auth/send-email` (AC-8). Checks the Standard Webhooks signature, skips a recovery email for a user who runs the store (answering 200 so the forgot page reveals nothing), picks the template and sends it within 4 seconds; the pure parts live in `src/lib/email/auth-hook.ts`. - `sign-up-email.ts`, `throwaway-password.ts`, `settle.ts`: the sign up call made in `after()` with a password nobody knows, and its failure as a reason code. - `src/lib/customer/session.ts`: `getCustomer`, `getCustomerId`, `requireCustomer`. `setup.ts`: `accountSetup` (customer row, guest order links, cart merge). `lock.ts`: `lockCustomer`. `addresses.ts`, `address-rules.ts`: the address book. `store-admin.ts`: `runsTheStore`. `log.ts`: `account.*` events. -- `src/lib/auth/password.ts`: the one password schema for admin pages, account pages and scripts, mirroring `password_requirements` in `supabase/config.toml`. Keep the two in step. +- `src/lib/auth/password.ts`: `passwordSchema` for the account pages, mirroring `password_requirements` in `supabase/config.toml` (keep the two in step), and `adminPasswordSchema`, the same rule plus at least 14 characters, which only the admin scripts use (spec 0019, AC-16). Supabase does not enforce the 14 characters; the scripts are the only way an admin password is set. ## Conventions @@ -25,7 +25,7 @@ Optional customer accounts under `/account`: sign up, confirm, choose a password - One advisory lock per customer (`lockCustomer`) guards every write to their cart, addresses or order links. Take it before any cart or order row lock. - Sign up never sets the real password: Supabase gets a throwaway one and the customer chooses theirs after confirming. Sign up and forgot password answer the same whatever the email, and the Supabase call runs in `after()`. - Email links use the token hash pattern: the GET shows Continue, only the POST calls `verifyOtp`. `/account/confirm` sends `no-referrer` (set in `next.config.ts`). -- A user who runs the store (an `admin_users` row in any state, or a verified TOTP factor) always gets the admin reset email, keeps their password when a sign up link is confirmed, and cannot delete their account here. `auth.users` and `auth.mfa_factors` are read only, through `$queryRaw`. +- A user who runs the store (`runsTheStore`: an `admin_users` row in any state) changes their password only from the terminal (`pnpm admin:reset-password`, spec 0019): they get no reset email, a recovery link they confirm is refused and signed out, a sign up link they confirm keeps their password, and set password, change password and delete account all refuse with `runs_the_store`. `auth.users` is read only, through `$queryRaw`. - Signed in checkout uses the account email, whatever was typed. An order number that is missing, someone else's or hidden renders the same not found content. - Logs carry the customer id, order ids and numbers, counts and reason codes only, never an email, name, address, password or token. diff --git a/src/features/account/actions/auth-actions.test.ts b/src/features/account/actions/auth-actions.test.ts index 176ee4b..4e4a0e6 100644 --- a/src/features/account/actions/auth-actions.test.ts +++ b/src/features/account/actions/auth-actions.test.ts @@ -225,13 +225,32 @@ describe("confirmEmail (AC-2)", () => { expect(mocks.updateUser).not.toHaveBeenCalled(); }); - it("sends anyone who runs the store to the authenticator step instead", async () => { + // spec 0019, AC-9: an old or hand made recovery link for an admin changes no password. + it("refuses a recovery for anyone who runs the store and ends the session it opened", async () => { mocks.verifyOtp.mockResolvedValue({ data: { user: confirmedUser }, error: null }); mocks.runsTheStore.mockResolvedValue(true); const result = await outcome(() => confirmEmail(form({ token_hash: "abc", type: "recovery" }))); - expect(result).toBe("redirect:/admin/mfa?next=%2Fadmin%2Freset-password"); + expect(result).toBe("redirect:/account/confirm?failed=1&type=recovery"); + expect(mocks.signOut).toHaveBeenCalledWith({ scope: "local" }); + expect(mocks.updateUser).not.toHaveBeenCalled(); + }); + + // covers: AC-9. A failed sign out must not turn the refusal into a way through to set password. + it("still refuses a store runner's recovery when ending the session fails, and logs a code", async () => { + mocks.verifyOtp.mockResolvedValue({ data: { user: confirmedUser }, error: null }); + mocks.runsTheStore.mockResolvedValue(true); + mocks.signOut.mockResolvedValue({ error: { status: 500, message: "boom" } }); + + const result = await outcome(() => confirmEmail(form({ token_hash: "abc", type: "recovery" }))); + + expect(result).toBe("redirect:/account/confirm?failed=1&type=recovery"); + expect(mocks.updateUser).not.toHaveBeenCalled(); + expect(mocks.error).toHaveBeenCalledWith( + expect.objectContaining({ userId: confirmedUser.id }), + "account.confirm.sign_out_failed", + ); }); it("shows the expired message for a used token and never verifies another type", async () => { diff --git a/src/features/account/actions/confirm.ts b/src/features/account/actions/confirm.ts index 160818a..cd292eb 100644 --- a/src/features/account/actions/confirm.ts +++ b/src/features/account/actions/confirm.ts @@ -10,11 +10,13 @@ import { logger } from "@/lib/logger"; import { createSupabaseServerClient } from "@/lib/supabase/server"; import { accountPaths } from "../paths"; -import { confirmLinkSchema } from "../schemas"; +import { type ConfirmLinkType, confirmLinkSchema } from "../schemas"; import { throwawayPassword } from "../throwaway-password"; -// The admin reset waits behind the authenticator (spec 0004): the same place /auth/confirm sends. -const adminResetPath = "/admin/mfa?next=%2Fadmin%2Freset-password"; +// The confirm page's failure view; a known type lets it offer a new link of that kind. +function failedPath(type?: ConfirmLinkType): string { + return `${accountPaths.confirm}?failed=1${type ? `&type=${type}` : ""}`; +} // spec 0012, AC-2: the POST half of /account/confirm. Only a person pressing Continue spends the // one time token, never a mail scanner that opens the link. @@ -23,18 +25,27 @@ export async function confirmEmail(formData: FormData): Promise { tokenHash: formData.get("token_hash"), type: formData.get("type"), }); - if (!parsed.success) redirect(`${accountPaths.confirm}?failed=1`); + if (!parsed.success) redirect(failedPath()); const { tokenHash, type } = parsed.data; const supabase = await createSupabaseServerClient(); const { data, error } = await supabase.auth.verifyOtp({ type, token_hash: tokenHash }); - if (error || !data.user) redirect(`${accountPaths.confirm}?failed=1&type=${type}`); + if (error || !data.user) redirect(failedPath(type)); - // A password that guards /admin is never changed from an inbox alone (Key invariants), even with - // an admin's link pasted here: a reset goes to the authenticator step like /auth/confirm, and a - // sign up keeps the admin's password as it is. + // A password that guards /admin is never changed from an inbox alone (spec 0019, AC-9), even + // with an old or hand made link: a store runner's reset is refused, and the session verifyOtp + // just opened is ended on this device. A sign up keeps the admin's password as it is. const storeAdmin = await runsTheStore(data.user.id); - if (type === "recovery" && storeAdmin) redirect(adminResetPath); + if (type === "recovery" && storeAdmin) { + const { error: signOutError } = await supabase.auth.signOut({ scope: "local" }); + if (signOutError) { + logger.error( + { userId: data.user.id, reason: classifyAuthError(signOutError) }, + "account.confirm.sign_out_failed", + ); + } + redirect(failedPath("recovery")); + } if (type === "signup") { if (!storeAdmin) await dropPlantedPassword(supabase, data.user.id); diff --git a/src/features/account/actions/password-reset.ts b/src/features/account/actions/password-reset.ts index a5bde23..5b1e30c 100644 --- a/src/features/account/actions/password-reset.ts +++ b/src/features/account/actions/password-reset.ts @@ -13,8 +13,8 @@ import { type EmailFormError, emailFormSchema } from "../schemas"; import { settle } from "../settle"; // spec 0012, AC-9: the same reply for any email, at the same speed: Supabase runs in after(). The -// Send Email Hook picks the email (AC-8): an admin, or a user with an authenticator, gets the -// admin reset whatever redirect this asks for. +// Send Email Hook decides what is sent (AC-8): a user who runs the store gets nothing, whatever +// redirect this asks for (spec 0019, AC-8). export async function requestPasswordReset( values: unknown, ): Promise> { diff --git a/src/features/account/actions/set-password.ts b/src/features/account/actions/set-password.ts index e6146a5..6cc87e0 100644 --- a/src/features/account/actions/set-password.ts +++ b/src/features/account/actions/set-password.ts @@ -37,7 +37,7 @@ export async function setPassword( const parsed = newPasswordSchema.safeParse(values); if (!parsed.success) return { ok: false, error: { fields: fieldErrors(parsed.error) } }; - // A password that guards /admin is only ever changed behind the authenticator (spec 0004). + // A password that guards /admin changes only from the terminal (spec 0019, AC-10). if (await runsTheStore(user.id)) return { ok: false, error: { form: "runs_the_store" } }; const { data, error } = await supabase.auth.updateUser({ password: parsed.data.password }); diff --git a/src/features/account/actions/settings.ts b/src/features/account/actions/settings.ts index 896b3b4..f99cdb3 100644 --- a/src/features/account/actions/settings.ts +++ b/src/features/account/actions/settings.ts @@ -49,7 +49,7 @@ export type ChangePasswordError = { }; // spec 0012, AC-19: the current password, then a new one under the AC-3 rule. Every other session -// ends and this one stays. Anyone who runs the store changes it behind the authenticator instead. +// ends and this one stays. Anyone who runs the store changes it from the terminal instead. export async function changePassword( values: unknown, ): Promise> { diff --git a/src/features/account/messages.ts b/src/features/account/messages.ts index cf36db3..a08aeee 100644 --- a/src/features/account/messages.ts +++ b/src/features/account/messages.ts @@ -21,8 +21,7 @@ export const accountMessages = { // AC-9 resetSent: "If there is an account for that email, a reset link is on its way.", // AC-19 - runsTheStorePassword: - "This account also runs the store. Use Forgot password on the admin sign in page.", + runsTheStorePassword: "This account also runs the store, so its password cannot be changed here.", wrongCurrentPassword: "Your current password is not right.", passwordChanged: "Your password is changed.", // AC-20 diff --git a/src/features/account/send-email-hook.test.ts b/src/features/account/send-email-hook.test.ts index c0951f0..3216bc1 100644 --- a/src/features/account/send-email-hook.test.ts +++ b/src/features/account/send-email-hook.test.ts @@ -109,15 +109,19 @@ describe("handleSendEmailHook", () => { expect(email.html).toContain("/account/confirm?token_hash=pkce_token_hash&type=recovery"); }); - it("sends the admin reset to /auth/confirm for an admin, whatever the redirect says", async () => { + // spec 0019, AC-8: a 200 so Supabase reports success and the forgot page answers as usual. + it("sends nothing for the reset of someone who runs the store, answering 200", async () => { mocks.runsTheStore.mockResolvedValue(true); - await handleSendEmailHook(signedRequest(payload("recovery"))); + const response = await handleSendEmailHook(signedRequest(payload("recovery"))); - const [email] = mocks.send.mock.calls[0]!; - expect(email.subject).toBe("Reset your admin password"); - expect(email.html).toContain("/auth/confirm?token_hash=pkce_token_hash&type=recovery"); - expect(email.html).not.toContain("/account/confirm"); + expect(response.status).toBe(200); + expect(await response.json()).toEqual({}); + expect(mocks.send).not.toHaveBeenCalled(); + expect(mocks.info).toHaveBeenCalledWith( + { event: "auth_email.skipped", userId, reason: "runs_the_store" }, + "auth_email.skipped", + ); }); it("refuses a bad signature with 401 and sends nothing", async () => { diff --git a/src/features/account/send-email-hook.ts b/src/features/account/send-email-hook.ts index a0aabc7..a67d949 100644 --- a/src/features/account/send-email-hook.ts +++ b/src/features/account/send-email-hook.ts @@ -15,6 +15,7 @@ import { hookErrorBody, hookPayloadSchema, hookSecretKey, + isStoreRunnerReset, usedActionType, } from "@/lib/email/auth-hook"; import { classifySendFailure } from "@/lib/email/classify"; @@ -72,8 +73,17 @@ export async function handleSendEmailHook(request: Request): Promise { let kind: AuthEmailKind | null = null; try { - kind = authEmailKind(type, type === "recovery" ? await runsTheStore(user.id) : false); - const link = authEmailLink(env.NEXT_PUBLIC_SITE_URL, kind, emailData.token_hash); + if (isStoreRunnerReset(type, type === "recovery" ? await runsTheStore(user.id) : false)) { + // A 200 makes Supabase report success, so the customer forgot page answers the same way, + // at the same speed, and never reveals that the email runs the store. + logger.info( + { event: "auth_email.skipped", userId: user.id, reason: "runs_the_store" }, + "auth_email.skipped", + ); + return json(200, {}); + } + kind = authEmailKind(type); + const link = authEmailLink(env.NEXT_PUBLIC_SITE_URL, type, emailData.token_hash); const rendered = await renderAuthEmail( kind, { diff --git a/src/features/admin-auth/access.test.ts b/src/features/admin-auth/access.test.ts index 79a4349..e0ebf34 100644 --- a/src/features/admin-auth/access.test.ts +++ b/src/features/admin-auth/access.test.ts @@ -4,7 +4,7 @@ import { ADMIN_SESSION_MAX_AGE_SECONDS, type AccessInput, decideAdminAccess, - isResetSession, + hasPasswordMethod, isSessionExpired, sessionStartedAtSeconds, } from "./access"; @@ -16,11 +16,7 @@ const row = { id: "u1", email: "a@example.com", name: "Ada", disabledAt: null }; function input(overrides: Partial = {}): AccessInput { return { userId: "u1", - aal: "aal2", - amr: [ - { method: "password", timestamp: signedInAt }, - { method: "totp", timestamp: signedInAt + 30 }, - ], + amr: [{ method: "password", timestamp: signedInAt }], adminRow: row, nowMs: now(60), ...overrides, @@ -28,7 +24,7 @@ function input(overrides: Partial = {}): AccessInput { } describe("decideAdminAccess", () => { - it("allows an active admin at aal2", () => { + it("allows an active admin whose session a password opened", () => { expect(decideAdminAccess(input())).toEqual({ kind: "allow", admin: { id: "u1", email: "a@example.com", name: "Ada" }, @@ -66,29 +62,45 @@ describe("decideAdminAccess", () => { expect(decideAdminAccess(input({ adminRow: { ...row, id: "u2" } })).kind).toBe("not-found"); }); - it("404s a non admin even at aal1, before any hint of the MFA step", () => { - expect(decideAdminAccess(input({ aal: "aal1", adminRow: null })).kind).toBe("not-found"); + // spec 0019, AC-2: an inbox alone never opens the panel. + it.each([ + ["recovery", [{ method: "recovery", timestamp: signedInAt }]], + ["otp", [{ method: "otp", timestamp: signedInAt }]], + ["no method", [{ timestamp: signedInAt }]], + ])("sends an admin whose session came from %s to sign in", (_label, amr) => { + expect(decideAdminAccess(input({ amr }))).toEqual({ kind: "sign-in" }); }); - it("sends aal1 to the MFA step unless the caller allows aal1", () => { - expect(decideAdminAccess(input({ aal: "aal1" }))).toEqual({ kind: "mfa" }); - expect(decideAdminAccess(input({ aal: null }))).toEqual({ kind: "mfa" }); - expect(decideAdminAccess(input({ aal: "aal1", allowAal1: true })).kind).toBe("allow"); + it("404s a non admin with an email link session, before the sign in method", () => { + const amr = [{ method: "recovery", timestamp: signedInAt }]; + expect(decideAdminAccess(input({ amr, adminRow: null })).kind).toBe("not-found"); }); }); -describe("session start", () => { - it("is the earliest amr timestamp", () => { - expect(sessionStartedAtSeconds(input().amr)).toBe(signedInAt); +describe("hasPasswordMethod", () => { + it("reads both amr entry shapes", () => { + expect(hasPasswordMethod([{ method: "password", timestamp: 1 }])).toBe(true); + expect(hasPasswordMethod(["password"])).toBe(true); + expect(hasPasswordMethod([{ method: "otp", timestamp: 1 }, "password"])).toBe(true); }); - it("does not move when TOTP is verified again later", () => { - const reverified = [ + it("is false for email link methods and anything unreadable", () => { + expect(hasPasswordMethod([{ method: "recovery", timestamp: 1 }])).toBe(false); + expect(hasPasswordMethod(["otp"])).toBe(false); + expect(hasPasswordMethod([])).toBe(false); + expect(hasPasswordMethod(null)).toBe(false); + expect(hasPasswordMethod({ method: "password" })).toBe(false); + }); +}); + +describe("session start", () => { + it("is the earliest amr timestamp", () => { + const amr = [ { method: "password", timestamp: signedInAt }, - { method: "totp", timestamp: signedInAt + 40_000 }, + { method: "otp", timestamp: signedInAt + 40_000 }, ]; - expect(sessionStartedAtSeconds(reverified)).toBe(signedInAt); - expect(isSessionExpired(reverified, now(ADMIN_SESSION_MAX_AGE_SECONDS + 1))).toBe(true); + expect(sessionStartedAtSeconds(amr)).toBe(signedInAt); + expect(isSessionExpired(amr, now(ADMIN_SESSION_MAX_AGE_SECONDS + 1))).toBe(true); }); it("fails closed when amr has no timestamps", () => { @@ -98,13 +110,3 @@ describe("session start", () => { expect(decideAdminAccess(input({ amr: [] }))).toEqual({ kind: "sign-in", reason: "expired" }); }); }); - -describe("isResetSession", () => { - it("spots recovery and otp sessions in either amr format", () => { - expect(isResetSession([{ method: "recovery", timestamp: 1 }])).toBe(true); - expect(isResetSession([{ method: "otp", timestamp: 1 }])).toBe(true); - expect(isResetSession(["otp"])).toBe(true); - expect(isResetSession(input().amr)).toBe(false); - expect(isResetSession(null)).toBe(false); - }); -}); diff --git a/src/features/admin-auth/access.ts b/src/features/admin-auth/access.ts index 5eb6029..5bfba08 100644 --- a/src/features/admin-auth/access.ts +++ b/src/features/admin-auth/access.ts @@ -1,11 +1,9 @@ // Pure access rules for /admin (spec 0004). No I/O: requireAdmin() and the proxy gather the // inputs and act on the verdict, so every rule lives here and is unit tested. -// The app enforced session cap, from the first factor's sign in (AC-7). +// The app enforced session cap, from the sign in (AC-7). export const ADMIN_SESSION_MAX_AGE_SECONDS = 43_200; -export type AssuranceLevel = "aal1" | "aal2"; - export type AdminRow = { readonly id: string; readonly email: string; @@ -22,25 +20,20 @@ export type Admin = { export type AccessInput = { // Confirmed by the Auth server (getUser), null when there is no valid session. readonly userId: string | null; - readonly aal: AssuranceLevel | null; // The `amr` list from the session token. readonly amr: unknown; // The live admin_users row for userId, if any. readonly adminRow: AdminRow | null; readonly nowMs: number; - // The MFA step and sign out run below aal2. - readonly allowAal1?: boolean; }; export type AccessVerdict = | { readonly kind: "allow"; readonly admin: Admin } | { readonly kind: "sign-in"; readonly reason?: "expired" } - | { readonly kind: "mfa" } | { readonly kind: "not-found" }; -// The session start is the earliest `amr` timestamp: the first factor. Token refreshes carry -// `amr` over and a later TOTP verify only adds or bumps the `totp` entry, so neither can move -// it forward. Returns null when no timestamp is present. +// The session start is the earliest `amr` timestamp: the sign in. Token refreshes carry `amr` +// over, so they cannot move it forward. Returns null when no timestamp is present. export function sessionStartedAtSeconds(amr: unknown): number | null { if (!Array.isArray(amr)) return null; const timestamps = amr.flatMap((entry: unknown) => @@ -62,8 +55,10 @@ export function isSessionExpired(amr: unknown, nowMs: number): boolean { return nowMs / 1000 - startedAt > ADMIN_SESSION_MAX_AGE_SECONDS; } -// A session opened by a password reset or email link can verify a factor, never enroll one. -export function isResetSession(amr: unknown): boolean { +// spec 0019, AC-2: only a password opens /admin. A session made by verifying an email link +// (`recovery`, `otp`, a sign up confirm) has no `password` entry, so an inbox alone never opens +// the panel. Supabase writes an entry as a plain string or as an object with `method`. +export function hasPasswordMethod(amr: unknown): boolean { if (!Array.isArray(amr)) return false; return amr.some((entry: unknown) => { const method = @@ -72,11 +67,11 @@ export function isResetSession(amr: unknown): boolean { : typeof entry === "object" && entry !== null && "method" in entry ? entry.method : null; - return method === "recovery" || method === "otp"; + return method === "password"; }); } -// Checked in the spec's order: session, age, admin row, assurance level. +// Checked in the spec's order: session, age, admin row, sign in method. export function decideAdminAccess(input: AccessInput): AccessVerdict { if (input.userId === null) return { kind: "sign-in" }; if (isSessionExpired(input.amr, input.nowMs)) return { kind: "sign-in", reason: "expired" }; @@ -85,8 +80,6 @@ export function decideAdminAccess(input: AccessInput): AccessVerdict { if (row === null || row.id !== input.userId || row.disabledAt !== null) { return { kind: "not-found" }; } - const admin = { id: row.id, email: row.email, name: row.name }; - - if (input.aal !== "aal2" && !input.allowAal1) return { kind: "mfa" }; - return { kind: "allow", admin }; + if (!hasPasswordMethod(input.amr)) return { kind: "sign-in" }; + return { kind: "allow", admin: { id: row.id, email: row.email, name: row.name } }; } diff --git a/src/features/admin-auth/actions/mfa.test.ts b/src/features/admin-auth/actions/mfa.test.ts deleted file mode 100644 index 52b44b2..0000000 --- a/src/features/admin-auth/actions/mfa.test.ts +++ /dev/null @@ -1,362 +0,0 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; - -import { brand } from "@/lib/brand/brand"; - -import type { AdminSession } from "../require-admin"; -import { enrollTotp, verifyTotp } from "./mfa"; - -const mocks = vi.hoisted(() => { - class NavigationSignal extends Error {} - return { - NavigationSignal, - redirect: vi.fn((url: string) => { - throw new NavigationSignal(`redirect:${url}`); - }), - requireAdminSession: vi.fn(), - listFactors: vi.fn(), - unenroll: vi.fn(), - enroll: vi.fn(), - challengeAndVerify: vi.fn(), - signOut: vi.fn(), - clearAuthCookies: vi.fn(), - countRecentWrongCodes: vi.fn(), - factorBusy: false, - info: vi.fn(), - warn: vi.fn(), - }; -}); - -vi.mock("server-only", () => ({})); -vi.mock("next/headers", () => ({ - headers: async () => new Headers({ "x-forwarded-for": "203.0.113.7" }), -})); -vi.mock("next/navigation", () => ({ redirect: mocks.redirect })); -vi.mock("../require-admin", () => ({ requireAdminSession: mocks.requireAdminSession })); -// The locked count reads auth.mfa_challenges (pinned by the stack test); the threshold stays -// real. -vi.mock("../mfa-lockout", async (importOriginal) => ({ - ...(await importOriginal()), - withFactorLock: async (factorId: string, run: (wrongCodes: number) => Promise) => - mocks.factorBusy - ? { ok: false, error: "busy" } - : { ok: true, data: await run(await mocks.countRecentWrongCodes(factorId)) }, -})); -vi.mock("@/lib/db", () => ({ db: {} })); -vi.mock("@/lib/supabase/server", () => ({ - createSupabaseServerClient: async () => ({ - auth: { - signOut: mocks.signOut, - mfa: { - listFactors: mocks.listFactors, - unenroll: mocks.unenroll, - enroll: mocks.enroll, - challengeAndVerify: mocks.challengeAndVerify, - }, - }, - }), - clearAuthCookies: mocks.clearAuthCookies, -})); -vi.mock("@/lib/logger", () => ({ logger: { info: mocks.info, warn: mocks.warn } })); - -const ADMIN_ID = "admin-1"; - -function session(overrides: Partial = {}): AdminSession { - return { - admin: { id: ADMIN_ID, email: "admin@example.com", name: "Ada" }, - aal: "aal1", - fromResetLink: false, - ...overrides, - }; -} - -const factor = (id: string, status: "verified" | "unverified", createdAt: string) => ({ - id, - factor_type: "totp", - status, - created_at: createdAt, -}); - -function factorsAre(...all: ReturnType[]) { - mocks.listFactors.mockResolvedValue({ data: { all }, error: null }); -} - -const verified = factor("verified-factor", "verified", "2026-09-01T00:00:00Z"); - -async function outcome(run: () => Promise): Promise { - try { - return await run(); - } catch (error) { - if (error instanceof mocks.NavigationSignal) return error.message; - throw error; - } -} - -beforeEach(() => { - vi.clearAllMocks(); - mocks.requireAdminSession.mockResolvedValue(session()); - mocks.countRecentWrongCodes.mockResolvedValue(0); - mocks.factorBusy = false; - mocks.challengeAndVerify.mockResolvedValue({ data: {}, error: null }); - mocks.unenroll.mockResolvedValue({ error: null }); - mocks.signOut.mockResolvedValue({ error: null }); - mocks.enroll.mockResolvedValue({ - data: { totp: { qr_code: "data:image/svg+xml;utf8,", secret: "ABCDEFGHIJKLMNOP" } }, - error: null, - }); -}); - -describe("enrollTotp", () => { - it("runs behind the guard, allowing only the password step", async () => { - factorsAre(); - - await enrollTotp(); - - expect(mocks.requireAdminSession).toHaveBeenCalledWith({ allowAal1: true }); - }); - - // covers: AC-4 - it("returns the QR code and key for a new admin, issued under the store name", async () => { - factorsAre(); - - const result = await enrollTotp(); - - expect(result).toEqual({ - ok: true, - data: { qrCode: "data:image/svg+xml;utf8,", secret: "ABCDEFGHIJKLMNOP" }, - }); - expect(mocks.enroll).toHaveBeenCalledWith( - expect.objectContaining({ factorType: "totp", issuer: brand.name }), - ); - }); - - // covers: AC-4 - it("removes a leftover unverified factor before issuing a new QR code", async () => { - factorsAre(factor("stale", "unverified", "2026-09-25T09:00:00Z")); - - await enrollTotp(); - - expect(mocks.unenroll).toHaveBeenCalledWith({ factorId: "stale" }); - expect(mocks.unenroll.mock.invocationCallOrder[0]).toBeLessThan( - mocks.enroll.mock.invocationCallOrder[0] ?? 0, - ); - }); - - // covers: AC-4 (a reset link session never enrolls a factor) - it("refuses a reset link session without touching any factor", async () => { - mocks.requireAdminSession.mockResolvedValue(session({ fromResetLink: true })); - - await expect(enrollTotp()).resolves.toEqual({ ok: false, error: "mfa_not_set_up" }); - expect(mocks.listFactors).not.toHaveBeenCalled(); - expect(mocks.enroll).not.toHaveBeenCalled(); - }); - - it("sends an admin who already has a verified factor back to the verify view", async () => { - factorsAre(verified); - - await expect(outcome(enrollTotp)).resolves.toBe("redirect:/admin/mfa"); - expect(mocks.enroll).not.toHaveBeenCalled(); - }); - - it("stops and reports the failure when a leftover factor cannot be removed", async () => { - factorsAre(factor("stale", "unverified", "2026-09-25T09:00:00Z")); - mocks.unenroll.mockResolvedValue({ error: { status: 429 } }); - - await expect(enrollTotp()).resolves.toEqual({ ok: false, error: "rate_limited" }); - expect(mocks.enroll).not.toHaveBeenCalled(); - }); - - it("shows an unexpected rejection from enroll as an outage", async () => { - factorsAre(); - mocks.enroll.mockResolvedValue({ data: null, error: { status: 422 } }); - - await expect(enrollTotp()).resolves.toEqual({ ok: false, error: "unavailable" }); - }); - - it("reports a failed factor lookup", async () => { - mocks.listFactors.mockResolvedValue({ data: null, error: { status: 500 } }); - - await expect(enrollTotp()).resolves.toEqual({ ok: false, error: "unavailable" }); - }); -}); - -describe("verifyTotp", () => { - // covers: AC-2 - it("verifies the code against the admin's verified factor and goes to next", async () => { - factorsAre(verified); - - await expect(outcome(() => verifyTotp({ code: "123456" }, "/admin/orders"))).resolves.toBe( - "redirect:/admin/orders", - ); - expect(mocks.challengeAndVerify).toHaveBeenCalledWith({ - factorId: "verified-factor", - code: "123456", - }); - expect(mocks.info).toHaveBeenCalledWith( - expect.objectContaining({ event: "auth.mfa.succeeded", adminId: ADMIN_ID }), - "auth.mfa.succeeded", - ); - }); - - it("accepts a code typed with a space, as authenticator apps show it", async () => { - factorsAre(verified); - - await outcome(() => verifyTotp({ code: "123 456" }, undefined)); - - expect(mocks.challengeAndVerify).toHaveBeenCalledWith( - expect.objectContaining({ code: "123456" }), - ); - }); - - // Value sourcing: a factor id is never taken from the browser. - it("ignores a factor id sent with the code", async () => { - factorsAre(verified); - - await outcome(() => verifyTotp({ code: "123456", factorId: "attacker-factor" }, undefined)); - - expect(mocks.challengeAndVerify).toHaveBeenCalledWith({ - factorId: "verified-factor", - code: "123456", - }); - }); - - // covers: AC-12 - it("goes to /admin when next points off site", async () => { - factorsAre(verified); - - await expect( - outcome(() => verifyTotp({ code: "123456" }, "https://evil.example/admin")), - ).resolves.toBe("redirect:/admin"); - }); - - // covers: AC-4 - it("enrolls the newest pending factor with the first correct code and logs both events", async () => { - factorsAre( - factor("older", "unverified", "2026-09-25T09:00:00Z"), - factor("newer", "unverified", "2026-09-25T10:00:00Z"), - ); - - await expect(outcome(() => verifyTotp({ code: "123456" }, undefined))).resolves.toBe( - "redirect:/admin", - ); - expect(mocks.challengeAndVerify).toHaveBeenCalledWith({ factorId: "newer", code: "123456" }); - const events = mocks.info.mock.calls.map((call: unknown[]) => call[1]); - expect(events).toEqual(["auth.mfa.enrolled", "auth.mfa.succeeded"]); - }); - - it("skips the check for a session that already reached aal2", async () => { - mocks.requireAdminSession.mockResolvedValue(session({ aal: "aal2" })); - - await expect(outcome(() => verifyTotp({ code: "123456" }, "/admin/orders"))).resolves.toBe( - "redirect:/admin/orders", - ); - expect(mocks.challengeAndVerify).not.toHaveBeenCalled(); - }); - - it("returns a field error for a code that is not 6 digits", async () => { - await expect(verifyTotp({ code: "12345" }, undefined)).resolves.toEqual({ - ok: false, - error: { fields: { code: ["Enter the 6 digit code"] } }, - }); - expect(mocks.challengeAndVerify).not.toHaveBeenCalled(); - }); - - // covers: AC-4 - it("answers a wrong code with invalid_code and logs it without the code", async () => { - factorsAre(verified); - mocks.challengeAndVerify.mockResolvedValue({ - data: null, - error: { code: "mfa_verification_failed", status: 422 }, - }); - - await expect(verifyTotp({ code: "654321" }, undefined)).resolves.toEqual({ - ok: false, - error: { form: "invalid_code" }, - }); - expect(mocks.warn).toHaveBeenCalledWith( - expect.objectContaining({ event: "auth.mfa.failed", adminId: ADMIN_ID }), - "auth.mfa.failed", - ); - expect(JSON.stringify(mocks.warn.mock.calls)).not.toContain("654321"); - }); - - it("reports a rate limit on the check without counting it as a wrong code", async () => { - factorsAre(verified); - mocks.challengeAndVerify.mockResolvedValue({ data: null, error: { status: 429 } }); - - await expect(verifyTotp({ code: "123456" }, undefined)).resolves.toEqual({ - ok: false, - error: { form: "rate_limited" }, - }); - expect(mocks.warn).not.toHaveBeenCalled(); - }); - - it("refuses when the admin has no factor at all", async () => { - factorsAre(); - - await expect(verifyTotp({ code: "123456" }, undefined)).resolves.toEqual({ - ok: false, - error: { form: "mfa_not_set_up" }, - }); - expect(mocks.challengeAndVerify).not.toHaveBeenCalled(); - }); - - // covers: AC-4 (a reset link session may only verify an existing factor) - it("refuses to enroll a pending factor from a reset link session", async () => { - mocks.requireAdminSession.mockResolvedValue(session({ fromResetLink: true })); - factorsAre(factor("pending", "unverified", "2026-09-25T10:00:00Z")); - - await expect(verifyTotp({ code: "123456" }, undefined)).resolves.toEqual({ - ok: false, - error: { form: "mfa_not_set_up" }, - }); - expect(mocks.challengeAndVerify).not.toHaveBeenCalled(); - }); - - it("lets a reset link session verify a factor that already exists", async () => { - mocks.requireAdminSession.mockResolvedValue(session({ fromResetLink: true })); - factorsAre(verified); - - await expect( - outcome(() => verifyTotp({ code: "123456" }, "/admin/reset-password")), - ).resolves.toBe("redirect:/admin/reset-password"); - }); - - // covers: AC-16 - it("after 5 wrong codes, ends the session and sends the admin to sign in without checking the code", async () => { - factorsAre(verified); - mocks.countRecentWrongCodes.mockResolvedValue(5); - - await expect(outcome(() => verifyTotp({ code: "123456" }, undefined))).resolves.toBe( - "redirect:/admin/sign-in?reason=too_many_codes", - ); - expect(mocks.countRecentWrongCodes).toHaveBeenCalledWith("verified-factor"); - expect(mocks.challengeAndVerify).not.toHaveBeenCalled(); - expect(mocks.signOut).toHaveBeenCalledWith({ scope: "local" }); - expect(mocks.warn).toHaveBeenCalledWith( - expect.objectContaining({ event: "auth.mfa.locked", adminId: ADMIN_ID }), - "auth.mfa.locked", - ); - }); - - // covers: AC-16 - it("still checks the code after 4 wrong ones", async () => { - factorsAre(verified); - mocks.countRecentWrongCodes.mockResolvedValue(4); - - await outcome(() => verifyTotp({ code: "123456" }, undefined)); - - expect(mocks.challengeAndVerify).toHaveBeenCalledOnce(); - }); - - it("asks the admin to wait when a code for the same factor is still being checked", async () => { - factorsAre(verified); - mocks.factorBusy = true; - - await expect(verifyTotp({ code: "123456" }, undefined)).resolves.toEqual({ - ok: false, - error: { form: "rate_limited" }, - }); - expect(mocks.challengeAndVerify).not.toHaveBeenCalled(); - expect(mocks.signOut).not.toHaveBeenCalled(); - }); -}); diff --git a/src/features/admin-auth/actions/mfa.ts b/src/features/admin-auth/actions/mfa.ts deleted file mode 100644 index 96c4483..0000000 --- a/src/features/admin-auth/actions/mfa.ts +++ /dev/null @@ -1,119 +0,0 @@ -"use server"; - -import { headers } from "next/headers"; -import { redirect } from "next/navigation"; - -import { brand } from "@/lib/brand/brand"; -import type { ActionResult } from "@/lib/result"; -import { createSupabaseServerClient } from "@/lib/supabase/server"; - -import { classifyAuthError } from "@/lib/auth/errors"; -import { factorToVerify, loadTotpFactors } from "../factors"; -import { logAuthEvent, requestIp } from "../log"; -import { isLockedOut, withFactorLock } from "../mfa-lockout"; -import { requireAdminSession } from "../require-admin"; -import { safeAdminPath, signInPath } from "../safe-admin-path"; -import { totpCodeSchema } from "../schemas"; -import { fieldErrors } from "@/lib/form-errors"; -import { endLocalSession } from "@/lib/supabase/session"; - -export type EnrollTotpData = { - // An SVG data URL from Supabase, shown in an . - readonly qrCode: string; - readonly secret: string; -}; - -export type EnrollTotpError = "mfa_not_set_up" | "rate_limited" | "unavailable"; - -// Started only by the "Set up authenticator" button, never on page render, so a reload, -// prefetch or second tab never replaces a QR code already scanned (AC-4). -export async function enrollTotp(): Promise> { - const session = await requireAdminSession({ allowAal1: true }); - if (session.fromResetLink) return { ok: false, error: "mfa_not_set_up" }; - - const supabase = await createSupabaseServerClient(); - const loaded = await loadTotpFactors(supabase); - if (!loaded.ok) return { ok: false, error: failureMessage(loaded.failure) }; - // Already enrolled: the page shows the verify view instead. - if (loaded.factors.verified) redirect("/admin/mfa"); - - // Leftover unverified factors go only now, when the admin asks for a new QR code. - for (const factor of loaded.factors.unverified) { - const { error } = await supabase.auth.mfa.unenroll({ factorId: factor.id }); - if (error) return { ok: false, error: failureMessage(classifyAuthError(error)) }; - } - - const { data, error } = await supabase.auth.mfa.enroll({ - factorType: "totp", - friendlyName: "Authenticator", - issuer: brand.name, - }); - if (error) return { ok: false, error: failureMessage(classifyAuthError(error)) }; - - return { ok: true, data: { qrCode: data.totp.qr_code, secret: data.totp.secret } }; -} - -export type VerifyTotpError = { - readonly form?: "invalid_code" | "mfa_not_set_up" | "rate_limited" | "unavailable"; - readonly fields?: Partial>; -}; - -export async function verifyTotp( - values: unknown, - next: unknown, -): Promise> { - const session = await requireAdminSession({ allowAal1: true }); - const target = safeAdminPath(next); - if (session.aal === "aal2") redirect(target); - - const parsed = totpCodeSchema.safeParse(values); - if (!parsed.success) return { ok: false, error: { fields: fieldErrors(parsed.error) } }; - - const ip = requestIp(await headers()); - const adminId = session.admin.id; - const supabase = await createSupabaseServerClient(); - - const loaded = await loadTotpFactors(supabase); - if (!loaded.ok) return { ok: false, error: { form: failureMessage(loaded.failure) } }; - const factorId = factorToVerify(loaded.factors); - const enrolling = loaded.factors.verified === null; - // A reset link session may only verify a factor that already exists. - if (factorId === null || (enrolling && session.fromResetLink)) { - return { ok: false, error: { form: "mfa_not_set_up" } }; - } - - const attempt = await withFactorLock(factorId, async (wrongCodes) => { - if (isLockedOut(wrongCodes)) return { locked: true } as const; - const { error } = await supabase.auth.mfa.challengeAndVerify({ - factorId, - code: parsed.data.code, - }); - return { locked: false, error } as const; - }); - // A double submit while the first code is still being checked. - if (!attempt.ok) return { ok: false, error: { form: "rate_limited" } }; - // Outside the lock: redirect() throws, and the session ends only after the lock is released. - if (attempt.data.locked) { - await endLocalSession(supabase); - logAuthEvent("auth.mfa.locked", { adminId, ip }); - redirect(signInPath({ reason: "too_many_codes" })); - } - - const { error } = attempt.data; - if (error) { - const failure = classifyAuthError(error); - if (failure !== "rejected") return { ok: false, error: { form: failureMessage(failure) } }; - logAuthEvent("auth.mfa.failed", { adminId, ip }); - return { ok: false, error: { form: "invalid_code" } }; - } - - if (enrolling) logAuthEvent("auth.mfa.enrolled", { adminId, ip }); - logAuthEvent("auth.mfa.succeeded", { adminId, ip }); - redirect(target); -} - -function failureMessage(failure: "rate_limited" | "unavailable" | "rejected") { - // A rejection outside the code check (an unexpected 4xx) is shown as an outage, never as a - // hint about the account. - return failure === "rate_limited" ? "rate_limited" : "unavailable"; -} diff --git a/src/features/admin-auth/actions/password-reset.test.ts b/src/features/admin-auth/actions/password-reset.test.ts deleted file mode 100644 index 5def021..0000000 --- a/src/features/admin-auth/actions/password-reset.test.ts +++ /dev/null @@ -1,301 +0,0 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; - -import { confirmResetLink, requestPasswordReset, resetPassword } from "./password-reset"; - -const mocks = vi.hoisted(() => { - class NavigationSignal extends Error {} - return { - NavigationSignal, - redirect: vi.fn((url: string) => { - throw new NavigationSignal(`redirect:${url}`); - }), - afterCallbacks: [] as (() => Promise)[], - requireAdminSession: vi.fn(), - findFirst: vi.fn(), - resetPasswordForEmail: vi.fn(), - verifyOtp: vi.fn(), - updateUser: vi.fn(), - signOut: vi.fn(), - info: vi.fn(), - error: vi.fn(), - }; -}); - -vi.mock("server-only", () => ({})); -vi.mock("next/headers", () => ({ - headers: async () => new Headers({ "x-forwarded-for": "203.0.113.7" }), -})); -vi.mock("next/navigation", () => ({ redirect: mocks.redirect })); -vi.mock("next/server", () => ({ - after: (callback: () => Promise) => { - mocks.afterCallbacks.push(callback); - }, -})); -vi.mock("../require-admin", () => ({ requireAdminSession: mocks.requireAdminSession })); -vi.mock("@/lib/db", () => ({ db: { adminUser: { findFirst: mocks.findFirst } } })); -vi.mock("@/lib/supabase/server", () => ({ - createSupabaseServerClient: async () => ({ - auth: { verifyOtp: mocks.verifyOtp, updateUser: mocks.updateUser, signOut: mocks.signOut }, - }), - createSupabaseStatelessClient: () => ({ - auth: { resetPasswordForEmail: mocks.resetPasswordForEmail }, - }), -})); -vi.mock("@/lib/logger", () => ({ - logger: { info: mocks.info, warn: vi.fn(), error: mocks.error }, -})); - -const ADMIN_ID = "admin-1"; -const NEW_PASSWORD = "Fresh-Start-42!"; - -async function outcome(run: () => Promise): Promise { - try { - return await run(); - } catch (error) { - if (error instanceof mocks.NavigationSignal) return error.message; - throw error; - } -} - -async function runAfterCallbacks() { - for (const callback of mocks.afterCallbacks) await callback(); -} - -function form(fields: Record) { - const data = new FormData(); - for (const [name, value] of Object.entries(fields)) data.set(name, value); - return data; -} - -beforeEach(() => { - vi.clearAllMocks(); - mocks.afterCallbacks.length = 0; - mocks.resetPasswordForEmail.mockResolvedValue({ data: {}, error: null }); - mocks.requireAdminSession.mockResolvedValue({ - admin: { id: ADMIN_ID, email: "admin@example.com", name: "Ada" }, - aal: "aal2", - fromResetLink: true, - }); - mocks.updateUser.mockResolvedValue({ data: {}, error: null }); - mocks.signOut.mockResolvedValue({ error: null }); -}); - -describe("requestPasswordReset", () => { - // covers: AC-9 - it("sends the reset email to an active admin after the reply", async () => { - mocks.findFirst.mockResolvedValue({ id: ADMIN_ID }); - - const result = await requestPasswordReset({ email: " Admin@Example.com" }); - - expect(result).toEqual({ ok: true, data: null }); - expect(mocks.resetPasswordForEmail).not.toHaveBeenCalled(); - await runAfterCallbacks(); - expect(mocks.resetPasswordForEmail).toHaveBeenCalledWith("admin@example.com"); - }); - - // covers: AC-9 (same answer whether or not the email is an admin's) - it("gives an unknown email the same reply and sends nothing", async () => { - mocks.findFirst.mockResolvedValue(null); - - const result = await requestPasswordReset({ email: "stranger@example.com" }); - await runAfterCallbacks(); - - expect(result).toEqual({ ok: true, data: null }); - expect(mocks.resetPasswordForEmail).not.toHaveBeenCalled(); - expect(mocks.info).toHaveBeenCalledWith( - expect.objectContaining({ event: "auth.reset.requested", sent: false }), - "auth.reset.requested", - ); - }); - - it("looks up only admins who are not disabled", async () => { - mocks.findFirst.mockResolvedValue(null); - - await requestPasswordReset({ email: "admin@example.com" }); - - expect(mocks.findFirst).toHaveBeenCalledWith( - expect.objectContaining({ where: { email: "admin@example.com", disabledAt: null } }), - ); - }); - - it("still answers the same when the email cannot be sent, and logs the failure", async () => { - mocks.findFirst.mockResolvedValue({ id: ADMIN_ID }); - mocks.resetPasswordForEmail.mockResolvedValue({ - data: null, - error: { code: "over_email_send_rate_limit", status: 429 }, - }); - - const result = await requestPasswordReset({ email: "admin@example.com" }); - await runAfterCallbacks(); - - expect(result).toEqual({ ok: true, data: null }); - expect(mocks.error).toHaveBeenCalledWith( - { adminId: ADMIN_ID, reason: "rate_limited" }, - "auth.reset.send_failed", - ); - expect(mocks.info).toHaveBeenCalledWith( - expect.objectContaining({ adminId: ADMIN_ID, sent: false }), - "auth.reset.requested", - ); - }); - - it("never logs the email address", async () => { - mocks.findFirst.mockResolvedValue(null); - - await requestPasswordReset({ email: "stranger@example.com" }); - await runAfterCallbacks(); - - expect(JSON.stringify(mocks.info.mock.calls)).not.toContain("stranger@example.com"); - }); - - it("returns a field error for an invalid email without a lookup", async () => { - const result = await requestPasswordReset({ email: "not-an-email" }); - - expect(result).toEqual({ - ok: false, - error: { fields: { email: ["Enter a valid email address"] } }, - }); - expect(mocks.findFirst).not.toHaveBeenCalled(); - expect(mocks.afterCallbacks).toHaveLength(0); - }); -}); - -describe("confirmResetLink", () => { - // covers: AC-10 (the inbox alone is not enough: the TOTP step comes first) - it("spends the token and sends the admin through the MFA step to the reset page", async () => { - mocks.verifyOtp.mockResolvedValue({ data: {}, error: null }); - - await expect( - outcome(() => confirmResetLink(form({ token_hash: "hash-1", type: "recovery" }))), - ).resolves.toBe("redirect:/admin/mfa?next=%2Fadmin%2Freset-password"); - expect(mocks.verifyOtp).toHaveBeenCalledWith({ type: "recovery", token_hash: "hash-1" }); - }); - - // covers: AC-9 - it("sends a used or expired link to sign in with invalid_link", async () => { - mocks.verifyOtp.mockResolvedValue({ data: null, error: { status: 403 } }); - - await expect( - outcome(() => confirmResetLink(form({ token_hash: "used", type: "recovery" }))), - ).resolves.toBe("redirect:/admin/sign-in?reason=invalid_link"); - }); - - it.each([ - ["a missing token", { type: "recovery" }], - ["an empty token", { token_hash: "", type: "recovery" }], - ["a token of another type", { token_hash: "hash-1", type: "magiclink" }], - ])("refuses %s without calling the Auth server", async (_label, fields) => { - await expect(outcome(() => confirmResetLink(form(fields)))).resolves.toBe( - "redirect:/admin/sign-in?reason=invalid_link", - ); - expect(mocks.verifyOtp).not.toHaveBeenCalled(); - }); -}); - -describe("resetPassword", () => { - const values = { password: NEW_PASSWORD, confirm: NEW_PASSWORD }; - - // covers: AC-5 (a new password needs a full aal2 session) - it("runs behind the full guard, not the aal1 one", async () => { - await outcome(() => resetPassword(values)); - - expect(mocks.requireAdminSession).toHaveBeenCalledWith(); - }); - - // covers: AC-10 - it("sets the new password, signs out other sessions and opens the panel", async () => { - await expect(outcome(() => resetPassword(values))).resolves.toBe("redirect:/admin"); - expect(mocks.updateUser).toHaveBeenCalledWith({ password: NEW_PASSWORD }); - expect(mocks.signOut).toHaveBeenCalledWith({ scope: "others" }); - expect(mocks.info).toHaveBeenCalledWith( - { event: "auth.password.changed", adminId: ADMIN_ID, ip: "203.0.113.7" }, - "auth.password.changed", - ); - }); - - // covers: AC-10 - it("still finishes the reset, and logs it, when other sessions cannot be signed out", async () => { - mocks.signOut.mockResolvedValue({ error: { status: 503 } }); - - await expect(outcome(() => resetPassword(values))).resolves.toBe("redirect:/admin"); - expect(mocks.error).toHaveBeenCalledWith( - { adminId: ADMIN_ID, reason: "unavailable" }, - "auth.password.revoke_others_failed", - ); - expect(mocks.info).toHaveBeenCalledWith( - expect.objectContaining({ event: "auth.password.changed", adminId: ADMIN_ID }), - "auth.password.changed", - ); - }); - - // covers: AC-10 - it("names each missing part of a weak password without calling the Auth server", async () => { - const result = await resetPassword({ password: "short", confirm: "short" }); - - expect(result).toEqual({ - ok: false, - error: { - fields: { - password: ["At least 8 characters", "An upper case letter", "A digit", "A symbol"], - }, - }, - }); - expect(mocks.updateUser).not.toHaveBeenCalled(); - }); - - it("flags a confirmation that does not match", async () => { - const result = await resetPassword({ password: NEW_PASSWORD, confirm: "Other-Pass-42!" }); - - expect(result).toEqual({ - ok: false, - error: { fields: { confirm: ["The passwords do not match."] } }, - }); - }); - - // covers: AC-10 - it("asks for a password not used before when Supabase says it is the same", async () => { - mocks.updateUser.mockResolvedValue({ - data: null, - error: { code: "same_password", status: 422 }, - }); - - await expect(resetPassword(values)).resolves.toEqual({ - ok: false, - error: { form: "same_password" }, - }); - expect(mocks.signOut).not.toHaveBeenCalled(); - }); - - it("logs a policy mismatch when Supabase finds a password weak that the schema allowed", async () => { - mocks.updateUser.mockResolvedValue({ - data: null, - error: { code: "weak_password", status: 422 }, - }); - - await expect(resetPassword(values)).resolves.toEqual({ - ok: false, - error: { form: "unavailable" }, - }); - expect(mocks.error).toHaveBeenCalledWith( - { adminId: ADMIN_ID }, - "auth.password.policy_mismatch", - ); - }); - - it("reports any other failure as unavailable", async () => { - mocks.updateUser.mockResolvedValue({ data: null, error: { status: 500 } }); - - await expect(resetPassword(values)).resolves.toEqual({ - ok: false, - error: { form: "unavailable" }, - }); - expect(mocks.error).not.toHaveBeenCalled(); - }); - - it("never logs the new password", async () => { - await outcome(() => resetPassword(values)); - - const logged = JSON.stringify([...mocks.info.mock.calls, ...mocks.error.mock.calls]); - expect(logged).not.toContain(NEW_PASSWORD); - }); -}); diff --git a/src/features/admin-auth/actions/password-reset.ts b/src/features/admin-auth/actions/password-reset.ts deleted file mode 100644 index 15756fd..0000000 --- a/src/features/admin-auth/actions/password-reset.ts +++ /dev/null @@ -1,119 +0,0 @@ -"use server"; - -import { headers } from "next/headers"; -import { redirect } from "next/navigation"; -import { after } from "next/server"; -import { z } from "zod"; - -import { db } from "@/lib/db"; -import { logger } from "@/lib/logger"; -import type { ActionResult } from "@/lib/result"; -import { createSupabaseServerClient, createSupabaseStatelessClient } from "@/lib/supabase/server"; - -import { classifyAuthError } from "@/lib/auth/errors"; -import { logAuthEvent, requestIp } from "../log"; -import { requireAdminSession } from "../require-admin"; -import { mfaPath, signInPath } from "../safe-admin-path"; -import { forgotPasswordSchema, resetPasswordSchema } from "../schemas"; -import { fieldErrors } from "@/lib/form-errors"; - -export type RequestResetError = { - readonly fields?: Partial>; -}; - -// Always the same answer, whether or not the email belongs to an admin (AC-9). The Supabase -// call runs in after(), so the reply takes the same path and time either way. -export async function requestPasswordReset( - values: unknown, -): Promise> { - const parsed = forgotPasswordSchema.safeParse(values); - if (!parsed.success) return { ok: false, error: { fields: fieldErrors(parsed.error) } }; - - const { email } = parsed.data; - const ip = requestIp(await headers()); - // Only active admins get the admin template; a future customer with that email gets nothing. - const admin = await db.adminUser.findFirst({ - where: { email, disabledAt: null }, - select: { id: true }, - }); - - after(async () => { - if (!admin) { - logAuthEvent("auth.reset.requested", { email, ip, sent: false }); - return; - } - const { error } = await createSupabaseStatelessClient().auth.resetPasswordForEmail(email); - if (error) { - logger.error( - { adminId: admin.id, reason: classifyAuthError(error) }, - "auth.reset.send_failed", - ); - } - logAuthEvent("auth.reset.requested", { adminId: admin.id, ip, sent: !error }); - }); - - return { ok: true, data: null }; -} - -const confirmSchema = z.object({ - token_hash: z.string().min(1), - type: z.literal("recovery"), -}); - -// The POST half of /auth/confirm: only a person pressing Continue spends the one time token, -// never a mail scanner that opens the link (spec 0004). -export async function confirmResetLink(formData: FormData): Promise { - const parsed = confirmSchema.safeParse({ - token_hash: formData.get("token_hash"), - type: formData.get("type"), - }); - if (!parsed.success) redirect(signInPath({ reason: "invalid_link" })); - - const supabase = await createSupabaseServerClient(); - const { error } = await supabase.auth.verifyOtp({ - type: "recovery", - token_hash: parsed.data.token_hash, - }); - if (error) redirect(signInPath({ reason: "invalid_link" })); - - // The new password waits behind the TOTP step: the inbox alone is not enough. - redirect(mfaPath("/admin/reset-password")); -} - -export type ResetPasswordError = { - readonly form?: "same_password" | "unavailable"; - readonly fields?: Partial>; -}; - -export async function resetPassword( - values: unknown, -): Promise> { - const { admin } = await requireAdminSession(); - - const parsed = resetPasswordSchema.safeParse(values); - if (!parsed.success) return { ok: false, error: { fields: fieldErrors(parsed.error) } }; - - const supabase = await createSupabaseServerClient(); - const { error } = await supabase.auth.updateUser({ password: parsed.data.password }); - if (error) { - if (error.code === "same_password") return { ok: false, error: { form: "same_password" } }; - // The schema mirrors the Supabase policy, so a weak password rejection means the two drifted. - if (error.code === "weak_password") { - logger.error({ adminId: admin.id }, "auth.password.policy_mismatch"); - } - return { ok: false, error: { form: "unavailable" } }; - } - - // Whoever else holds a session (perhaps the reason for the reset) is signed out everywhere. - // The password is changed either way; a failed revoke is logged because the other sessions - // may still be live. - const { error: revokeError } = await supabase.auth.signOut({ scope: "others" }); - if (revokeError) { - logger.error( - { adminId: admin.id, reason: classifyAuthError(revokeError) }, - "auth.password.revoke_others_failed", - ); - } - logAuthEvent("auth.password.changed", { adminId: admin.id, ip: requestIp(await headers()) }); - redirect("/admin"); -} diff --git a/src/features/admin-auth/actions/sign-in.test.ts b/src/features/admin-auth/actions/sign-in.test.ts index d84f214..6aee6d6 100644 --- a/src/features/admin-auth/actions/sign-in.test.ts +++ b/src/features/admin-auth/actions/sign-in.test.ts @@ -91,13 +91,13 @@ describe("signIn", () => { }); }); - // covers: AC-2 - it("sends an active admin on to the MFA step, keeping a safe next", async () => { + // covers: spec 0019 AC-1 + it("sends an active admin straight to a safe next", async () => { passwordAccepted(); mocks.findAdmin.mockResolvedValue({ id: USER_ID, disabledAt: null }); await expect(outcome(() => signIn(credentials, "/admin/orders?status=paid"))).resolves.toBe( - "redirect:/admin/mfa?next=%2Fadmin%2Forders%3Fstatus%3Dpaid", + "redirect:/admin/orders?status=paid", ); expect(mocks.info).toHaveBeenCalledWith( { event: "auth.sign_in.succeeded", adminId: USER_ID, ip: "203.0.113.7" }, @@ -111,16 +111,16 @@ describe("signIn", () => { mocks.findAdmin.mockResolvedValue({ id: USER_ID, disabledAt: null }); await expect(outcome(() => signIn(credentials, "//evil.example/admin"))).resolves.toBe( - "redirect:/admin/mfa?next=%2Fadmin", + "redirect:/admin", ); }); - it("goes to the bare MFA step when next is not a string", async () => { + it("goes to /admin when next is not a string", async () => { passwordAccepted(); mocks.findAdmin.mockResolvedValue({ id: USER_ID, disabledAt: null }); - await expect(outcome(() => signIn(credentials, { next: "/admin" }))).resolves.toBe( - "redirect:/admin/mfa", + await expect(outcome(() => signIn(credentials, { next: "/admin/orders" }))).resolves.toBe( + "redirect:/admin", ); }); diff --git a/src/features/admin-auth/actions/sign-in.ts b/src/features/admin-auth/actions/sign-in.ts index 6dc7d8d..727b32f 100644 --- a/src/features/admin-auth/actions/sign-in.ts +++ b/src/features/admin-auth/actions/sign-in.ts @@ -9,7 +9,7 @@ import { createSupabaseServerClient } from "@/lib/supabase/server"; import { classifyAuthError } from "@/lib/auth/errors"; import { logAuthEvent, requestIp } from "../log"; -import { mfaPath } from "../safe-admin-path"; +import { safeAdminPath } from "../safe-admin-path"; import { signInSchema } from "../schemas"; import { fieldErrors } from "@/lib/form-errors"; import { endLocalSession } from "@/lib/supabase/session"; @@ -52,5 +52,5 @@ export async function signIn( } logAuthEvent("auth.sign_in.succeeded", { adminId: admin.id, ip }); - redirect(mfaPath(typeof next === "string" ? next : undefined)); + redirect(safeAdminPath(next)); } diff --git a/src/features/admin-auth/components/auth-frame.tsx b/src/features/admin-auth/components/auth-frame.tsx index 156c014..cda814c 100644 --- a/src/features/admin-auth/components/auth-frame.tsx +++ b/src/features/admin-auth/components/auth-frame.tsx @@ -11,7 +11,7 @@ import { CardTitle, } from "@/components/ui/card"; -// The plain frame for the sign in, MFA, password and reset link pages: no admin shell, since +// The plain frame for the sign in page: no admin shell, since // nothing inside it is granted yet. All admin text is Inter, like AdminShell. export function AuthFrame({ children }: { readonly children: React.ReactNode }) { return ( diff --git a/src/features/admin-auth/components/confirm-reset-link-form.tsx b/src/features/admin-auth/components/confirm-reset-link-form.tsx deleted file mode 100644 index 16cf294..0000000 --- a/src/features/admin-auth/components/confirm-reset-link-form.tsx +++ /dev/null @@ -1,16 +0,0 @@ -import { Button } from "@/components/ui/button"; - -import { confirmResetLink } from "../actions/password-reset"; - -// A plain form: the token travels in hidden fields and is only spent on this POST. -export function ConfirmResetLinkForm({ tokenHash }: { readonly tokenHash: string }) { - return ( -
- - - -
- ); -} diff --git a/src/features/admin-auth/components/forgot-password-form.tsx b/src/features/admin-auth/components/forgot-password-form.tsx deleted file mode 100644 index 7f360a5..0000000 --- a/src/features/admin-auth/components/forgot-password-form.tsx +++ /dev/null @@ -1,60 +0,0 @@ -"use client"; - -import { zodResolver } from "@hookform/resolvers/zod"; -import { useState, useTransition } from "react"; -import { useForm } from "react-hook-form"; - -import { Button } from "@/components/ui/button"; -import { Field, FieldError, FieldGroup, FieldLabel } from "@/components/ui/field"; -import { Input } from "@/components/ui/input"; -import { Spinner } from "@/components/ui/spinner"; - -import { requestPasswordReset } from "../actions/password-reset"; -import { authMessages } from "../messages"; -import { forgotPasswordSchema } from "../schemas"; -import { applyFieldErrors } from "@/lib/form-errors"; -import { FormNotice } from "@/components/form-notice"; - -export function ForgotPasswordForm() { - const [sent, setSent] = useState(false); - const [pending, startTransition] = useTransition(); - const form = useForm({ - resolver: zodResolver(forgotPasswordSchema), - defaultValues: { email: "" }, - }); - const { errors } = form.formState; - - const onSubmit = form.handleSubmit((values) => { - setSent(false); - startTransition(async () => { - const result = await requestPasswordReset(values); - if (result.ok) setSent(true); - else applyFieldErrors(form.setError, result.error.fields); - }); - }); - - return ( -
- - {/* The same reply whether or not the email belongs to an admin (AC-9). */} - - - Email - - - - - -
- ); -} diff --git a/src/features/admin-auth/components/reset-password-form.tsx b/src/features/admin-auth/components/reset-password-form.tsx deleted file mode 100644 index 8926a57..0000000 --- a/src/features/admin-auth/components/reset-password-form.tsx +++ /dev/null @@ -1,89 +0,0 @@ -"use client"; - -import { zodResolver } from "@hookform/resolvers/zod"; -import { useState, useTransition } from "react"; -import { useForm, useWatch } from "react-hook-form"; - -import { Button } from "@/components/ui/button"; -import { Field, FieldDescription, FieldError, FieldGroup, FieldLabel } from "@/components/ui/field"; -import { Input } from "@/components/ui/input"; -import { Spinner } from "@/components/ui/spinner"; -import { missingPasswordRules, passwordRuleList, passwordRuleMessages } from "@/lib/auth/password"; - -import { resetPassword } from "../actions/password-reset"; -import { authMessages } from "../messages"; -import { resetPasswordSchema } from "../schemas"; -import { applyFieldErrors } from "@/lib/form-errors"; -import { FormNotice } from "@/components/form-notice"; - -export function ResetPasswordForm() { - const [formError, setFormError] = useState(null); - const [pending, startTransition] = useTransition(); - const form = useForm({ - resolver: zodResolver(resetPasswordSchema), - defaultValues: { password: "", confirm: "" }, - }); - const { errors } = form.formState; - const password = useWatch({ control: form.control, name: "password" }); - // Name every missing part, not only the first one Zod reports (AC-10). - const missing = errors.password ? missingPasswordRules(password) : []; - - const onSubmit = form.handleSubmit((values) => { - setFormError(null); - startTransition(async () => { - const result = await resetPassword(values); - if (result.ok) return; - applyFieldErrors(form.setError, result.error.fields); - if (result.error.form) { - setFormError(authMessages[result.error.form]); - form.setFocus("password"); - } - }); - }); - - return ( -
- - - - New password - - It needs {passwordRuleList}. - 0 - ? missing.map((rule) => ({ - message: `Missing: ${passwordRuleMessages[rule].toLowerCase()}`, - })) - : [errors.password] - } - /> - - - Type it again - - - - - -
- ); -} diff --git a/src/features/admin-auth/components/sign-in-form.tsx b/src/features/admin-auth/components/sign-in-form.tsx index 17ec66f..cde5c05 100644 --- a/src/features/admin-auth/components/sign-in-form.tsx +++ b/src/features/admin-auth/components/sign-in-form.tsx @@ -1,7 +1,6 @@ "use client"; import { zodResolver } from "@hookform/resolvers/zod"; -import Link from "next/link"; import { useState, useTransition } from "react"; import { useForm } from "react-hook-form"; @@ -66,15 +65,7 @@ export function SignInForm({ next, reason }: SignInFormProps) { -
- Password - - Forgot password? - -
+ Password - - - ); -} diff --git a/src/features/admin-auth/components/totp-code-form.tsx b/src/features/admin-auth/components/totp-code-form.tsx deleted file mode 100644 index bb42c8b..0000000 --- a/src/features/admin-auth/components/totp-code-form.tsx +++ /dev/null @@ -1,74 +0,0 @@ -"use client"; - -import { zodResolver } from "@hookform/resolvers/zod"; -import { useState, useTransition } from "react"; -import { useForm } from "react-hook-form"; - -import { Button } from "@/components/ui/button"; -import { Field, FieldDescription, FieldError, FieldGroup, FieldLabel } from "@/components/ui/field"; -import { Input } from "@/components/ui/input"; -import { Spinner } from "@/components/ui/spinner"; - -import { verifyTotp } from "../actions/mfa"; -import { authMessages } from "../messages"; -import { totpCodeSchema } from "../schemas"; -import { applyFieldErrors } from "@/lib/form-errors"; -import { FormNotice } from "@/components/form-notice"; - -type TotpCodeFormProps = { - readonly next: string | null; - readonly submitLabel: string; -}; - -// Used by both the verify view and the enroll view: the server finds the factor itself. -export function TotpCodeForm({ next, submitLabel }: TotpCodeFormProps) { - const [formError, setFormError] = useState(null); - const [pending, startTransition] = useTransition(); - const form = useForm({ resolver: zodResolver(totpCodeSchema), defaultValues: { code: "" } }); - const { errors } = form.formState; - - const onSubmit = form.handleSubmit((values) => { - setFormError(null); - startTransition(async () => { - const result = await verifyTotp(values, next); - if (result.ok) return; - applyFieldErrors(form.setError, result.error.fields); - if (result.error.form) { - setFormError(authMessages[result.error.form]); - form.resetField("code"); - form.setFocus("code"); - } - }); - }); - - const describedBy = ["code-help", errors.code ? "code-error" : null].filter(Boolean).join(" "); - - return ( -
- - - - 6 digit code - - - The code changes every 30 seconds. Type the one your app shows now. - - - - - -
- ); -} diff --git a/src/features/admin-auth/components/totp-setup.tsx b/src/features/admin-auth/components/totp-setup.tsx deleted file mode 100644 index 4fd17f7..0000000 --- a/src/features/admin-auth/components/totp-setup.tsx +++ /dev/null @@ -1,106 +0,0 @@ -"use client"; - -import { useState, useTransition } from "react"; - -import { Button } from "@/components/ui/button"; -import { Separator } from "@/components/ui/separator"; -import { Spinner } from "@/components/ui/spinner"; - -import { enrollTotp, type EnrollTotpData } from "../actions/mfa"; -import { authMessages } from "../messages"; -import { FormNotice } from "@/components/form-notice"; -import { TotpCodeForm } from "./totp-code-form"; - -// "ABCD EFGH ..." so the secret can be read aloud or typed by hand. -function groupSecret(secret: string): string { - return secret.match(/.{1,4}/g)?.join(" ") ?? secret; -} - -// The enroll view. The QR code and secret stay in this component's state, so a wrong code -// retry keeps the same factor and never invalidates a QR code already scanned (AC-4). -type TotpSetupProps = { - readonly next: string | null; - // A factor from an earlier QR code is still waiting for its first code. - readonly pending: boolean; -}; - -export function TotpSetup({ next, pending: hasPendingFactor }: TotpSetupProps) { - const [enrollment, setEnrollment] = useState(null); - const [error, setError] = useState(null); - const [pending, startTransition] = useTransition(); - - const start = () => { - setError(null); - startTransition(async () => { - const result = await enrollTotp(); - if (result.ok) setEnrollment(result.data); - else setError(authMessages[result.error]); - }); - }; - - if (!enrollment) { - return ( -
- -

- You need an authenticator app on your phone, such as Google Authenticator, 1Password or - Microsoft Authenticator. -

- - {hasPendingFactor ? ( - <> - -
-

Already scanned the QR code?

-

- Type the code from your app to finish. Setting up again shows a new QR code and - replaces the one you scanned. -

- -
- - ) : null} -
- ); - } - - return ( - // The setup button is gone now, so focus moves to the steps that replaced it. -
node?.focus()} tabIndex={-1} className="flex flex-col gap-6"> -
    -
  1. -

    Scan this QR code with your authenticator app.

    -
    - {/* An SVG data URL from Supabase; next/image adds nothing for it. */} - {/* eslint-disable-next-line @next/next/no-img-element */} - QR code for your authenticator app. The same key is written out below. -
    -
  2. -
  3. -

    Or type this key into the app instead:

    -

    - {groupSecret(enrollment.secret)} -

    -
  4. -
  5. -

    Type the 6 digit code the app now shows.

    -
  6. -
- -
- ); -} diff --git a/src/features/admin-auth/factors.test.ts b/src/features/admin-auth/factors.test.ts deleted file mode 100644 index 6185a96..0000000 --- a/src/features/admin-auth/factors.test.ts +++ /dev/null @@ -1,115 +0,0 @@ -import type { SupabaseClient } from "@supabase/supabase-js"; -import { describe, expect, it, vi } from "vitest"; - -import { factorToVerify, loadTotpFactors, type TotpFactors } from "./factors"; - -vi.mock("server-only", () => ({})); - -type Factor = { - readonly id: string; - readonly factor_type: string; - readonly status: string; - readonly created_at: string; -}; - -function clientListing(result: { data: { all: Factor[] } | null; error: unknown }) { - return { - auth: { mfa: { listFactors: vi.fn().mockResolvedValue(result) } }, - } as unknown as SupabaseClient; -} - -const totp = (id: string, status: string, createdAt = "2026-09-25T10:00:00Z"): Factor => ({ - id, - factor_type: "totp", - status, - created_at: createdAt, -}); - -describe("loadTotpFactors", () => { - it("splits TOTP factors into the verified one and the unverified ones", async () => { - const client = clientListing({ - data: { all: [totp("v1", "verified"), totp("u1", "unverified", "2026-09-25T11:00:00Z")] }, - error: null, - }); - - await expect(loadTotpFactors(client)).resolves.toEqual({ - ok: true, - factors: { - verified: { id: "v1" }, - unverified: [{ id: "u1", createdAt: "2026-09-25T11:00:00Z" }], - }, - }); - }); - - it("ignores factors that are not TOTP", async () => { - const client = clientListing({ - data: { - all: [ - { - id: "p1", - factor_type: "phone", - status: "verified", - created_at: "2026-09-25T10:00:00Z", - }, - ], - }, - error: null, - }); - - await expect(loadTotpFactors(client)).resolves.toEqual({ - ok: true, - factors: { verified: null, unverified: [] }, - }); - }); - - it("reports a rate limit from the Auth server as a failure value", async () => { - const client = clientListing({ data: null, error: { status: 429 } }); - - await expect(loadTotpFactors(client)).resolves.toEqual({ ok: false, failure: "rate_limited" }); - }); - - it("reports an outage as unavailable", async () => { - const client = clientListing({ data: null, error: { status: 503 } }); - - await expect(loadTotpFactors(client)).resolves.toEqual({ ok: false, failure: "unavailable" }); - }); -}); - -describe("factorToVerify", () => { - it("prefers the verified factor over any being enrolled", () => { - const factors: TotpFactors = { - verified: { id: "v1" }, - unverified: [{ id: "u1", createdAt: "2026-09-25T11:00:00Z" }], - }; - - expect(factorToVerify(factors)).toBe("v1"); - }); - - it("picks the newest unverified factor when none is verified", () => { - const factors: TotpFactors = { - verified: null, - unverified: [ - { id: "old", createdAt: "2026-09-25T09:00:00Z" }, - { id: "new", createdAt: "2026-09-25T11:00:00Z" }, - { id: "mid", createdAt: "2026-09-25T10:00:00Z" }, - ], - }; - - expect(factorToVerify(factors)).toBe("new"); - }); - - it("does not reorder the factors it was given", () => { - const unverified = [ - { id: "old", createdAt: "2026-09-25T09:00:00Z" }, - { id: "new", createdAt: "2026-09-25T11:00:00Z" }, - ]; - - factorToVerify({ verified: null, unverified }); - - expect(unverified.map((factor) => factor.id)).toEqual(["old", "new"]); - }); - - it("returns null when there is no factor at all", () => { - expect(factorToVerify({ verified: null, unverified: [] })).toBeNull(); - }); -}); diff --git a/src/features/admin-auth/factors.ts b/src/features/admin-auth/factors.ts deleted file mode 100644 index 90b59ad..0000000 --- a/src/features/admin-auth/factors.ts +++ /dev/null @@ -1,38 +0,0 @@ -import "server-only"; - -import type { SupabaseClient } from "@supabase/supabase-js"; - -import { type AuthFailure, classifyAuthError } from "@/lib/auth/errors"; - -export type TotpFactors = { - readonly verified: { readonly id: string } | null; - readonly unverified: readonly { readonly id: string; readonly createdAt: string }[]; -}; - -// The factor is always looked up on the server; a factor id from the browser is never trusted. -export async function loadTotpFactors( - supabase: SupabaseClient, -): Promise<{ ok: true; factors: TotpFactors } | { ok: false; failure: AuthFailure }> { - const { data, error } = await supabase.auth.mfa.listFactors(); - if (error) return { ok: false, failure: classifyAuthError(error) }; - - const totp = data.all.filter((factor) => factor.factor_type === "totp"); - const verified = totp.find((factor) => factor.status === "verified"); - return { - ok: true, - factors: { - verified: verified ? { id: verified.id } : null, - unverified: totp - .filter((factor) => factor.status === "unverified") - .map((factor) => ({ id: factor.id, createdAt: factor.created_at })), - }, - }; -} - -// The factor a code is checked against: the verified one, else the one being enrolled. -// enrollTotp leaves at most one unverified factor; the newest wins if that ever slips. -export function factorToVerify(factors: TotpFactors): string | null { - if (factors.verified) return factors.verified.id; - const newest = [...factors.unverified].sort((a, b) => b.createdAt.localeCompare(a.createdAt))[0]; - return newest?.id ?? null; -} diff --git a/src/features/admin-auth/log.ts b/src/features/admin-auth/log.ts index 5659552..18705c5 100644 --- a/src/features/admin-auth/log.ts +++ b/src/features/admin-auth/log.ts @@ -6,16 +6,10 @@ import { logger } from "@/lib/logger"; export type AuthEvent = | "auth.sign_in.succeeded" | "auth.sign_in.failed" - | "auth.mfa.enrolled" - | "auth.mfa.succeeded" - | "auth.mfa.failed" - | "auth.mfa.locked" | "auth.sign_out" | "auth.session.expired" | "auth.access.denied" - | "auth.proxy.lookup_failed" - | "auth.reset.requested" - | "auth.password.changed"; + | "auth.proxy.lookup_failed"; export type AuthEventFields = { // The admin or user id when known. @@ -24,7 +18,6 @@ export type AuthEventFields = { readonly email?: string | null; readonly ip: string | null; readonly reason?: string; - readonly sent?: boolean; }; // Unsalted on purpose: it only correlates repeated attempts, and emails are guessable anyway. @@ -39,19 +32,18 @@ export function requestIp(headers: Pick): string | null { } export function authEventRecord(event: AuthEvent, fields: AuthEventFields) { - const { adminId, email, ip, reason, sent } = fields; + const { adminId, email, ip, reason } = fields; return { event, ...(adminId ? { adminId } : email ? { emailHash: hashEmail(email) } : {}), ip, ...(reason === undefined ? {} : { reason }), - ...(sent === undefined ? {} : { sent }), }; } export function logAuthEvent(event: AuthEvent, fields: AuthEventFields) { const record = authEventRecord(event, fields); - const failed = event.endsWith("failed") || event.endsWith(".locked") || event.endsWith(".denied"); + const failed = event.endsWith("failed") || event.endsWith(".denied"); if (failed) logger.warn(record, event); else logger.info(record, event); } diff --git a/src/features/admin-auth/messages.test.ts b/src/features/admin-auth/messages.test.ts index 034d673..8cd393e 100644 --- a/src/features/admin-auth/messages.test.ts +++ b/src/features/admin-auth/messages.test.ts @@ -5,16 +5,15 @@ import { signInReason } from "./messages"; // The sign in page maps its `reason` query param through this, so nothing from the URL is ever // echoed onto the page (spec 0004, value sourcing). describe("signInReason", () => { - it.each(["expired", "signed_out", "invalid_link", "too_many_codes"] as const)( - "accepts the fixed reason %s", - (reason) => { - expect(signInReason(reason)).toBe(reason); - }, - ); + it.each(["expired", "signed_out"] as const)("accepts the fixed reason %s", (reason) => { + expect(signInReason(reason)).toBe(reason); + }); it.each([ ["markup", ""], ["another message key", "invalid_credentials"], + ["an old reset link bookmark (spec 0019, AC-5)", "invalid_link"], + ["an old wrong code bookmark (spec 0019, AC-5)", "too_many_codes"], ["a different case", "EXPIRED"], ["an empty string", ""], ["an array from a repeated param", ["expired"]], diff --git a/src/features/admin-auth/messages.ts b/src/features/admin-auth/messages.ts index 136877a..0659b26 100644 --- a/src/features/admin-auth/messages.ts +++ b/src/features/admin-auth/messages.ts @@ -4,15 +4,8 @@ export const authMessages = { invalid_credentials: "Email or password is incorrect.", rate_limited: "Too many attempts. Wait a minute and try again.", unavailable: "Sign in is unavailable right now. Try again shortly.", - invalid_code: "That code is incorrect. Try the current code from your app.", - too_many_codes: "Too many incorrect codes. Please sign in again.", - mfa_not_set_up: "Your authenticator is not set up. Contact the store owner.", expired: "Your session has expired. Please sign in again.", signed_out: "You have signed out.", - invalid_link: "That link is invalid or has expired. Request a new one.", - reset_sent: "If that email belongs to an admin, a reset link is on its way.", - same_password: "Choose a password you have not used here before.", - mismatch: "The passwords do not match.", } as const; export type AuthMessageKey = keyof typeof authMessages; @@ -21,11 +14,10 @@ export const fieldMessages = { emailRequired: "Enter your email address", emailInvalid: "Enter a valid email address", passwordRequired: "Enter your password", - codeRequired: "Enter the 6 digit code", } as const; // The reasons the sign in page shows from its `reason` query param. -const signInReasons = ["expired", "signed_out", "invalid_link", "too_many_codes"] as const; +const signInReasons = ["expired", "signed_out"] as const; export type SignInReason = (typeof signInReasons)[number]; diff --git a/src/features/admin-auth/mfa-lockout.test.ts b/src/features/admin-auth/mfa-lockout.test.ts deleted file mode 100644 index c207d0e..0000000 --- a/src/features/admin-auth/mfa-lockout.test.ts +++ /dev/null @@ -1,103 +0,0 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; - -import { countRecentWrongCodes, isLockedOut, MAX_WRONG_CODES, withFactorLock } from "./mfa-lockout"; - -const { queryRaw, transaction } = vi.hoisted(() => ({ - queryRaw: vi.fn(), - transaction: vi.fn(), -})); - -vi.mock("server-only", () => ({})); -vi.mock("@/lib/db", () => ({ db: { $queryRaw: queryRaw, $transaction: transaction } })); - -// covers: AC-16 (5 wrong codes in 15 minutes lock the MFA step). The SQL itself is pinned -// against the local stack in tests/stack/mfa-challenges.stack.test.ts. -describe("isLockedOut", () => { - it("allows a code while fewer than 5 wrong codes are counted", () => { - expect(isLockedOut(0)).toBe(false); - expect(isLockedOut(MAX_WRONG_CODES - 1)).toBe(false); - }); - - it("locks out from the 5th wrong code on", () => { - expect(MAX_WRONG_CODES).toBe(5); - expect(isLockedOut(5)).toBe(true); - expect(isLockedOut(9)).toBe(true); - }); -}); - -describe("countRecentWrongCodes", () => { - beforeEach(() => { - queryRaw.mockReset(); - }); - - it("returns the count from the query", async () => { - queryRaw.mockResolvedValue([{ count: 3 }]); - - await expect(countRecentWrongCodes("factor-1")).resolves.toBe(3); - }); - - it("returns 0 when the query yields no row", async () => { - queryRaw.mockResolvedValue([]); - - await expect(countRecentWrongCodes("factor-1")).resolves.toBe(0); - }); - - it("passes the factor id as a bound parameter, never spliced into the SQL", async () => { - queryRaw.mockResolvedValue([{ count: 0 }]); - - await countRecentWrongCodes("factor-1'; DROP TABLE x; --"); - - const [strings, ...values] = queryRaw.mock.calls[0] as [TemplateStringsArray, ...unknown[]]; - expect(strings.join("?")).not.toContain("DROP TABLE"); - expect(values).toContain("factor-1'; DROP TABLE x; --"); - }); -}); - -describe("withFactorLock", () => { - const tx = { $queryRaw: queryRaw }; - - beforeEach(() => { - queryRaw.mockReset(); - transaction.mockReset(); - transaction.mockImplementation((fn: (client: typeof tx) => Promise) => fn(tx)); - }); - - it("tries the factor's lock, then counts and runs inside the same transaction", async () => { - queryRaw.mockResolvedValueOnce([{ acquired: true }]).mockResolvedValueOnce([{ count: 2 }]); - - const result = await withFactorLock("factor-1", async (wrongCodes) => wrongCodes); - - expect(result).toEqual({ ok: true, data: 2 }); - const [strings, ...values] = queryRaw.mock.calls[0] as [TemplateStringsArray, ...unknown[]]; - expect(strings.join("?")).toContain("pg_try_advisory_xact_lock"); - expect(values).toEqual(["factor-1"]); - }); - - it("refuses as busy without counting or running when another attempt holds the lock", async () => { - queryRaw.mockResolvedValueOnce([{ acquired: false }]); - const run = vi.fn(); - - await expect(withFactorLock("factor-1", run)).resolves.toEqual({ ok: false, error: "busy" }); - expect(queryRaw).toHaveBeenCalledOnce(); - expect(run).not.toHaveBeenCalled(); - }); - - it("keeps the check's outcome when the commit fails after a slow Auth call", async () => { - queryRaw.mockResolvedValueOnce([{ acquired: true }]).mockResolvedValueOnce([{ count: 0 }]); - transaction.mockImplementation(async (fn: (client: typeof tx) => Promise) => { - await fn(tx); - throw Object.assign(new Error("Transaction already closed"), { code: "P2028" }); - }); - - await expect(withFactorLock("factor-1", async () => "verified")).resolves.toEqual({ - ok: true, - data: "verified", - }); - }); - - it("rethrows when the transaction fails before the check runs", async () => { - transaction.mockRejectedValue(new Error("pool timeout")); - - await expect(withFactorLock("factor-1", vi.fn())).rejects.toThrow("pool timeout"); - }); -}); diff --git a/src/features/admin-auth/mfa-lockout.ts b/src/features/admin-auth/mfa-lockout.ts deleted file mode 100644 index 52e4c83..0000000 --- a/src/features/admin-auth/mfa-lockout.ts +++ /dev/null @@ -1,66 +0,0 @@ -import "server-only"; - -import { db } from "@/lib/db"; -import type { ActionResult } from "@/lib/result"; - -// AC-16: 5 wrong codes for one factor in 15 minutes locks the MFA step for that factor. -export const MAX_WRONG_CODES = 5; -export const WRONG_CODE_WINDOW_MINUTES = 15; - -type RawQuery = Pick; - -// Each challengeAndVerify call leaves one auth.mfa_challenges row, verified or not, so this -// counts per account across every session and IP. A counter cookie would be deleted by an -// attacker. Read only: this reads a Supabase internal table, which -// tests/stack/mfa-challenges.stack.test.ts pins against the local stack. -async function countWith(client: RawQuery, factorId: string): Promise { - const rows = await client.$queryRaw<{ count: number }[]>` - SELECT count(*)::int AS count - FROM auth.mfa_challenges - WHERE factor_id = ${factorId}::uuid - AND verified_at IS NULL - AND created_at > now() - make_interval(mins => ${WRONG_CODE_WINDOW_MINUTES}) - `; - return rows[0]?.count ?? 0; -} - -export function countRecentWrongCodes(factorId: string): Promise { - return countWith(db, factorId); -} - -// Count and check must be one step per factor: otherwise a burst of parallel requests all -// read the same count before any of their challenge rows exist, and get past the limit. -// The advisory lock lives until the transaction ends, so run (the Supabase check, whose row -// Supabase commits before it answers) finishes before the next attempt counts. -// The lock is tried, never waited on: a waiter would hold a pooled connection and burn its -// transaction timeout in the queue, so a burst could drain the pool. A second attempt for a -// factor while one is in flight is refused as "busy" instead. -export async function withFactorLock( - factorId: string, - run: (wrongCodes: number) => Promise, -): Promise> { - let running: Promise | undefined; - try { - return await db.$transaction( - async (tx) => { - const [lock] = await tx.$queryRaw<{ acquired: boolean }[]>` - SELECT pg_try_advisory_xact_lock(hashtext(${factorId})) AS acquired`; - if (!lock?.acquired) return { ok: false, error: "busy" } as const; - running = run(await countWith(tx, factorId)); - return { ok: true, data: await running } as const; - }, - // Covers one round trip to the Auth server. - { timeout: 15_000 }, - ); - } catch (error) { - // The transaction writes nothing, it only holds the lock. When a slow Auth call outlives - // the timeout, the commit fails but the check already happened (maybe setting aal2 - // cookies), so its own outcome wins over the timeout. - if (running) return { ok: true, data: await running }; - throw error; - } -} - -export function isLockedOut(wrongCodes: number): boolean { - return wrongCodes >= MAX_WRONG_CODES; -} diff --git a/src/features/admin-auth/mfa-view.test.ts b/src/features/admin-auth/mfa-view.test.ts deleted file mode 100644 index d1e9779..0000000 --- a/src/features/admin-auth/mfa-view.test.ts +++ /dev/null @@ -1,80 +0,0 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; - -import { mfaViewFor } from "./mfa-view"; -import type { AdminSession } from "./require-admin"; - -const { listFactors } = vi.hoisted(() => ({ listFactors: vi.fn() })); - -vi.mock("server-only", () => ({})); -vi.mock("@/lib/supabase/server", () => ({ - createSupabaseServerClient: async () => ({ auth: { mfa: { listFactors } } }), -})); - -const session = (fromResetLink: boolean): AdminSession => ({ - admin: { id: "admin-1", email: "admin@example.com", name: "Ada" }, - aal: "aal1", - fromResetLink, -}); - -const factor = (id: string, status: "verified" | "unverified") => ({ - id, - factor_type: "totp", - status, - created_at: "2026-09-25T10:00:00Z", -}); - -function factorsAre(...all: ReturnType[]) { - listFactors.mockResolvedValue({ data: { all }, error: null }); -} - -beforeEach(() => { - listFactors.mockReset(); -}); - -describe("mfaViewFor", () => { - // covers: AC-2 - it("shows the verify view to an admin with a verified factor", async () => { - factorsAre(factor("v1", "verified")); - - await expect(mfaViewFor(session(false))).resolves.toBe("verify"); - }); - - it("shows the verify view to a reset link session whose admin has a factor", async () => { - factorsAre(factor("v1", "verified")); - - await expect(mfaViewFor(session(true))).resolves.toBe("verify"); - }); - - // covers: AC-4 - it("offers setup to a new admin with no factor", async () => { - factorsAre(); - - await expect(mfaViewFor(session(false))).resolves.toBe("setup"); - }); - - // covers: AC-4 (a reload after scanning keeps the code form) - it("keeps the code form for a factor enrolled but not verified yet", async () => { - factorsAre(factor("u1", "unverified")); - - await expect(mfaViewFor(session(false))).resolves.toBe("setup-pending"); - }); - - // covers: AC-4 (a reset link session never enrolls a factor) - it("tells a reset link session with no verified factor to contact the store owner", async () => { - factorsAre(factor("u1", "unverified")); - - await expect(mfaViewFor(session(true))).resolves.toBe("not-set-up"); - }); - - it("reports a rate limit from the Auth server", async () => { - listFactors.mockResolvedValue({ data: null, error: { status: 429 } }); - - await expect(mfaViewFor(session(false))).resolves.toBe("rate_limited"); - }); - - it("reports any other Auth failure as unavailable, never as setup", async () => { - listFactors.mockResolvedValue({ data: null, error: { status: 400 } }); - - await expect(mfaViewFor(session(false))).resolves.toBe("unavailable"); - }); -}); diff --git a/src/features/admin-auth/mfa-view.ts b/src/features/admin-auth/mfa-view.ts deleted file mode 100644 index 861556f..0000000 --- a/src/features/admin-auth/mfa-view.ts +++ /dev/null @@ -1,21 +0,0 @@ -import "server-only"; - -import { createSupabaseServerClient } from "@/lib/supabase/server"; - -import { loadTotpFactors } from "./factors"; -import type { AdminSession } from "./require-admin"; - -// `setup-pending`: a factor was enrolled but not verified yet (a reload after scanning the QR -// code), so the code form stays offered for it. -export type MfaView = - "verify" | "setup" | "setup-pending" | "not-set-up" | "rate_limited" | "unavailable"; - -// Which MFA step to show: verify a factor, offer setup, or tell a reset link session with no -// factor to ask the store owner (it may never enroll one, AC-4). -export async function mfaViewFor(session: AdminSession): Promise { - const loaded = await loadTotpFactors(await createSupabaseServerClient()); - if (!loaded.ok) return loaded.failure === "rate_limited" ? "rate_limited" : "unavailable"; - if (loaded.factors.verified) return "verify"; - if (session.fromResetLink) return "not-set-up"; - return loaded.factors.unverified.length > 0 ? "setup-pending" : "setup"; -} diff --git a/src/features/admin-auth/proxy-decision.test.ts b/src/features/admin-auth/proxy-decision.test.ts index 2eb4707..68d9184 100644 --- a/src/features/admin-auth/proxy-decision.test.ts +++ b/src/features/admin-auth/proxy-decision.test.ts @@ -41,12 +41,22 @@ describe("decideProxy", () => { }); }); - it("leaves the public auth pages and /auth open", () => { - for (const pathname of ["/admin/sign-in", "/admin/forgot-password", "/auth/confirm"]) { + it("leaves the sign in page and paths outside /admin open", () => { + for (const pathname of ["/admin/sign-in", "/auth/confirm"]) { expect(decideProxy(input({ pathname, claims: null }))).toEqual({ kind: "continue" }); } }); + // spec 0019, AC-4: the removed pages are ordinary admin paths now. + it("sends a signed out visitor from a removed auth page to sign in", () => { + for (const pathname of ["/admin/mfa", "/admin/forgot-password", "/admin/reset-password"]) { + expect(decideProxy(input({ pathname, search: "", claims: null }))).toEqual({ + kind: "redirect", + to: `/admin/sign-in?next=${encodeURIComponent(pathname)}`, + }); + } + }); + it("never redirects a server action request", () => { expect(decideProxy(input({ claims: null, isServerAction: true }))).toEqual({ kind: "continue", @@ -101,7 +111,7 @@ function gate(overrides: Partial = {}): AdminGateInput { search: "?page=2", method: "GET", isServerAction: false, - claims: { sub: "admin-1", aal: "aal2", amr: fresh.amr }, + claims: { sub: "admin-1", amr: fresh.amr }, nowMs: at(60), ...overrides, }; @@ -116,8 +126,8 @@ describe("needsAdminGate", () => { expect(needsAdminGate(gate({ method: "HEAD" }))).toBe(true); }); - it("skips sign in, forgot password and sign out", () => { - for (const pathname of ["/admin/sign-in", "/admin/forgot-password", "/admin/sign-out"]) { + it("skips sign in and sign out", () => { + for (const pathname of ["/admin/sign-in", "/admin/sign-out"]) { expect(needsAdminGate(gate({ pathname }))).toBe(false); } }); @@ -135,7 +145,7 @@ describe("needsAdminGate", () => { // covers: spec 0005 AC-19 describe("decideAdminGate", () => { - it("lets an active aal2 admin through", () => { + it("lets an active admin with a password session through", () => { expect(decideAdminGate({ ...gate(), adminRow })).toEqual({ kind: "continue" }); }); @@ -148,44 +158,35 @@ describe("decideAdminGate", () => { expect(decideAdminGate({ ...gate(), adminRow: disabled })).toEqual({ kind: "not-found" }); }); - it("answers a non admin with a 404 on the MFA page too", () => { + it("answers a non admin with a 404 on a removed auth page too (spec 0019, AC-4)", () => { expect(decideAdminGate({ ...gate({ pathname: "/admin/mfa" }), adminRow: null })).toEqual({ kind: "not-found", }); }); - it("sends an aal1 admin to the MFA step, keeping the path and query as next", () => { - const aal1 = gate({ claims: { sub: "admin-1", aal: "aal1", amr: fresh.amr } }); - expect(decideAdminGate({ ...aal1, adminRow })).toEqual({ - kind: "redirect", - to: "/admin/mfa?next=%2Fadmin%2Forders%3Fpage%3D2", + // spec 0019, AC-2: a real redirect, not the 200 a page's own redirect() streams. + it("sends an admin whose session no password opened to sign in, keeping next", () => { + const recovery = gate({ + claims: { sub: "admin-1", amr: [{ method: "recovery", timestamp: signedInAt }] }, }); - }); - - it("lets an aal1 admin open the MFA page itself", () => { - const aal1 = gate({ pathname: "/admin/mfa", claims: { sub: "admin-1", aal: "aal1" } }); - expect( - decideAdminGate({ ...aal1, claims: { ...aal1.claims, amr: fresh.amr }, adminRow }), - ).toEqual({ - kind: "continue", + expect(decideAdminGate({ ...recovery, adminRow })).toEqual({ + kind: "redirect", + to: "/admin/sign-in?next=%2Fadmin%2Forders%3Fpage%3D2", }); }); - it("never grants: a session without a subject goes on to the page's own guard", () => { + it("never grants: a session without a subject is sent to sign in", () => { expect(decideAdminGate({ ...gate({ claims: { amr: fresh.amr } }), adminRow: null })).toEqual({ - kind: "continue", + kind: "redirect", + to: "/admin/sign-in?next=%2Fadmin%2Forders%3Fpage%3D2", }); }); }); describe("proxy matcher", () => { // spec 0012, AC-22: the account pages and checkout join; catalog pages never call Supabase. - it("runs only on admin, auth, account and checkout routes", () => { - expect(config.matcher).toEqual([ - "/admin/:path*", - "/auth/:path*", - "/account/:path*", - "/checkout/:path*", - ]); + // spec 0019, AC-4: /auth has no pages left. + it("runs only on admin, account and checkout routes", () => { + expect(config.matcher).toEqual(["/admin/:path*", "/account/:path*", "/checkout/:path*"]); }); }); diff --git a/src/features/admin-auth/proxy-decision.ts b/src/features/admin-auth/proxy-decision.ts index cb0d29c..d5daa19 100644 --- a/src/features/admin-auth/proxy-decision.ts +++ b/src/features/admin-auth/proxy-decision.ts @@ -1,16 +1,12 @@ import { type AdminRow, decideAdminAccess, isSessionExpired } from "./access"; -import { mfaPath, signInPath, signOutPath } from "./safe-admin-path"; +import { signInPath, signOutPath } from "./safe-admin-path"; -// Carries the requested admin path to requireAdmin(), so a page or action it bounces to the MFA -// step can come back. The proxy sets it on every request, so a client supplied value never survives. +// Carries the requested admin path to requireAdmin(), so a page or action it bounces to sign in +// can come back. The proxy sets it on every request, so a client supplied value never survives. export const adminPathHeader = "x-bestore-admin-path"; // Pages a visitor with no session may open. -const publicAdminPages = ["/admin/sign-in", "/admin/forgot-password"] as const; - -// Pages that call requireAdminSession({ allowAal1: true }); the two lists must match, or the -// proxy bounces an aal1 admin away from a page that allows them. -export const aal1AdminPages = ["/admin/mfa"] as const; +const publicAdminPages = ["/admin/sign-in"] as const; export type ProxyInput = { readonly pathname: string; @@ -59,7 +55,6 @@ export type AdminGateInput = { readonly isServerAction: boolean; readonly claims: { readonly sub?: unknown; - readonly aal?: unknown; readonly amr?: unknown; }; readonly nowMs: number; @@ -88,22 +83,22 @@ export function needsAdminGate(input: Omit): export function decideAdminGate( input: AdminGateInput & { readonly adminRow: AdminRow | null }, ): AdminGateDecision { - const { sub, aal, amr } = input.claims; + const { sub, amr } = input.claims; const verdict = decideAdminAccess({ userId: typeof sub === "string" ? sub : null, - aal: aal === "aal2" ? "aal2" : "aal1", amr, adminRow: input.adminRow, nowMs: input.nowMs, - allowAal1: aal1AdminPages.some((page) => input.pathname === page), }); switch (verdict.kind) { case "not-found": return { kind: "not-found" }; - case "mfa": - return { kind: "redirect", to: mfaPath(`${input.pathname}${input.search}`) }; - case "allow": + // spec 0019, AC-2: a session no password opened (an email link) is sent to sign in here, so + // the page load gets a real redirect instead of a streamed 200. Expiry was decided earlier, + // in decideProxy. case "sign-in": + return { kind: "redirect", to: signInPath({ next: `${input.pathname}${input.search}` }) }; + case "allow": return { kind: "continue" }; } } diff --git a/src/features/admin-auth/proxy.test.ts b/src/features/admin-auth/proxy.test.ts index 6e6c99a..5e940d6 100644 --- a/src/features/admin-auth/proxy.test.ts +++ b/src/features/admin-auth/proxy.test.ts @@ -32,14 +32,14 @@ function request(path: string, init: { method?: string; headers?: Record { vi.clearAllMocks(); mocks.findUnique.mockResolvedValue(adminRow); - signedIn("aal2"); + signedIn(); }); // covers: spec 0005 AC-19 @@ -74,21 +74,21 @@ describe("adminAuthProxy admin gate", () => { expect(response.headers.get("x-middleware-rewrite")).toMatch(/\/admin\/__denied$/); }); - it("redirects an aal1 session to the MFA step with the requested path", async () => { - signedIn("aal1"); + // spec 0019, AC-2: a real 3xx, not the 200 a page's own redirect() streams. + it("redirects a session an email link opened to sign in with the requested path", async () => { + signedIn([{ method: "recovery", timestamp: now - 60 }]); const response = await adminAuthProxy(request("/admin/orders?page=2")); expect(response.status).toBe(307); expect(response.headers.get("location")).toBe( - "https://shop.example/admin/mfa?next=%2Fadmin%2Forders%3Fpage%3D2", + "https://shop.example/admin/sign-in?next=%2Fadmin%2Forders%3Fpage%3D2", ); + expect(response.headers.get("cache-control")).toBe("no-store"); }); - it("lets an aal1 session open the MFA page", async () => { - signedIn("aal1"); - - const response = await adminAuthProxy(request("/admin/mfa")); + it("lets a password session through to the page", async () => { + const response = await adminAuthProxy(request("/admin/orders")); expect(response.headers.get("x-middleware-next")).toBe("1"); }); diff --git a/src/features/admin-auth/proxy.ts b/src/features/admin-auth/proxy.ts index 7e15531..878fa23 100644 --- a/src/features/admin-auth/proxy.ts +++ b/src/features/admin-auth/proxy.ts @@ -31,7 +31,7 @@ async function lookupAdminRow(id: string): Promise { } } -// Runs on /admin and /auth only (see proxy.ts at the root), never on storefront routes (AC-1). +// Runs on /admin only (see proxy.ts at the root), never on storefront routes (AC-1). export async function adminAuthProxy(request: NextRequest): Promise { const { pathname, search } = request.nextUrl; const requestHeaders = new Headers(request.headers); diff --git a/src/features/admin-auth/require-admin.test.ts b/src/features/admin-auth/require-admin.test.ts index 8e39606..836e2ea 100644 --- a/src/features/admin-auth/require-admin.test.ts +++ b/src/features/admin-auth/require-admin.test.ts @@ -2,12 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { AdminRow } from "./access"; import { adminPathHeader } from "./proxy-decision"; -import { - adminMetadata, - redirectIfSignedIn, - requireAdmin, - requireAdminSession, -} from "./require-admin"; +import { adminMetadata, redirectIfSignedIn, requireAdmin } from "./require-admin"; const mocks = vi.hoisted(() => { class NavigationSignal extends Error {} @@ -21,7 +16,7 @@ const mocks = vi.hoisted(() => { }), requestHeaders: new Headers(), getUser: vi.fn(), - getAal: vi.fn(), + getClaims: vi.fn(), findAdmin: vi.fn(), warn: vi.fn(), }; @@ -39,7 +34,7 @@ vi.mock("next/navigation", () => ({ redirect: mocks.redirect, notFound: mocks.no vi.mock("@/lib/db", () => ({ db: { adminUser: { findUnique: mocks.findAdmin } } })); vi.mock("@/lib/supabase/server", () => ({ createSupabaseServerClient: async () => ({ - auth: { getUser: mocks.getUser, mfa: { getAuthenticatorAssuranceLevel: mocks.getAal } }, + auth: { getUser: mocks.getUser, getClaims: mocks.getClaims }, }), })); vi.mock("@/lib/logger", () => ({ logger: { info: vi.fn(), warn: mocks.warn } })); @@ -56,18 +51,16 @@ const activeRow: AdminRow = { }; type SessionSetup = { - readonly aal?: "aal1" | "aal2"; readonly amr?: unknown; readonly row?: AdminRow | null; }; -function signedIn({ aal = "aal2", amr, row = activeRow }: SessionSetup = {}) { +const recoveryAmr = [{ method: "recovery", timestamp: nowSeconds - 30 }]; + +function signedIn({ amr, row = activeRow }: SessionSetup = {}) { mocks.getUser.mockResolvedValue({ data: { user: { id: ADMIN_ID } } }); - mocks.getAal.mockResolvedValue({ - data: { - currentLevel: aal, - currentAuthenticationMethods: amr ?? [{ method: "password", timestamp: nowSeconds - 60 }], - }, + mocks.getClaims.mockResolvedValue({ + data: { claims: { amr: amr ?? [{ method: "password", timestamp: nowSeconds - 60 }] } }, }); mocks.findAdmin.mockResolvedValue(row); } @@ -97,21 +90,21 @@ afterEach(() => { vi.useRealTimers(); }); -describe("requireAdminSession", () => { - it("returns the admin without the row's disabled flag for a full aal2 session", async () => { +describe("requireAdmin", () => { + it("returns the admin without the row's disabled flag for a password session", async () => { signedIn(); - await expect(requireAdminSession()).resolves.toEqual({ - admin: { id: ADMIN_ID, email: "admin@example.com", name: "Ada" }, - aal: "aal2", - fromResetLink: false, + await expect(requireAdmin()).resolves.toEqual({ + id: ADMIN_ID, + email: "admin@example.com", + name: "Ada", }); }); it("looks up the admin row by the id the Auth server confirmed", async () => { signedIn(); - await requireAdminSession(); + await requireAdmin(); expect(mocks.findAdmin).toHaveBeenCalledWith( expect.objectContaining({ where: { id: ADMIN_ID } }), @@ -122,7 +115,7 @@ describe("requireAdminSession", () => { it("sends a visitor with no session to sign in, keeping the page they asked for", async () => { signedOut(); - await expect(outcome(requireAdminSession)).resolves.toBe( + await expect(outcome(requireAdmin)).resolves.toBe( "redirect:/admin/sign-in?next=%2Fadmin%2Forders%3Fstatus%3Dpaid", ); expect(mocks.findAdmin).not.toHaveBeenCalled(); @@ -132,32 +125,28 @@ describe("requireAdminSession", () => { mocks.requestHeaders = new Headers(); signedOut(); - await expect(outcome(requireAdminSession)).resolves.toBe("redirect:/admin/sign-in"); + await expect(outcome(requireAdmin)).resolves.toBe("redirect:/admin/sign-in"); }); it("never carries an unsafe path from the header into next", async () => { mocks.requestHeaders = new Headers({ [adminPathHeader]: "//evil.example/admin" }); signedOut(); - await expect(outcome(requireAdminSession)).resolves.toBe( - "redirect:/admin/sign-in?next=%2Fadmin", - ); + await expect(outcome(requireAdmin)).resolves.toBe("redirect:/admin/sign-in?next=%2Fadmin"); }); // covers: AC-7 it("sends a session older than 12 hours to sign in with the expired reason and no next", async () => { signedIn({ amr: [{ method: "password", timestamp: nowSeconds - 12 * 3600 - 1 }] }); - await expect(outcome(requireAdminSession)).resolves.toBe( - "redirect:/admin/sign-in?reason=expired", - ); + await expect(outcome(requireAdmin)).resolves.toBe("redirect:/admin/sign-in?reason=expired"); }); // covers: AC-6 it("answers 404 for a signed in user who is not an admin, and logs the denial", async () => { signedIn({ row: null }); - await expect(outcome(requireAdminSession)).resolves.toBe("not-found"); + await expect(outcome(requireAdmin)).resolves.toBe("not-found"); expect(mocks.warn).toHaveBeenCalledWith( expect.objectContaining({ event: "auth.access.denied", adminId: ADMIN_ID }), "auth.access.denied", @@ -165,56 +154,37 @@ describe("requireAdminSession", () => { }); // covers: AC-6 - it("answers 404 for a disabled admin, even with a full aal2 session", async () => { + it("answers 404 for a disabled admin, even with a password session", async () => { signedIn({ row: { ...activeRow, disabledAt: new Date(NOW_MS - 1000) } }); - await expect(outcome(requireAdminSession)).resolves.toBe("not-found"); + await expect(outcome(requireAdmin)).resolves.toBe("not-found"); }); - // covers: AC-5 - it("sends a password only (aal1) session to the MFA step, keeping the page", async () => { - signedIn({ aal: "aal1" }); + // spec 0019, AC-2: an inbox alone never opens the panel. + it("sends an admin whose session an email link opened to sign in, keeping the page", async () => { + signedIn({ amr: recoveryAmr }); - await expect(outcome(requireAdminSession)).resolves.toBe( - "redirect:/admin/mfa?next=%2Fadmin%2Forders%3Fstatus%3Dpaid", + await expect(outcome(requireAdmin)).resolves.toBe( + "redirect:/admin/sign-in?next=%2Fadmin%2Forders%3Fstatus%3Dpaid", ); }); - it("lets an aal1 session through only where the caller allows it", async () => { - signedIn({ aal: "aal1" }); + it("reads amr from the claims of the confirmed session", async () => { + signedIn(); - await expect(requireAdminSession({ allowAal1: true })).resolves.toMatchObject({ aal: "aal1" }); - }); + await requireAdmin(); - // covers: AC-4 (a reset link session may verify a factor, never enroll one) - it("marks a session opened by a password reset link", async () => { - signedIn({ - aal: "aal1", - amr: [{ method: "recovery", timestamp: nowSeconds - 30 }], - }); - - await expect(requireAdminSession({ allowAal1: true })).resolves.toMatchObject({ - fromResetLink: true, - }); + expect(mocks.getClaims).toHaveBeenCalled(); }); -}); -describe("requireAdmin", () => { - it("returns just the admin for an allowed session", async () => { + it("sends a session whose token cannot be read to a plain sign in, not the expired one", async () => { signedIn(); + mocks.getClaims.mockResolvedValue({ data: null, error: new Error("invalid JWT") }); - await expect(requireAdmin()).resolves.toEqual({ - id: ADMIN_ID, - email: "admin@example.com", - name: "Ada", - }); - }); - - // covers: AC-5 (every admin action calls this guard, so aal1 never runs one) - it("refuses an aal1 session", async () => { - signedIn({ aal: "aal1" }); - - await expect(outcome(requireAdmin)).resolves.toMatch(/^redirect:\/admin\/mfa/); + await expect(outcome(requireAdmin)).resolves.toBe( + "redirect:/admin/sign-in?next=%2Fadmin%2Forders%3Fstatus%3Dpaid", + ); + expect(mocks.findAdmin).not.toHaveBeenCalled(); }); }); @@ -246,11 +216,10 @@ describe("adminMetadata", () => { await expect(adminMetadata(metadata, { layout: true })).resolves.toEqual({}); }); - it("gives an aal1 session the metadata only where aal1 is allowed", async () => { - signedIn({ aal: "aal1" }); + it("returns the not found title to an email link session", async () => { + signedIn({ amr: recoveryAmr }); await expect(adminMetadata(metadata)).resolves.toEqual({ title: "Page not found" }); - await expect(adminMetadata(metadata, { allowAal1: true })).resolves.toBe(metadata); }); it("never redirects or 404s itself", async () => { @@ -263,7 +232,7 @@ describe("adminMetadata", () => { }); }); -// covers: AC-17 +// covers: spec 0004 AC-17, spec 0019 AC-6 describe("redirectIfSignedIn", () => { it("leaves a signed out visitor on the page", async () => { signedOut(); @@ -271,15 +240,14 @@ describe("redirectIfSignedIn", () => { await expect(redirectIfSignedIn(undefined)).resolves.toBeUndefined(); }); - it("sends an aal1 admin to the MFA step, keeping next", async () => { - signedIn({ aal: "aal1" }); + // spec 0019, AC-6 + it("leaves an admin whose session an email link opened on the page", async () => { + signedIn({ amr: recoveryAmr }); - await expect(outcome(() => redirectIfSignedIn("/admin/orders"))).resolves.toBe( - "redirect:/admin/mfa?next=%2Fadmin%2Forders", - ); + await expect(redirectIfSignedIn("/admin/orders")).resolves.toBeUndefined(); }); - it("sends an aal2 admin to next", async () => { + it("sends an admin with a password session to next", async () => { signedIn(); await expect(outcome(() => redirectIfSignedIn("/admin/orders"))).resolves.toBe( @@ -288,7 +256,7 @@ describe("redirectIfSignedIn", () => { }); // covers: AC-12 - it("sends an aal2 admin to /admin when next points off site", async () => { + it("sends an admin to /admin when next points off site", async () => { signedIn(); await expect(outcome(() => redirectIfSignedIn("//evil.example/admin"))).resolves.toBe( diff --git a/src/features/admin-auth/require-admin.ts b/src/features/admin-auth/require-admin.ts index f95d019..660db27 100644 --- a/src/features/admin-auth/require-admin.ts +++ b/src/features/admin-auth/require-admin.ts @@ -8,25 +8,12 @@ import { cache } from "react"; import { db } from "@/lib/db"; import { createSupabaseServerClient } from "@/lib/supabase/server"; -import { - type AccessInput, - type Admin, - type AssuranceLevel, - decideAdminAccess, - isResetSession, -} from "./access"; +import { type AccessInput, type Admin, decideAdminAccess } from "./access"; import { logAuthEvent, requestIp } from "./log"; import { adminPathHeader } from "./proxy-decision"; -import { mfaPath, safeAdminPath, signInPath } from "./safe-admin-path"; +import { safeAdminPath, signInPath } from "./safe-admin-path"; -export type AdminSession = { - readonly admin: Admin; - readonly aal: AssuranceLevel; - // Opened by a password reset link: may verify a factor, never enroll one (AC-4). - readonly fromResetLink: boolean; -}; - -type SessionState = Omit; +type SessionState = Omit; // Once per request: the layout, the page and an action in the same request share it. const loadSessionState = cache(async (): Promise => { @@ -35,52 +22,38 @@ const loadSessionState = cache(async (): Promise => { const { data: { user }, } = await supabase.auth.getUser(); - if (!user) return { userId: null, aal: null, amr: null, adminRow: null }; + if (!user) return { userId: null, amr: null, adminRow: null }; - // Decodes the token getUser() just confirmed; no second network call. - const { data: level } = await supabase.auth.mfa.getAuthenticatorAssuranceLevel(); + // The `amr` of the token getUser() just confirmed, read the way the proxy reads it. A token + // that cannot be read counts as no session: a plain sign in, not "your session expired". + const { data } = await supabase.auth.getClaims(); + if (!data) return { userId: null, amr: null, adminRow: null }; const adminRow = await db.adminUser.findUnique({ where: { id: user.id }, select: { id: true, email: true, name: true, disabledAt: true }, }); - return { - userId: user.id, - aal: level?.currentLevel === "aal2" ? "aal2" : "aal1", - amr: level?.currentAuthenticationMethods ?? null, - adminRow, - }; + return { userId: user.id, amr: data?.claims.amr ?? null, adminRow }; }); // The one guard for every admin page, layout, action and route handler (spec 0004). It checks -// the session with the Auth server, the 12 hour cap, the live admin_users row and aal2, then -// redirects or 404s; it only returns for an active admin. -export async function requireAdminSession( - options: { readonly allowAal1?: boolean } = {}, -): Promise { +// the session with the Auth server, the 12 hour cap, the live admin_users row and that a +// password opened the session (spec 0019), then redirects or 404s; it only returns for an +// active admin. +export async function requireAdmin(): Promise { const state = await loadSessionState(); - const verdict = decideAdminAccess({ - ...state, - nowMs: Date.now(), - allowAal1: options.allowAal1 ?? false, - }); + const verdict = decideAdminAccess({ ...state, nowMs: Date.now() }); const requestHeaders = await headers(); const path = requestHeaders.get(adminPathHeader) ?? undefined; switch (verdict.kind) { case "allow": - return { - admin: verdict.admin, - aal: state.aal ?? "aal1", - fromResetLink: isResetSession(state.amr), - }; + return verdict.admin; case "sign-in": redirect( verdict.reason === "expired" ? signInPath({ reason: "expired" }) : signInPath(path === undefined ? {} : { next: path }), ); - case "mfa": - redirect(mfaPath(path)); case "not-found": logAuthEvent("auth.access.denied", { adminId: state.userId, ip: requestIp(requestHeaders) }); notFound(); @@ -96,30 +69,20 @@ export async function requireAdminSession( // 404 title entirely); the page's own guard does that. export async function adminMetadata( metadata: Metadata, - options: { readonly allowAal1?: boolean; readonly layout?: boolean } = {}, + options: { readonly layout?: boolean } = {}, ): Promise { const state = await loadSessionState(); - const verdict = decideAdminAccess({ - ...state, - nowMs: Date.now(), - allowAal1: options.allowAal1 ?? false, - }); + const verdict = decideAdminAccess({ ...state, nowMs: Date.now() }); if (verdict.kind === "allow") return metadata; return options.layout ? {} : { title: "Page not found" }; } -export async function requireAdmin(): Promise { - const { admin } = await requireAdminSession(); - return admin; -} - -// AC-17: the sign in and forgot password pages send an admin who is already signed in onward. -// Anyone else (no session, expired, not an admin) sees the page. +// spec 0019, AC-6: the sign in page sends an active admin with a password session onward. +// Anyone else (no session, expired, not an admin, an email link session) sees the page. export async function redirectIfSignedIn(next: unknown): Promise { const state = await loadSessionState(); if (state.userId === null) return; const verdict = decideAdminAccess({ ...state, nowMs: Date.now() }); - const target = typeof next === "string" ? next : undefined; - if (verdict.kind === "mfa") redirect(mfaPath(target)); - if (verdict.kind === "allow") redirect(safeAdminPath(target)); + if (verdict.kind === "allow") + redirect(safeAdminPath(typeof next === "string" ? next : undefined)); } diff --git a/src/features/admin-auth/safe-admin-path.test.ts b/src/features/admin-auth/safe-admin-path.test.ts index fc26381..9790ea9 100644 --- a/src/features/admin-auth/safe-admin-path.test.ts +++ b/src/features/admin-auth/safe-admin-path.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; -import { mfaPath, safeAdminPath, signInPath } from "./safe-admin-path"; +import { safeAdminPath, signInPath } from "./safe-admin-path"; describe("safeAdminPath (AC-12)", () => { it.each([ @@ -11,8 +11,6 @@ describe("safeAdminPath (AC-12)", () => { "/admin-evil", "/admin/sign-in", "/admin/sign-in?next=/admin", - "/admin/mfa", - "/admin/forgot-password", "/admin/../evil", "/admin/%2e%2e/evil", "/admin/\tx", @@ -28,6 +26,8 @@ describe("safeAdminPath (AC-12)", () => { it.each([ ["/admin", "/admin"], ["/admin/orders/1?x=y", "/admin/orders/1?x=y"], + // spec 0019: the removed auth pages are ordinary admin paths, which 404 after sign in. + ["/admin/mfa", "/admin/mfa"], ["/admin/reset-password", "/admin/reset-password"], ["/admin/orders#top", "/admin/orders"], ])("accepts %j", (next, expected) => { @@ -44,10 +44,4 @@ describe("redirect paths", () => { expect(signInPath({ next: "//evil.test" })).toBe("/admin/sign-in?next=%2Fadmin"); expect(signInPath({ reason: "expired" })).toBe("/admin/sign-in?reason=expired"); }); - - it("builds the MFA URL", () => { - expect(mfaPath()).toBe("/admin/mfa"); - expect(mfaPath("/admin")).toBe("/admin/mfa?next=%2Fadmin"); - expect(mfaPath("https://evil.test")).toBe("/admin/mfa?next=%2Fadmin"); - }); }); diff --git a/src/features/admin-auth/safe-admin-path.ts b/src/features/admin-auth/safe-admin-path.ts index 4083209..6ce7ec7 100644 --- a/src/features/admin-auth/safe-admin-path.ts +++ b/src/features/admin-auth/safe-admin-path.ts @@ -3,9 +3,8 @@ export const adminHome = "/admin"; // A POST route handler (spec 0005, AC-20). export const signOutPath = "/admin/sign-out"; -// Pages a `next` must never point back at; the reset page is allowed, since the reset link -// sends the admin through the MFA step on the way there. -const authPages = ["/admin/sign-in", "/admin/mfa", "/admin/forgot-password"] as const; +// Pages a `next` must never point back at. +const authPages = ["/admin/sign-in"] as const; const base = "http://admin.invalid"; @@ -42,9 +41,3 @@ export function signInPath(options: { next?: string; reason?: string } = {}): st const query = params.toString(); return query ? `/admin/sign-in?${query}` : "/admin/sign-in"; } - -export function mfaPath(next?: string): string { - return next === undefined - ? "/admin/mfa" - : `/admin/mfa?next=${encodeURIComponent(safeAdminPath(next))}`; -} diff --git a/src/features/admin-auth/schemas.test.ts b/src/features/admin-auth/schemas.test.ts index 0826d2f..0e9f228 100644 --- a/src/features/admin-auth/schemas.test.ts +++ b/src/features/admin-auth/schemas.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; -import { emailSchema, resetPasswordSchema, signInSchema, totpCodeSchema } from "./schemas"; +import { emailSchema, signInSchema } from "./schemas"; describe("form schemas", () => { it("lower cases and trims the email", () => { @@ -17,20 +17,4 @@ describe("form schemas", () => { "Enter a valid email address", ); }); - - it("accepts a 6 digit code with spaces and rejects anything else", () => { - expect(totpCodeSchema.parse({ code: "123 456" })).toEqual({ code: "123456" }); - for (const code of ["12345", "1234567", "abcdef", ""]) { - expect(totpCodeSchema.safeParse({ code }).error?.issues[0]?.message).toBe( - "Enter the 6 digit code", - ); - } - }); - - it("requires the two new passwords to match", () => { - const result = resetPasswordSchema.safeParse({ password: "Abcdef1!", confirm: "Abcdef1?" }); - expect(result.error?.issues).toMatchObject([ - { path: ["confirm"], message: "The passwords do not match." }, - ]); - }); }); diff --git a/src/features/admin-auth/schemas.ts b/src/features/admin-auth/schemas.ts index 3933b73..256fa70 100644 --- a/src/features/admin-auth/schemas.ts +++ b/src/features/admin-auth/schemas.ts @@ -1,8 +1,6 @@ import { z } from "zod"; -import { passwordSchema } from "@/lib/auth/password"; - -import { authMessages, fieldMessages } from "./messages"; +import { fieldMessages } from "./messages"; export const emailSchema = z .string() @@ -17,26 +15,3 @@ export const signInSchema = z.object({ }); export type SignInValues = z.input; - -export const totpCodeSchema = z.object({ - // Authenticator apps show the code as "123 456"; spaces are dropped before checking. - code: z - .string() - .transform((code) => code.replace(/\s/g, "")) - .pipe(z.string().regex(/^\d{6}$/, fieldMessages.codeRequired)), -}); - -export type TotpCodeValues = z.input; - -export const forgotPasswordSchema = z.object({ email: emailSchema }); - -export type ForgotPasswordValues = z.input; - -export const resetPasswordSchema = z - .object({ password: passwordSchema, confirm: z.string() }) - .refine((values) => values.password === values.confirm, { - path: ["confirm"], - message: authMessages.mismatch, - }); - -export type ResetPasswordValues = z.input; diff --git a/src/features/admin-auth/sign-out.ts b/src/features/admin-auth/sign-out.ts index 93b7112..5587753 100644 --- a/src/features/admin-auth/sign-out.ts +++ b/src/features/admin-auth/sign-out.ts @@ -9,7 +9,8 @@ import { signInPath } from "./safe-admin-path"; import { endLocalSession } from "@/lib/supabase/session"; // The one admin route without requireAdmin(): it only ends the caller's own session on this -// device, so it must work from any state (aal1, expired, disabled) and grants nothing. +// device, so it must work from any state (expired, disabled, an email link session) and grants +// nothing. // A plain form POST and a 303, so the browser does a full page load and Cache Components keeps // no admin page alive for Back (spec 0005, AC-20). export async function signOutRequest(request: NextRequest): Promise { diff --git a/src/features/dashboard/log.test.ts b/src/features/dashboard/log.test.ts index 6a47e9c..d83b198 100644 --- a/src/features/dashboard/log.test.ts +++ b/src/features/dashboard/log.test.ts @@ -1,7 +1,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; // spec 0015, AC-14: the failure line goes through the real logger, redaction included, so a -// field name the logger censors (such as `code`, kept for TOTP codes) would hide the SQLSTATE. +// field name the logger censors (such as `code`, kept for auth codes) would hide the SQLSTATE. const { lines, sink } = await vi.hoisted(async () => { const { Writable } = await import("node:stream"); diff --git a/src/features/dashboard/log.ts b/src/features/dashboard/log.ts index e2fa801..3a72f22 100644 --- a/src/features/dashboard/log.ts +++ b/src/features/dashboard/log.ts @@ -11,7 +11,7 @@ export type DashboardLogFields = { }; export function logDashboardQueryFailed(fields: DashboardLogFields, error: unknown) { - // `pgCode`, not `code`: the logger censors any `code` key (TOTP codes, spec 0004). + // `pgCode`, not `code`: the logger censors any `code` key (auth codes, spec 0004). logger.error( { event: "dashboard.query_failed", ...fields, pgCode: pgErrorCode(error) }, "dashboard.query_failed", diff --git a/src/lib/auth/password.test.ts b/src/lib/auth/password.test.ts index a234ff1..75ffa08 100644 --- a/src/lib/auth/password.test.ts +++ b/src/lib/auth/password.test.ts @@ -1,6 +1,11 @@ import { describe, expect, it } from "vitest"; -import { isPasswordTooLong, missingPasswordRules, passwordSchema } from "./password"; +import { + adminPasswordSchema, + isPasswordTooLong, + missingPasswordRules, + passwordSchema, +} from "./password"; describe("passwordSchema (spec 0004 AC-10, spec 0012 AC-3)", () => { it("accepts a password with all four classes and 8 characters", () => { @@ -30,3 +35,25 @@ describe("passwordSchema (spec 0004 AC-10, spec 0012 AC-3)", () => { expect(passwordSchema.safeParse(`Aa1!${"x".repeat(69)}`).success).toBe(false); }); }); + +describe("adminPasswordSchema (spec 0019 AC-16)", () => { + it("accepts 14 characters that pass the shared rule", () => { + expect(adminPasswordSchema.safeParse("Abcdefghijk1!x").success).toBe(true); + }); + + it("names the 14 character rule as its own missing part", () => { + const result = adminPasswordSchema.safeParse("Abcdefghij1!x"); + expect(result.error?.issues.map((issue) => issue.message)).toEqual(["At least 14 characters"]); + }); + + it("names the shared rule parts too", () => { + const result = adminPasswordSchema.safeParse("abc"); + expect(result.error?.issues.map((issue) => issue.message)).toEqual([ + "At least 8 characters", + "An upper case letter", + "A digit", + "A symbol", + "At least 14 characters", + ]); + }); +}); diff --git a/src/lib/auth/password.ts b/src/lib/auth/password.ts index 9a5b32a..bff1b94 100644 --- a/src/lib/auth/password.ts +++ b/src/lib/auth/password.ts @@ -1,8 +1,8 @@ import { z } from "zod"; -// Pure (no server-only imports): the admin pages, the account pages and the admin scripts all -// check passwords with this one schema, which mirrors the project wide Supabase policy -// (spec 0004, AC-10; supabase/config.toml `password_requirements`). Keep the two in step. +// Pure (no server-only imports): the account pages and the admin scripts check passwords with +// this schema, which mirrors the project wide Supabase policy (spec 0004, AC-10; +// supabase/config.toml `password_requirements`). Keep the two in step. export const passwordRuleMessages = { length: "At least 8 characters", @@ -47,5 +47,18 @@ export const passwordSchema = z.string().superRefine((value, ctx) => { if (isPasswordTooLong(value)) ctx.addIssue({ code: "custom", message: passwordTooLongMessage }); }); +// Admin passwords (spec 0019, AC-16): the password is the only factor in front of `/admin`, so +// length is the cheapest defense against guessing that slips past rate limits. Only the admin +// scripts set one; the customer forms refuse store runners. +export const ADMIN_PASSWORD_MIN_LENGTH = 14; + +export const adminPasswordLengthMessage = `At least ${ADMIN_PASSWORD_MIN_LENGTH} characters`; + +export const adminPasswordSchema = passwordSchema.superRefine((value, ctx) => { + if (value.length < ADMIN_PASSWORD_MIN_LENGTH) { + ctx.addIssue({ code: "custom", message: adminPasswordLengthMessage }); + } +}); + // The rule list a form prints under a new password field. export const passwordRuleList = Object.values(passwordRuleMessages).join(", ").toLowerCase(); diff --git a/src/lib/customer/store-admin.ts b/src/lib/customer/store-admin.ts index 11fc64d..0ae363c 100644 --- a/src/lib/customer/store-admin.ts +++ b/src/lib/customer/store-admin.ts @@ -4,15 +4,11 @@ import { db, type Tx } from "@/lib/db"; type Client = Pick; -// spec 0012, AC-8 and AC-19: a user who runs the store (an admin_users row in any state, or a -// verified authenticator) always takes the admin path for passwords, so an inbox alone never -// resets a password that guards /admin. auth.mfa_factors is Supabase's own table: read only. +// spec 0019, AC-10: a user who runs the store (an admin_users row in any state) never changes +// their password from an inbox or an account page; an admin password changes only from the +// terminal, so an inbox alone never opens /admin. export async function runsTheStore(userId: string, client: Client = db): Promise { const rows = await client.$queryRaw<{ found: boolean }[]>` - SELECT EXISTS (SELECT 1 FROM admin_users WHERE id = ${userId}::uuid) - OR EXISTS ( - SELECT 1 FROM auth.mfa_factors - WHERE user_id = ${userId}::uuid AND status = 'verified' - ) AS found`; + SELECT EXISTS (SELECT 1 FROM admin_users WHERE id = ${userId}::uuid) AS found`; return rows[0]?.found ?? false; } diff --git a/src/lib/email/auth-hook.test.ts b/src/lib/email/auth-hook.test.ts index c380e4f..afd7dd1 100644 --- a/src/lib/email/auth-hook.test.ts +++ b/src/lib/email/auth-hook.test.ts @@ -8,6 +8,7 @@ import { authIdempotencyKey, hookPayloadSchema, hookSecretKey, + isStoreRunnerReset, usedActionType, } from "./auth-hook"; @@ -23,35 +24,36 @@ describe("usedActionType", () => { }); }); -describe("authEmailKind", () => { - it("confirms a sign up whoever the user is", () => { - expect(authEmailKind("signup", false)).toBe("confirm_signup"); - expect(authEmailKind("signup", true)).toBe("confirm_signup"); +describe("isStoreRunnerReset (spec 0019, AC-8)", () => { + it("skips only a reset for someone who runs the store", () => { + expect(isStoreRunnerReset("recovery", true)).toBe(true); + expect(isStoreRunnerReset("recovery", false)).toBe(false); + expect(isStoreRunnerReset("signup", true)).toBe(false); + expect(isStoreRunnerReset("signup", false)).toBe(false); }); +}); - it("sends the admin reset to anyone who runs the store, else the customer reset", () => { - expect(authEmailKind("recovery", true)).toBe("admin_reset"); - expect(authEmailKind("recovery", false)).toBe("customer_reset"); +describe("authEmailKind", () => { + it("confirms a sign up and resets a customer", () => { + expect(authEmailKind("signup")).toBe("confirm_signup"); + expect(authEmailKind("recovery")).toBe("customer_reset"); }); }); describe("authEmailLink", () => { const site = "https://shop.example"; - it("points customers at /account/confirm and admins at /auth/confirm", () => { - expect(authEmailLink(site, "confirm_signup", "abc")).toBe( + it("points every link at /account/confirm", () => { + expect(authEmailLink(site, "signup", "abc")).toBe( "https://shop.example/account/confirm?token_hash=abc&type=signup", ); - expect(authEmailLink(site, "customer_reset", "abc")).toBe( + expect(authEmailLink(site, "recovery", "abc")).toBe( "https://shop.example/account/confirm?token_hash=abc&type=recovery", ); - expect(authEmailLink(site, "admin_reset", "abc")).toBe( - "https://shop.example/auth/confirm?token_hash=abc&type=recovery", - ); }); it("encodes the token hash", () => { - expect(authEmailLink(site, "confirm_signup", "a&b=c")).toContain("token_hash=a%26b%3Dc"); + expect(authEmailLink(site, "signup", "a&b=c")).toContain("token_hash=a%26b%3Dc"); }); }); diff --git a/src/lib/email/auth-hook.ts b/src/lib/email/auth-hook.ts index b009589..a1ef047 100644 --- a/src/lib/email/auth-hook.ts +++ b/src/lib/email/auth-hook.ts @@ -23,33 +23,24 @@ export function usedActionType(type: string): AuthActionType | null { return type === "signup" || type === "recovery" ? type : null; } -export type AuthEmailKind = "confirm_signup" | "customer_reset" | "admin_reset"; +export type AuthEmailKind = "confirm_signup" | "customer_reset"; -// Admin first: whether the user runs the store cannot be steered by the caller, unlike -// redirect_to, which anyone with the anon key can set. So an inbox alone never resets a password -// that guards /admin. -export function authEmailKind(type: AuthActionType, runsTheStore: boolean): AuthEmailKind { - if (type === "signup") return "confirm_signup"; - return runsTheStore ? "admin_reset" : "customer_reset"; +// spec 0019, AC-8: a store runner gets no reset email from any flow, since an admin password +// changes only from the terminal. Decided from the user id, which the caller cannot steer, unlike +// redirect_to, which anyone with the anon key can set. +export function isStoreRunnerReset(type: AuthActionType, runsTheStore: boolean): boolean { + return type === "recovery" && runsTheStore; } -const linkPaths: Record = { - confirm_signup: "/account/confirm", - customer_reset: "/account/confirm", - admin_reset: "/auth/confirm", -}; - -const linkTypes: Record = { - confirm_signup: "signup", - customer_reset: "recovery", - admin_reset: "recovery", -}; +export function authEmailKind(type: AuthActionType): AuthEmailKind { + return type === "signup" ? "confirm_signup" : "customer_reset"; +} // The token hash link pattern: the page shows Continue first, and only that POST spends it. -export function authEmailLink(siteUrl: string, kind: AuthEmailKind, tokenHash: string): string { - const url = new URL(linkPaths[kind], siteUrl); +export function authEmailLink(siteUrl: string, type: AuthActionType, tokenHash: string): string { + const url = new URL("/account/confirm", siteUrl); url.searchParams.set("token_hash", tokenHash); - url.searchParams.set("type", linkTypes[kind]); + url.searchParams.set("type", type); return url.toString(); } diff --git a/src/lib/email/render-auth.tsx b/src/lib/email/render-auth.tsx index ac00e4f..a99d18c 100644 --- a/src/lib/email/render-auth.tsx +++ b/src/lib/email/render-auth.tsx @@ -1,9 +1,5 @@ import { render } from "react-email"; -import { - AdminResetPasswordEmail, - adminResetPasswordSubject, -} from "../../../emails/admin-reset-password"; import type { EmailFrame } from "../../../emails/components/layout"; import { ConfirmSignupEmail, confirmSignupSubject } from "../../../emails/confirm-signup"; import { ResetPasswordEmail, resetPasswordSubject } from "../../../emails/reset-password"; @@ -22,11 +18,6 @@ function element(kind: AuthEmailKind, frame: EmailFrame, link: string) { subject: resetPasswordSubject, node: , }; - case "admin_reset": - return { - subject: adminResetPasswordSubject, - node: , - }; } } diff --git a/supabase/config.toml b/supabase/config.toml index 29444eb..e239a82 100644 --- a/supabase/config.toml +++ b/supabase/config.toml @@ -326,9 +326,10 @@ auth_token = "env(SUPABASE_AUTH_SMS_TWILIO_AUTH_TOKEN)" max_enrolled_factors = 10 # Control MFA via App Authenticator (TOTP) +# Off since spec 0019: admins sign in with a password only. [auth.mfa.totp] -enroll_enabled = true -verify_enabled = true +enroll_enabled = false +verify_enabled = false # Configure MFA via Phone Messaging [auth.mfa.phone] diff --git a/tests/db/account-settings.db.test.ts b/tests/db/account-settings.db.test.ts index 643b4fa..0a86115 100644 --- a/tests/db/account-settings.db.test.ts +++ b/tests/db/account-settings.db.test.ts @@ -1,6 +1,6 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; -import { addAuthUser, addVerifiedFactor, resetAuthStub } from "./auth-stub"; +import { addAuthUser, resetAuthStub } from "./auth-stub"; import { resetDatabaseBeforeEach, testDb } from "./client"; import { createCustomer, createOrder, createSimpleProduct, inThirtyDays } from "./fixtures"; @@ -129,14 +129,16 @@ describe("changePassword (AC-19)", () => { expect(mocks.updateUser).not.toHaveBeenCalled(); }); - it("refuses an admin and anyone with an authenticator", async () => { + it("refuses an admin, enabled or disabled (spec 0019, AC-10)", async () => { await testDb.adminUser.create({ data: { id: mocks.customer.id, email: "ada@example.com", name: "Ada" }, }); expect(await changePassword(values)).toEqual({ ok: false, error: { form: "runs_the_store" } }); - await testDb.adminUser.deleteMany(); - await addVerifiedFactor(mocks.customer.id); + await testDb.adminUser.update({ + where: { id: mocks.customer.id }, + data: { disabledAt: new Date() }, + }); expect(await changePassword(values)).toEqual({ ok: false, error: { form: "runs_the_store" } }); expect(mocks.statelessSignIn).not.toHaveBeenCalled(); }); @@ -219,17 +221,6 @@ describe("deleteAccount (AC-20)", () => { ); }); - it("refuses anyone with an authenticator, like change password", async () => { - await addVerifiedFactor(mocks.customer.id); - - expect(await deleteAccount({ password: "Pass-word-1" })).toEqual({ - ok: false, - error: { form: "runs_the_store" }, - }); - expect(mocks.statelessSignIn).not.toHaveBeenCalled(); - expect(mocks.deleteUser).not.toHaveBeenCalled(); - }); - it("needs the right password", async () => { mocks.statelessSignIn.mockResolvedValue({ data: { user: null }, diff --git a/tests/db/auth-stub.ts b/tests/db/auth-stub.ts index 5035a10..eee230c 100644 --- a/tests/db/auth-stub.ts +++ b/tests/db/auth-stub.ts @@ -1,22 +1,13 @@ import { testDb } from "./client"; -// The test database is plain Postgres, without Supabase's auth schema. Account code reads two of -// its tables (auth.users, auth.mfa_factors) read only, so the suites that need them get a minimal -// stand in. It is marked, and only a marked stand in is ever cleared, so a database with the real +// The test database is plain Postgres, without Supabase's auth schema. Account code reads one of +// its tables (auth.users) read only, so the suites that need it get a minimal stand in. It is marked, and only a marked stand in is ever cleared, so a database with the real // auth schema is never touched. const marker = "bestore test stub"; export async function ensureAuthStub() { await testDb.$executeRawUnsafe(`CREATE SCHEMA IF NOT EXISTS auth`); await testDb.$executeRawUnsafe(`CREATE TABLE IF NOT EXISTS auth.users (id uuid PRIMARY KEY)`); - await testDb.$executeRawUnsafe( - `CREATE TABLE IF NOT EXISTS auth.mfa_factors ( - id uuid PRIMARY KEY DEFAULT gen_random_uuid(), - user_id uuid NOT NULL, - factor_type text NOT NULL DEFAULT 'totp', - status text NOT NULL - )`, - ); const [row] = await testDb.$queryRaw<{ comment: string | null }[]>` SELECT obj_description('auth.users'::regclass, 'pg_class') AS comment`; if (row?.comment === null) { @@ -28,14 +19,9 @@ export async function ensureAuthStub() { export async function resetAuthStub() { await ensureAuthStub(); - await testDb.$executeRawUnsafe(`TRUNCATE auth.users, auth.mfa_factors`); + await testDb.$executeRawUnsafe(`TRUNCATE auth.users`); } export async function addAuthUser(id: string) { await testDb.$executeRaw`INSERT INTO auth.users (id) VALUES (${id}::uuid)`; } - -export async function addVerifiedFactor(userId: string) { - await testDb.$executeRaw` - INSERT INTO auth.mfa_factors (user_id, status) VALUES (${userId}::uuid, 'verified')`; -} diff --git a/tests/e2e/account/orders.spec.ts b/tests/e2e/account/orders.spec.ts index 84d2f55..77210f7 100644 --- a/tests/e2e/account/orders.spec.ts +++ b/tests/e2e/account/orders.spec.ts @@ -1,6 +1,6 @@ import { expect, test } from "@playwright/test"; -import { createTestUser, signInFully, testPassword } from "../admin/support"; +import { createTestUser, signInAsAdmin, testPassword } from "../admin/support"; import { seedPaidOrder, signInAsCustomer } from "./support"; // spec 0012: order history and one order (AC-10, AC-11, AC-12, AC-13). @@ -61,8 +61,8 @@ test("the admin order pages show Account for a linked order and Guest for the re await signInAsCustomer(customerPage, customer.email, testPassword); await expect(customerPage).toHaveURL("/account"); - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); for (const [order, label] of [ [linked, "Account"], [guest, "Guest"], diff --git a/tests/e2e/account/password-reset.spec.ts b/tests/e2e/account/password-reset.spec.ts index f7dcf63..17a5b97 100644 --- a/tests/e2e/account/password-reset.spec.ts +++ b/tests/e2e/account/password-reset.spec.ts @@ -1,9 +1,10 @@ import { expect, test } from "@playwright/test"; -import { createTestUser, latestEmailHtml, testPassword } from "../admin/support"; +import { createTestUser, recoveryTokenHash, testPassword } from "../admin/support"; import { accountLinkFor, choosePassword, signInAsCustomer, uniqueEmail } from "./support"; -// spec 0012: forgot password through the Send Email Hook (AC-8, AC-9, AC-2, AC-3). +// spec 0012: forgot password through the Send Email Hook (AC-8, AC-9, AC-2, AC-3), and spec 0019 +// AC-8 and AC-9: nothing for a user who runs the store. const sent = "If there is an account for that email, a reset link is on its way."; @@ -49,36 +50,37 @@ test("a customer resets their password from the email and every old session ends await expect(page).toHaveURL("/account"); }); -test("an unknown email gets the same reply and no email", async ({ page }) => { - const email = uniqueEmail("nobody"); - await requestReset(page, email); - - await page.waitForTimeout(1_500); +async function expectNoEmail(email: string) { + await new Promise((resolve) => setTimeout(resolve, 1_500)); const search = await fetch( `http://127.0.0.1:55324/api/v1/search?query=${encodeURIComponent(`to:"${email}"`)}`, ); expect(((await search.json()) as { messages_count: number }).messages_count).toBe(0); +} + +test("an unknown email gets the same reply and no email", async ({ page }) => { + const email = uniqueEmail("nobody"); + await requestReset(page, email); + await expectNoEmail(email); }); -test("an admin asking here gets the admin email, and its link never skips the authenticator", async ({ - page, -}) => { - const admin = await createTestUser({ enrolled: true }); +test("an admin asking here gets the same reply and no email", async ({ page }) => { + const admin = await createTestUser(); await requestReset(page, admin.email); + await expectNoEmail(admin.email); +}); - const html = await latestEmailHtml(admin.email); - expect(html).toContain("Reset your admin password"); - expect(html).not.toContain("/account/confirm"); - const adminLink = html - .match(/href="([^"]*\/auth\/confirm\?[^"]*)"/)?.[1] - ?.replaceAll("&", "&"); - expect(adminLink).toBeDefined(); +test("a recovery link for an admin made outside the app changes no password", async ({ page }) => { + const admin = await createTestUser(); + const tokenHash = await recoveryTokenHash(admin.email); - // The same token pasted into the customer confirm page still lands on the authenticator step. - const pasted = adminLink!.replace("/auth/confirm", "/account/confirm"); - await page.goto(pasted); + await page.goto(`/account/confirm?token_hash=${tokenHash}&type=recovery`); await page.getByRole("button", { name: "Continue" }).click(); - await expect(page).toHaveURL("/admin/mfa?next=%2Fadmin%2Freset-password"); + await expect(page).toHaveURL("/account/confirm?failed=1&type=recovery"); + await expect(page.getByText("This link has expired or was already used.")).toBeVisible(); + // The session the link opened is ended on this device. + await page.goto("/account"); + await expect(page).toHaveURL(/\/account\/sign-in/); }); test("an expired reset link offers a new one", async ({ page }) => { diff --git a/tests/e2e/account/settings.spec.ts b/tests/e2e/account/settings.spec.ts index 73ec506..a6fe113 100644 --- a/tests/e2e/account/settings.spec.ts +++ b/tests/e2e/account/settings.spec.ts @@ -89,9 +89,7 @@ test("an admin cannot change their password or delete their account here", async await page.getByLabel("Type it again").fill("Fresh-start-88"); await page.getByRole("button", { name: "Change password" }).click(); await expect( - page.getByText( - "This account also runs the store. Use Forgot password on the admin sign in page.", - ), + page.getByText("This account also runs the store, so its password cannot be changed here."), ).toBeVisible(); await page.getByRole("button", { name: "Delete my account" }).click(); diff --git a/tests/e2e/account/sign-in.spec.ts b/tests/e2e/account/sign-in.spec.ts index 35a9237..23b7d23 100644 --- a/tests/e2e/account/sign-in.spec.ts +++ b/tests/e2e/account/sign-in.spec.ts @@ -1,6 +1,6 @@ import { expect, test } from "@playwright/test"; -import { createTestUser, signInFully, testPassword } from "../admin/support"; +import { createTestUser, signInAsAdmin, testPassword } from "../admin/support"; import { seedProduct } from "../catalog/support"; import { addToCart } from "../checkout/support"; import { signInAsCustomer, uniqueEmail } from "./support"; @@ -61,8 +61,8 @@ test("sign in goes to a safe next and refuses any other", async ({ page }) => { test("a signed in admin opening the customer sign in page keeps their admin session", async ({ page, }) => { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto("/account/sign-in"); await expect(page).toHaveURL("/account"); diff --git a/tests/e2e/admin/a11y.spec.ts b/tests/e2e/admin/a11y.spec.ts index 980da88..e539e18 100644 --- a/tests/e2e/admin/a11y.spec.ts +++ b/tests/e2e/admin/a11y.spec.ts @@ -1,37 +1,26 @@ import { expect, test } from "@playwright/test"; import { expectNoA11yViolations } from "../a11y"; -import { - createTestUser, - enterCode, - latestEmailHtml, - resetLinkFrom, - signInFully, - signInWithPassword, - totpCode, - wrongCode, -} from "./support"; +import { createTestUser, signInAsAdmin } from "./support"; -// Spec 0004, AC-14: every auth page, in its error states too, on desktop and phone. +// Spec 0004, AC-14, narrowed by spec 0019, AC-13 to the one admin auth page left: sign in, in +// its error states too, on desktop and phone. test("sign in works by keyboard alone", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); + const admin = await createTestUser(); await page.goto("/admin/sign-in"); await page.getByLabel("Email").focus(); await page.keyboard.type(admin.email); await page.keyboard.press("Tab"); - await expect(page.getByRole("link", { name: "Forgot password?" })).toBeFocused(); - await page.keyboard.press("Tab"); await expect(page.getByLabel("Password")).toBeFocused(); await page.keyboard.type(admin.password); + await page.keyboard.press("Tab"); + await expect(page.getByRole("button", { name: "Sign in" })).toBeFocused(); await page.keyboard.press("Enter"); - await expect(page).toHaveURL("/admin/mfa"); - await page.getByLabel("6 digit code").focus(); - await page.keyboard.type(await totpCode(admin.secret ?? "")); - await page.keyboard.press("Enter"); await expect(page).toHaveURL("/admin"); + await expect(page.getByText(`Signed in as ${admin.name}`)).toBeVisible(); }); test("sign in has no axe violations, empty and with errors", async ({ page }) => { @@ -49,68 +38,8 @@ test("sign in has no axe violations, empty and with errors", async ({ page }) => await expectNoA11yViolations(page); }); -test("forgot password has no axe violations", async ({ page }) => { - await page.goto("/admin/forgot-password"); - await expectNoA11yViolations(page); - - await page.getByLabel("Email").fill("nobody-here@example.com"); - await page.getByRole("button", { name: "Send reset link" }).click(); - await expect(page.getByText(/a reset link is on its way/)).toBeVisible(); - await expectNoA11yViolations(page); -}); - -test("the MFA views have no axe violations", async ({ page }) => { - const newAdmin = await createTestUser(); - await signInWithPassword(page, newAdmin); - await expectNoA11yViolations(page); - - await page.getByRole("button", { name: "Set up authenticator" }).click(); - await expect(page.getByRole("img", { name: /QR code/ })).toBeVisible(); - await expectNoA11yViolations(page); - - await enterCode(page, "000000", "Verify and continue"); - await expect(page.getByText("That code is incorrect.", { exact: false })).toBeVisible(); - await expectNoA11yViolations(page); -}); - -test("the MFA code view of an enrolled admin has no axe violations", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); - await signInWithPassword(page, admin); - await expect(page.getByLabel("6 digit code")).toBeVisible(); - await expectNoA11yViolations(page); - - await enterCode(page, await wrongCode(admin.secret ?? "")); - await expect(page.getByText("That code is incorrect.", { exact: false })).toBeVisible(); - await expectNoA11yViolations(page); -}); - -test("the MFA not set up view of a reset link session has no axe violations", async ({ page }) => { - const admin = await createTestUser(); - await page.goto("/admin/forgot-password"); - await page.getByLabel("Email").fill(admin.email); - await page.getByRole("button", { name: "Send reset link" }).click(); - await page.goto(resetLinkFrom(await latestEmailHtml(admin.email))); - await page.getByRole("button", { name: "Continue" }).click(); - - await expect( - page.getByText("Your authenticator is not set up. Contact the store owner."), - ).toBeVisible(); - await expectNoA11yViolations(page); -}); - -test("reset password has no axe violations, with errors", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); - await page.goto("/admin/reset-password"); - await expectNoA11yViolations(page); - - await page.getByRole("button", { name: "Save new password" }).click(); - await expect(page.getByText("Missing: at least 8 characters")).toBeVisible(); - await expectNoA11yViolations(page); -}); - test("the dashboard has no axe violations", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await expectNoA11yViolations(page); }); diff --git a/tests/e2e/admin/catalog-denials.spec.ts b/tests/e2e/admin/catalog-denials.spec.ts index 00f0eb7..5ee9662 100644 --- a/tests/e2e/admin/catalog-denials.spec.ts +++ b/tests/e2e/admin/catalog-denials.spec.ts @@ -1,7 +1,7 @@ import { expect, test } from "@playwright/test"; import { seedProduct, waitForEditor } from "../catalog/support"; -import { createTestUser, disableAdmin, injectSession, signInFully } from "./support"; +import { createTestUser, disableAdmin, injectSession, signInAsAdmin } from "./support"; // spec 0009, AC-22: every new admin page answers a non admin with the store's 404, and every // action checks the admin again, whatever the page showed. @@ -31,8 +31,8 @@ test("a signed out visitor is sent to sign in", async ({ page }) => { test("a disabled admin's catalog save changes nothing", async ({ page }) => { const product = await seedProduct({ stock: 1, label: "Disabled save" }); - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto(`/admin/products/${product.id}`); await waitForEditor(page, product.name); diff --git a/tests/e2e/admin/dashboard.spec.ts b/tests/e2e/admin/dashboard.spec.ts index 41fa222..77f3a52 100644 --- a/tests/e2e/admin/dashboard.spec.ts +++ b/tests/e2e/admin/dashboard.spec.ts @@ -2,7 +2,7 @@ import { expect, type Page, test } from "@playwright/test"; import { expectNoA11yViolations, tabTo } from "../a11y"; import { blockAppScripts } from "../no-js"; -import { createTestUser, signInFully, withDb } from "./support"; +import { createTestUser, signInAsAdmin, withDb } from "./support"; // spec 0015: the sales dashboard on /admin. The exact sums are proven in tests/db; here, the // page, its links, its states and its access. Other tests write orders at the same time, so the @@ -42,8 +42,8 @@ async function removeOrders(ids: readonly string[]) { } async function signedInAdmin(page: Page) { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); return admin; } diff --git a/tests/e2e/admin/denials.spec.ts b/tests/e2e/admin/denials.spec.ts index 21d7d6c..a916f32 100644 --- a/tests/e2e/admin/denials.spec.ts +++ b/tests/e2e/admin/denials.spec.ts @@ -1,6 +1,7 @@ import { expect, type Page, test } from "@playwright/test"; -import { createTestUser, disableAdmin, injectSession, signInFully } from "./support"; +import { seedProduct, waitForEditor } from "../catalog/support"; +import { createTestUser, disableAdmin, injectSession, signInAsAdmin } from "./support"; // Spec 0004, AC-6: anyone who is not an active admin gets the store's plain 404. async function expectNotFound(page: Page) { @@ -31,24 +32,20 @@ test("a signed in non admin gets a 404 on admin pages", async ({ page }) => { expect(mfa?.status()).toBe(404); await expectNotFound(page); await expectNotFoundTitle(page); - - const reset = await page.goto("/admin/reset-password"); - expect(reset?.status()).toBe(404); - await expectNotFoundTitle(page); }); test("a disabled admin gets a 404 on their next page and their next action", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); - await page.goto("/admin/reset-password"); - await expect(page.getByRole("heading", { name: "Choose a new password" })).toBeVisible(); + const product = await seedProduct({ stock: 1, label: "Disabled action" }); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); + await page.goto(`/admin/products/${product.id}`); + await waitForEditor(page, product.name); await disableAdmin(admin.id); // The action runs requireAdmin() itself, even though the page was rendered while active. - await page.getByLabel("New password").fill("Another-pass-9!"); - await page.getByLabel("Type it again").fill("Another-pass-9!"); - await page.getByRole("button", { name: "Save new password" }).click(); + await page.getByLabel("Name", { exact: true }).fill("Changed by a disabled admin"); + await page.getByRole("button", { name: "Save details" }).click(); await expectNotFound(page); const response = await page.goto("/admin"); diff --git a/tests/e2e/admin/discounts.spec.ts b/tests/e2e/admin/discounts.spec.ts index 85d5977..a8e1df4 100644 --- a/tests/e2e/admin/discounts.spec.ts +++ b/tests/e2e/admin/discounts.spec.ts @@ -6,15 +6,15 @@ import { expectNoA11yViolations, tabTo } from "../a11y"; import { seedProduct } from "../catalog/support"; import { seedCode, seedPaidOrderWithCode } from "../checkout/support"; import { blockAppScripts } from "../no-js"; -import { createTestUser, disableAdmin, injectSession, signInFully } from "./support"; +import { createTestUser, disableAdmin, injectSession, signInAsAdmin } from "./support"; // spec 0014, AC-10 to AC-14, AC-17 and AC-20: the promo code admin. Codes are shared by every // spec running at the same time, so each test searches for its own unique code. async function signedInAdmin(browser: Browser): Promise { - const admin = await createTestUser({ enrolled: true }); + const admin = await createTestUser(); const page = await (await browser.newContext()).newPage(); - await signInFully(page, admin); + await signInAsAdmin(page, admin); return page; } @@ -171,9 +171,9 @@ test("a signed in non admin gets a 404 on every discount page, and a signed out }); test("a disabled admin's code save changes nothing", async ({ browser }) => { - const admin = await createTestUser({ enrolled: true }); + const admin = await createTestUser(); const page = await (await browser.newContext()).newPage(); - await signInFully(page, admin); + await signInAsAdmin(page, admin); await openNewCode(page); await disableAdmin(admin.id); diff --git a/tests/e2e/admin/order-emails.spec.ts b/tests/e2e/admin/order-emails.spec.ts index 4baafb3..8b39eb9 100644 --- a/tests/e2e/admin/order-emails.spec.ts +++ b/tests/e2e/admin/order-emails.spec.ts @@ -3,7 +3,7 @@ import { expect, type Page, test } from "@playwright/test"; import { expectNoA11yViolations, tabTo } from "../a11y"; import { seedProduct } from "../catalog/support"; import { type SeededOrder, seedPendingOrder } from "../checkout/support"; -import { createTestUser, signInFully, withDb } from "./support"; +import { createTestUser, signInAsAdmin, withDb } from "./support"; // spec 0011, AC-13, AC-14 and AC-18: the order page's Emails section and Send again, by keyboard, // with no axe violations, on desktop and phone. The app runs with EMAIL_TRANSPORT=log, so a @@ -67,8 +67,8 @@ test("an admin sees the order's emails and sends a failed one again by keyboard" page, }) => { const order = await paidOrderWithEmails(); - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto(`/admin/orders/${order.number}`); await expect(page.getByRole("heading", { level: 2, name: "Emails" })).toBeVisible(); @@ -145,8 +145,8 @@ test("an order with no emails says so", async ({ page }) => { return cart.rows[0]!.id; }); const order = await seedPendingOrder(cartId, product, { quantity: 1, priceCents: 1500 }); - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto(`/admin/orders/${order.number}`); await expect(page.getByText("No emails sent for this order.")).toBeVisible(); diff --git a/tests/e2e/admin/orders.spec.ts b/tests/e2e/admin/orders.spec.ts index 4f98617..de77b4e 100644 --- a/tests/e2e/admin/orders.spec.ts +++ b/tests/e2e/admin/orders.spec.ts @@ -5,7 +5,7 @@ import { expectNoA11yViolations, tabTo } from "../a11y"; import { seedProduct, type SeededProduct } from "../catalog/support"; import { hasRealStripeKey, type SeededOrder, seedPendingOrder } from "../checkout/support"; import { blockAppScripts } from "../no-js"; -import { createTestUser, signInFully, withDb } from "./support"; +import { createTestUser, signInAsAdmin, withDb } from "./support"; // spec 0010: run an order after payment from the admin pages, by keyboard, with no axe // violations (AC-4 to AC-7, AC-19, AC-21, AC-24) and the list filters (AC-1 to AC-3). The refund @@ -90,8 +90,8 @@ test("an admin ships, delivers, undoes, edits tracking and notes an order by key }) => { const socks = await seedProduct({ stock: 5, priceCents: 1250 }); const order = await seedPaidOrder(socks, { quantity: 1, priceCents: 1250 }); - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await openOrder(page, order); await expectNoA11yViolations(page); @@ -139,8 +139,8 @@ test("an admin ships, delivers, undoes, edits tracking and notes an order by key test("the refund and cancel dialogs work by keyboard alone", async ({ page }) => { const socks = await seedProduct({ stock: 5, priceCents: 1000 }); const order = await seedPaidOrder(socks, { quantity: 2, priceCents: 1000 }); - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await openOrder(page, order); // AC-24: focus moves into the refund form; every field is reachable by Tab; an error is @@ -190,8 +190,8 @@ test("the orders list filters by number, keeps filters in the URL and has an emp }) => { const socks = await seedProduct({ stock: 5, priceCents: 900 }); const order = await seedPaidOrder(socks, { quantity: 1, priceCents: 900 }); - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto("/admin/orders"); await page.getByLabel("Order number, email or name").fill(String(order.number)); @@ -210,8 +210,8 @@ test("the orders list filters by number, keeps filters in the URL and has an emp // spec 0016: the filters are a GET form, so they work before the app's JavaScript loads. test("without the app's JavaScript the orders filters still filter", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await blockAppScripts(page); await page.goto("/admin/orders"); @@ -257,8 +257,8 @@ test("@stripe a partial refund with restock goes through Stripe", async ({ page priceCents: 1000, paymentIntentId: await realPayment(2000), }); - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await openOrder(page, order); await openByKeyboard(page, "Refund"); @@ -299,8 +299,8 @@ test("@stripe cancelling a pending order closes its open checkout first", async session.id, ]), ); - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await openOrder(page, order); await openByKeyboard(page, "Cancel order"); @@ -325,8 +325,8 @@ test("@stripe cancelling a paid order refunds it in full", async ({ page }) => { priceCents: 1500, paymentIntentId: await realPayment(1500), }); - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await openOrder(page, order); await openByKeyboard(page, "Cancel and refund"); diff --git a/tests/e2e/admin/password-reset.spec.ts b/tests/e2e/admin/password-reset.spec.ts deleted file mode 100644 index 5f01ebc..0000000 --- a/tests/e2e/admin/password-reset.spec.ts +++ /dev/null @@ -1,104 +0,0 @@ -import { expect, test } from "@playwright/test"; - -import { - createTestUser, - enterCode, - latestEmailHtml, - resetLinkFrom, - signInWithPassword, - totpCode, -} from "./support"; - -// Spec 0004, AC-9 and AC-10. The email comes from Mailpit on the local Supabase stack. -const sentMessage = "If that email belongs to an admin, a reset link is on its way."; - -async function requestReset(page: import("@playwright/test").Page, email: string) { - await page.goto("/admin/forgot-password"); - await page.getByLabel("Email").fill(email); - await page.getByRole("button", { name: "Send reset link" }).click(); - await expect(page.getByRole("status").filter({ hasText: sentMessage })).toBeVisible(); -} - -test("an admin resets their password with the email link and their TOTP code", async ({ - page, - request, -}) => { - const admin = await createTestUser({ enrolled: true }); - await requestReset(page, admin.email); - const link = resetLinkFrom(await latestEmailHtml(admin.email)); - - // A plain GET, like a mail scanner, spends nothing. - const scanned = await request.get(link, { maxRedirects: 0 }); - expect(scanned.status()).toBe(200); - - await page.goto(link); - await page.getByRole("button", { name: "Continue" }).click(); - await expect(page).toHaveURL("/admin/mfa?next=%2Fadmin%2Freset-password"); - await enterCode(page, await totpCode(admin.secret ?? "")); - - await expect(page).toHaveURL("/admin/reset-password"); - await page.getByLabel("New password").fill("short"); - await page.getByLabel("Type it again").fill("short"); - await page.getByRole("button", { name: "Save new password" }).click(); - for (const part of ["at least 8 characters", "an upper case letter", "a digit", "a symbol"]) { - await expect(page.getByText(`Missing: ${part}`)).toBeVisible(); - } - await expect(page.getByText("Missing: a lower case letter")).toHaveCount(0); - - await page.getByLabel("New password").fill(admin.password); - await page.getByLabel("Type it again").fill(admin.password); - await page.getByRole("button", { name: "Save new password" }).click(); - await expect(page.getByText("Choose a password you have not used here before.")).toBeVisible(); - - const newPassword = "Brand-new-pass-7"; - await page.getByLabel("New password").fill(newPassword); - await page.getByLabel("Type it again").fill(newPassword); - await page.getByRole("button", { name: "Save new password" }).click(); - await expect(page).toHaveURL("/admin"); - await expect(page.getByText(`Signed in as ${admin.name}`)).toBeVisible(); - - // The link works once. - await page.context().clearCookies(); - await page.goto(link); - await page.getByRole("button", { name: "Continue" }).click(); - await expect(page).toHaveURL("/admin/sign-in?reason=invalid_link"); - await expect( - page.getByText("That link is invalid or has expired. Request a new one."), - ).toBeVisible(); - - await signInWithPassword(page, { ...admin, password: newPassword }); -}); - -test("an unknown email gets the same reply and no email", async ({ page }) => { - const customer = await createTestUser({ admin: false }); - await requestReset(page, customer.email); - - // Give a send that should not happen time to land, then check Mailpit holds nothing. - await page.waitForTimeout(1_500); - const search = await fetch( - `http://127.0.0.1:55324/api/v1/search?query=${encodeURIComponent(`to:"${customer.email}"`)}`, - ); - expect(((await search.json()) as { messages_count: number }).messages_count).toBe(0); -}); - -test("an admin with no authenticator cannot set one up from a reset link", async ({ page }) => { - const admin = await createTestUser(); - await requestReset(page, admin.email); - const link = resetLinkFrom(await latestEmailHtml(admin.email)); - - await page.goto(link); - await page.getByRole("button", { name: "Continue" }).click(); - - await expect( - page.getByText("Your authenticator is not set up. Contact the store owner."), - ).toBeVisible(); - await expect(page.getByRole("button", { name: "Set up authenticator" })).toHaveCount(0); - await expect(page.getByRole("button", { name: "Sign out" })).toBeVisible(); -}); - -test("a tampered link lands on sign in with the invalid link message", async ({ page }) => { - await page.goto("/auth/confirm?token_hash=not-a-real-token&type=recovery"); - await page.getByRole("button", { name: "Continue" }).click(); - - await expect(page).toHaveURL("/admin/sign-in?reason=invalid_link"); -}); diff --git a/tests/e2e/admin/purged-order.spec.ts b/tests/e2e/admin/purged-order.spec.ts index 182b76b..e6eab0d 100644 --- a/tests/e2e/admin/purged-order.spec.ts +++ b/tests/e2e/admin/purged-order.spec.ts @@ -3,7 +3,7 @@ import { expect, test } from "@playwright/test"; import { expectNoA11yViolations, tabTo } from "../a11y"; import { seedProduct } from "../catalog/support"; import { seedPendingOrder } from "../checkout/support"; -import { createTestUser, signInFully, withDb } from "./support"; +import { createTestUser, signInAsAdmin, withDb } from "./support"; // spec 0008, AC-4 and AC-8: a real cron call purges an old expired order, and both admin pages // say its personal data was removed, by keyboard and with no axe violations. @@ -50,8 +50,8 @@ test("an expired order past retention shows its personal data as removed", async }); expect(purgedAt).not.toBeNull(); - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto("/admin/orders?view=all"); const row = page.getByRole("row", { name: new RegExp(`#${order.number}`) }); // Email and Ship to both say so. diff --git a/tests/e2e/admin/settings.spec.ts b/tests/e2e/admin/settings.spec.ts index 4c10d35..485b5a3 100644 --- a/tests/e2e/admin/settings.spec.ts +++ b/tests/e2e/admin/settings.spec.ts @@ -3,7 +3,7 @@ import { type Browser, expect, type Page, test } from "@playwright/test"; import { expectNoA11yViolations, tabTo } from "../a11y"; import { seedProduct } from "../catalog/support"; import { addToCart, fillCheckout, hasRealStripeKey } from "../checkout/support"; -import { createTestUser, injectSession, signInFully } from "./support"; +import { createTestUser, injectSession, signInAsAdmin } from "./support"; // spec 0007, AC-4, AC-7, AC-8, AC-10 and AC-16: the admin sets the delivery fee and the free // delivery threshold, and the cart and checkout follow on their next load. store_settings is one @@ -30,9 +30,9 @@ test.afterEach(async ({}, testInfo) => { }); async function signedInAdmin(browser: Browser): Promise { - const admin = await createTestUser({ enrolled: true }); + const admin = await createTestUser(); const page = await (await browser.newContext()).newPage(); - await signInFully(page, admin); + await signInAsAdmin(page, admin); return page; } diff --git a/tests/e2e/admin/sign-in.spec.ts b/tests/e2e/admin/sign-in.spec.ts index 1e0123a..d88ba0b 100644 --- a/tests/e2e/admin/sign-in.spec.ts +++ b/tests/e2e/admin/sign-in.spec.ts @@ -1,15 +1,9 @@ import { expect, test } from "@playwright/test"; -import { - authCookieNames, - createTestUser, - enterCode, - signInFully, - signInWithPassword, - totpCode, -} from "./support"; +import { authCookieNames, createTestUser, injectSession, signInAsAdmin } from "./support"; -// Spec 0004. Every test makes its own admin, so the tests run in parallel and on both projects. +// Spec 0004, with spec 0019: email and password only. Every test makes its own admin, so the +// tests run in parallel and on both projects. test("a signed out visitor is sent to sign in, keeping the page asked for", async ({ page }) => { await page.goto("/admin/orders?status=paid"); @@ -24,19 +18,12 @@ test("the storefront sets no auth cookie", async ({ page }) => { expect(await authCookieNames(page)).toEqual([]); }); -test("a new admin sets up TOTP, sees their name, and signs out", async ({ page }) => { +// spec 0019, AC-1 +test("an admin signs in with a password, sees their name, and signs out", async ({ page }) => { const admin = await createTestUser(); - await signInWithPassword(page, admin); - await expect(page.getByRole("heading", { name: "Set up your authenticator" })).toBeVisible(); - await page.getByRole("button", { name: "Set up authenticator" }).click(); - - await expect(page.getByRole("img", { name: /QR code/ })).toBeVisible(); - const secret = (await page.locator(".font-mono").first().innerText()).replaceAll(" ", ""); - await enterCode(page, await totpCode(secret), "Verify and continue"); - + await signInAsAdmin(page, admin); await expect(page).toHaveURL("/admin"); - await expect(page.getByText(`Signed in as ${admin.name}`)).toBeVisible(); await page.getByRole("button", { name: admin.name }).click(); await expect(page.getByRole("menu")).toContainText(admin.email); @@ -50,25 +37,17 @@ test("a new admin sets up TOTP, sees their name, and signs out", async ({ page } await expect(page.getByText(`Signed in as ${admin.name}`)).toHaveCount(0); }); -test("an enrolled admin verifies a code and returns to the page they asked for", async ({ - page, -}) => { - const admin = await createTestUser({ enrolled: true }); - - await signInWithPassword(page, admin, "/admin?from=link"); - await expect(page.getByRole("heading", { name: "Enter your code" })).toBeVisible(); - await enterCode(page, await totpCode(admin.secret ?? "")); +test("an admin returns to the page they asked for", async ({ page }) => { + const admin = await createTestUser(); + await signInAsAdmin(page, admin, "/admin?from=link"); await expect(page).toHaveURL("/admin?from=link"); - await expect(page.getByText(`Signed in as ${admin.name}`)).toBeVisible(); }); test("an unsafe next falls back to /admin", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); - - await signInWithPassword(page, admin, "//evil.example/admin"); - await enterCode(page, await totpCode(admin.secret ?? "")); + const admin = await createTestUser(); + await signInAsAdmin(page, admin, "//evil.example/admin"); await expect(page).toHaveURL("/admin"); }); @@ -76,7 +55,7 @@ test.describe("failed sign in looks the same whatever the cause", () => { const message = "Email or password is incorrect."; test("wrong password, unknown email and a non admin", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); + const admin = await createTestUser(); const customer = await createTestUser({ admin: false }); for (const [email, password] of [ @@ -110,86 +89,68 @@ test.describe("failed sign in looks the same whatever the cause", () => { }); }); -test.describe("MFA enforcement", () => { - test("a password only session cannot open the panel or the reset page", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); - await signInWithPassword(page, admin); - - await page.goto("/admin"); - await expect(page).toHaveURL("/admin/mfa?next=%2Fadmin"); - - await page.goto("/admin/reset-password"); - await expect(page).toHaveURL("/admin/mfa?next=%2Fadmin%2Freset-password"); - }); +// spec 0019, AC-5 +test("the sign in page offers no reset and ignores old reasons", async ({ page }) => { + await page.goto("/admin/sign-in?reason=too_many_codes"); - test("a wrong code is refused, then the right one works", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); - await signInWithPassword(page, admin); + await expect(page.getByRole("link", { name: /forgot/i })).toHaveCount(0); + await expect(page.getByText(/reset/i)).toHaveCount(0); + await expect(page.getByText("Too many incorrect codes", { exact: false })).toHaveCount(0); + // Next's route announcer is an empty alert; the form notice must be empty too. + await expect(page.getByRole("alert").filter({ hasText: /\S/ })).toHaveCount(0); +}); - await enterCode(page, "000000"); - await expect( - page.getByRole("alert").filter({ hasText: "That code is incorrect." }), - ).toBeVisible(); - await expect(page.getByLabel("6 digit code")).toBeFocused(); +// spec 0019, AC-4 +test.describe("the removed auth pages", () => { + const removed = ["/admin/mfa", "/admin/forgot-password", "/admin/reset-password"]; - await enterCode(page, await totpCode(admin.secret ?? "")); - await expect(page).toHaveURL("/admin"); + test("send a signed out visitor to sign in", async ({ page }) => { + for (const path of removed) { + await page.goto(path); + await expect(page).toHaveURL(`/admin/sign-in?next=${encodeURIComponent(path)}`); + } }); - test("a wrong code then a reload still enrolls with the QR code already scanned", async ({ - page, - }) => { + test("answer an admin with a 404", async ({ page }) => { const admin = await createTestUser(); - await signInWithPassword(page, admin); - await page.getByRole("button", { name: "Set up authenticator" }).click(); - const secret = (await page.locator(".font-mono").first().innerText()).replaceAll(" ", ""); - - await enterCode(page, "000000", "Verify and continue"); - await expect( - page.getByRole("alert").filter({ hasText: "That code is incorrect." }), - ).toBeVisible(); - - await page.reload(); - await expect(page.getByRole("heading", { name: "Already scanned the QR code?" })).toBeVisible(); - await enterCode(page, await totpCode(secret), "Verify and continue"); - - await expect(page).toHaveURL("/admin"); - }); - - test("5 wrong codes lock the MFA step, even after signing in again", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); - await signInWithPassword(page, admin); - - for (let attempt = 0; attempt < 5; attempt += 1) { - await enterCode(page, "000000"); - await expect( - page.getByRole("alert").filter({ hasText: "That code is incorrect." }), - ).toBeVisible(); + await injectSession(page, admin); + for (const path of removed) { + const response = await page.goto(path); + expect(response?.status(), path).toBe(404); } - await enterCode(page, "000000"); - await expect(page).toHaveURL("/admin/sign-in?reason=too_many_codes"); - await expect(page.getByText("Too many incorrect codes. Please sign in again.")).toBeVisible(); + }); - await signInWithPassword(page, admin); - await enterCode(page, await totpCode(admin.secret ?? "")); - await expect(page).toHaveURL("/admin/sign-in?reason=too_many_codes"); + test("/auth/confirm answers 404", async ({ page }) => { + const response = await page.goto("/auth/confirm?token_hash=abc&type=recovery"); + expect(response?.status()).toBe(404); }); }); -test.describe("signed in visitors skip the auth pages", () => { - test("at aal1 they go to the MFA step", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); - await signInWithPassword(page, admin); +// spec 0019, AC-2: an inbox alone never opens the panel. +test("an admin session opened by a recovery link is sent to sign in with a real redirect", async ({ + page, +}) => { + const admin = await createTestUser(); + await injectSession(page, admin, "recovery"); + + const response = await page.request.get("/admin/orders", { maxRedirects: 0 }); + expect(response.status()).toBeGreaterThanOrEqual(300); + expect(response.status()).toBeLessThan(400); + expect(response.headers().location).toBe("/admin/sign-in?next=%2Fadmin%2Forders"); - await page.goto("/admin/sign-in"); - await expect(page).toHaveURL("/admin/mfa"); - }); + await page.goto("/admin"); + await expect(page).toHaveURL("/admin/sign-in?next=%2Fadmin"); + await expect(page.getByText(`Signed in as ${admin.name}`)).toHaveCount(0); - test("at aal2 they go to the panel", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + // spec 0019, AC-6: the sign in page does not send that session on. + await page.goto("/admin/sign-in"); + await expect(page).toHaveURL("/admin/sign-in"); +}); - await page.goto("/admin/forgot-password"); - await expect(page).toHaveURL("/admin"); - }); +test("a signed in admin skips the sign in page", async ({ page }) => { + const admin = await createTestUser(); + await signInAsAdmin(page, admin); + + await page.goto("/admin/sign-in?next=%2Fadmin%2Forders"); + await expect(page).toHaveURL("/admin/orders"); }); diff --git a/tests/e2e/admin/support.ts b/tests/e2e/admin/support.ts index 90f348c..249c738 100644 --- a/tests/e2e/admin/support.ts +++ b/tests/e2e/admin/support.ts @@ -4,7 +4,6 @@ import { expect, type Page } from "@playwright/test"; import { createServerClient } from "@supabase/ssr"; import { createClient } from "@supabase/supabase-js"; import { config } from "dotenv"; -import * as OTPAuth from "otpauth"; import pg from "pg"; // Real local Supabase, never a mock (spec 0004): every test makes its own admin with a unique @@ -39,8 +38,6 @@ export type TestUser = { readonly email: string; readonly name: string; readonly password: string; - // Base32 TOTP secret, when a factor is enrolled. - readonly secret: string | null; }; export async function withDb(run: (client: pg.Client) => Promise): Promise { @@ -56,11 +53,10 @@ export async function withDb(run: (client: pg.Client) => Promise): Promise type CreateOptions = { // An auth user with no admin_users row, such as a future customer. readonly admin?: boolean; - readonly enrolled?: boolean; }; export async function createTestUser(options: CreateOptions = {}): Promise { - const { admin = true, enrolled = false } = options; + const { admin = true } = options; const email = `e2e-${randomUUID()}@example.com`; const name = `Admin ${email.slice(4, 10)}`; const { data, error } = await serviceClient.auth.admin.createUser({ @@ -76,64 +72,19 @@ export async function createTestUser(options: CreateOptions = {}): Promise { - const client = createClient(supabaseUrl, anonKey, { - auth: { persistSession: false, autoRefreshToken: false }, - }); - const signIn = await client.auth.signInWithPassword({ email, password: testPassword }); - if (signIn.error) throw signIn.error; - const enroll = await client.auth.mfa.enroll({ factorType: "totp", friendlyName: "e2e" }); - if (enroll.error) throw enroll.error; - const verify = await client.auth.mfa.challengeAndVerify({ - factorId: enroll.data.id, - code: await totpCode(enroll.data.totp.secret), - }); - if (verify.error) throw verify.error; - await client.auth.signOut({ scope: "local" }); - return enroll.data.totp.secret; + return { id, email, name, password: testPassword }; } export async function disableAdmin(id: string) { await withDb((db) => db.query("UPDATE admin_users SET disabled_at = now() WHERE id = $1", [id])); } -// A current code, never one about to expire: with under 5 seconds left in the 30 second step, -// it waits for the next step so the server does not see a stale code. -export async function totpCode(secret: string): Promise { - const remainingMs = 30_000 - (Date.now() % 30_000); - if (remainingMs < 5_000) await new Promise((resolve) => setTimeout(resolve, remainingMs + 250)); - return new OTPAuth.TOTP({ secret: OTPAuth.Secret.fromBase32(secret) }).generate(); -} - -// Any code but the current one. -export async function wrongCode(secret: string): Promise { - const right = await totpCode(secret); - return right === "000000" ? "111111" : "000000"; -} - -export async function signInWithPassword(page: Page, user: TestUser, next?: string) { +// Email and password: an admin lands in the panel, on `next` when it is safe (spec 0019, AC-1). +export async function signInAsAdmin(page: Page, user: TestUser, next?: string) { await page.goto(next ? `/admin/sign-in?next=${encodeURIComponent(next)}` : "/admin/sign-in"); await page.getByLabel("Email").fill(user.email); await page.getByLabel("Password").fill(user.password); await page.getByRole("button", { name: "Sign in" }).click(); - await expect(page).toHaveURL(/\/admin\/mfa/); -} - -export async function enterCode(page: Page, code: string, submit = "Verify") { - await page.getByLabel("6 digit code").fill(code); - await page.getByRole("button", { name: submit, exact: true }).click(); -} - -// Password plus TOTP: the admin lands in the panel (aal2). -export async function signInFully(page: Page, user: TestUser) { - if (!user.secret) throw new Error("signInFully needs an enrolled admin."); - await signInWithPassword(page, user); - await enterCode(page, await totpCode(user.secret)); await expect(page.getByText(`Signed in as ${user.name}`)).toBeVisible(); } @@ -165,15 +116,22 @@ export async function latestEmailHtml(to: string): Promise { return html; } -export function resetLinkFrom(html: string): string { - const match = html.match(/href="([^"]*\/auth\/confirm\?[^"]*)"/); - if (!match?.[1]) throw new Error("No reset link in the email."); - return match[1].replaceAll("&", "&"); +// A recovery token made outside the app, as an old or hand made link would carry: generateLink +// sends no email, so the Send Email Hook never sees it. +export async function recoveryTokenHash(email: string): Promise { + const { data, error } = await serviceClient.auth.admin.generateLink({ type: "recovery", email }); + if (error) throw error; + return data.properties.hashed_token; } // A browser session for someone who never passes the sign in form: signs in through -// @supabase/ssr itself and hands its cookies to the page's context. -export async function injectSession(page: Page, user: TestUser) { +// @supabase/ssr itself and hands its cookies to the page's context. `via: "recovery"` opens it +// by verifying a recovery link instead, as an inbox alone could (spec 0019, AC-2). +export async function injectSession( + page: Page, + user: TestUser, + via: "password" | "recovery" = "password", +) { const jar: { name: string; value: string }[] = []; const client = createServerClient(supabaseUrl, anonKey, { cookies: { @@ -187,10 +145,13 @@ export async function injectSession(page: Page, user: TestUser) { }, }, }); - const { error } = await client.auth.signInWithPassword({ - email: user.email, - password: user.password, - }); + const { error } = + via === "password" + ? await client.auth.signInWithPassword({ email: user.email, password: user.password }) + : await client.auth.verifyOtp({ + type: "recovery", + token_hash: await recoveryTokenHash(user.email), + }); if (error) throw error; await page .context() diff --git a/tests/e2e/catalog/a11y.spec.ts b/tests/e2e/catalog/a11y.spec.ts index b1c8705..fcb1a0c 100644 --- a/tests/e2e/catalog/a11y.spec.ts +++ b/tests/e2e/catalog/a11y.spec.ts @@ -1,7 +1,7 @@ import { expect, type Page, test } from "@playwright/test"; import { expectNoA11yViolations } from "../a11y"; -import { createTestUser, signInFully } from "../admin/support"; +import { createTestUser, signInAsAdmin } from "../admin/support"; import { adminProductUrl, seedProduct, seedProductWithOptions, setStock } from "./support"; // Spec 0005, AC-17: axe on every page of the buy loop, in its states, on desktop and phone @@ -78,8 +78,8 @@ test("a customer picks, adds and changes the cart by keyboard alone", async ({ p }); test("the admin product pages have no axe violations, errors included", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto("/admin/products"); await expect(page.getByRole("heading", { level: 1, name: "Products" })).toBeVisible(); @@ -95,8 +95,8 @@ test("the admin product pages have no axe violations, errors included", async ({ }); test("an admin creates a product by keyboard alone", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto("/admin/products/new"); const name = `Keyboard tote ${Date.now()}`; diff --git a/tests/e2e/catalog/buy-loop.spec.ts b/tests/e2e/catalog/buy-loop.spec.ts index 417ec42..ab5d24c 100644 --- a/tests/e2e/catalog/buy-loop.spec.ts +++ b/tests/e2e/catalog/buy-loop.spec.ts @@ -3,7 +3,7 @@ import { randomUUID } from "node:crypto"; import { expect, type Page, test } from "@playwright/test"; import { brand } from "../../../src/lib/brand/brand"; -import { createTestUser, signInFully } from "../admin/support"; +import { createTestUser, signInAsAdmin } from "../admin/support"; import { adminProductUrl } from "./support"; // Spec 0005, milestone 1: one real product from the admin form to a cart that survives reload. @@ -15,8 +15,8 @@ async function createProductAsAdmin( product: NewProduct, button: "Publish" | "Save as draft", ) { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto("/admin/products"); await page.getByRole("link", { name: "New product" }).first().click(); diff --git a/tests/e2e/catalog/categories-arrange.spec.ts b/tests/e2e/catalog/categories-arrange.spec.ts index b29628d..5e7c2c2 100644 --- a/tests/e2e/catalog/categories-arrange.spec.ts +++ b/tests/e2e/catalog/categories-arrange.spec.ts @@ -3,7 +3,7 @@ import { randomUUID } from "node:crypto"; import { expect, type Page, test } from "@playwright/test"; import { expectNoA11yViolations } from "../a11y"; -import { createTestUser, signInFully, withDb } from "../admin/support"; +import { createTestUser, signInAsAdmin, withDb } from "../admin/support"; import { blockAppScripts } from "../no-js"; import { seedProduct, setProductStatus, waitForEditor } from "./support"; @@ -24,8 +24,8 @@ async function seedCategory(): Promise<{ id: string }> { } async function signIn(page: Page) { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); } test("an admin creates a category, adds a product from both sides and deletes it", async ({ diff --git a/tests/e2e/catalog/discovery.spec.ts b/tests/e2e/catalog/discovery.spec.ts index 29fa759..c1d1643 100644 --- a/tests/e2e/catalog/discovery.spec.ts +++ b/tests/e2e/catalog/discovery.spec.ts @@ -3,7 +3,7 @@ import { randomUUID } from "node:crypto"; import { expect, type Page, test } from "@playwright/test"; import { expectNoA11yViolations, tabTo } from "../a11y"; -import { createTestUser, signInFully, withDb } from "../admin/support"; +import { createTestUser, signInAsAdmin, withDb } from "../admin/support"; import { blockAppScripts } from "../no-js"; import { seedProduct, type SeededProduct } from "./support"; @@ -97,8 +97,8 @@ async function seedCategory(products: readonly SeededProduct[], description = "" } async function signIn(page: Page) { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); } // The admin action that expires the cached reads: add `last` from the category page. diff --git a/tests/e2e/catalog/edit.spec.ts b/tests/e2e/catalog/edit.spec.ts index f94dc8b..76e4f53 100644 --- a/tests/e2e/catalog/edit.spec.ts +++ b/tests/e2e/catalog/edit.spec.ts @@ -1,15 +1,15 @@ import { expect, type Page, test } from "@playwright/test"; import { expectNoA11yViolations } from "../a11y"; -import { createTestUser, signInFully } from "../admin/support"; +import { createTestUser, signInAsAdmin } from "../admin/support"; import { seedProduct, waitForEditor } from "./support"; // spec 0009, milestone 1: one edit from the admin editor to the storefront, and the status // buttons that take a product off the storefront and put it back first in line. -async function signInAsAdmin(page: Page) { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); +async function signInAsNewAdmin(page: Page) { + const admin = await createTestUser(); + await signInAsAdmin(page, admin); } async function openEditor(page: Page, product: { readonly id: string; readonly name: string }) { @@ -25,7 +25,7 @@ test("an admin renames a live product and the storefront follows on its next req browser, }) => { const product = await seedProduct({ stock: 4, label: "Canvas tote" }); - await signInAsAdmin(page); + await signInAsNewAdmin(page); await openEditor(page, product); const renamed = `${product.name} renamed`; @@ -49,11 +49,11 @@ test("a second editor's Details save is refused instead of overwriting the first browser, }) => { const product = await seedProduct({ stock: 1, label: "Clay bowl" }); - await signInAsAdmin(page); + await signInAsNewAdmin(page); await openEditor(page, product); const other = await (await browser.newContext()).newPage(); - await signInAsAdmin(other); + await signInAsNewAdmin(other); await openEditor(other, product); await other.getByLabel("Name", { exact: true }).fill(`${product.name} first`); await other.getByRole("button", { name: "Save details" }).click(); @@ -75,7 +75,7 @@ test("hiding takes a product off the storefront and publishing puts it back firs const product = await seedProduct({ stock: 2, label: "Oak spoon" }); // Another live product, so "first" means something. await seedProduct({ stock: 2, label: "Birch spoon" }); - await signInAsAdmin(page); + await signInAsNewAdmin(page); await openEditor(page, product); await page.getByRole("button", { name: "Hide" }).click(); @@ -101,7 +101,7 @@ test("hiding takes a product off the storefront and publishing puts it back firs test("archive and restore never put a product straight back live", async ({ page }) => { const product = await seedProduct({ stock: 2, label: "Linen apron" }); - await signInAsAdmin(page); + await signInAsNewAdmin(page); await openEditor(page, product); await page.getByRole("button", { name: "Archive", exact: true }).click(); @@ -123,7 +123,7 @@ test("the edit page has no axe violations and an unknown id shows the not found page, }) => { const product = await seedProduct({ stock: 2, label: "Wax candle" }); - await signInAsAdmin(page); + await signInAsNewAdmin(page); await page.goto(`/admin/products/${product.id}`); await waitForEditor(page, product.name); await expectNoA11yViolations(page); diff --git a/tests/e2e/catalog/image.spec.ts b/tests/e2e/catalog/image.spec.ts index 1f39971..337f4aa 100644 --- a/tests/e2e/catalog/image.spec.ts +++ b/tests/e2e/catalog/image.spec.ts @@ -3,7 +3,7 @@ import path from "node:path"; import { expect, test } from "@playwright/test"; -import { createTestUser, signInFully } from "../admin/support"; +import { createTestUser, signInAsAdmin } from "../admin/support"; import { adminProductUrl } from "./support"; // Spec 0005, AC-5 and AC-6 (spec 0009 makes it up to 8): images go straight to Storage, each @@ -16,8 +16,8 @@ test("an admin uploads a product image that the storefront shows with its alt te browser, }) => { const name = `Stoneware mug ${randomUUID().slice(0, 8)}`; - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto("/admin/products/new"); await page.getByLabel("Name", { exact: true }).fill(name); @@ -54,8 +54,8 @@ test("an admin uploads a product image that the storefront shows with its alt te }); test("a file that is not an image is refused before any upload", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto("/admin/products/new"); await page.getByLabel("Add an image").setInputFiles({ diff --git a/tests/e2e/catalog/images-markdown.spec.ts b/tests/e2e/catalog/images-markdown.spec.ts index d61cdd2..e870566 100644 --- a/tests/e2e/catalog/images-markdown.spec.ts +++ b/tests/e2e/catalog/images-markdown.spec.ts @@ -3,7 +3,7 @@ import path from "node:path"; import { expect, type Page, test } from "@playwright/test"; import { expectNoA11yViolations } from "../a11y"; -import { createTestUser, signInFully } from "../admin/support"; +import { createTestUser, signInAsAdmin } from "../admin/support"; import { seedProduct, seedProductWithOptions, type SeededProduct, waitForEditor } from "./support"; // spec 0009, milestone 3: several images per product, ordered by keyboard, tied to an option @@ -12,8 +12,8 @@ import { seedProduct, seedProductWithOptions, type SeededProduct, waitForEditor const fixture = path.resolve("tests/e2e/fixtures/product.png"); async function openEditor(page: Page, product: SeededProduct) { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto(`/admin/products/${product.id}`); await waitForEditor(page, product.name); } diff --git a/tests/e2e/catalog/variants-stock.spec.ts b/tests/e2e/catalog/variants-stock.spec.ts index c17fc36..8729403 100644 --- a/tests/e2e/catalog/variants-stock.spec.ts +++ b/tests/e2e/catalog/variants-stock.spec.ts @@ -1,7 +1,7 @@ import { expect, type Page, test } from "@playwright/test"; import { expectNoA11yViolations } from "../a11y"; -import { createTestUser, signInFully } from "../admin/support"; +import { createTestUser, signInAsAdmin } from "../admin/support"; import { seedProduct, seedProductWithOptions, @@ -14,8 +14,8 @@ import { // storefront, and a new option value from the editor. async function openEditor(page: Page, product: SeededProduct) { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto(`/admin/products/${product.id}`); await waitForEditor(page, product.name); return admin; diff --git a/tests/e2e/catalog/variants.spec.ts b/tests/e2e/catalog/variants.spec.ts index 5b7abf8..b4ccfd6 100644 --- a/tests/e2e/catalog/variants.spec.ts +++ b/tests/e2e/catalog/variants.spec.ts @@ -2,14 +2,14 @@ import { randomUUID } from "node:crypto"; import { expect, type Page, test } from "@playwright/test"; -import { createTestUser, signInFully } from "../admin/support"; +import { createTestUser, signInAsAdmin } from "../admin/support"; import { adminProductUrl } from "./support"; // Spec 0005, milestone 2: options generate the variant grid, and the product page picks one. -async function signInAsAdmin(page: Page) { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); +async function openNewProduct(page: Page) { + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto("/admin/products/new"); } @@ -26,7 +26,7 @@ test("options build a variant grid that keeps typed rows, and the picker follows page, }) => { const name = `Merino crew ${randomUUID().slice(0, 8)}`; - await signInAsAdmin(page); + await openNewProduct(page); await page.getByLabel("Name", { exact: true }).fill(name); await addOption(page, 0, "Size", ["S", "M"]); await addOption(page, 1, "Color", ["Navy"]); @@ -73,7 +73,7 @@ test("options build a variant grid that keeps typed rows, and the picker follows }); test("the form shows each refusal on its field", async ({ page }) => { - await signInAsAdmin(page); + await openNewProduct(page); await page.getByLabel("Name", { exact: true }).fill("Bad input"); await page.getByLabel("URL name").fill("Bad Slug"); await page.getByLabel("Price for the product", { exact: true }).fill("1.999"); @@ -100,7 +100,7 @@ test("the form shows each refusal on its field", async ({ page }) => { }); test("a URL name another product uses is refused by the server", async ({ page }) => { - await signInAsAdmin(page); + await openNewProduct(page); const name = `Twin ${randomUUID().slice(0, 8)}`; for (const attempt of [1, 2]) { if (attempt === 2) await page.goto("/admin/products/new"); diff --git a/tests/e2e/checkout/delivery.spec.ts b/tests/e2e/checkout/delivery.spec.ts index 3f46252..cdd32d5 100644 --- a/tests/e2e/checkout/delivery.spec.ts +++ b/tests/e2e/checkout/delivery.spec.ts @@ -1,7 +1,7 @@ import { expect, test } from "@playwright/test"; import { expectNoA11yViolations, tabTo } from "../a11y"; -import { createTestUser, signInFully } from "../admin/support"; +import { createTestUser, signInAsAdmin } from "../admin/support"; import { seedProduct } from "../catalog/support"; import { addToCart, @@ -144,9 +144,9 @@ test("a paid order shows where it goes, to the customer and to the admin", async await expect(page.getByText("€22.40")).toBeVisible(); await expectNoA11yViolations(page); - const admin = await createTestUser({ enrolled: true }); + const admin = await createTestUser(); const adminPage = await (await browser.newContext()).newPage(); - await signInFully(adminPage, admin); + await signInAsAdmin(adminPage, admin); await adminPage.goto("/admin/orders"); const row = adminPage.getByRole("row", { name: new RegExp(`#${order.number}`) }); await expect(row).toContainText(`${testAddress.fullName}, ${testAddress.city}`); @@ -187,9 +187,9 @@ test("an order made before delivery existed says no address was recorded", async ).toBeVisible(); await expect(page.getByRole("heading", { name: "Delivering to" })).toHaveCount(0); - const admin = await createTestUser({ enrolled: true }); + const admin = await createTestUser(); const adminPage = await (await browser.newContext()).newPage(); - await signInFully(adminPage, admin); + await signInAsAdmin(adminPage, admin); await adminPage.goto("/admin/orders"); await expect(adminPage.getByRole("row", { name: new RegExp(`#${order.number}`) })).toContainText( "Not recorded", diff --git a/tests/e2e/checkout/payment.spec.ts b/tests/e2e/checkout/payment.spec.ts index e3f0c97..ba3bb29 100644 --- a/tests/e2e/checkout/payment.spec.ts +++ b/tests/e2e/checkout/payment.spec.ts @@ -1,7 +1,7 @@ import { expect, test } from "@playwright/test"; import { expectNoA11yViolations, tabTo } from "../a11y"; -import { createTestUser, injectSession, signInFully } from "../admin/support"; +import { createTestUser, injectSession, signInAsAdmin } from "../admin/support"; import { seedProduct } from "../catalog/support"; import { addToCart, @@ -88,9 +88,9 @@ test("a paid webhook turns the checkout into a paid order the customer and admin await expect(page.getByRole("link", { name: "Cart", exact: true })).toBeVisible(); await expectNoA11yViolations(page); - const admin = await createTestUser({ enrolled: true }); + const admin = await createTestUser(); const adminPage = await (await browser.newContext()).newPage(); - await signInFully(adminPage, admin); + await signInAsAdmin(adminPage, admin); await adminPage.goto("/admin/orders"); await expect(adminPage.getByRole("heading", { level: 1, name: "Orders" })).toBeVisible(); const row = adminPage.getByRole("row", { name: new RegExp(`#${order.number}`) }); @@ -169,9 +169,9 @@ test("the admin order pages work by keyboard alone", async ({ page, browser, req }); expect((await postPaidEvent(request, order)).status()).toBe(200); - const admin = await createTestUser({ enrolled: true }); + const admin = await createTestUser(); const adminPage = await (await browser.newContext()).newPage(); - await signInFully(adminPage, admin); + await signInAsAdmin(adminPage, admin); await adminPage.goto("/admin/orders"); await expect(adminPage.getByRole("heading", { level: 1, name: "Orders" })).toBeVisible(); diff --git a/tests/e2e/legal/legal-pages.spec.ts b/tests/e2e/legal/legal-pages.spec.ts index f3824e8..5f9975e 100644 --- a/tests/e2e/legal/legal-pages.spec.ts +++ b/tests/e2e/legal/legal-pages.spec.ts @@ -3,7 +3,7 @@ import { randomUUID } from "node:crypto"; import { expect, type Page, type Request, test } from "@playwright/test"; import { expectNoA11yViolations } from "../a11y"; -import { createTestUser, signInFully, withDb } from "../admin/support"; +import { createTestUser, signInAsAdmin, withDb } from "../admin/support"; import { seedProduct } from "../catalog/support"; import { addToCart, @@ -90,8 +90,8 @@ test("printing a legal page prints only its text", async ({ page }) => { }); test("the admin panel shows no legal footer and no consent banner", async ({ page }) => { - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto("/admin"); await expect(page.getByRole("heading", { level: 1 })).toBeVisible(); @@ -231,9 +231,9 @@ test("a ticked Pay records the terms version on the order, shown to the admin", accepted: true, }); - const admin = await createTestUser({ enrolled: true }); + const admin = await createTestUser(); const adminPage = await (await browser.newContext()).newPage(); - await signInFully(adminPage, admin); + await signInAsAdmin(adminPage, admin); await adminPage.goto(`/admin/orders/${row.rows[0]!.number}`); await expect(adminPage.getByText(/^version \w+ \d{1,2}, \d{4}, on /)).toBeVisible(); }); @@ -249,9 +249,9 @@ test("an order from before the feature reads 'not recorded' on the admin page", priceCents: 1250, }); - const admin = await createTestUser({ enrolled: true }); + const admin = await createTestUser(); const adminPage = await (await browser.newContext()).newPage(); - await signInFully(adminPage, admin); + await signInAsAdmin(adminPage, admin); await adminPage.goto(`/admin/orders/${order.number}`); const details = adminPage.locator("dl").filter({ hasText: "Terms accepted" }); await expect(details.getByText("not recorded")).toBeVisible(); diff --git a/tests/e2e/seo/seo.spec.ts b/tests/e2e/seo/seo.spec.ts index 5cb65f2..3e0e608 100644 --- a/tests/e2e/seo/seo.spec.ts +++ b/tests/e2e/seo/seo.spec.ts @@ -4,7 +4,7 @@ import { expect, type Page, test } from "@playwright/test"; import { brand } from "../../../src/lib/brand/brand"; import { seedPaidOrder, signInAsCustomer } from "../account/support"; -import { createTestUser, signInFully, testPassword, withDb } from "../admin/support"; +import { createTestUser, signInAsAdmin, testPassword, withDb } from "../admin/support"; import { type SeededProduct, seedProduct, @@ -315,8 +315,8 @@ test.describe("listings", () => { ]); return row.rows[0]!.id; }); - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto(`/admin/categories/${category}?q=${encodeURIComponent(last.name)}`); await page.getByRole("button", { name: `Add ${last.name}` }).click(); await expect( @@ -411,8 +411,8 @@ test.describe("site files", () => { expect(before).toContain(`${siteUrl}/terms`); expect(before).not.toMatch(/\/(search|admin|cart|checkout|account|auth)\b/); - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); await page.goto(`/admin/products/${product.id}`); await waitForEditor(page, product.name); await page.getByRole("button", { name: "Publish" }).click(); @@ -451,8 +451,8 @@ test.describe("site files", () => { const sitemap = async () => (await request.get("/sitemap.xml")).text(); expect(await sitemap()).not.toContain(loc); - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); const visible = page.getByRole("checkbox", { name: "Visible" }); const setVisible = async (checked: boolean) => { await page.goto(`/admin/categories/${id}`); @@ -475,8 +475,8 @@ async function expectPrivateHead(page: Page) { expect(await jsonLd(page)).toEqual([]); } -// AC-22: the private pages keep their noindex and gain nothing. Signed out, /account/set-password, -// /admin/reset-password and /auth/confirm redirect to a sign in page listed here. +// AC-22: the private pages keep their noindex and gain nothing. Signed out, /account/set-password +// redirects to a sign in page listed here. test.describe("a private page", () => { for (const [path, landsOn = path] of [ ["/cart"], @@ -488,7 +488,6 @@ test.describe("a private page", () => { ["/account/forgot-password"], ["/account/confirm"], ["/admin/sign-in"], - ["/admin/forgot-password"], ] as const) { test(`${path} stays noindex with no canonical or JSON-LD`, async ({ page }) => { await page.goto(path); @@ -518,8 +517,8 @@ test.describe("a private page", () => { test("every admin page stays noindex", async ({ page }) => { // Ten pages in one test; on a cold dev server each compiles on its first visit. test.slow(); - const admin = await createTestUser({ enrolled: true }); - await signInFully(page, admin); + const admin = await createTestUser(); + await signInAsAdmin(page, admin); for (const path of [ "/admin", diff --git a/tests/stack/admin-amr.stack.test.ts b/tests/stack/admin-amr.stack.test.ts new file mode 100644 index 0000000..2bac9c7 --- /dev/null +++ b/tests/stack/admin-amr.stack.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, it } from "vitest"; + +import { decideAdminAccess } from "@/features/admin-auth/access"; +import { decideAdminGate } from "@/features/admin-auth/proxy-decision"; +import { anonClient, serviceClient, stackPassword, uniqueEmail } from "./stack"; + +// spec 0019, AC-2: the whole password only rule rests on what Supabase writes into `amr`. A +// password sign in records a `password` entry; a session made by verifying a recovery link +// records none. A Supabase upgrade that changes this fails here, not silently in the guard. + +async function claimsOf(client: ReturnType) { + const { data, error } = await client.auth.getClaims(); + if (error || !data) throw error ?? new Error("no claims"); + return data.claims; +} + +async function newUser() { + const email = uniqueEmail(); + const { data, error } = await serviceClient().auth.admin.createUser({ + email, + password: stackPassword, + email_confirm: true, + }); + if (error) throw error; + return { email, id: data.user.id }; +} + +const adminRow = (id: string, email: string) => ({ id, email, name: "Ada", disabledAt: null }); + +describe("amr", () => { + it("has a password entry after a password sign in, which the guard allows", async () => { + const { email, id } = await newUser(); + const client = anonClient(); + await client.auth.signInWithPassword({ email, password: stackPassword }); + + const claims = await claimsOf(client); + expect(JSON.stringify(claims.amr)).toContain('"password"'); + const verdict = decideAdminAccess({ + userId: id, + amr: claims.amr, + adminRow: adminRow(id, email), + nowMs: Date.now(), + }); + expect(verdict.kind).toBe("allow"); + }); + + it("has no password entry after a recovery link, which the guard and proxy send to sign in", async () => { + const { email, id } = await newUser(); + const link = await serviceClient().auth.admin.generateLink({ type: "recovery", email }); + if (link.error) throw link.error; + const client = anonClient(); + const { error } = await client.auth.verifyOtp({ + type: "recovery", + token_hash: link.data.properties.hashed_token, + }); + expect(error).toBeNull(); + + const claims = await claimsOf(client); + expect(JSON.stringify(claims.amr)).not.toContain('"password"'); + const input = { userId: id, amr: claims.amr, adminRow: adminRow(id, email), nowMs: Date.now() }; + expect(decideAdminAccess(input)).toEqual({ kind: "sign-in" }); + expect( + decideAdminGate({ + pathname: "/admin", + search: "", + method: "GET", + isServerAction: false, + claims, + nowMs: Date.now(), + adminRow: adminRow(id, email), + }), + ).toEqual({ kind: "redirect", to: "/admin/sign-in?next=%2Fadmin" }); + }); +}); diff --git a/tests/stack/admin-scripts.stack.test.ts b/tests/stack/admin-scripts.stack.test.ts index 58fc8b9..08ee8cd 100644 --- a/tests/stack/admin-scripts.stack.test.ts +++ b/tests/stack/admin-scripts.stack.test.ts @@ -1,21 +1,27 @@ +import { spawnSync } from "node:child_process"; + import { afterAll, describe, expect, it } from "vitest"; -import { - connect, - createAdmin, - disableAdmin, - findAuthUserId, - resetAdminMfa, -} from "../../scripts/admin/lib"; +import { connect, createAdmin, disableAdmin, findAuthUserId } from "../../scripts/admin/lib"; import { anonClient, serviceClient, stackEnv, stackPassword, uniqueEmail } from "./stack"; -// Spec 0004, AC-11: the admin:create, admin:disable and admin:reset-mfa cores against the -// local stack. The CLIs around them only read flags and prompt for the password. +// Spec 0004, AC-11 and spec 0019, AC-7 and AC-15: the admin:create and admin:disable cores, and +// the whole admin:reset-password script, against the local stack. stackEnv(); const deps = connect(undefined); afterAll(() => deps.close()); +// The script as an operator runs it, the two typed passwords piped in (its prompt reads lines). +function resetPassword(email: string, passwords: readonly string[]) { + const result = spawnSync( + "node_modules/.bin/tsx", + ["scripts/admin/reset-password.ts", "--email", email], + { input: passwords.map((p) => `${p}\n`).join(""), encoding: "utf8", env: process.env }, + ); + return { status: result.status, stdout: result.stdout, stderr: result.stderr.trim() }; +} + describe("createAdmin", () => { it("creates a confirmed auth user and the admin row, email lower cased", async () => { const email = uniqueEmail(); @@ -38,28 +44,34 @@ describe("createAdmin", () => { expect(error).toBeNull(); }); - it("keeps an existing user's password, confirms the email, and re-enables a disabled row", async () => { + it("sets a new password on an existing user, confirms the email, and re-enables a disabled row", async () => { const email = uniqueEmail(); - const customerPassword = "Customer-pass-1"; + // A password planted through public sign up on an unconfirmed user (spec 0019, AC-15). + const plantedPassword = "Planted-pass-1"; const created = await serviceClient().auth.admin.createUser({ email, - password: customerPassword, + password: plantedPassword, email_confirm: false, }); const userId = created.data.user?.id; - const first = await createAdmin(deps, { email, name: "Grace", password: null }); + const first = await createAdmin(deps, { email, name: "Grace", password: stackPassword }); expect(first).toEqual({ id: userId, existingUser: true }); await disableAdmin(deps, email); - await createAdmin(deps, { email, name: "Grace Hopper", password: null }); + await createAdmin(deps, { email, name: "Grace Hopper", password: stackPassword }); expect(await deps.db.adminUser.findUnique({ where: { id: first.id } })).toMatchObject({ name: "Grace Hopper", disabledAt: null, }); + const planted = await anonClient().auth.signInWithPassword({ + email, + password: plantedPassword, + }); + expect(planted.error).not.toBeNull(); const { error } = await anonClient().auth.signInWithPassword({ email, - password: customerPassword, + password: stackPassword, }); expect(error).toBeNull(); expect(await findAuthUserId(deps.db, email.toUpperCase())).toBe(first.id); @@ -74,25 +86,96 @@ describe("disableAdmin", () => { await disableAdmin(deps, email); const row = await deps.db.adminUser.findUnique({ where: { id } }); expect(row?.disabledAt).toBeInstanceOf(Date); - await expect(disableAdmin(deps, uniqueEmail())).rejects.toThrow("No admin has that email."); + expect(await disableAdmin(deps, uniqueEmail())).toBeNull(); }); }); -describe("resetAdminMfa", () => { - it("removes every factor and every session", async () => { +describe("admin:reset-password", () => { + const newPassword = "Another-horse-42"; + + it("changes an enabled admin's password, found by lower cased email", async () => { + const email = uniqueEmail(); + await createAdmin(deps, { email, name: "Ada", password: stackPassword }); + + const run = resetPassword(email.toUpperCase(), [newPassword, newPassword]); + expect(run.status).toBe(0); + expect(run.stdout).toContain(`Password changed for ${email.toUpperCase()}.`); + expect(run.stdout).toContain("Every session they had is signed out."); + const old = await anonClient().auth.signInWithPassword({ email, password: stackPassword }); + expect(old.error).not.toBeNull(); + const fresh = await anonClient().auth.signInWithPassword({ email, password: newPassword }); + expect(fresh.error).toBeNull(); + }); + + it("changes a disabled admin's password", async () => { + const email = uniqueEmail(); + await createAdmin(deps, { email, name: "Alan", password: stackPassword }); + await disableAdmin(deps, email); + + expect(resetPassword(email, [newPassword, newPassword]).status).toBe(0); + const { error } = await anonClient().auth.signInWithPassword({ email, password: newPassword }); + expect(error).toBeNull(); + }); + + it("refuses an email with no admin row, a customer's included", async () => { + const email = uniqueEmail(); + await serviceClient().auth.admin.createUser({ + email, + password: stackPassword, + email_confirm: true, + }); + + for (const target of [email, uniqueEmail()]) { + const run = resetPassword(target, [newPassword, newPassword]); + expect(run).toMatchObject({ status: 1, stderr: "No admin has that email." }); + // Refused before any prompt. + expect(run.stdout).not.toContain("Password:"); + } + const { error } = await anonClient().auth.signInWithPassword({ + email, + password: stackPassword, + }); + expect(error).toBeNull(); + }); + + // covers: AC-16 + it("refuses a 13 character password and one typed differently twice, changing nothing", async () => { + const email = uniqueEmail(); + await createAdmin(deps, { email, name: "Edsger", password: stackPassword }); + + expect(resetPassword(email, ["Garden-gnome4"])).toMatchObject({ + status: 1, + stderr: "The password needs:\n- At least 14 characters", + }); + expect(resetPassword(email, [newPassword, `${newPassword}x`])).toMatchObject({ + status: 1, + stderr: "The passwords do not match.", + }); + const { error } = await anonClient().auth.signInWithPassword({ + email, + password: stackPassword, + }); + expect(error).toBeNull(); + }); + + // Records what the runbook states (spec 0019, Security model): Supabase's admin API password + // change deletes every session of the user, so an attacker holding one is out at once. + it("ends every session the admin had", async () => { const email = uniqueEmail(); const { id } = await createAdmin(deps, { email, name: "Linus", password: stackPassword }); const client = anonClient(); - await client.auth.signInWithPassword({ email, password: stackPassword }); - await client.auth.mfa.enroll({ factorType: "totp" }); + const { data } = await client.auth.signInWithPassword({ email, password: stackPassword }); + if (!data.session) throw new Error("sign in failed"); + const { access_token, refresh_token } = data.session; - const result = await resetAdminMfa(deps, email); + expect(resetPassword(email, [newPassword, newPassword]).status).toBe(0); - expect(result).toEqual({ id, factorsRemoved: 1 }); + const { error } = await client.auth.getUser(access_token); + expect(error).not.toBeNull(); + const refreshed = await client.auth.refreshSession({ refresh_token }); + expect(refreshed.error).not.toBeNull(); const sessions = await deps.db.$queryRaw<{ count: number }[]>` SELECT count(*)::int AS count FROM auth.sessions WHERE user_id = ${id}::uuid`; expect(sessions[0]?.count).toBe(0); - const { error } = await client.auth.getUser(); - expect(error).not.toBeNull(); }); }); diff --git a/tests/stack/mfa-challenges.stack.test.ts b/tests/stack/mfa-challenges.stack.test.ts deleted file mode 100644 index 548507c..0000000 --- a/tests/stack/mfa-challenges.stack.test.ts +++ /dev/null @@ -1,82 +0,0 @@ -import { afterAll, describe, expect, it, vi } from "vitest"; - -import { anonClient, serviceClient, stackPassword, uniqueEmail } from "./stack"; - -vi.mock("server-only", () => ({})); - -// Spec 0004, AC-16: the wrong code limit reads Supabase's internal auth.mfa_challenges table. -// This pins the columns it relies on and the one row per attempt behavior, so a Supabase -// upgrade that reshapes them fails here rather than silently disabling the lockout. -const { countRecentWrongCodes, isLockedOut, withFactorLock } = - await import("@/features/admin-auth/mfa-lockout"); -const { db } = await import("@/lib/db"); - -afterAll(() => db.$disconnect()); - -describe("auth.mfa_challenges", () => { - it("has the columns the lockout query reads", async () => { - const columns = await db.$queryRaw<{ column_name: string; data_type: string }[]>` - SELECT column_name, data_type FROM information_schema.columns - WHERE table_schema = 'auth' AND table_name = 'mfa_challenges'`; - const types = Object.fromEntries(columns.map((c) => [c.column_name, c.data_type])); - expect(types).toMatchObject({ - factor_id: "uuid", - created_at: "timestamp with time zone", - verified_at: "timestamp with time zone", - }); - }); - - it("counts each wrong code once per factor, and not the right one", async () => { - const email = uniqueEmail(); - const created = await serviceClient().auth.admin.createUser({ - email, - password: stackPassword, - email_confirm: true, - }); - expect(created.error).toBeNull(); - - const client = anonClient(); - await client.auth.signInWithPassword({ email, password: stackPassword }); - const { data: factor } = await client.auth.mfa.enroll({ factorType: "totp" }); - if (!factor) throw new Error("enroll failed"); - - expect(await countRecentWrongCodes(factor.id)).toBe(0); - for (const code of ["000000", "111111"]) { - const { error } = await client.auth.mfa.challengeAndVerify({ factorId: factor.id, code }); - expect(error?.code).toBe("mfa_verification_failed"); - } - expect(await countRecentWrongCodes(factor.id)).toBe(2); - }); - - it("checks one code at a time per factor, refusing the rest of a burst as busy", async () => { - const email = uniqueEmail(); - const created = await serviceClient().auth.admin.createUser({ - email, - password: stackPassword, - email_confirm: true, - }); - expect(created.error).toBeNull(); - - const client = anonClient(); - await client.auth.signInWithPassword({ email, password: stackPassword }); - const { data: factor } = await client.auth.mfa.enroll({ factorType: "totp" }); - if (!factor) throw new Error("enroll failed"); - - // The same check verifyTotp runs, fired as one burst larger than the connection pool. - const attempts = await Promise.all( - Array.from({ length: 12 }, () => - withFactorLock(factor.id, async (wrongCodes) => { - if (isLockedOut(wrongCodes)) return "locked"; - await client.auth.mfa.challengeAndVerify({ factorId: factor.id, code: "000000" }); - return "checked"; - }), - ), - ); - - expect(attempts.filter((a) => a.ok)).toEqual([{ ok: true, data: "checked" }]); - expect(attempts.filter((a) => !a.ok)).toHaveLength(11); - expect(await countRecentWrongCodes(factor.id)).toBe(1); - // No waiter kept a connection: the pool still answers at once. - await expect(db.$queryRaw`SELECT 1`).resolves.toBeDefined(); - }); -});