diff --git a/docs/deploy.md b/docs/deploy.md index 7d9fcba..9be372b 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -105,21 +105,43 @@ GOOGLE_CLIENT_ID= GOOGLE_CLIENT_SECRET= AUTH_BASE_URL=http://localhost:3000 AUTH_ALLOWED_EMAIL_DOMAIN=cornell.edu -AUTH_STAFF_EMAILS=you@cornell.edu # gives you the Refresh catalogue button +AUTH_SUPER_ADMIN_EMAILS=you@cornell.edu # the floor — see below ``` +`AUTH_SECRET` alone is enough for sessions; the two `GOOGLE_*` variables are +what make *starting* one possible. With them unset, `/api/auth/sign-in/social` +answers 503 and the header says sign-in is not set up here. + +`AUTH_SUPER_ADMIN_EMAILS` is a **floor, not a roster**. Everyone else's role is +the `user.role` column, changed on `/admin/users`; an address listed here is +created as `super_admin` on first sign-in and stays one whatever its row says. +It is the only way the first super admin comes to exist (no user row exists +until somebody signs in) and the reason the lab cannot lock itself out. +`AUTH_STAFF_EMAILS` and `AUTH_ADMIN_EMAILS` were removed in Phase 4 — nothing +reads them, so delete them from the deployment's environment rather than +leaving a list that grants nothing. + Tickets and projects now record the **verified session** instead of a typed name. Anonymous browsing and chat keep working — sign-in unlocks, it does not gate the front door. -## Stage 4 · Backups locally (optional) +## Stage 4 · The nightly job locally (optional) Cron does not run locally. Trigger it by hand: ```bash curl -H "x-admin-secret: $ADMIN_REVALIDATE_SECRET" \ - http://localhost:3000/api/admin/backup + http://localhost:3000/api/cron/daily ``` +It exports every Postgres table to a private blob and sweeps photos that were uploaded but +never attached to anything. It needs `BLOB_READ_WRITE_TOKEN` and refuses without it — the +old `/api/admin/backup` route it replaces dumped Notion and is no longer scheduled. + +Two tables are held out of the file on purpose: `session` and `verification` are sign-in +credentials, not records, and the Google tokens on `account` are blanked. A backup is +something you might email to yourself at 2am; it must not double as a way to sign in as +somebody. People, roles and bans are all still in there. + --- # Part 2 — On Vercel @@ -145,8 +167,17 @@ ADMIN_REVALIDATE_SECRET cache invalidation **Sign-in** — same as Stage 3, but `AUTH_BASE_URL=https://` and the Google redirect URI updated to match. -**Backups** — link a Vercel Blob store (sets `BLOB_READ_WRITE_TOKEN`), then set -`CRON_SECRET`. The cron itself is already in `vercel.json`, nightly at 07:17. +**Blob store** — link one (it sets `BLOB_READ_WRITE_TOKEN`). It carries **both** jobs now: +every photo a student uploads through the chat or the project form, and the nightly backup. +Without it the site still runs — uploads say so and the catalogue is unaffected — but +nothing is backed up and no photo can be attached. + +**Backups** — with the store linked, set `CRON_SECRET`. The cron is already in +`vercel.json`, nightly at 07:17, pointing at `/api/cron/daily`. + +**`LAB_TIMEZONE`** — optional, defaults to `America/New_York`. It decides the date on a +maintenance ticket; a function running in UTC would otherwise date an evening report +tomorrow. Set it before the first ticket is filed, or leave it to the default. **Optional** — `NOTION_DB_PROJECTS`, `UPSTASH_REDIS_REST_*` (rate limits enforced across instances rather than per-process), `MCP_TOKEN` (also the switch that exposes write tools @@ -188,10 +219,9 @@ Send alerts to a shared address, never one person. | | Blocks | |---|---| | Google OAuth client | Sign-in anywhere | -| `AUTH_STAFF_EMAILS` / `AUTH_ADMIN_EMAILS` | Staff features — **these two lists are the entire role system**; there is no user database | -| Notion: Flags `status` → `New` option | Corrections, silently | -| Notion: Projects DB + `published` checkbox | The gallery | -| Vercel Blob + `CRON_SECRET` | Backups — **there is currently no backup at all** | +| `AUTH_SUPER_ADMIN_EMAILS` | The first super admin, and therefore **every role**: nobody can be promoted until somebody can reach `/admin/users`. Roles live in `user.role` now; `AUTH_STAFF_EMAILS` / `AUTH_ADMIN_EMAILS` are retired and should be deleted from the environment | +| Vercel Blob store | **Photo uploads** (chat, maintenance, projects) and backups. Without it uploads refuse with a translated message rather than failing silently | +| Vercel Blob + `CRON_SECRET` | The nightly backup — **there is currently no backup at all** | | Inference spend limit | Nothing, until it does | | Uptime monitor | Nothing, until something breaks quietly | diff --git a/docs/handover.md b/docs/handover.md index a96b648..363a4a6 100644 --- a/docs/handover.md +++ b/docs/handover.md @@ -128,9 +128,12 @@ for what remains to verify before production traffic goes through it. ### Handle a maintenance ticket -Tickets from the assistant land in **Maintenance_Logs**, with photos if the student attached -any. Work them in Notion: set `status` to `In Progress`, then `Resolved`, and fill in -`resolution`. +Tickets from the assistant land in the **`maintenance_logs`** table in Postgres, with photos +if the student attached any. Corrections students report land in **`feedback`**, and project +submissions in **`projects`** (unpublished until staff publish them). None of the three go +to Notion any more, and the admin queues that work them are a later phase — until then, +working a ticket (`status` → `in_progress` → `resolved`, plus `resolution`) needs a +developer **[dev]**. Nothing in the app enforces this. **A ticket queue nobody reads is worse than no ticket queue** — students stop reporting after a couple of unanswered reports. Decide who checks @@ -157,21 +160,26 @@ Environment variables in Vercel, no code change: `NEXT_PUBLIC_SITE_NAME`, ### Check the nightly backup is still running -**Every night at 07:17 UTC (about 03:17 New York) the site backs up Notion.** Vercel Cron -calls `/api/admin/backup`, which reads every Notion database and writes one file to private -Vercel Blob storage as `backups/YYYY-MM-DD.json`. Files older than **30 days** are deleted -by the same job, so the store holds roughly a month at any time. +**Every night at 07:17 UTC (about 03:17 New York) the site backs itself up.** Vercel Cron +calls `/api/cron/daily`, which exports **every Postgres table** and writes one file to +private Vercel Blob storage as `backups/YYYY-MM-DD.json`. Files older than **30 days** are +deleted by the same job, so the store holds roughly a month at any time. The same run also +deletes photos that were uploaded but never attached to anything within 24 hours. -**This is the only copy of the Notion data outside Notion.** Before it existed, one deleted -database meant ~100 machines of staff work was gone for good. +**This is the only copy of the data outside Neon.** Before it existed, one deleted database +meant ~100 machines of staff work was gone for good. + +> The job used to dump Notion at `/api/admin/backup`. Postgres is the source of truth now, +> so the file holds database rows and its `source` field reads `postgres`. A file written +> before September 2026 holds raw Notion pages instead. Three settings in Vercel make it work, and it does nothing without all three: | Setting | Where | What it is | |---|---|---| -| A **Blob store** linked to the project | Vercel → Storage | Sets `BLOB_READ_WRITE_TOKEN` automatically | +| A **Blob store** linked to the project | Vercel → Storage | Sets `BLOB_READ_WRITE_TOKEN` automatically. **Also required for photo uploads** — with no store, the chat and the project form say photo uploads are unavailable instead of failing silently | | `CRON_SECRET` | Vercel env vars | Vercel sends it so the route knows the nightly call is genuine | -| `ADMIN_REVALIDATE_SECRET` | Vercel env vars | Lets a person trigger a backup by hand (same secret as §4) | +| `ADMIN_REVALIDATE_SECRET` | Vercel env vars | Lets a person trigger the job by hand (same secret as §4) | **How to check it, once a month:** Vercel dashboard → your project → **Cron Jobs**. A green run means a file was written. **A red run means the backup did not happen** — the route @@ -181,21 +189,28 @@ is discovered on the day you need it. The failure reason is in the run's log. To run one by hand, or to confirm it works after changing anything: ``` -GET https:///api/admin/backup +GET https:///api/cron/daily Header: x-admin-secret: ``` -It answers with the file it wrote, how many rows came from each database, and which old -files it deleted. Anything other than `200` is a real failure. +It answers with the file it wrote, how many rows came from each table, which old files it +deleted, and what the orphaned-photo sweep removed. Anything other than `200` is a real +failure, and the body names which stage broke. > [!WARNING] -> **The backup contains student names and email addresses** from Maintenance_Logs. It is -> written to *private* blob storage and must stay that way — never make the store public, -> never share a download link, and list it in whatever data inventory the university keeps. - -**To restore:** download the file from Vercel → Storage → Blob, and re-import the affected -database. The file holds the raw Notion rows, so a person can read it and rebuild from it. -There is no automated restore, on purpose — it is far more work than the failure justifies. +> **The backup contains student names and email addresses** from `maintenance_logs` and +> `feedback`. It is written to *private* blob storage and must stay that way — never make +> the store public, never share a download link, and list it in whatever data inventory the +> university keeps. +> +> It deliberately contains **no sign-in credentials**: the `session` and `verification` +> tables are skipped and the Google tokens on `account` are blanked, so somebody holding a +> backup file cannot use it to sign in as anybody. People, their roles and their bans *are* +> in it, because that is the state a restore most needs to get right. + +**To restore:** download the file from Vercel → Storage → Blob. The file holds the table +rows as JSON, so a person can read it and rebuild from it. There is no automated restore, on +purpose — it is far more work than the failure justifies. **[dev]** for anything beyond reading the file. --- @@ -225,7 +240,7 @@ effect. | Vercel logs | `DbUnavailableError` (Postgres unreachable) | Whenever the catalogue looks odd | | Vercel → Storage | The Neon database is reachable | Whenever the catalogue looks odd | | Vercel → Cron Jobs | The nightly backup ran green | Monthly — see §3 | -| Notion: Maintenance_Logs | Open tickets | Per §3 | +| Postgres: `maintenance_logs` | Open tickets | Per §3 | **The one alert that matters: an Anthropic spend threshold.** Everything else is recoverable; an unbounded bill is not. diff --git a/docs/specs/2026-09-14-v5-data-platform-design.md b/docs/specs/2026-09-14-v5-data-platform-design.md index c9cfa90..35f3bc8 100644 --- a/docs/specs/2026-09-14-v5-data-platform-design.md +++ b/docs/specs/2026-09-14-v5-data-platform-design.md @@ -1029,3 +1029,335 @@ Appended per [`DRIFT.md`](DRIFT.md). Original text above is never edited — the - **The demo seed has one published sample project** (a laser-cut lamp built with both demo tools, with two bundled photos under `public/sample-projects/`, materials and an outside link) and gives the Trotec a photo attachment, since its bundled image file is not named after the tool. The gallery, the project page and "built with this" are therefore exercised end to end without a database. **Status.** Accepted. The bridge and the prerender change are the two a reader should know about before Phase 3. + +### 2026-09-21 — Phase 3 built, with as-built details + +**What changed.** Phase 3 (§9) is implemented: the last three student-facing writes are on Postgres (`report_issue` → `maintenance_logs`, `report_correction` → `feedback`, `POST /api/projects` → `projects` + `project_tools`), `POST /api/uploads` replaces `POST /api/upload-notion` (Vercel Blob plus an `attachments` row), and `GET /api/cron/daily` replaces `GET /api/admin/backup` (a JSON export of every Postgres table, a 30-day prune, and an orphaned-upload sweep). The whole phase runs with every environment variable unset. Details the spec did not name: + +- **The upload id is a cross-phase contract.** `UploadedImage.file_upload_id` became `attachmentId`, and the chat hint became `[Attached photos: attachment_id= name=; …]`. `report_issue`'s input is `photo_attachment_ids: string[]`. The ripple the spec does not mention is intake: §9 leaves `create_tool` on Notion until Phase 6, and a Postgres uuid is not a Notion `file_upload_id` — Notion rejects the *whole page* when it does not recognise one. So `create_tool` now sends **no** images and pushes a warning saying the photos stayed in the app, and the intake prompt tells the model to relay that rather than claim the photo is on the listing. Losing the picture beats losing the listing (Article 4). +- **`previewUrl` is `string | null`, not the unconditional string §3.3 implies.** A private blob has no URL an unauthenticated viewer can follow, so a maintenance photo comes back with `previewUrl: null`; both clients already hold a local `URL.createObjectURL` preview, so nothing is lost. +- **Chat uploads are private, which §3.3's table does not cover.** The table lists maintenance photos as private and tool/resource/project photos as public, but a photo attached in chat is not yet either — it may become a ticket photo (which may show a person) or a pending-tool photo. It is stored `private`, the conservative half, since nothing renders a chat photo on a public page. Phase 6 decides what happens when such a photo is approved onto a tool. +- **`src/lib/blob.ts` gained a second write verb rather than a parameter.** Its doc comment said `access: "private"` and `addRandomSuffix: false` were "not a caller's decision"; both are wrong for uploads. `put` still writes backups privately at the exact pathname it is given, because that pathname *is* the retention key. `putUpload` writes at a random pathname (so an unpublished tool image is unguessable, and two `IMG_0001.jpg`s do not collide) with the caller's access. The filename is sanitised before it becomes part of a pathname. +- **`/api/cron/daily` kept both of `/api/admin/backup`'s accepted callers.** §3.10 folds the backup into the cron without saying whether the documented hand-trigger survives. It does: the cron bearer *and* `ADMIN_REVALIDATE_SECRET`, because `docs/deploy.md` tells an operator to run the job by hand after the first deploy. "Unconfigured" therefore means neither secret is set, which is the credential-free default. +- **The backup discovers its tables rather than listing them.** `backupTables()` filters the schema module for Drizzle tables, the same reasoning the old route used when it derived its targets from the env contract: Phase 4's four Better Auth tables and Phase 6's `pending_tools` are backed up because they exist, not because somebody remembered. The file is `version: 2, source: "postgres"`; version 1 held raw Notion pages, and a restore has to be able to tell them apart. Its date is UTC, not `LAB_TIMEZONE` — the retention window reads that date back, and it names a file rather than a ticket. +- **Cleanup deletes the blob before the row.** A row without a blob is a broken image on a page; a blob without a row is invisible and gets swept next run. Of the two half-failures the second is preferable, so the order is deliberate and tested. Pending-tool expiry (§4.10) is deliberately out of scope: that table has no writer until Phase 6. +- **Uploads clean up after themselves.** A blob write that succeeds and whose `attachments` insert then fails deletes the blob before answering 502 — the cron only sweeps files that *have* a row, so those bytes would otherwise be unreachable forever. +- **Phase 3 adds no `revalidateTag`.** §3.9 says writes invalidate their tags, but none of these writes touches a cached read: tickets and corrections are not cached, and a submitted project is unpublished so it cannot appear in `getPublishedProjects()`. The Phase 2 amendment deferred the `tool:` tag to "the write paths that would invalidate it" — that turns out to be Phase 5's publish. +- **No migration was needed.** Phase 1 created every column and CHECK these writes use, including `attachments.owner_type`'s list. +- **Retired but not deleted** (constitution working agreements — each deletion is proposed separately): `src/lib/data/notion-ids.ts` and its test, now importer-less; `src/app/api/upload-notion/` and `src/app/api/admin/backup/`, both superseded and no longer scheduled or linked. Each file carries a header saying so. `src/lib/notion.ts` itself cannot be retired — intake still uses `createTool`/`createUnit`/`createResource`/`findOrCreateCategory`/`findOrCreateLocation`, and `src/lib/import/source.ts` still uses `hasProjectsEnv`/`fetchAllProjects` — but `createMaintenanceLog`, `createProject` and `ProjectWriteFields` are now dead exports. +- **E2E got more real.** `corrections.spec.ts` and `projects.spec.ts` each gained one uncaptured, end-to-end submission against the E2E server's own PGlite, which was impossible while the write needed a Notion credential. The project one asserts that the submitted project does **not** appear in the gallery — Article 5's draft-by-default, made visible. `playwright.config.ts` now blanks `BLOB_READ_WRITE_TOKEN` and `CRON_SECRET` too, so a developer with a real store linked gets the same run as CI and no test can put bytes in it. + +**Status.** Accepted. The upload-id rename and intake's photo warning are the two a reader should know about before Phase 4. + +### 2026-09-21 — Phase 4 part 1 built (accounts and permissions), with as-built details + +**What changed.** The first half of Phase 4 (§9) is implemented: Better Auth on the Drizzle adapter with **database sessions** and the admin plugin (§3.4), the access-control declaration and `can()` (§3.5), `requiredPermission` on capabilities, `role` in `/api/identity` from the database, and `AUTH_STAFF_EMAILS` / `AUTH_ADMIN_EMAILS` retired in favour of `user.role` with `AUTH_SUPER_ADMIN_EMAILS` kept as the floor. Migration `0003_better_auth.sql` adds `user` / `session` / `account` / `verification`, the `user_role_check` CHECK, and the `created_by` / `updated_by` / `audit_events.actor_user_id` foreign keys Phase 1 deferred. The whole suite — 1208 unit/integration tests and 39 E2E — runs with every environment variable unset. Still to come in part 2: `/admin/users` and its server actions, `src/lib/data/audit.ts` and `users.ts`, and the sign-in requirement on project submission. + +Details the spec did not name, or named differently: + +- **`hasAuthEnv()` had to split.** It required `AUTH_SECRET` *and* both `GOOGLE_*`, which after Phase 4 would mean no database sessions without a Google client — and §10's E2E needs exactly that. It is now `hasSessionEnv()` (the secret alone) and `hasGoogleEnv()`. The Google gate moved to the route: `POST /api/auth/sign-in/social` answers 503 when the client is unconfigured, which is the `"unconfigured"` notice `sign-in-client.ts` already renders. +- **`getAuth()` is async.** `drizzleAdapter` needs the handle synchronously and `getDb()` is a promise (PGlite migrates and seeds on first use), so `getAuth(): Promise` and its memo key is the env fingerprint **plus `dataSubstrate()`** — a memo keyed only on env hands back an instance pointed at the previous database. +- **`resolveIdentity` is memoized with a `WeakMap>`, not React `cache()`.** `cache()` only memoizes inside a React render scope; in a Route Handler and under Vitest it silently does nothing, turning one session lookup per request into one per caller. A `resolveIdentityFromHeaders()` variant over `next/headers` exists for the server components part 2 needs; it is deliberately not memoized, because `next/headers` returns a fresh object per call. +- **`resolveIdentity` gained a `DbUnavailableError` branch.** Neon configured but unreachable logs and resolves anonymous rather than 500-ing a public page (Article 4). +- **The four tables are hand-written from the library, not from `npx @better-auth/cli generate`.** That CLI is not a dependency and wants the network. The authoritative offline source is `@better-auth/core/dist/db/get-tables.mjs` plus `better-auth/dist/plugins/admin/schema.d.mts`, and the constraint that actually matters is that each Drizzle table's *property keys* are Better Auth's camelCase field names (`@better-auth/drizzle-adapter` resolves a field by property name); the SQL column names stay snake_case like the rest of the schema. No `updated_at` trigger is attached — the library writes `updatedAt` itself, and a table it owns should not have a second writer. +- **`statement` includes the admin plugin's own `user` and `session` resources.** §3.5's sketch lists only the app's resources, but the plugin authorizes `set-role` against `{ user: ["set-role"] }`, so a declaration that omitted them would make every admin endpoint refuse everybody, super admins included. `super_admin` holds `user: [list, get, set-role, ban, update]` and `session: [list, revoke]`; impersonation, create, delete, set-password and set-email are granted to **nobody**, because v5 never signs in as somebody else and accounts come from Google and nowhere else. +- **The schema has a new leaf module, `src/lib/db/schema/checks.ts`.** `helpers.ts` now imports `auth.ts` (for the `user.id` reference on `actorColumns()`), and `auth.ts` needs a CHECK for `user.role`. Moving `inList` / `inListCheck` into a leaf keeps the pair acyclic rather than relying on ESM's tolerance for a cycle; `helpers.ts` re-exports both names, so every existing import still resolves. +- **`created_by` / `updated_by` are `on delete set null`, not `cascade`.** Deleting a person must never delete the catalogue they built. The same applies to `audit_events.actor_user_id` — deleting somebody must not delete the record that their role was changed. +- **`roles.ts` kept the domain rule and lost everything else.** `roleRank`, `isAtLeast`, `staffEmails`, `adminEmails` and `roleForEmail` are gone; `IDENTITY_ROLES` is `["anonymous", ...ROLES]` built from `db/schema/vocabulary.ts`, so the type and the CHECK cannot drift. Because `admin` means different things in the two vocabularies, every fixture was rewritten rather than renamed: `student` → `user`, `staff` → `admin`, old `admin` → `super_admin`. +- **`isSuperAdminFloor` applies the domain rule too.** An address outside `AUTH_ALLOWED_EMAIL_DOMAIN` can never be a row, so honouring a floor entry for one would grant the highest role to an account that cannot otherwise exist. A typo in the env list fails closed. +- **A Better Auth session cookie can be minted by hand, and that is what makes roles testable without Google.** `better-call/dist/crypto.mjs` signs a cookie as `encodeURIComponent(value + "." + btoa(HMAC-SHA256(secret, value)))` under `better-auth.session_token` (unprefixed on http). `test/utils/session.ts` does that against seeded rows, and `test/utils/session.test.ts` proves the format against a real `auth.api.getSession()` — the one assertion that licenses every other test to trust the helper. No backdoor route exists in the app. +- **The demo seed ships one account per role** (`DEMO_ACCOUNTS`) with constant session tokens, and `playwright.config.ts` boots with a test-only `AUTH_SECRET` and blank `GOOGLE_*`. `e2e/auth.spec.ts` no longer stubs `/api/identity`: it sets a genuinely signed cookie and the real route reads the real row. The E2E therefore proves the property the phase exists for — same cookie kind, different row, different controls. Those rows exist only in the PGlite substrate; a deployment with `DATABASE_URL` set never sees them, and the tokens are worthless without the secret that signs them. +- **`AUTH_STAFF_EMAILS` / `AUTH_ADMIN_EMAILS` cannot "seed the first admin rows" as §3.11 says**, because no `user` row exists until somebody signs in. In practice the floor is the only bootstrap: the first super admin signs in, then promotes people on `/admin/users`. Open question 3 has no automatic implementation — it is a manual step at the SuperMaker session. +- **Retired but not deleted** (constitution working agreements — each deletion is proposed separately): `src/lib/auth/session-cookie.ts` and its test. Nothing in the application imports them; the file carries a header saying so. +- **Unchanged on purpose:** `/api/mcp` still has no role gate — `MCP_TOKEN` is MCP's whole trust model and an MCP caller has no role — and `/api/admin/backup` (already superseded by `/api/cron/daily`) remains secret-gated rather than role-gated. + +**Status.** Accepted. The `getAuth()` async signature, the `WeakMap` memoization, and the fact that `created_by` now has a foreign key are the three a reader should know before touching part 2. + +### 2026-09-21 — Phase 4 part 2 built (the admin surface and the audit trail), with as-built details + +**What changed.** Phase 4 is complete. `/admin/users` (§5.2, §6) lists everyone who has signed in and changes their role or bans them, through two server actions — the first in the app. `src/lib/data/audit.ts` writes `role.changed` and `user.banned` into `audit_events` (§4.11). `POST /api/projects` now requires sign-in (§5.5) and takes the byline from the session; `/projects/new` shows "Sign in to share your project" to an anonymous visitor. `AdminLink` in the header opens `/admin` for anyone holding an admin-surface permission. The whole suite — 1290 unit/integration tests and 49 E2E — still runs with every environment variable unset. + +Details the spec did not name, or named differently: + +- **`/admin` has an index page the spec put in Phase 5.** §6 gives the header's `AdminLink` to anyone with *any* admin permission and points it at `/admin`, but `/admin` was to be `AdminHome` with counts and queues that do not exist yet. Linking a SuperMaker to a 404 is worse than a short page, so `/admin/page.tsx` lists the surfaces the viewer can actually open — one, so far — and says the rest arrive later. Phase 5 replaces its body, not its route. +- **The gate is in two places on purpose.** The layout answers the coarse question (signed in? any admin-surface permission?) so every page under it can check only what it needs. A SuperMaker therefore gets *through* the layout and is refused on `/admin/users`, which is the honest answer — `users.manage` is a director's, and saying so beats a 404. `canReachAdmin` / `ADMIN_SURFACE_PERMISSIONS` were added to `auth/permissions.ts` so the header and the layout cannot disagree about what "an admin surface" means. +- **Refusals are return values, not exceptions.** A thrown error in a server action reaches the browser as a digest and an error boundary, which is the wrong shape for "you cannot demote the floor address, and here is why". Each action answers `{ ok: false, error: }` and the island renders the matching `admin.errors.` string. The codes and `ADMIN_USERS_PATH` live in `action-result.ts`, because a `"use server"` module may export **only async functions** — every export becomes a callable endpoint. +- **Server actions travel to the islands as props.** `RoleSelect` and `BanToggle` take the action rather than importing it. A client component importing `actions.ts` would pull `next/headers`, the rate limiter and `server-only` into its graph and make it untestable without a Next runtime; the page that renders them is a server component that already holds the action. `UsersTable` is a server component with no `async` for the same reason — everything it needs is a prop, so RTL can mount it. +- **There are two lock-out guards, not one.** §8 names the floor; §10 separately names "the last super admin demotes themselves", and a deployment with `AUTH_SUPER_ADMIN_EMAILS` unset — a preview, a fork, the E2E server — can genuinely do it. So a demotion is refused when the target is on the floor (`protected_floor`) *or* when no unbanned `super_admin` would remain (`last_super_admin`). Banned super admins are not counted: they resolve to anonymous and can undo nothing. +- **Banning has no "last super admin" guard, deliberately.** Reaching that code means the *caller* holds `users.manage`, so banning somebody else cannot leave the lab without a director; the only self-directed case is refused earlier as `self_ban`. Better Auth refuses a self-ban too, but with an error code the page would have to translate. +- **The islands do not use `useTransition`, and that was a bug fix rather than a style choice.** A transition's pending state covers the action *and* the `revalidatePath` re-render it triggers, so the select sat on "Saving…" until a whole page had been rendered again — seconds under load, and the E2E caught it as a save that looked stuck for twenty seconds. The confirmation now comes from the awaited result; the revalidation still happens, it just no longer holds the confirmation hostage. Anything Phase 5 builds on this pattern should do the same. +- **`AUDIT_ACTIONS` has no `user.unbanned`**, so lifting a ban is recorded as `user.banned` with `detail.banned: false`. Adding a vocabulary term the spec does not list would be the worse drift. +- **`audit.ts` exports no mutator, and a test asserts it.** Append-only is only as good as the guarantee that nobody rewrote a row; with the app and the migrations sharing one connection string, a Postgres privilege is not available, so the enforcement is that the update does not exist in the codebase — checked by asserting the module's export shape. +- **A new `test/mocks/next-headers.ts`**, matching `next-cache.ts`: `nextHeadersMock()` plus a `setMockHeaders()` whose state lives in the mock module, because `vi.mock`'s factory is hoisted above the test's imports and may not close over anything. `next-cache.ts` gained `revalidatePath`. +- **The demo seed gained a fourth account, `DEMO_ACCOUNTS.promotable`.** E2E files run in parallel against one server, so the test that *changes* a role must change a row nobody else asserts on — promoting `DEMO_ACCOUNTS.user` would race `auth.spec.ts`'s "an ordinary signed-in user gets no admin controls". `e2e/utils/session.ts` now holds the `signIn` helper all three specs share. +- **`POST /api/projects` stopped reading `payload.author` entirely.** With the field gone from the form, leaving the fallback in would mean a request that simply omitted its cookie could choose its own byline. A signed-in account with no display name writes a null byline, which the gallery already renders as "Anonymous" — an account we know but cannot name. +- **A database outage now answers 401 on that route, not 502.** `resolveIdentity` treats an unreachable database as "nobody is signed in" so public pages keep serving (Article 4), and the sign-in check runs before the write — so a signed-in student sees "sign in to share a project" during an outage. It leaks nothing and writes nothing, but it is a worse sentence than it could be. Both branches are asserted in `route.test.ts`; a Phase 5 improvement would be to distinguish them with a `pingDb()`. +- **The roster shows email addresses.** The one surface in the app that does, because telling two accounts apart is its whole job. It goes no further — not into a prompt, not into the mirror, not into a log line (§8). +- **Dates in the roster are ISO, not localized.** `createdAt.toISOString().slice(0, 10)` in the mono treatment the design system gives every timestamp: locale-neutral, and it cannot render differently on the server and the client. +- **English-only strings, as Article 6 (amended) allows.** About 40 keys under `admin.*` plus the project sign-in prompt went into `messages/en.json`; the other 11 locales fall back to English until Phase 9. +- **Still pre-existing and unrelated:** `e2e/theme-i18n.spec.ts`'s two language-switch tests are flaky under a loaded machine (the server action plus `router.refresh()` occasionally exceeds the 5s expect timeout). They pass on retry and pass outright when run alone. + +**Status.** Accepted. What a reader should know before Phase 5: the two-gate pattern in `/admin/layout.tsx`, that server actions arrive at client islands as props, and that `"use server"` modules may export only async functions. + +### 2026-09-21 — Phases 3 and 4 integrated, with one seam closed + +**What changed.** The four implementation passes were run together as one branch and the +whole gate observed green with every environment variable unset: `npm run lint` (0 errors, +3 pre-existing warnings), `npm run typecheck` (clean), `npx vitest run` (**95 files, 1303 +tests**), `npx playwright test` (**49 passed**, no retries), `npm run spec:coverage` +(73 surface items · 0 undocumented) and `npm run build` (succeeds with no database). One +defect was found between the phases and fixed. + +- **The nightly export was about to archive live credentials, and now does not.** Phase 3 + built `backupTables()` to discover tables from the schema module rather than list them, + so that a table added later is backed up because it exists — and wrote a note asking + whoever landed Better Auth to decide about its tables. Phase 4 landed them. Nothing + connected the two, so `select *` over `session` would have written bearer tokens, and + over `account` Google's refresh tokens, into a private file kept for **thirty days**. + Anyone holding one backup could have signed in as anybody. + + The fix is `src/lib/cron/backup-policy.ts`, and it keeps discovery's virtue intact: the + default is still "back it up", and the exceptions are named and argued. `session` and + `verification` are skipped whole — a session row *is* a bearer token and a verification + row is a half-finished handshake; neither is worth restoring, and restoring them would + revive sign-ins that should have ended with the outage. `account` is kept with its four + secret columns blanked, because the row that matters is the link (this person is this + Google `sub`), which is what a restore needs, while the tokens are reissued on the next + sign-in. `user` is kept whole and deliberately so: `role` and `banned` are the state a + restore would most need to get right, and the row carries no secret. Everything is named + through the table objects, so renaming a column fails the typecheck rather than quietly + un-redacting it. `backup.test.ts` asserts the strong form — the demo seed's session token + does not appear in the written bytes at all. + +- **Nothing else needed fixing.** The seams that were looked for and were not there: no two + modules invalidate the same cache tag (`createProjectSubmission` deliberately invalidates + nothing and says why; the only `revalidateTag` is still `/api/admin/revalidate`), no + capability is registered twice, `claimAttachments` and the four functions added around it + live in one `attachments.ts` with no duplicate, the upload-id contract reads + `attachmentId` / `attachment_id` everywhere the four files that must move together, and + the E2E signing secret has one definition (`e2e/utils/session.ts`) that + `playwright.config.ts` documents itself as having to match. + +- **Two live routes still write `backups/YYYY-MM-DD.json`.** `/api/admin/backup` is + superseded and unscheduled but still reachable with a secret, and it writes a version-1 + Notion dump to the *same pathname* the nightly Postgres export uses — so a hand-trigger + of the retired route would overwrite that day's real backup. Deleting the route removes + the hazard and is already on the deletion-approval list; it was left in place under the + working agreement rather than half-fixed. + +**Status.** Accepted. Phases 3 and 4 are integrated and green; what remains before +production is credentials, which no test can stand in for. + +### 2026-09-22 — Nine review findings, and one sentence in §3.4 that needs choosing + +**What changed.** The Phase 3/4 branch was reviewed three times over and nine findings +came back. Four described code that was already on the branch — the review had read an +earlier working state, and the fixes had been amended into `1c967c4` itself, whose message +still says they are "outstanding and fixed in the next commit". They are not outstanding; +that line is stale. For the record, the three the message meant are the admin plugin's HTTP +endpoints (`/api/auth/admin/*` refused with 403 `admin_api_not_exposed` before the auth +instance is constructed), the super-admin floor the plugin could not see +(`src/lib/auth/floor-role.ts`), and `POST /api/uploads` granting a public Blob URL on the +caller's say-so (`KIND_POLICY` plus `uploadRefusal`, which pairs each `kind` with both its +access and the permission the consuming surface enforces). **Anyone reading a review of +this branch should check the current file before trusting a cited line number** — every one +of them is off by the size of those fixes. + +Four findings held up and were fixed. Details the spec did not name: + +- **A banned floor address is no longer anonymous, and this contradicts §3.4.** The section + says both "resolves as `super_admin` whatever its row says" and "A banned user resolves + to anonymous" (also §4.11's table and §6's signed-out note), which cannot both be true of + the same row. `identityFromSession` now reads the floor *first*: a listed address survives + a ban on a session it already holds. The domain rule still runs before both, and + `isSuperAdminFloor` applies it itself, so an out-of-domain floor entry still fails closed. + Half a recovery was the alternative, and the floor exists precisely so that "a mistaken + demotion **or ban** cannot lock the lab out". + + It does not rescue a sign-in, and the reason is worth not rediscovering: the admin plugin + registers `databaseHooks.session.create.before` and throws `BANNED_USER` there, and + `runPluginInit` (`better-auth/dist/context/helpers.mjs`) pushes plugin hooks *ahead* of + the app's own, so no hook this app can register runs in front of it. The row itself has to + change. `reconcileSuperAdminFloor` therefore reconciles `banned` as well as `role` + (clearing `banReason` and `banExpires` with it, so the plugin's auto-unban branch cannot + fire later against a row nobody banned), which means the first admin write a recovered + director performs restores ordinary sign-in. It still only ever promotes and unbans, only + for an address the environment already names, and the lift is recorded as `user.banned` + with `detail.banned: false` because `AUDIT_ACTIONS` has no `user.unbanned` (§4.11) — the + same shape `setUserBanned` writes. + + **This is the one item in this entry that is a decision and not a detail.** The code, both + module docstrings and §8's "so the lab can always recover" agree; §3.4's sentence does + not. It is written up here rather than edited into §3.4 because the original text is never + edited, and flagged rather than settled because it is the lab's call: if the ban should + win instead, it is a two-line revert in `identity.ts` plus the `lift` branch in + `floor-role.ts`, and the docstrings are what need correcting. + +- **An audit write that fails after the change committed is a warning on a success.** + `recordAuditEvent` was awaited unguarded after `auth.api.setRole` had already returned, so + a transient failure threw out of the server action and the island restored the *old* role + over a database holding the new one — asserting a state that does not exist, which is the + quiet lie Article 4 forbids, with the sign flipped. `AdminActionResult`'s ok variant gains + `warning?: AdminActionWarning`, a `record()` helper reports rather than throws, and both + islands keep the new value and render `admin.warnings.audit_unavailable` in + `.admin-row-status.is-warning`. The rule this sets, which Phase 5's admin writes inherit: + **a change that landed minus a guarantee is `{ ok: true, …, warning }`, never + `{ ok: false }`**, because a refusal is what the islands answer by rolling back. + + One deliberate asymmetry: `reconcileSuperAdminFloor`'s own audit writes are *not* on this + channel. If one fails, `authorize()` answers `failed` and the requested action never runs + — which is honest, because nothing the director asked for was saved — and the row it + already wrote makes the next attempt a no-op that succeeds. It self-heals in one click, + and plumbing a warning out through the gate would cost more than it buys. + +- **`POST /api/projects` answers `photosSubmitted` and `photosAttached`.** It discarded + `createProjectSubmission`'s count and returned a bare 201, so a student whose photos had + been swept by the nightly cron — a form left open overnight submits ids that are already + gone — was thanked for a write-up with no pictures. The form now says so on the + confirmation. **Partial loss counts too** (`photosAttached < photosSubmitted`), which is a + deliberate departure from the sibling write path in `capabilities/maintenance.ts`, whose + check is `photosAttached === 0`: two of three lost is exactly as silent as three of three. + Making maintenance symmetric is a reasonable follow-up; its tests pin the current + behaviour. + +- **`/projects/new` tells three states apart, not two.** A failed `/api/identity` fetch was + read as "not signed in", so a 429 or a dropped connection replaced the whole form with a + sign-in wall. The distinction was already in the data and was being thrown away: + `/api/identity` answers **200 `{role:"anonymous"}`** for a signed-out visitor, so + `fetchIdentity`'s `null` means only "could not ask". `IdentityStatus` is now + `pending | answered | unavailable`; `unavailable` keeps the form up with a notice and a + "Check again" control, and the server stays the authority, so a signed-in student can + still submit. A 401 from the submit now renders a translated sentence rather than the + route's English prose. **That invariant is now load-bearing** (`src/lib/auth/ + sign-in-client.ts`, both halves pinned by its test): if `/api/identity` is ever changed to + answer 401 for anonymous, this branch starts catching genuinely signed-out visitors. + `PrimaryNav`'s opposite choice was checked and left alone — it maps `null` to "signed + out" deliberately and documents why, and it withholds nothing, so there is no false + assertion with a cost. + +**Refusal strings on the two write routes are hardcoded English**, not `next-intl` keys — +`POST /api/uploads` matches `POST /api/projects`' existing idiom and `ProjectSubmitForm` +renders `data.error` verbatim, so a refusal reads English in all twelve locales. Translating +them is one job across both routes, not one route. Everything else new is seven keys in +`messages/en.json`, inherited by the other eleven through `withEnglishFallback` (Article 6 +as amended), which `src/i18n/messages.test.ts` enforces. + +**Gate.** Observed green with every environment variable unset: `npm run lint` (0 errors, the +same 3 pre-existing warnings), `npm run typecheck` (clean), `npx vitest run` (**97 files, +1356 tests**), `npx playwright test` (**49 passed**; an earlier run of the same commit was +48 passed and one flaky — `tool-detail.spec.ts`'s gallery-card click, green on retry, the +client-router race under load the Phase 2 amendment describes and not a new defect), +`npm run spec:coverage` (73 surface items · 0 undocumented) and `npm run build` +(succeeds with no database). Each of the three code fixes was confirmed red with the fix +reverted. Not covered: nothing in `e2e/` exercises a banned floor address or a failed audit +write — both are proved at unit and component level only. + +**Status.** Accepted, except the §3.4 sentence, which is open. Everything else is a detail +the spec left to implementation. + +### 2026-09-22 — §3.4 settled, the floor's own audit gap closed, and Phase 6's engine confirmed + +**§3.4 is settled: the environment variable wins.** The sentence left open by the previous +amendment — §3.4 says both "resolves as `super_admin` whatever its row says" and "a banned user +resolves to anonymous", which conflict when a floor address is banned — is decided in favour of +the floor. A banned address named in `AUTH_SUPER_ADMIN_EMAILS` resolves `super_admin`, and +`reconcileSuperAdminFloor` lifts the ban off the row on that person's first admin write. + +The reason is that the alternative is a dead end. The floor exists so the lab can always recover; +if a ban outranked it, the documented recovery — add the address, redeploy — would leave the +person still locked out, with no UI able to lift the ban and a manual `UPDATE` the only way +back. The cost is that anyone who can edit the production environment can un-ban themselves, +which is already true of anyone who can deploy, and who could reach the database directly +regardless. Decided by Isaac, 2026-09-22. + +**One hole remains, and it is narrow.** `auth.api.banUser` deletes the target's sessions, so if a +ban was applied through the app *before* the address was added to the floor, there is no session +left to carry the override and `session.create.before` refuses a fresh sign-in with +`BANNED_USER`. The app refuses to ban an address already on the floor, so reaching this state +takes a manual `UPDATE`, a restored backup, or a late addition to the list. Recovery there is +`UPDATE "user" SET banned = false` by hand. Documented rather than fixed: closing it means +running ahead of a plugin hook that better-auth pushes in front of the app's own. + +**The floor's own audit writes are guarded now.** Adversarial verification of `3c76839` found the +guard it added to `app/admin/users/actions.ts` missing one function away. +`reconcileSuperAdminFloor` committed its row `UPDATE` and then wrote two audit events unguarded, +so an unreachable `audit_events` threw past a committed change; `authorize()` caught it and +returned `failed`, which the page renders as "That did not save. Nothing was changed" — over a +row that had just been promoted and un-banned. A ban lifted with no trail, reported as nothing +having happened, which is the Article 4 lie in its purest form. + +The function now returns `{ changed, audited }` rather than a bare boolean and guards its audit +writes the way `actions.ts` guards its own. The gap travels back through `authorize()` as the +existing `audit_unavailable` warning, so both halves of a two-write action answer the admin's one +question — *did the trail record this* — with one warning. Only the row `UPDATE` itself still +throws. `actions.audit.test.ts` covers both shapes and both were confirmed red against the +unguarded version. + +**Phase 6's engine is confirmed: the Workflow SDK, as §3.7 specifies.** Re-decided rather than +assumed, because the question was reopened. Every API name in §3.7 is still current against +`workflow@4.8.9`, and the composition was verified by building a scratch app on this project's +exact stack — Next 16.1.6, Turbopack, `cacheComponents: true`, `next-intl` — where +`withWorkflow(withNextIntl(nextConfig))` compiles clean. Three corrections to §3.7: +`@workflow/world-vercel` is never installed (it is selected automatically); `@workflow/world-postgres` +needs a long-lived polling worker and is a real escape hatch rather than a config flip; and +`maxRetries` is a property on the step function (`researchItem.maxRetries = 2`), not an option. + +**eve was considered and rejected.** eve is a consumer of the Workflow SDK — "every session runs +as one durable workflow" — not an alternative to it, so adopting it would mean taking this same +layer plus an agent runtime, a session model and a second deploy surface (it runs as a peer Nitro +service, not a library). Three hard blockers independent of that: it requires `ai@^7` as a +non-optional peer dependency against this app's `ai@^6`, forcing an AI SDK major upgrade across +the live chat surface for a background job; it requires Node 24; and its eval runner always +targets an HTTP URL in a separate process, which Article 3's "every test passes with no +environment variables and no network" cannot accommodate. It is also in preview. Worth +revisiting only if the assistant itself ever becomes a durable multi-channel agent. + +**Phase 6 is sized for the Hobby plan.** The binding constraint is not Workflow but the Function +duration behind each step: Hobby caps it at 300s with no extension, and a model call with eight +web searches and eight fetches can exceed that. Decided by Isaac, 2026-09-22: stay on Hobby and +engineer around it. + +- **Four searches and four fetches per item**, not eight and eight. Halves the cost, halves what a + retry re-buys, and fits inside 300s with room. Thin results surface as low confidence, which is + the behaviour gate §5.4 already describes — never as an invented answer. +- **Two steps per item** — search, then fetch and verify — so neither alone approaches the ceiling. +- **A 240s `AbortSignal` inside each step**, so a slow item fails cleanly into `research_error` + rather than being killed mid-flight by the platform. +- **25 items per batch**, matching the per-request limit §5.4 already sets, rather than 100. + +**Two things Phase 6 must not do.** `mapWithConcurrency` (`src/lib/capabilities/intake.ts`) must +not move into the workflow function: it is a shared-cursor worker pool whose index claims depend +on completion order, so a replay can issue a different sequence of step calls and diverge. The +chunked `Promise.allSettled` in §3.7's sketch is correct and is deterministic. And `vi.mock()` +does not reach step code — `@workflow/vitest` loads steps from a pre-built esbuild bundle through +native `import()`, outside Vite's module graph — so this project's `vi.mock("ai")` pattern must +become an MSW handler on `api.anthropic.com` at that tier. MSW *does* reach step code, verified, +so the no-network guarantee holds. Testing steps and the workflow function as plain functions in +the existing config needs no new infrastructure and covers everything except retry semantics. + +**A scope correction to §3.7's "reused code".** Today's `research_tool` makes no model call at +all — the chat model does the searching with its native tools, and `research_tool` only dedupes, +verifies links and scores confidence. So `verifyResourceLinks` and `confidence.ts` genuinely +move, but the server-side research prompt and its `generateText` call are **new code**. Phase 6 is +larger than "lift and shift". + +**`research_error` is the diagnosis record, not the dashboard.** vercel/workflow#3373 (run history +on Next 16 with Turbopack) is still open, and Hobby retains run history for one day. §3.7 already +says the column is the record; build as though the dashboard does not exist. + +**Phase plan changes.** Decided by Isaac, 2026-09-22: **Phase 7 leaves the build plan** — Isaac and +Luis will review the imported inventory on their own time, and it was never code. **Phase 9 is +deferred** until the app is otherwise in good shape, then layered on; English-only keys with the +`withEnglishFallback` behaviour remain correct in the meantime. Phase 8, the Notion mirror, stays. +Open questions 4 and 5 — reporter names in the mirror, and whether the mirror key is derived from +`AUTH_SECRET` — are still unanswered and are due before Phase 8. + +**Residual risks accepted, not fixed.** A lost audit event survives only as a `console.error`: no +retry, no outbox, so the gap in `audit_events` is invisible to anyone reading the table later. +`report_issue` still reports photo loss only when *every* photo is lost, where `POST /api/projects` +now reports partial loss — the same Article 4 hole, on the other side, and worth closing when the +maintenance path is next touched. `POST /api/uploads` parses the multipart body before checking the +permission, so an anonymous caller can make the server read up to 18 MB before its 401; nothing is +stored and no URL is returned, and the 15/min-per-IP limiter is what bounds it. + +**Status.** Accepted. §3.4 is no longer open. diff --git a/v5/.env.example b/v5/.env.example index f514281..a8a05e1 100644 --- a/v5/.env.example +++ b/v5/.env.example @@ -28,23 +28,31 @@ NEXT_PUBLIC_LOGO="/makerlab-logo-transparent.png" NEXT_PUBLIC_COLOR_PRIMARY="#ff6b35" NEXT_PUBLIC_COLOR_PRIMARY_DARK="#cc4f1f" -# ── Notion data layer ──────────────────────────────────────────────── +# ── Notion data layer — IMPORT ONLY ────────────────────────────────── +# Postgres is the source of truth. Nothing the site serves reads Notion, and +# as of Phase 3 nothing the site writes goes to Notion either, except intake's +# `create_tool` (which uses NOTION_DB_TOOLS/CATEGORIES/LOCATIONS/UNITS/ +# RESOURCES until Phase 6). Everything below is needed by +# `npm run import:notion`; MAINTENANCE_LOGS, FLAGS and PROJECTS are needed by +# NOTHING ELSE and can be removed from the deployment once the import has run. +# # Internal integration token from https://www.notion.so/my-integrations NOTION_API_KEY=ntn_YourTokenHere -# Notion database IDs (required — share each database with the integration) +# Notion database IDs (required by the import — share each with the integration) NOTION_DB_TOOLS=YourDatabaseIdHere NOTION_DB_CATEGORIES=YourDatabaseIdHere NOTION_DB_LOCATIONS=YourDatabaseIdHere NOTION_DB_UNITS=YourDatabaseIdHere NOTION_DB_RESOURCES=YourDatabaseIdHere +# Import only — tickets are written to Postgres `maintenance_logs` now. NOTION_DB_MAINTENANCE_LOGS=YourDatabaseIdHere +# Import only — corrections are written to Postgres `feedback` now. NOTION_DB_FLAGS=YourDatabaseIdHere -# Optional, and separate from the seven above: the Student Projects gallery. -# Unset, /projects renders an empty state and submission returns a clear error; -# the rest of the catalog is unaffected. The database needs a `published` -# checkbox — that checkbox is the entire moderation gate. +# Optional, and import only: the Student Projects gallery's source database. +# Submissions are written to Postgres `projects` (unpublished until staff +# publish them), so the gallery works with this unset. NOTION_DB_PROJECTS= # ── AI APIs ────────────────────────────────────────────────────────── @@ -72,21 +80,32 @@ ANTHROPIC_API_KEY=sk-ant-api03-YourKeyHere # ── Admin ──────────────────────────────────────────────────────────── # Shared secret guarding POST /api/admin/revalidate. Also accepted by -# GET /api/admin/backup, so a person can trigger a backup by hand. +# GET /api/cron/daily, so a person can trigger the nightly job by hand. ADMIN_REVALIDATE_SECRET=YourSecretHere -# ── Nightly Notion backup ──────────────────────────────────────────── -# Both are set in Vercel, not locally. Without them GET /api/admin/backup -# answers 503 — deliberately, so a missing backup is never silent. +# ── Files and the nightly job ──────────────────────────────────────── +# Set in Vercel, not locally. # # Sent by Vercel Cron as `Authorization: Bearer $CRON_SECRET`, which is how -# the backup route knows the nightly call is genuine. +# GET /api/cron/daily knows the nightly call is genuine. Unset, that route +# answers 503 — deliberately, so a missing backup is never silent. CRON_SECRET=YourCronSecretHere # Injected automatically when a Blob store is linked to the Vercel project. -# The backup file contains student names and emails, so the store must stay -# PRIVATE. +# It gates TWO things now: +# 1. Every photo upload (POST /api/uploads). Unset, the route answers 503 +# `blob_not_configured` and the chat and project form say photo uploads +# are unavailable — they never hand back an id for a file nobody stored. +# 2. The nightly Postgres export written by GET /api/cron/daily. +# Uploads are stored public or private per kind; maintenance photos and the +# backup file contain student names and emails and are always PRIVATE. BLOB_READ_WRITE_TOKEN=vercel_blob_rw_YourTokenHere +# The lab's timezone, used for the date on a maintenance ticket. Defaults to +# America/New_York. A Vercel function runs in UTC, so without this a report +# filed at 9pm in New York would be dated tomorrow. A value Intl does not +# recognise falls back to UTC with a warning rather than losing the report. +LAB_TIMEZONE=America/New_York + # ── Database (Neon Postgres) ───────────────────────────────────────── # Injected automatically when Neon is installed from the Vercel Marketplace. # With it unset the app, the tests and `npm run import:notion -- --dry-run` @@ -97,13 +116,19 @@ DATABASE_URL= # ── Sign-in (Google Workspace) ─────────────────────────────────────── # Optional. With these unset the app runs exactly as before: everyone is # anonymous, the catalog and the assistant still work, and /api/auth returns -# 503. Setting all three turns sign-in on. +# 503. AUTH_SECRET alone gives you sessions; add the two GOOGLE_* variables to +# make signing in possible. # -# Signing key for the stateless session cookie. Generate with: +# Signing key for the session cookie. Generate with: # openssl rand -base64 32 -# ROTATING THIS SIGNS EVERYONE OUT — there is no server-side session -# revocation, so rotation is the emergency lever. Also salts the hashed IPs -# the rate limiter stores, so rotating resets anonymous allowances too. +# ROTATING THIS SIGNS EVERYONE OUT: every existing cookie stops verifying. +# (Individual revocation no longer needs it — sessions are rows since Phase 4, +# so a ban takes effect on the person's next request.) Also salts the hashed +# IPs the rate limiter stores, so rotating resets anonymous allowances too. +# +# This alone is enough for sessions. Without the two GOOGLE_* variables below +# there is simply no way to start one, and the header says sign-in is not set +# up here — which is how the E2E suite and a pre-OAuth deployment both run. AUTH_SECRET= # Google OAuth client (Web application) from console.cloud.google.com. @@ -121,11 +146,22 @@ AUTH_BASE_URL=http://localhost:3000 # also sent to Google as the `hd` hint, which only narrows the account picker. AUTH_ALLOWED_EMAIL_DOMAIN=cornell.edu -# Role rosters — comma-separated addresses. Anyone else who signs in with an -# address on the allowed domain is a "student". There is no user database; -# these lists are the whole role assignment mechanism. -AUTH_STAFF_EMAILS= -AUTH_ADMIN_EMAILS= +# The super-admin FLOOR — comma-separated addresses. Not a roster: everyone +# else's role is the `user.role` column, changed on /admin/users. +# +# An address listed here is created as `super_admin` on first sign-in and +# resolves as `super_admin` whatever its row says, and cannot be demoted or +# banned. Two reasons, both structural: +# 1. Bootstrap — no user row exists until somebody signs in, so there is no +# admin to promote the first one. This is how the first one comes to be. +# 2. Lock-out — a super admin who demotes themselves would otherwise leave +# nobody able to undo it and no UI to fix it with. +# Use a permanent address. The Cornell Tech deployment uses ies22@cornell.edu. +# +# AUTH_STAFF_EMAILS and AUTH_ADMIN_EMAILS were removed in Phase 4. Nothing +# reads them; delete them from the deployment's environment, because a list +# that grants nothing is a misleading roster. +AUTH_SUPER_ADMIN_EMAILS= # ── Rate limiting (optional) ───────────────────────────────────────── # Upstash Redis backs the API rate limiter. When both are set, limits are diff --git a/v5/AGENTS.md b/v5/AGENTS.md index fe0b927..f7dae9d 100644 --- a/v5/AGENTS.md +++ b/v5/AGENTS.md @@ -49,35 +49,182 @@ variable list. - **Notion is read only by the one-time import** (`npm run import:notion`). No request path reads Notion. A one-way mirror (app → an admin's own Notion workspace) is a later phase, not built yet. -- **The three writes still on Notion address pages, not rows.** A correction, - a maintenance ticket and a project submission create Notion pages whose - `relation` properties need *Notion page ids*, while everything the app hands - around is now a Postgres uuid. `src/lib/data/notion-ids.ts` translates - through `notion_page_id`, and a row that has none is written without the - relation rather than with an id Notion would reject — a ticket a human has - to link by hand beats a ticket that never arrived (Article 4). It goes away - with those writes in Phase 3. -- **Files** (tool images, manuals, project photos) live in **Vercel Blob**, - not Notion attachments — see `next.config.ts`'s `images.remotePatterns`. +- **Every student-facing write is on Postgres** as of Phase 3. A correction + goes to `feedback`, a maintenance ticket to `maintenance_logs`, a project + submission to `projects` + `project_tools` — see `src/lib/data/*.ts`. + `src/lib/data/notion-ids.ts` (the Phase-2 page-id bridge) has no importers + left and is awaiting deletion approval, as are `/api/upload-notion` and + `/api/admin/backup`. +- **The one write still on Notion is intake's `create_tool`** (`capabilities/ + intake.ts`), which stays there until Phase 6. Its photos no longer travel + with it: an upload id is a Postgres uuid now, Notion would reject the page, + so the tool is created without pictures and says so in its `warnings[]`. +- **Files** (tool images, manuals, project photos, maintenance photos) live in + **Vercel Blob**, recorded row-by-row in `attachments` — see + `next.config.ts`'s `images.remotePatterns`. `POST /api/uploads` is the one + upload route; it writes the blob, inserts an **unowned** `attachments` row and + returns `{ attachmentId, previewUrl }`. The write that follows *claims* those + ids (`claimAttachments`), and `/api/cron/daily` deletes anything still + unclaimed after 24 hours. **Both write paths say when a photo did not stick** + — `report_issue` appends it to the message the assistant paraphrases, and + `POST /api/projects` answers `photosSubmitted` / `photosAttached` so the form + can say it on the confirmation. A form left open overnight submits ids the + cron has already swept, and thanking a student for pictures nobody has is the + quiet lie Article 4 forbids. With no `BLOB_READ_WRITE_TOKEN` the route answers + 503 `{ code: "blob_not_configured" }` and both clients show a translated + "photo uploads are unavailable" — never a fabricated id (Article 4). - **Failing toward stale, not wrong (Article 4).** `DATABASE_URL` unset serves the PGlite demo seed with `DemoDataBanner` shown. `DATABASE_URL` set but unreachable never falls back to demo or invented data — cached pages keep serving and an uncached read renders the error state. +## Accounts, roles and permissions (Phase 4) + +**Better Auth runs the way it is meant to be run**, on the Drizzle adapter over +the same `getDb()` handle everything else uses. It owns four tables — `user`, +`session`, `account`, `verification` (`src/lib/db/schema/auth.ts`, migration +`0003`). The stateless `makerlab.identity` cookie is **retired**: +`src/lib/auth/session-cookie.ts` has no importers and is awaiting deletion +approval. Do not mint one. + +- **Sessions are rows.** The cookie carries only a token; `resolveIdentity` + looks the session and its user up on every request. That is why a role change + lands on the person's next request and a ban bites immediately — **with one + exception, a floor address, described under `AUTH_SUPER_ADMIN_EMAILS` + below.** Better Auth's cookie cache is deliberately off. +- **Roles** are `anonymous` (never a row) plus the stored `user | admin | + super_admin` (`src/lib/db/schema/vocabulary.ts`, which also backs the + `user_role_check` constraint). `student` and `staff` are gone; today's `admin` + is the old `staff`, and today's `super_admin` is the old `admin`. +- **What each role may do is declared in code**, not in a table: + `src/lib/auth/permissions.ts` (`statement` / `ac` / `roles` / `can()`), the + same declaration the admin plugin is configured with. **One check, + everywhere:** routes, server actions and capability composition call + `can(identity, "tools.add")`; client components call it with the role + `/api/identity` reports, to hide a control. Hiding is presentation; the + server check is the control. +- **Capabilities declare `requiredPermission`**, enforced once by + `capabilitiesForIdentity` in `src/lib/capabilities/access.ts` — never inside a + tool's `run()`. +- **`AUTH_STAFF_EMAILS` / `AUTH_ADMIN_EMAILS` are retired.** Nothing reads them. + The one env list left is **`AUTH_SUPER_ADMIN_EMAILS`, a floor, not a roster** + (`src/lib/auth/super-admins.ts`): a listed address is created as + `super_admin` and resolves as `super_admin` whatever its row says. It is the + bootstrap (no user row exists until somebody signs in) and the lock-out + guarantee. +- **"Whatever its row says" includes `banned`**, and that is the one exception + to "a ban bites immediately". `identityFromSession` reads the floor *before* + the ban check, so a listed address keeps resolving `super_admin` on a session + it already holds. It does not rescue a *sign-in*: the admin plugin throws + `BANNED_USER` from its own `session.create.before` hook, which runs ahead of + anything this app can register, so the row itself has to change. + `src/lib/auth/floor-role.ts` (`reconcileSuperAdminFloor`) is where it does — + called from `/admin/users`' `authorize()` after the permission check, it + writes the floor's `role` **and** clears `banned` on the caller's own row, so + the first admin write a recovered director performs makes ordinary sign-in + work again. It only ever promotes and unbans, only for an address the + environment already names, and it records both as audit events with a null + actor. It exists because `can()` honours the floor and the admin plugin does + not: the plugin reads the stored `role` and `banned` for itself, so a row left + disagreeing produces an opaque `failed` on every save. +- **Everything stays optional.** `AUTH_SECRET` alone gives database sessions; + the two `GOOGLE_*` variables are what make *starting* one possible, and + without them `/api/auth/sign-in/social` answers 503 and the header says + sign-in is not set up. With neither, nobody is signed in and the catalogue and + chat are unchanged. **Sign-in unlocks; it never gates the front door.** +- **Testing a role needs no Google.** `test/utils/session.ts` seeds a `user` and + a `session` row and mints the cookie Better Auth would have set; the demo seed + ships one account per role for E2E. See `test/README.md`. +- **`created_by` / `updated_by` now reference `user.id`** (`on delete set null`), + the foreign keys Phase 1 deferred. A write whose author is not a row is + refused — correct, because in production that id comes from a session. + +## The admin surface (`/admin`) + +`/admin/users` is the first admin page and the first server action in the app; +Phase 5 extends both. The shape it sets: + +- **Two gates, coarse then exact.** `src/app/admin/layout.tsx` resolves the + identity from headers and answers "may this person see an admin surface at + all" (`canReachAdmin`, any admin-surface permission). Each page then checks + the permission it actually needs — `/admin/users` on `users.manage`, which + only `super_admin` holds. A SuperMaker gets past the layout and is refused on + the page, **and is told so**: `AdminNotice` renders "not signed in" or "not + permitted", never a 404 and never an error boundary (Article 4). +- **Server actions check themselves.** A server action is a POST endpoint with + a generated name, reachable without the page that offers it, so + `src/app/admin/users/actions.ts` resolves the identity, rate-limits + (`ADMIN_ACTION_TIER`, 120/min per person), and re-checks `users.manage` — it + trusts nothing from the page that rendered the control (spec §8). Refusals are + **values** (`{ ok: false, error }`), not exceptions, so the island can render + the reason; every code has an `admin.errors.` string. +- **The server actions are the only way in.** The admin plugin also mounts its + own HTTP endpoints under `/api/auth/admin/*`, which would be a second, + unreviewed door onto the same writes. `src/app/api/auth/[...all]/route.ts` + refuses every path under that prefix with 403 `admin_api_not_exposed`, before + the auth instance is even constructed — percent-decoding and lower-casing the + path first, so `%61dmin` and `/ADMIN/` are the same refusal. +- **A change that landed minus a guarantee is a warning, not an error.** The + audit write happens *after* `auth.api.setRole` / `banUser` has committed, so + throwing there would make the page show the old value over a database holding + the new one. `record()` reports instead, and the action answers + `{ ok: true, …, warning: "audit_unavailable" }`. Both islands keep the new + value and show `admin.warnings.` in `.admin-row-status.is-warning`. + **Never answer `{ ok: false }` for a write that landed** — both islands + respond to a refusal by restoring the previous value, which would then assert + a state the database does not hold. Phase 5's admin writes should reuse this. +- **Two things cannot be undone, so they cannot be done.** An address in + `AUTH_SUPER_ADMIN_EMAILS` cannot be demoted or banned, and the last + unbanned `super_admin` cannot be demoted. The table disables those rows with + the reason showing; the action derives both again before it writes. +- **Reads go straight to Postgres, writes go through the plugin.** + `src/lib/data/users.ts` selects the roster; `auth.api.setRole` / `banUser` / + `unbanUser` perform the change, because a ban there also deletes the person's + sessions. +- **Every security-relevant change is recorded.** `src/lib/data/audit.ts` is + insert-and-select only — there is deliberately no update or delete export, + and a test asserts the module's shape. `AUDIT_ACTIONS` has no + `user.unbanned`, so lifting a ban is `user.banned` with `detail.banned: + false`. +- **Server actions pass down as props.** `RoleSelect` and `BanToggle` take the + action rather than importing it, which keeps `next/headers`, the limiter and + `server-only` out of a client component's graph and makes both testable with + a `vi.fn`. +- **Submitting a project needs an account** (spec §5.5) — the one place in the + app where sign-in is required. `POST /api/projects` answers 401 + `sign_in_required` to an anonymous caller, the byline comes from the session + and the typed-name field is gone. Browsing, chatting and reporting a problem + are all still anonymous. + ## Key files | Path | Purpose | |---|---| | `src/lib/site-config.ts` | White-label branding (env-driven, all have defaults) | | `src/lib/db/client.ts` | `getDb()`, `dataSubstrate()`, `pingDb()` — the one entry point to Postgres/PGlite | -| `src/lib/notion.ts` | Notion API client — write paths still on Notion until Phase 3 (tickets, corrections, project submission); no request path reads it | +| `src/lib/notion.ts` | Notion API client — read by the one-time import and by intake's `create_tool` (Phase 6); no other write and no request path reads it | +| `src/lib/data/attachments.ts` | `attachments` rows: create, claim onto an owner, list orphans, delete | +| `src/lib/blob.ts` | The Blob seam — `put` (private backups, fixed pathname) and `putUpload` (random pathname, caller's access) | +| `src/lib/cron/backup.ts`, `src/lib/cron/cleanup.ts` | The nightly Postgres export and the orphaned-upload sweep | +| `src/lib/cron/backup-policy.ts` | What the nightly export holds back — `session` / `verification` skipped, `account` tokens blanked. A backup is data, not credentials | | `src/lib/catalog.ts` | Catalog orchestration + cache, reading Postgres | -| `src/lib/rate-limit.ts` | In-memory (or Upstash) sliding-window limiter | +| `src/lib/rate-limit.ts` | In-memory (or Upstash) sliding-window limiter, tiered by role | +| `src/lib/auth/config.ts` | The Better Auth instance: Drizzle adapter, database sessions, admin plugin, domain enforcement | +| `src/lib/auth/identity.ts` | `resolveIdentity(req)` — the one way to learn who is calling. Never throws | +| `src/lib/auth/permissions.ts` | `statement` / `ac` / `roles` / `can()` — what each role may do | +| `src/lib/auth/super-admins.ts` | `AUTH_SUPER_ADMIN_EMAILS`, the lock-out floor | +| `src/lib/auth/floor-role.ts` | `reconcileSuperAdminFloor` — writes the floor's role and lifts its ban onto the row, because the admin plugin reads the row and not `can()` | +| `src/app/admin/layout.tsx` | The `/admin` front door — signed in? holds an admin permission? | +| `src/app/admin/users/actions.ts` | `setUserRole` / `setUserBanned` — the app's first server actions | +| `src/lib/data/users.ts` | The `/admin/users` roster, read straight from Postgres | +| `src/lib/data/audit.ts` | `audit_events` — insert and select, never update or delete | +| `src/lib/db/schema/auth.ts` | Better Auth's four tables; property keys are its field names | | `src/lib/types.ts` / `src/components/catalog-types.ts` | Notion record types / resolved view types | | `src/app/api/chat/route.ts` | Claude chat: streaming, tools (`get_unit_details`, `report_issue`, `web_fetch`), PDF manual attach | | `src/app/api/mcp/route.ts` | MCP JSON-RPC server (5 tools), bearer-token auth | -| `src/app/api/upload-notion/route.ts` | Image upload proxy → Notion file_uploads | -| `src/app/api/admin/revalidate/route.ts` | Cache invalidation (`x-admin-secret`) | +| `src/app/api/uploads/route.ts` | The one upload route → Vercel Blob + an `attachments` row | +| `src/app/api/cron/daily/route.ts` | The single nightly cron (`vercel.json`): backup, then orphaned-upload cleanup | +| `src/app/api/admin/revalidate/route.ts` | Cache invalidation (`tools.edit`, or `x-admin-secret` for session-less callers) | | `src/components/ChatFab.tsx` | Chat UI (`useChat`, citations stripped, photo upload) | | `src/app/page.tsx`, `tools/[id]/page.tsx` | Gallery + tool detail | @@ -88,7 +235,8 @@ variable list. - Server-only modules import `"server-only"` (e.g. `rate-limit.ts`). - Theme/brand via **CSS variables** (`--primary`, `--background`, …) — `[data-theme="light|dark"]` on ``, never hardcoded colors. - All branding strings come from `siteConfig` (`@/lib/site-config`). -- Every API route is **rate-limited by IP** before expensive work. +- Every API route is **rate-limited by identity** before expensive work — user id when signed in, hashed IP when not. +- Authorization is **always** `can(subject, permission)` from `src/lib/auth/permissions.ts`. Never compare role names, and never gate inside a capability tool's `run()`. - Maintenance tickets are always written in **English** even when the chat replies in another locale. ## Testing @@ -105,9 +253,10 @@ Or individually: `npm test` (Vitest: unit + integration + component), first), `npm run test:coverage`. - **Vitest** (jsdom) + React Testing Library + MSW; **Playwright** for E2E. -- Catalogue reads run against an in-process PGlite database seeded with demo - data (`getCatalogTools()` needs no Notion env at all); write paths still on - Notion (§ above) are covered with `vi.stubEnv` + MSW as before. +- **The whole suite runs with every environment variable unset.** Reads *and* + writes go to an in-process PGlite database seeded with demo data; Vercel Blob + is stubbed at the `src/lib/blob.ts` seam (`vi.mock`), never called for real. + The only MSW-stubbed Notion left is intake's `create_tool`. - E2E boots its own server on **port 3100** with `DATABASE_URL` unset (PGlite demo catalog) and intercepts `/api/chat` — it never touches your `:3000` dev server or real services. - Tests are colocated (`*.test.ts(x)` next to source); shared harness in `test/`. - **Read these before writing tests:** `TESTING.md` (runbook), `test/README.md` (harness internals + the `streamText`-capture and env-stubbing patterns), and `docs/specs/2026-05-29-v5-test-suite-design.md` (design + coverage matrix). The harness deps/scripts are already wired — don't hand-edit `package.json` for them. diff --git a/v5/e2e/admin-users.spec.ts b/v5/e2e/admin-users.spec.ts new file mode 100644 index 0000000..2ee214d --- /dev/null +++ b/v5/e2e/admin-users.spec.ts @@ -0,0 +1,172 @@ +import { test, expect } from "@playwright/test"; + +import { DEMO_ACCOUNTS } from "../src/lib/db/demo-seed"; +import { signIn } from "./utils/session"; + +/** + * `/admin/users`, end to end (data platform design spec §5.2, §10 scenario 6). + * + * The scenario the spec names is one sentence long and is the whole reason + * Phase 4 exists: **a super admin changes a user to admin, and that person's + * Add button appears on their next page load.** Nothing is cached across + * requests, nothing is carried in a cookie, and the only thing that changed is + * a column. + * + * The account it promotes is `DEMO_ACCOUNTS.promotable`, a spare that exists + * for exactly this: the suite runs its files in parallel against one server, + * so a test that mutates a shared row must mutate one nobody else asserts on. + * + * Sign-in is still unconfigured on this server (`GOOGLE_*` blank), and no test + * here touches Google. Being somebody is a signed cookie for a seeded session + * row — see `e2e/utils/session.ts`. + */ + +const PLACEHOLDER_LEAK = "{institution}"; + +/** The tests below run in order: one promotes, the next reads the result. */ +test.describe.configure({ mode: "serial" }); + +/** + * Headroom for a save. It normally lands in a couple of hundred milliseconds — + * `RoleSelect` confirms from the action's result rather than waiting out the + * revalidation that follows it — but this runs beside thirteen other workers + * against one PGlite database, and five seconds has not always been enough. + */ +const SAVE_TIMEOUT = 15_000; + +test.describe("/admin/users — who may open it", () => { + test("an anonymous visitor is told to sign in, not 404ed", async ({ page }) => { + await page.goto("/admin/users"); + + // A 404 would lie about the page existing; an error boundary would say + // something went wrong when nothing did (§6). + await expect( + page.getByRole("heading", { name: "You are not signed in", level: 1 }) + ).toBeVisible(); + await expect(page.getByRole("table")).toHaveCount(0); + await expect(page.locator("body")).not.toContainText(PLACEHOLDER_LEAK); + }); + + test("an ordinary student is refused, and told why", async ({ + page, + context, + baseURL, + }) => { + await signIn(context, DEMO_ACCOUNTS.user, baseURL); + await page.goto("/admin/users"); + + await expect( + page.getByRole("heading", { name: /do not have access/i, level: 1 }) + ).toBeVisible(); + await expect(page.getByRole("table")).toHaveCount(0); + }); + + test("a SuperMaker reaches /admin but not the people page", async ({ + page, + context, + baseURL, + }) => { + await signIn(context, DEMO_ACCOUNTS.admin, baseURL); + + // `tools.edit` gets them through the layout… + await page.goto("/admin"); + await expect(page.getByRole("heading", { name: "Admin surfaces" })).toBeVisible(); + await expect(page.getByRole("link", { name: "People" })).toHaveCount(0); + + // …and `users.manage`, which only a director holds, stops them here. + await page.goto("/admin/users"); + await expect( + page.getByRole("heading", { name: /do not have access/i, level: 1 }) + ).toBeVisible(); + }); + + test("the header offers the admin link only to those who can use it", async ({ + page, + context, + baseURL, + }) => { + const nav = page.getByRole("navigation", { name: "Primary navigation" }); + + await page.goto("/"); + await expect(nav.getByRole("link", { name: "ADMIN" })).toHaveCount(0); + + await signIn(context, DEMO_ACCOUNTS.admin, baseURL); + await page.reload(); + await expect(nav.getByRole("link", { name: "ADMIN" })).toBeVisible(); + }); +}); + +test.describe("/admin/users — changing a role", () => { + test("a director sees the roster, with the last director's own row locked", async ({ + page, + context, + baseURL, + }) => { + await signIn(context, DEMO_ACCOUNTS.superAdmin, baseURL); + await page.goto("/admin/users"); + + await expect(page.getByRole("heading", { name: "People" })).toBeVisible(); + + const ownRow = page.getByRole("row", { name: new RegExp(DEMO_ACCOUNTS.superAdmin.name) }); + await expect(ownRow.getByText("you", { exact: true })).toBeVisible(); + // This server boots with AUTH_SUPER_ADMIN_EMAILS blank, so there is no + // floor — the only thing standing between the lab and a lock-out is the + // "last director" guard, and it is visible rather than a surprise on save. + await expect( + ownRow.getByRole("combobox", { name: new RegExp(DEMO_ACCOUNTS.superAdmin.name) }) + ).toBeDisabled(); + await expect(ownRow.getByText(/last director/i)).toBeVisible(); + + await expect(page.locator("body")).not.toContainText(PLACEHOLDER_LEAK); + }); + + test("a director promotes a student, and the change is in the database", async ({ + page, + context, + baseURL, + }) => { + await signIn(context, DEMO_ACCOUNTS.superAdmin, baseURL); + await page.goto("/admin/users"); + + const row = page.getByRole("row", { name: new RegExp(DEMO_ACCOUNTS.promotable.name) }); + const select = row.getByRole("combobox", { + name: new RegExp(DEMO_ACCOUNTS.promotable.name), + }); + + // This test changes a row and the suite retries a failed test once, so it + // puts the account back where it expects to find it first. Without this a + // retry starts from the half-applied state the first attempt left. + if ((await select.inputValue()) !== "user") { + await select.selectOption("user"); + await expect(select).toBeEnabled({ timeout: SAVE_TIMEOUT }); + } + await expect(select).toHaveValue("user"); + + await select.selectOption("admin"); + await expect(row.getByText("Saved")).toBeVisible({ timeout: SAVE_TIMEOUT }); + + // A reload, not the optimistic state: the row is what the server holds. + await page.reload(); + await expect( + page + .getByRole("row", { name: new RegExp(DEMO_ACCOUNTS.promotable.name) }) + .getByRole("combobox", { name: new RegExp(DEMO_ACCOUNTS.promotable.name) }) + ).toHaveValue("admin"); + }); + + test("and that person's Add button appears on their next page load", async ({ + page, + context, + baseURL, + }) => { + // Spec §10 scenario 6, the half that matters. Same cookie they had before + // the promotion — sessions are rows, the role is read per request, and + // nothing had to expire. + await signIn(context, DEMO_ACCOUNTS.promotable, baseURL); + await page.goto("/"); + + const nav = page.getByRole("navigation", { name: "Primary navigation" }); + await expect(nav.getByRole("button", { name: /Add new equipment/i })).toBeVisible(); + await expect(nav.getByRole("link", { name: "ADMIN" })).toBeVisible(); + }); +}); diff --git a/v5/e2e/auth.spec.ts b/v5/e2e/auth.spec.ts index 59e6a90..d696e2d 100644 --- a/v5/e2e/auth.spec.ts +++ b/v5/e2e/auth.spec.ts @@ -1,59 +1,39 @@ import { test, expect } from "@playwright/test"; -// Sign-in (auth design spec 2026-07-29 §5, §6, §10). Two properties are worth an +import { DEMO_ACCOUNTS } from "../src/lib/db/demo-seed"; +import { + BETTER_AUTH_SESSION_COOKIE, + E2E_AUTH_SECRET, + signCookieValue, + signIn, +} from "./utils/session"; + +// Sign-in (data platform design spec §3.4, §10). Two properties are worth an // E2E each: signing in never gates the front door, and the header reflects who // the server says you are. // -// **No real Google OAuth.** Driving it in CI is neither possible nor desirable -// (spec §10), and the E2E server boots with no credentials at all. +// **No real Google OAuth.** Driving it in CI is neither possible nor desirable, +// and the E2E server boots with GOOGLE_CLIENT_ID and GOOGLE_CLIENT_SECRET blank +// — so `/api/auth/sign-in/social` answers 503 and the header says sign-in is +// not set up here. // -// Which means a *genuinely* signed cookie cannot be minted here either: the -// session cookie is an HMAC over `AUTH_SECRET`, and the E2E server has none, so -// anything the browser sends resolves to anonymous by design. The header's whole -// view of the session is `GET /api/identity` (see the spec's `/api/identity` -// amendment), so the stub below sits at that boundary and answers *from the -// cookie the browser actually sent*. The cookie is still what flips the header; -// the stub stands in only for the signature check the server cannot perform. - -/** Mirrors SESSION_COOKIE_NAME in src/lib/auth/session-cookie.ts. */ -const SESSION_COOKIE = "makerlab.identity"; - -/** Opaque: nothing verifies it. Shaped like a real token so it is not mistaken for one. */ -const STUB_TOKEN = "e2e-stub-session.not-a-real-signature"; - -const USER_NAME = "Casey Rivera"; +// It is nonetheless a *genuine* session that is asserted below, not a stub. +// Sessions are database rows since Phase 4, so the demo seed ships one account +// per role with a known session token (`DEMO_ACCOUNTS`), the Playwright server +// boots with a test-only `AUTH_SECRET`, and the cookie below is signed with it +// exactly the way Better Auth signs one. Nothing is intercepted: the real +// `/api/identity` reads the real session row and reports the real role. + +const SIGNED_IN = DEMO_ACCOUNTS.user; /** PrimaryNav shows the first name only (spec §6). */ -const USER_FIRST_NAME = "Casey"; - -/** - * Answer `/api/identity` as a server holding `AUTH_SECRET` would: signed in when - * the request carries the session cookie, anonymous when it does not. - */ -async function stubIdentityFromCookie(page: import("@playwright/test").Page) { - await page.route("**/api/identity", async (route) => { - const headers = await route.request().allHeaders(); - const signedIn = (headers["cookie"] || "").includes(`${SESSION_COOKIE}=`); - await route.fulfill({ - status: 200, - headers: { - "content-type": "application/json", - "cache-control": "no-store, private", - }, - body: JSON.stringify( - signedIn - ? { role: "student", name: USER_NAME } - : { role: "anonymous", name: null } - ), - }); - }); -} +const USER_FIRST_NAME = SIGNED_IN.name.split(" ")[0]; test.describe("Sign-in", () => { test("anonymous visitors browse the catalog and open a tool page", async ({ page, }) => { - // No stub and no cookie: this is the real /api/identity answering anonymous, - // which is what an ISAM attendee who never creates an account will get. + // No cookie: this is the real /api/identity answering anonymous, which is + // what an ISAM attendee who never creates an account will get. await page.goto("/"); await expect( @@ -89,32 +69,79 @@ test.describe("Sign-in", () => { await expect(nav.getByRole("button", { name: /sign out/i })).toHaveCount(0); }); - test("with a stubbed session cookie the header shows the user's name", async ({ + test("a real signed session cookie shows the user's name in the header", async ({ page, context, baseURL, }) => { - await stubIdentityFromCookie(page); - const nav = page.getByRole("navigation", { name: "Primary navigation" }); - // Same stub, no cookie: the header must still offer sign-in. Asserting both - // halves is what makes the cookie — rather than the stub — the thing under - // test. + // No cookie first: the header must offer sign-in. Asserting both halves is + // what makes the session — rather than the page — the thing under test. await page.goto("/"); await expect(nav.getByRole("button", { name: /sign in/i })).toBeVisible(); + await signIn(context, SIGNED_IN, baseURL); + await page.reload(); + + await expect(nav.getByText(USER_FIRST_NAME, { exact: true })).toBeVisible(); + await expect(nav.getByRole("button", { name: /sign out/i })).toBeVisible(); + await expect(nav.getByRole("button", { name: /sign in/i })).toHaveCount(0); + }); + + test("an ordinary signed-in user gets no admin controls", async ({ + page, + context, + baseURL, + }) => { + await signIn(context, DEMO_ACCOUNTS.user, baseURL); + await page.goto("/"); + + const nav = page.getByRole("navigation", { name: "Primary navigation" }); + await expect(nav.getByRole("button", { name: /sign out/i })).toBeVisible(); + + // `tools.add` and `tools.edit` are not granted to `user` (auth/permissions). + await expect( + nav.getByRole("button", { name: /Add new equipment/i }) + ).toHaveCount(0); + await expect(nav.getByRole("button", { name: /Refresh the/i })).toHaveCount(0); + }); + + test("an admin's role comes from their row, and unlocks the admin controls", async ({ + page, + context, + baseURL, + }) => { + // The end-to-end proof that the role is read per request from the database + // rather than carried in the cookie: the same kind of cookie, a different + // row, a different set of controls. + await signIn(context, DEMO_ACCOUNTS.admin, baseURL); + await page.goto("/"); + + const nav = page.getByRole("navigation", { name: "Primary navigation" }); + await expect(nav.getByRole("button", { name: /Add new equipment/i })).toBeVisible(); + await expect(nav.getByRole("button", { name: /Refresh the/i })).toBeVisible(); + }); + + test("a cookie signed with the wrong secret is nobody", async ({ + page, + context, + baseURL, + }) => { await context.addCookies([ { - name: SESSION_COOKIE, - value: STUB_TOKEN, + name: BETTER_AUTH_SESSION_COOKIE, + // Same real session token, signed with anything but the server's key. + value: await signCookieValue( + SIGNED_IN.sessionToken, + `${E2E_AUTH_SECRET}-but-wrong` + ), url: baseURL ?? "http://localhost:3100", }, ]); - await page.reload(); + await page.goto("/"); - await expect(nav.getByText(USER_FIRST_NAME, { exact: true })).toBeVisible(); - await expect(nav.getByRole("button", { name: /sign out/i })).toBeVisible(); - await expect(nav.getByRole("button", { name: /sign in/i })).toHaveCount(0); + const nav = page.getByRole("navigation", { name: "Primary navigation" }); + await expect(nav.getByRole("button", { name: /sign in/i })).toBeVisible(); }); }); diff --git a/v5/e2e/corrections.spec.ts b/v5/e2e/corrections.spec.ts index dadd94c..c82e05e 100644 --- a/v5/e2e/corrections.spec.ts +++ b/v5/e2e/corrections.spec.ts @@ -5,10 +5,11 @@ import { test, expect } from "@playwright/test"; // description; the confirmation replaces the form in place rather than firing a // toast. // -// The E2E server boots with NOTION_DB_FLAGS unset (see playwright.config.ts), so -// the real POST /api/flags would answer 503 `not_configured`. The submit test -// intercepts that request with page.route() instead — the same technique -// chat.spec.ts uses — so nothing leaves the machine. +// The write is local as of Phase 3: POST /api/flags inserts into the E2E +// server's PGlite database with no credential at all, so one test submits for +// real end to end. The other submit test keeps its page.route() interception — +// not because the route would refuse, but because asserting the exact request +// body is the cheapest way to prove the field travels. // // Demo catalogue: "Form 4" is slug `form-4`; its id is the Postgres uuid the // seed assigned (src/lib/db/demo-seed.ts). Strings are `flag.*` in @@ -111,3 +112,33 @@ test.describe("Report a correction", () => { }); }); }); + +// One genuinely end-to-end submission. Nothing is intercepted: the browser +// posts to the real route, which writes a `feedback` row into the E2E server's +// PGlite database. This is only possible because the write no longer needs a +// Notion credential (data platform spec §3.10). +test.describe("Report a correction — the real write path", () => { + test("a report submitted with no interception reaches the route and confirms", async ({ + page, + }) => { + const posted: number[] = []; + page.on("response", (res) => { + if (res.url().includes("/api/flags")) posted.push(res.status()); + }); + + await page.goto("/tools/form-4"); + await page.getByRole("button", { name: "Report a correction" }).click(); + + const dialog = page.getByRole("dialog"); + await dialog + .getByRole("textbox", { name: /what.s wrong/i }) + .fill("This lives in the Resin Bench, not the Wood Shop."); + await dialog.getByRole("button", { name: "Send report" }).click(); + + await expect( + dialog.getByRole("heading", { name: "Report sent" }) + ).toBeVisible(); + // A 503 here would mean the route still thinks it needs a credential. + expect(posted).toEqual([201]); + }); +}); diff --git a/v5/e2e/projects.spec.ts b/v5/e2e/projects.spec.ts index c5a09ba..dd984d2 100644 --- a/v5/e2e/projects.spec.ts +++ b/v5/e2e/projects.spec.ts @@ -1,14 +1,19 @@ import { test, expect } from "@playwright/test"; +import { DEMO_ACCOUNTS } from "../src/lib/db/demo-seed"; +import { signIn } from "./utils/session"; + // The app boots with no DATABASE_URL and NOTION_* unset (see // playwright.config.ts webServer.env), so the gallery reads the PGlite demo // seed — one published sample project, "Laser-cut plywood lamp" -// (src/lib/db/demo-seed.ts) — and `POST /api/projects`, still a Notion write -// until Phase 3, would answer 503 "not configured". +// (src/lib/db/demo-seed.ts). As of Phase 3 `POST /api/projects` writes into +// that same database and needs no credential, so one test below submits for +// real, end to end. // -// The submit path is therefore exercised with `page.route("**/api/projects")` -// standing in for the route handler, exactly as chat.spec.ts stands in for -// /api/chat. Nothing in this file reaches a real service. +// The other submit tests keep `page.route("**/api/projects")` standing in for +// the route handler — not because the route would refuse, but because the +// exact request body and the failure branch are cheaper to assert that way. +// Nothing in this file reaches a real service either way. // // Strings come from messages/en.json (`projects.*`, `projectForm.*`). Branding // is deliberately NOT asserted literally — `siteConfig.institution` is @@ -18,15 +23,15 @@ import { test, expect } from "@playwright/test"; const PLACEHOLDER_LEAK = "{institution}"; -/** Fills the three fields the form requires before it will POST. */ +/** Fills the two fields the form requires before it will POST. */ async function fillRequiredFields(page: import("@playwright/test").Page) { await page .getByRole("textbox", { name: "Project title" }) .fill("Parametric stool"); - await page.getByRole("textbox", { name: "Your name" }).fill("Ada Lovelace"); await page .getByRole("textbox", { name: /Write-up/ }) .fill("Cut on the Trotec, assembled with wedged tenons."); + // No name field since Phase 4: the byline is the session's (spec §5.5). } test.describe("Projects gallery", () => { @@ -82,6 +87,13 @@ test.describe("Projects gallery", () => { }); test.describe("Project submission form", () => { + // Submitting requires an account since Phase 4 (spec §5.5). Every test in + // this block is a signed-in student; the anonymous path is its own block + // below, and browsing the gallery above never needed a cookie. + test.beforeEach(async ({ context, baseURL }) => { + await signIn(context, DEMO_ACCOUNTS.user, baseURL); + }); + test("/projects/new renders every field a submission needs", async ({ page, }) => { @@ -100,11 +112,12 @@ test.describe("Project submission form", () => { await expect( page.getByRole("textbox", { name: "Project title" }) ).toBeVisible(); - // Anonymous in E2E (no auth env, /api/identity answers role "anonymous"), - // so the byline is typed rather than pre-filled and read-only. - const author = page.getByRole("textbox", { name: "Your name" }); - await expect(author).toBeVisible(); - await expect(author).toBeEditable(); + // The byline is stated, not asked for: it is the session's display name + // and the server writes it whatever the request says. + await expect( + page.getByText(`Your project will be credited to ${DEMO_ACCOUNTS.user.name}`) + ).toBeVisible(); + await expect(page.getByRole("textbox", { name: "Your name" })).toHaveCount(0); await expect(page.getByRole("textbox", { name: /Write-up/ })).toBeVisible(); await expect( page.getByRole("textbox", { name: "Link (optional)" }) @@ -175,11 +188,10 @@ test.describe("Project submission form", () => { .fill("Half-filled submission"); await page.getByRole("button", { name: "Submit project" }).click(); - // Validation moves to the next missing field rather than letting a - // half-filled submission through. - await expect( - page.getByRole("textbox", { name: "Your name" }) - ).toBeFocused(); + // Validation moves to the next missing field — the write-up, now that the + // byline is not a field — rather than letting a half-filled submission + // through. + await expect(page.getByRole("textbox", { name: /Write-up/ })).toBeFocused(); expect(posts).toBe(0); }); @@ -227,7 +239,9 @@ test.describe("Project submission form", () => { expect(bodies).toHaveLength(1); const payload = bodies[0] as Record; expect(payload.title).toBe("Parametric stool"); - expect(payload.author).toBe("Ada Lovelace"); + // No author in the body since Phase 4: the byline is the session's, and + // the form sends nothing the server would ignore (spec §5.5). + expect(payload).not.toHaveProperty("author"); // The chosen tool travels as its database id (a uuid), not its slug. expect(payload.tools).toEqual([ expect.stringMatching(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/), @@ -268,3 +282,109 @@ test.describe("Project submission form", () => { ).toBeEnabled(); }); }); + +// A genuinely end-to-end submission, possible for the first time in Phase 3. +// What it proves is Article 5 made visible: a submitted project is a draft, so +// the gallery does not show it until staff publish it. +test.describe("Project submission — the real write path", () => { + test.beforeEach(async ({ context, baseURL }) => { + await signIn(context, DEMO_ACCOUNTS.user, baseURL); + }); + + test("a submission with no interception is accepted and does NOT appear in the gallery", async ({ + page, + }) => { + const statuses: number[] = []; + page.on("response", (res) => { + if (res.url().includes("/api/projects")) statuses.push(res.status()); + }); + + await page.goto("/projects/new"); + await page + .getByRole("textbox", { name: "Project title" }) + .fill("Unpublished by design"); + await page + .getByRole("textbox", { name: /Write-up/ }) + .fill("Submitted end to end against the real route."); + await page.getByRole("button", { name: "Submit project" }).click(); + + await expect( + page.getByRole("heading", { name: /pending review/i, level: 1 }) + ).toBeVisible(); + // A 503 here would mean the route still thinks it needs a credential. + expect(statuses).toEqual([201]); + + // The whole point of `published: false`: it is stored, and invisible. + await page.goto("/projects"); + await expect(page.getByText("Unpublished by design")).toHaveCount(0); + // The published sample is still there, so the assertion above is about + // publication rather than an empty gallery. + await expect( + page.getByRole("link").filter({ hasText: "Laser-cut plywood lamp" }).first() + ).toBeVisible(); + }); + + test("the photo control says uploads are unavailable with no blob store", async ({ + page, + }) => { + // The E2E server runs with BLOB_READ_WRITE_TOKEN unset, which is the whole + // credential-free premise. The honest degradation is worth asserting: the + // route refuses, the message is translated, and the form still submits. + await page.goto("/projects/new"); + + await page + .locator('input[type="file"]') + .setInputFiles({ + name: "lamp.png", + mimeType: "image/png", + buffer: Buffer.from( + "89504e470d0a1a0a0000000d49484452000000010000000108060000001f15c489", + "hex" + ), + }); + + // `.project-form-error` rather than role=alert: Next's route announcer is + // also a live region, and two matches is a strict-mode violation. + await expect(page.locator("p.project-form-error")).toContainText( + "Photo uploads are unavailable" + ); + await expect( + page.getByRole("button", { name: "Submit project" }) + ).toBeEnabled(); + }); +}); + +// Spec §10 scenario 1's last clause: an anonymous visitor browses, opens a +// tool, and *cannot submit a project*. +test.describe("Project submission — anonymous visitors", () => { + test("/projects/new offers the sign-in prompt instead of the form", async ({ + page, + }) => { + // No cookie. The real /api/identity answers anonymous, which is what an + // ISAM attendee who never creates an account gets. + await page.goto("/projects/new"); + + await expect( + page.getByRole("heading", { name: "Sign in to share your project", level: 1 }) + ).toBeVisible(); + await expect(page.getByRole("button", { name: "Submit project" })).toHaveCount(0); + await expect(page.getByRole("textbox", { name: "Project title" })).toHaveCount(0); + + // Not a dead end and not a redirect: browsing stays open to them. + await expect(page.getByRole("link", { name: "Browse projects" })).toBeVisible(); + await expect(page).toHaveURL(/\/projects\/new$/); + await expect(page.locator("body")).not.toContainText(PLACEHOLDER_LEAK); + }); + + test("the route itself refuses an anonymous POST, not only the form", async ({ + request, + }) => { + // The form's prompt is presentation; this is the control (§8). + const res = await request.post("/api/projects", { + data: { title: "Sneaked in", body: "Posted without a session." }, + }); + + expect(res.status()).toBe(401); + expect(await res.json()).toMatchObject({ code: "sign_in_required" }); + }); +}); diff --git a/v5/e2e/utils/session.ts b/v5/e2e/utils/session.ts new file mode 100644 index 0000000..4abbebe --- /dev/null +++ b/v5/e2e/utils/session.ts @@ -0,0 +1,42 @@ +import type { BrowserContext } from "@playwright/test"; + +import { + BETTER_AUTH_SESSION_COOKIE, + signCookieValue, +} from "../../test/utils/better-auth-cookie"; + +/** + * Being somebody in an E2E run, without Google. + * + * Sessions are database rows since Phase 4, so the demo seed ships an account + * per role with a known session token (`src/lib/db/demo-seed.ts`), the + * Playwright server boots with a test-only `AUTH_SECRET`, and this signs a + * cookie for one of them exactly the way Better Auth would. Nothing is + * intercepted: the real `/api/identity` reads the real session row. + * + * It imports only `test/utils/better-auth-cookie.ts`, which has no imports of + * its own — deliberately, so an E2E spec never drags a `server-only` module + * into the Playwright process. + */ + +/** Must match `AUTH_SECRET` in playwright.config.ts's `webServer.env`. */ +export const E2E_AUTH_SECRET = "e2e-only-secret-not-used-anywhere-else"; + +export const E2E_BASE_URL = "http://localhost:3100"; + +/** Put a properly signed session cookie for a demo account in the browser. */ +export async function signIn( + context: BrowserContext, + account: { sessionToken: string }, + baseURL?: string +): Promise { + await context.addCookies([ + { + name: BETTER_AUTH_SESSION_COOKIE, + value: await signCookieValue(account.sessionToken, E2E_AUTH_SECRET), + url: baseURL ?? E2E_BASE_URL, + }, + ]); +} + +export { BETTER_AUTH_SESSION_COOKIE, signCookieValue }; diff --git a/v5/messages/ar.json b/v5/messages/ar.json index 6b77e51..62173e7 100644 --- a/v5/messages/ar.json +++ b/v5/messages/ar.json @@ -135,6 +135,7 @@ "cancel": "إلغاء", "onlyImages": "يتم دعم ملفات الصور فقط.", "uploadFailed": "فشل رفع الصورة.", + "uploadsUnavailable": "رفع الصور غير متاح حاليًا. لا يزال بإمكانك إرسال مشروعك بدون صور.", "requiredError": "العنوان واسمك والوصف حقول مطلوبة.", "submitError": "حدث خطأ ما. يرجى المحاولة مرة أخرى.", "thanksTitle": "شكرًا — مشروعك قيد المراجعة", @@ -175,6 +176,7 @@ "uploadingAria": "جارٍ رفع الصورة", "onlyImages": "يُسمح بملفات الصور فقط.", "uploadFailed": "فشل رفع الصورة", + "uploadsUnavailable": "رفع الصور غير متاح حاليًا. لا يزال بإمكانك إرسال رسالتك بدون صورة.", "toolRunningAria": "الأداة قيد التشغيل", "typingAria": "المساعد يكتب", "readingManualsAria": "جارٍ قراءة الأدلة", diff --git a/v5/messages/en.json b/v5/messages/en.json index ed11fa2..d51ebcc 100644 --- a/v5/messages/en.json +++ b/v5/messages/en.json @@ -21,7 +21,8 @@ "signInUnconfigured": "Sign-in isn't set up on this deployment yet.", "signInFailed": "Couldn't start sign-in. Try again.", "signOut": "SIGN OUT", - "signedInAria": "Signed in as {name}" + "signedInAria": "Signed in as {name}", + "admin": "ADMIN" }, "catalogRefresh": { "action": "REFRESH", @@ -137,11 +138,22 @@ "cancel": "Cancel", "onlyImages": "Only image files are supported.", "uploadFailed": "Photo upload failed.", - "requiredError": "Title, your name, and a write-up are required.", + "uploadsUnavailable": "Photo uploads are unavailable right now. You can still submit your project without photos.", + "requiredError": "A title and a write-up are required.", "submitError": "Something went wrong. Please try again.", + "signInRequiredError": "You need to be signed in to submit a project. Use the Sign in control in the header, then submit again — nothing you have typed is lost.", "thanksTitle": "Thanks — your project is pending review", "thanksBody": "A staff member will review your submission and publish it to the gallery soon.", - "backToGallery": "Back to gallery" + "thanksPhotosNone": "Your photos were not attached, so the write-up was saved without them. An uploaded photo is discarded after a day, so pictures added to a form left open overnight are already gone.", + "thanksPhotosSome": "Some of your photos were not attached, so the write-up was saved without them. An uploaded photo is discarded after a day, so pictures added to a form left open overnight are already gone.", + "backToGallery": "Back to gallery", + "signInTitle": "Sign in to share your project", + "signInBody": "Project write-ups are credited to your {institution} account, so you need to be signed in to submit one. Use the Sign in control in the header.", + "signInBrowse": "Browse projects", + "identityUnknown": "We could not check whether you are signed in just now, so this page cannot tell you either way. You can keep writing — if you are signed in, submitting will work.", + "identityRetry": "Check again", + "identityChecking": "Checking…", + "authorNote": "Your project will be credited to {name}." }, "about": { "eyebrow": "About", @@ -177,6 +189,7 @@ "uploadingAria": "Uploading photo", "onlyImages": "Only image files are supported.", "uploadFailed": "Photo upload failed", + "uploadsUnavailable": "Photo uploads are unavailable right now. You can still send your message without a photo.", "toolRunningAria": "Tool running", "typingAria": "Assistant is typing", "readingManualsAria": "Reading manuals", @@ -265,5 +278,55 @@ "demoBanner": { "label": "Demo data", "body": "This catalogue is built-in sample data, not {institution}'s real inventory. Notion is not configured." + }, + "admin": { + "eyebrow": "Admin", + "title": "Admin", + "loading": "Checking who you are…", + "backToCatalog": "Back to the catalog", + "signedOutTitle": "You are not signed in", + "signedOutBody": "Admin pages need an account. Use the Sign in control in the header, then come back.", + "forbiddenTitle": "You do not have access to this page", + "forbiddenBody": "Your account is signed in, but it does not hold the permission this page needs. Ask a super admin if you think it should.", + "indexTitle": "Admin surfaces", + "indexNothingYet": "Nothing here is open to your account yet.", + "indexMoreComing": "Inventory, intake, maintenance and corrections arrive in later phases.", + "usersTitle": "People", + "usersLede": "Everyone who has signed in. A role change takes effect on that person's next request.", + "tableLabel": "People and their roles", + "columnPerson": "Person", + "columnRole": "Role", + "columnAccess": "Access", + "columnJoined": "First signed in", + "noUsers": "Nobody has signed in yet. The first person to sign in appears here.", + "you": "you", + "roleForPerson": "Role for {name}", + "saving": "Saving…", + "saved": "Saved", + "banned": "Banned", + "bannedWithReason": "Banned — {reason}", + "banFor": "Ban", + "liftBanFor": "Lift ban", + "banReasonPlaceholder": "Reason (optional)", + "banReasonFor": "Reason for banning {name}", + "roles": { + "user": "Student", + "admin": "SuperMaker", + "super_admin": "Director" + }, + "errors": { + "not_signed_in": "You are not signed in any more. Sign in and try again.", + "not_permitted": "Your account does not hold the permission this needs.", + "rate_limited": "Too many changes at once. Wait a moment and try again.", + "unknown_user": "That account no longer exists.", + "invalid_role": "That is not a role this app knows.", + "protected_floor": "This address is protected in the deployment's settings and cannot be demoted or banned.", + "last_super_admin": "This is the last director. Promote somebody else first, or nobody could undo it.", + "self_ban": "You cannot ban yourself.", + "failed": "That did not save. Nothing was changed." + }, + "warnings": { + "audit_unavailable": "Saved, but this change could not be written to the audit log. Tell whoever runs the deployment." + } } } diff --git a/v5/messages/es.json b/v5/messages/es.json index aeb4250..e5419e3 100644 --- a/v5/messages/es.json +++ b/v5/messages/es.json @@ -135,6 +135,7 @@ "cancel": "Cancelar", "onlyImages": "Solo se admiten archivos de imagen.", "uploadFailed": "Error al subir la foto.", + "uploadsUnavailable": "La subida de fotos no está disponible en este momento. Puedes enviar tu proyecto sin fotos.", "requiredError": "El título, tu nombre y una descripción son obligatorios.", "submitError": "Algo salió mal. Inténtalo de nuevo.", "thanksTitle": "Gracias: tu proyecto está pendiente de revisión", @@ -175,6 +176,7 @@ "uploadingAria": "Subiendo foto", "onlyImages": "Solo se admiten archivos de imagen.", "uploadFailed": "Error al subir la foto", + "uploadsUnavailable": "La subida de fotos no está disponible en este momento. Puedes enviar tu mensaje sin foto.", "toolRunningAria": "Herramienta en ejecución", "typingAria": "El asistente está escribiendo", "readingManualsAria": "Leyendo manuales", diff --git a/v5/messages/fr.json b/v5/messages/fr.json index 2302fd0..4dbe57d 100644 --- a/v5/messages/fr.json +++ b/v5/messages/fr.json @@ -135,6 +135,7 @@ "cancel": "Annuler", "onlyImages": "Seuls les fichiers image sont pris en charge.", "uploadFailed": "Échec du téléversement de la photo.", + "uploadsUnavailable": "L'envoi de photos est indisponible pour le moment. Vous pouvez soumettre votre projet sans photos.", "requiredError": "Le titre, votre nom et une description sont obligatoires.", "submitError": "Une erreur est survenue. Veuillez réessayer.", "thanksTitle": "Merci — votre projet est en attente de validation", @@ -175,6 +176,7 @@ "uploadingAria": "Téléversement de la photo", "onlyImages": "Seuls les fichiers image sont pris en charge.", "uploadFailed": "Échec du téléversement de la photo", + "uploadsUnavailable": "L'envoi de photos est indisponible pour le moment. Vous pouvez envoyer votre message sans photo.", "toolRunningAria": "Outil en cours d'exécution", "typingAria": "L'assistant est en train d'écrire", "readingManualsAria": "Lecture des manuels", diff --git a/v5/messages/he.json b/v5/messages/he.json index abb3e72..786f619 100644 --- a/v5/messages/he.json +++ b/v5/messages/he.json @@ -135,6 +135,7 @@ "cancel": "ביטול", "onlyImages": "נתמכים קבצי תמונה בלבד.", "uploadFailed": "העלאת התמונה נכשלה.", + "uploadsUnavailable": "העלאת תמונות אינה זמינה כרגע. עדיין אפשר להגיש את הפרויקט בלי תמונות.", "requiredError": "שם, שמכם ותיאור הם שדות חובה.", "submitError": "משהו השתבש. נסו שוב.", "thanksTitle": "תודה — הפרויקט שלכם ממתין לבדיקה", @@ -175,6 +176,7 @@ "uploadingAria": "מעלה תמונה", "onlyImages": "נתמכים קבצי תמונה בלבד.", "uploadFailed": "העלאת התמונה נכשלה", + "uploadsUnavailable": "העלאת תמונות אינה זמינה כרגע. עדיין אפשר לשלוח את ההודעה בלי תמונה.", "toolRunningAria": "הכלי פועל", "typingAria": "העוזר מקליד", "readingManualsAria": "קורא מדריכים", diff --git a/v5/messages/hi.json b/v5/messages/hi.json index 2dfe5ca..ce3ac87 100644 --- a/v5/messages/hi.json +++ b/v5/messages/hi.json @@ -135,6 +135,7 @@ "cancel": "रद्द करें", "onlyImages": "केवल छवि फ़ाइलें समर्थित हैं।", "uploadFailed": "तस्वीर अपलोड विफल।", + "uploadsUnavailable": "फ़ोटो अपलोड अभी उपलब्ध नहीं है। आप बिना फ़ोटो के भी अपना प्रोजेक्ट जमा कर सकते हैं।", "requiredError": "शीर्षक, आपका नाम और विवरण आवश्यक हैं।", "submitError": "कुछ गलत हुआ। कृपया पुनः प्रयास करें।", "thanksTitle": "धन्यवाद — आपकी परियोजना समीक्षा हेतु लंबित है", @@ -175,6 +176,7 @@ "uploadingAria": "फ़ोटो अपलोड हो रही है", "onlyImages": "केवल छवि फ़ाइलें समर्थित हैं।", "uploadFailed": "फ़ोटो अपलोड विफल", + "uploadsUnavailable": "फ़ोटो अपलोड अभी उपलब्ध नहीं है। आप बिना फ़ोटो के भी अपना संदेश भेज सकते हैं।", "toolRunningAria": "उपकरण चल रहा है", "typingAria": "सहायक टाइप कर रहा है", "readingManualsAria": "मैनुअल पढ़ रहा है", diff --git a/v5/messages/ja.json b/v5/messages/ja.json index 51fbbed..ed432bd 100644 --- a/v5/messages/ja.json +++ b/v5/messages/ja.json @@ -135,6 +135,7 @@ "cancel": "キャンセル", "onlyImages": "画像ファイルのみ対応しています。", "uploadFailed": "写真のアップロードに失敗しました。", + "uploadsUnavailable": "写真のアップロードは現在利用できません。写真なしでもプロジェクトを投稿できます。", "requiredError": "タイトル、お名前、説明は必須です。", "submitError": "問題が発生しました。もう一度お試しください。", "thanksTitle": "ありがとうございます — プロジェクトは審査待ちです", @@ -175,6 +176,7 @@ "uploadingAria": "写真をアップロード中", "onlyImages": "画像ファイルのみ対応しています。", "uploadFailed": "写真のアップロードに失敗しました", + "uploadsUnavailable": "写真のアップロードは現在利用できません。写真なしでもメッセージを送信できます。", "toolRunningAria": "ツール実行中", "typingAria": "アシスタントが入力中です", "readingManualsAria": "マニュアルを読み込み中", diff --git a/v5/messages/ko.json b/v5/messages/ko.json index 1d38b4e..0b14239 100644 --- a/v5/messages/ko.json +++ b/v5/messages/ko.json @@ -135,6 +135,7 @@ "cancel": "취소", "onlyImages": "이미지 파일만 지원됩니다.", "uploadFailed": "사진 업로드에 실패했습니다.", + "uploadsUnavailable": "사진 업로드를 현재 사용할 수 없습니다. 사진 없이도 프로젝트를 제출할 수 있습니다.", "requiredError": "제목, 이름, 설명은 필수입니다.", "submitError": "문제가 발생했습니다. 다시 시도해 주세요.", "thanksTitle": "감사합니다 — 프로젝트가 검토 대기 중입니다", @@ -175,6 +176,7 @@ "uploadingAria": "사진 업로드 중", "onlyImages": "이미지 파일만 지원됩니다.", "uploadFailed": "사진 업로드 실패", + "uploadsUnavailable": "사진 업로드를 현재 사용할 수 없습니다. 사진 없이도 메시지를 보낼 수 있습니다.", "toolRunningAria": "도구 실행 중", "typingAria": "어시스턴트가 입력 중입니다", "readingManualsAria": "매뉴얼 읽는 중", diff --git a/v5/messages/pt-BR.json b/v5/messages/pt-BR.json index d2c9bcb..fdc17fc 100644 --- a/v5/messages/pt-BR.json +++ b/v5/messages/pt-BR.json @@ -135,6 +135,7 @@ "cancel": "Cancelar", "onlyImages": "Apenas arquivos de imagem são suportados.", "uploadFailed": "Falha ao enviar a foto.", + "uploadsUnavailable": "O envio de fotos está indisponível no momento. Você ainda pode enviar seu projeto sem fotos.", "requiredError": "Título, seu nome e uma descrição são obrigatórios.", "submitError": "Algo deu errado. Tente novamente.", "thanksTitle": "Obrigado — seu projeto está aguardando revisão", @@ -175,6 +176,7 @@ "uploadingAria": "Enviando foto", "onlyImages": "Somente arquivos de imagem são aceitos.", "uploadFailed": "Falha ao enviar a foto", + "uploadsUnavailable": "O envio de fotos está indisponível no momento. Você ainda pode enviar sua mensagem sem foto.", "toolRunningAria": "Ferramenta em execução", "typingAria": "O assistente está digitando", "readingManualsAria": "Lendo manuais", diff --git a/v5/messages/ru.json b/v5/messages/ru.json index ba642d7..2cabca7 100644 --- a/v5/messages/ru.json +++ b/v5/messages/ru.json @@ -135,6 +135,7 @@ "cancel": "Отмена", "onlyImages": "Поддерживаются только файлы изображений.", "uploadFailed": "Не удалось загрузить фото.", + "uploadsUnavailable": "Загрузка фото сейчас недоступна. Вы можете отправить проект без фотографий.", "requiredError": "Название, ваше имя и описание обязательны.", "submitError": "Что-то пошло не так. Попробуйте снова.", "thanksTitle": "Спасибо — ваш проект ожидает проверки", @@ -175,6 +176,7 @@ "uploadingAria": "Загрузка фото", "onlyImages": "Поддерживаются только файлы изображений.", "uploadFailed": "Не удалось загрузить фото", + "uploadsUnavailable": "Загрузка фото сейчас недоступна. Вы можете отправить сообщение без фото.", "toolRunningAria": "Инструмент выполняется", "typingAria": "Ассистент печатает", "readingManualsAria": "Чтение руководств", diff --git a/v5/messages/tr.json b/v5/messages/tr.json index 68d2053..e7e5874 100644 --- a/v5/messages/tr.json +++ b/v5/messages/tr.json @@ -135,6 +135,7 @@ "cancel": "İptal", "onlyImages": "Yalnızca resim dosyaları desteklenir.", "uploadFailed": "Fotoğraf yüklenemedi.", + "uploadsUnavailable": "Fotoğraf yükleme şu anda kullanılamıyor. Projenizi fotoğrafsız da gönderebilirsiniz.", "requiredError": "Başlık, adın ve bir açıklama gereklidir.", "submitError": "Bir şeyler ters gitti. Lütfen tekrar deneyin.", "thanksTitle": "Teşekkürler — projen inceleme bekliyor", @@ -175,6 +176,7 @@ "uploadingAria": "Fotoğraf yükleniyor", "onlyImages": "Yalnızca görsel dosyaları desteklenir.", "uploadFailed": "Fotoğraf yüklenemedi", + "uploadsUnavailable": "Fotoğraf yükleme şu anda kullanılamıyor. Mesajınızı fotoğrafsız da gönderebilirsiniz.", "toolRunningAria": "Araç çalışıyor", "typingAria": "Asistan yazıyor", "readingManualsAria": "Kılavuzlar okunuyor", diff --git a/v5/messages/zh-CN.json b/v5/messages/zh-CN.json index 804200e..c1ab976 100644 --- a/v5/messages/zh-CN.json +++ b/v5/messages/zh-CN.json @@ -135,6 +135,7 @@ "cancel": "取消", "onlyImages": "仅支持图片文件。", "uploadFailed": "照片上传失败。", + "uploadsUnavailable": "照片上传当前不可用。您仍可以不带照片提交项目。", "requiredError": "标题、你的姓名和说明为必填项。", "submitError": "出现问题，请重试。", "thanksTitle": "谢谢——你的项目正在等待审核", @@ -175,6 +176,7 @@ "uploadingAria": "正在上传照片", "onlyImages": "仅支持图片文件。", "uploadFailed": "照片上传失败", + "uploadsUnavailable": "照片上传当前不可用。您仍可以不带照片发送消息。", "toolRunningAria": "工具运行中", "typingAria": "助手正在输入", "readingManualsAria": "正在阅读手册", diff --git a/v5/playwright.config.ts b/v5/playwright.config.ts index d31f250..b9855dd 100644 --- a/v5/playwright.config.ts +++ b/v5/playwright.config.ts @@ -64,6 +64,26 @@ export default defineConfig({ NOTION_DB_RESOURCES: "", NOTION_DB_MAINTENANCE_LOGS: "", NOTION_DB_FLAGS: "", + // Blanked so the run is the same on a machine that happens to have a + // Blob store linked: uploads must take the "unavailable" branch, which + // is itself asserted in projects.spec.ts, and no test may put bytes in + // somebody's real store. + BLOB_READ_WRITE_TOKEN: "", + // Same reasoning for the nightly job: no E2E test should be able to + // trigger a real backup. + CRON_SECRET: "", + // A test-only signing key, so sessions are real rows and a spec can be + // somebody by presenting a properly signed cookie for one of the demo + // accounts (src/lib/db/demo-seed.ts). GOOGLE_* stay blank below, so + // *starting* a session is still impossible here: /api/auth/sign-in/social + // answers 503 and the header says sign-in is not set up. That is the + // state a deployment is in before its OAuth client exists, and it is the + // state the anonymous specs assert against. + AUTH_SECRET: "e2e-only-secret-not-used-anywhere-else", + AUTH_BASE_URL: "http://localhost:3100", + GOOGLE_CLIENT_ID: "", + GOOGLE_CLIENT_SECRET: "", + AUTH_SUPER_ADMIN_EMAILS: "", }, }, }); diff --git a/v5/src/app/admin/layout.tsx b/v5/src/app/admin/layout.tsx new file mode 100644 index 0000000..8823761 --- /dev/null +++ b/v5/src/app/admin/layout.tsx @@ -0,0 +1,62 @@ +import { Suspense } from "react"; +import { getTranslations } from "next-intl/server"; +import { AdminNotice } from "../../components/admin/AdminNotice"; +import { resolveIdentityFromHeaders } from "../../lib/auth/identity"; +import { canReachAdmin } from "../../lib/auth/permissions"; +import { siteConfig } from "../../lib/site-config"; + +/** + * The `/admin` shell and its front door (spec §6, §8). + * + * **The gate is here, once, and each page checks again.** This layout answers + * the coarse question — may this person see an admin surface at all — so that + * every page under it can assume a signed-in someone and check only the + * permission it actually needs. `/admin/users` gates on `users.manage`; a + * SuperMaker gets past this layout and is refused there, which is the honest + * answer rather than a 404. + * + * **It refuses, it never throws.** No `notFound()` and no redirect: a 404 would + * lie about the page existing and a redirect to sign-in would lose where they + * were going. `AdminNotice` says which of the two situations this is. + * + * The identity read lives in its own `Suspense` boundary because + * `cacheComponents` is enabled: reading request headers marks this subtree + * dynamic, and the boundary is what lets the rest of the shell stay static. + */ + +export const metadata = { + title: `Admin — ${siteConfig.name}`, +}; + +export default async function AdminLayout({ + children, +}: Readonly<{ children: React.ReactNode }>) { + const t = await getTranslations("admin"); + + return ( +
+
+
+ +

{t("title")}

+
+
+ + {t("loading")}

}> + {children} + +
+ ); +} + +/** Resolves who is asking, and renders the children only if they may be here. */ +async function AdminGate({ children }: { children: React.ReactNode }) { + const identity = await resolveIdentityFromHeaders(); + + if (identity.role === "anonymous") return ; + if (!canReachAdmin(identity)) return ; + + return <>{children}; +} diff --git a/v5/src/app/admin/page.tsx b/v5/src/app/admin/page.tsx new file mode 100644 index 0000000..b121104 --- /dev/null +++ b/v5/src/app/admin/page.tsx @@ -0,0 +1,44 @@ +import Link from "next/link"; +import { getTranslations } from "next-intl/server"; +import { resolveIdentityFromHeaders } from "../../lib/auth/identity"; +import { can } from "../../lib/auth/permissions"; + +/** + * `/admin` — the index the header's `AdminLink` points at. + * + * Phase 4 builds one surface, so this is a short list rather than the + * `AdminHome` of spec §6 (counts, the intake queue, open tickets, mirror + * status) — those need the tables and queues later phases add. What it must do + * now is be honest: a SuperMaker who holds `tools.edit` but not `users.manage` + * reaches this page, sees nothing they can open yet, and is told that, instead + * of following a link into a refusal. + * + * The layout above has already established that this person may see an admin + * surface at all. + */ + +export default async function AdminHomePage() { + const t = await getTranslations("admin"); + const identity = await resolveIdentityFromHeaders(); + const manageUsers = can(identity, "users.manage"); + + return ( +
+

{t("eyebrow")}

+

{t("indexTitle")}

+ + {manageUsers ? ( +
    +
  • + {t("usersTitle")} + {t("usersLede")} +
    • +
+ ) : ( +

{t("indexNothingYet")}

+ )} + +

{t("indexMoreComing")}

+
+ ); +} diff --git a/v5/src/app/admin/users/action-result.ts b/v5/src/app/admin/users/action-result.ts new file mode 100644 index 0000000..2208f48 --- /dev/null +++ b/v5/src/app/admin/users/action-result.ts @@ -0,0 +1,56 @@ +import type { Role } from "../../../lib/db/schema/vocabulary"; + +/** + * What a `/admin/users` server action answers, and where it lives. + * + * Its own module because `actions.ts` carries `"use server"`, and a module with + * that directive may export **only async functions** — every export becomes a + * callable endpoint. A shared constant and a result type therefore cannot live + * there, and a client island that only needs the shape should not have to + * import the endpoints to get it. + */ + +/** The page these actions belong to, and the path they invalidate. */ +export const ADMIN_USERS_PATH = "/admin/users"; + +/** + * Why an action did nothing. Every code has an `admin.errors.` message in + * `messages/en.json`; the union is what keeps the two in step. + * + * - `not_signed_in` / `not_permitted` — told apart on purpose: one is + * actionable and the other is not. + * - `protected_floor` — the address is in `AUTH_SUPER_ADMIN_EMAILS`. + * - `last_super_admin` — the change would leave nobody holding `users.manage`. + * - `self_ban` — banning yourself; the plugin refuses it too. + * - `failed` — the write did not land. Deliberately opaque to the browser. + */ +export type AdminActionError = + | "not_signed_in" + | "not_permitted" + | "rate_limited" + | "unknown_user" + | "invalid_role" + | "protected_floor" + | "last_super_admin" + | "self_ban" + | "failed"; + +/** + * A change that landed with less than the full guarantee behind it. + * + * - `audit_unavailable` — the row changed and `audit_events` did not record it. + * + * It rides on `ok: true` deliberately. The plugin's write commits in its own + * statement, and with the Neon HTTP driver the audit insert is a second request + * that can fail on its own; reporting that as a failure would make the island + * snap back to the previous value and leave the page asserting a role the + * database no longer holds — the one thing `RoleSelect` promises never to do. + * Reporting it as a plain success would leave a hole in the trail nobody was + * told about (spec §4.11, Article 4). So it is a success that says what is + * missing, and every code has an `admin.warnings.` message. + */ +export type AdminActionWarning = "audit_unavailable"; + +export type AdminActionResult = + | { ok: true; role?: Role; banned?: boolean; warning?: AdminActionWarning } + | { ok: false; error: AdminActionError }; diff --git a/v5/src/app/admin/users/actions.audit.test.ts b/v5/src/app/admin/users/actions.audit.test.ts new file mode 100644 index 0000000..0bae903 --- /dev/null +++ b/v5/src/app/admin/users/actions.audit.test.ts @@ -0,0 +1,207 @@ +// @vitest-environment node +import { nextCacheMock } from "../../../../test/mocks/next-cache"; +import { nextHeadersMock, setMockHeaders } from "../../../../test/mocks/next-headers"; + +vi.mock("next/cache", () => nextCacheMock()); +vi.mock("next/headers", () => nextHeadersMock()); + +// The one seam `actions.test.ts` cannot have: a database that answers the +// *second* statement with an error. `vi.hoisted` because the `vi.mock` factory +// runs before module scope exists. +const audit = vi.hoisted(() => ({ failing: false })); + +vi.mock("../../../lib/data/audit", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + recordAuditEvent: async (event: Parameters[0]) => { + if (audit.failing) throw new Error("connection terminated unexpectedly"); + return actual.recordAuditEvent(event); + }, + }; +}); + +import { eq } from "drizzle-orm"; + +import { resetAuthForTests } from "../../../lib/auth/config"; +import { getDb, resetDbForTests } from "../../../lib/db/client"; +import { auditEvents, session, user } from "../../../lib/db/schema/index"; +import { findUserById } from "../../../lib/data/users"; +import { seedUser, signInAsNew } from "../../../../test/utils/session"; +import { setUserBanned, setUserRole } from "./actions"; + +/** + * What happens when the change lands and the audit event does not (§4.11). + * + * Its own file because it is the only one here that replaces `data/audit.ts`: + * `actions.test.ts` asserts against the real table, and a module mocked for one + * test in a file is a module mocked for all of them. The failure is worth + * provoking because it is not exotic — `auth.api.setRole` commits in its own + * statement and, on the Neon HTTP driver, the audit insert is a separate + * request that can fail on its own. + * + * **The property under test is that the browser is never told the change failed + * when it did not.** `RoleSelect` and `BanToggle` answer a refusal by restoring + * the previous value, so an exception here would leave the page asserting a + * role the database no longer holds. + */ + +const AUTH_SECRET = "admin-users-audit-test-secret"; + +beforeEach(async () => { + vi.stubEnv("DATABASE_URL", ""); + vi.stubEnv("AUTH_SECRET", AUTH_SECRET); + vi.stubEnv("AUTH_SUPER_ADMIN_EMAILS", ""); + resetAuthForTests(); + audit.failing = false; + + const db = await getDb(); + await db.delete(auditEvents); + await db.delete(session); + await db.delete(user); +}); + +afterEach(() => { + audit.failing = false; + resetAuthForTests(); + resetDbForTests(); +}); + +async function asDirector() { + const signedIn = await signInAsNew({ + email: "director@cornell.edu", + role: "super_admin", + name: "Dee Rector", + }); + setMockHeaders({ cookie: signedIn.cookie }); + return signedIn; +} + +describe("an audit write that fails after the change landed", () => { + it("reports the role change as a success, with the gap named", async () => { + await asDirector(); + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + audit.failing = true; + + expect(await setUserRole({ userId: target.id, role: "admin" })).toEqual({ + ok: true, + role: "admin", + warning: "audit_unavailable", + }); + // The half that makes the answer honest: the row really did move. + expect((await findUserById(target.id))?.role).toBe("admin"); + }); + + it("does not throw out of the server action", async () => { + await asDirector(); + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + audit.failing = true; + + // A rejected action reaches the island as a caught failure, and the island + // answers that by putting the old role back — over a database holding the + // new one. + await expect(setUserRole({ userId: target.id, role: "admin" })).resolves.toMatchObject( + { ok: true } + ); + }); + + it("reports a ban the same way, and the ban still stands", async () => { + await asDirector(); + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + audit.failing = true; + + expect( + await setUserBanned({ userId: target.id, banned: true, reason: "spam" }) + ).toEqual({ + ok: true, + banned: true, + warning: "audit_unavailable", + }); + expect((await findUserById(target.id))?.banned).toBe(true); + }); + + it("carries no warning when the trail was written", async () => { + await asDirector(); + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + + // The ordinary path, asserted here too so the warning cannot become a + // constant that nobody notices is always set. + expect(await setUserRole({ userId: target.id, role: "admin" })).toEqual({ + ok: true, + role: "admin", + }); + }); +}); + +/** + * The same property, one function earlier. + * + * `authorize()` reconciles the super-admin floor *before* the action's own + * write, and that reconciliation is itself a row UPDATE followed by audit + * inserts. While those inserts threw, a floor director whose row was stale hit + * this: their own row was promoted — and any ban on it lifted — and then the + * throw was caught and returned as `failed`, so the page said "nothing was + * changed" over a database that had changed two columns and recorded neither. + * + * The recovery path is exactly where a silent, unrecorded promotion is least + * acceptable, which is why it is asserted separately from the actions above. + */ +describe("an audit write that fails during the floor reconciliation", () => { + it("still performs the action, and names the gap instead of denying it", async () => { + vi.stubEnv("AUTH_SUPER_ADMIN_EMAILS", "founder@cornell.edu"); + resetAuthForTests(); + + // Signed in first, then the row is spoiled behind them: a restored backup + // or a manual UPDATE, which is the only way a floor row is banned at all. + const founder = await signInAsNew({ + email: "founder@cornell.edu", + role: "user", + name: "Fou Nder", + }); + setMockHeaders({ cookie: founder.cookie }); + const db = await getDb(); + await db + .update(user) + .set({ banned: true, banReason: "mistake" }) + .where(eq(user.id, founder.user.id)); + + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + audit.failing = true; + + expect(await setUserRole({ userId: target.id, role: "admin" })).toEqual({ + ok: true, + role: "admin", + warning: "audit_unavailable", + }); + + // Both rows moved, and the caller is told the trail is incomplete rather + // than being told nothing happened. + expect((await findUserById(target.id))?.role).toBe("admin"); + const reconciled = await findUserById(founder.user.id); + expect(reconciled?.role).toBe("super_admin"); + expect(reconciled?.banned).toBe(false); + }); + + it("warns even when the action itself changes nothing", async () => { + vi.stubEnv("AUTH_SUPER_ADMIN_EMAILS", "founder@cornell.edu"); + resetAuthForTests(); + + const founder = await signInAsNew({ + email: "founder@cornell.edu", + role: "user", + name: "Fou Nder", + }); + setMockHeaders({ cookie: founder.cookie }); + const target = await seedUser({ email: "student@cornell.edu", role: "admin" }); + audit.failing = true; + + // "admin → admin" writes no event of its own, so the only gap in the trail + // is the reconciliation's — and it is still a gap. + expect(await setUserRole({ userId: target.id, role: "admin" })).toEqual({ + ok: true, + role: "admin", + warning: "audit_unavailable", + }); + expect((await findUserById(founder.user.id))?.role).toBe("super_admin"); + }); +}); diff --git a/v5/src/app/admin/users/actions.test.ts b/v5/src/app/admin/users/actions.test.ts new file mode 100644 index 0000000..a9f80d6 --- /dev/null +++ b/v5/src/app/admin/users/actions.test.ts @@ -0,0 +1,467 @@ +// @vitest-environment node +import { nextCacheMock } from "../../../../test/mocks/next-cache"; +import { nextHeadersMock, setMockHeaders } from "../../../../test/mocks/next-headers"; + +vi.mock("next/cache", () => nextCacheMock()); +vi.mock("next/headers", () => nextHeadersMock()); + +import { eq } from "drizzle-orm"; +import { revalidatePath } from "next/cache"; +import { resetAuthForTests } from "../../../lib/auth/config"; +import { getDb, resetDbForTests } from "../../../lib/db/client"; +import { auditEvents, session, user } from "../../../lib/db/schema/index"; +import { listAuditEvents } from "../../../lib/data/audit"; +import { findUserById } from "../../../lib/data/users"; +import { seedUser, signInAs, signInAsNew } from "../../../../test/utils/session"; +import { setUserBanned, setUserRole } from "./actions"; + +/** + * The two `/admin/users` writes, end to end over PGlite: a real Better Auth + * instance, real session rows, the real admin plugin, the real audit table. + * The only things stubbed are the two Next modules that need a request scope — + * `next/headers` (fed a cookie `test/utils/session.ts` minted) and + * `next/cache`. No network, no Google, no `DATABASE_URL` (Article 3). + * + * `resetAuthForTests()` runs in both hooks: `getAuth()` memoizes on the env + * fingerprint *and* the substrate, so a test that stubs `AUTH_SECRET` and one + * that resets the database must not share an instance. + */ + +const AUTH_SECRET = "admin-users-actions-test-secret"; + +beforeEach(async () => { + vi.stubEnv("DATABASE_URL", ""); + vi.stubEnv("AUTH_SECRET", AUTH_SECRET); + vi.stubEnv("AUTH_SUPER_ADMIN_EMAILS", ""); + resetAuthForTests(); + vi.mocked(revalidatePath).mockClear(); + + // The demo seed ships one account per role; these tests build their own + // cast, so the table starts empty and a count of super admins means what it + // says. Sessions and audit rows go with them (cascade / explicit). + const db = await getDb(); + await db.delete(auditEvents); + await db.delete(session); + await db.delete(user); +}); + +afterEach(() => { + resetAuthForTests(); + resetDbForTests(); +}); + +/** Sign in as a director and point `next/headers` at their cookie. */ +async function asDirector(email = "director@cornell.edu") { + const signedIn = await signInAsNew({ email, role: "super_admin", name: "Dee Rector" }); + setMockHeaders({ cookie: signedIn.cookie }); + return signedIn; +} + +async function roleOf(userId: string) { + return (await findUserById(userId))?.role; +} + +// ── Who may call these at all (§8) ────────────────────────────────── + +describe("the permission gate", () => { + it("refuses an anonymous caller and changes nothing", async () => { + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + setMockHeaders(); + + expect(await setUserRole({ userId: target.id, role: "admin" })).toEqual({ + ok: false, + error: "not_signed_in", + }); + expect(await roleOf(target.id)).toBe("user"); + expect(await listAuditEvents()).toEqual([]); + }); + + it("refuses an ordinary signed-in user", async () => { + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + const caller = await signInAsNew({ email: "someone@cornell.edu", role: "user" }); + setMockHeaders({ cookie: caller.cookie }); + + expect(await setUserRole({ userId: target.id, role: "admin" })).toEqual({ + ok: false, + error: "not_permitted", + }); + expect(await roleOf(target.id)).toBe("user"); + }); + + it("refuses a SuperMaker — `users.manage` belongs to the director alone", async () => { + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + const caller = await signInAsNew({ email: "maker@cornell.edu", role: "admin" }); + setMockHeaders({ cookie: caller.cookie }); + + expect(await setUserRole({ userId: target.id, role: "admin" })).toEqual({ + ok: false, + error: "not_permitted", + }); + expect(await listAuditEvents()).toEqual([]); + }); + + it("refuses a forged cookie rather than trusting it", async () => { + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + const director = await signInAsNew({ + email: "director@cornell.edu", + role: "super_admin", + }); + setMockHeaders({ cookie: director.cookie.replace(/.$/, (c) => (c === "A" ? "B" : "A")) }); + + expect(await setUserRole({ userId: target.id, role: "admin" })).toEqual({ + ok: false, + error: "not_signed_in", + }); + }); + + it("refuses a banned director — a ban bites on the next request", async () => { + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + const banned = await seedUser({ + email: "expired@cornell.edu", + role: "super_admin", + banned: true, + }); + const signedIn = await signInAs(banned); + setMockHeaders({ cookie: signedIn.cookie }); + + expect(await setUserRole({ userId: target.id, role: "admin" })).toEqual({ + ok: false, + error: "not_signed_in", + }); + }); + + it("is bounded before it checks anything (Article 4, §8: 120/min)", async () => { + // The limiter is a per-process singleton and an anonymous caller is keyed + // by a hash of their IP, so this test needs an address no other test in + // the file shares — otherwise it inherits their spent allowance. + setMockHeaders({ "x-forwarded-for": "198.51.100.7" }); + + // Anonymous, so every call is refused on the permission check — until the + // 121st, which is refused *before* it, which is the ordering under test. + const results: string[] = []; + for (let i = 0; i < 121; i += 1) { + const result = await setUserRole({ userId: "whoever", role: "admin" }); + if (!result.ok) results.push(result.error); + } + + expect(results.slice(0, 120).every((error) => error === "not_signed_in")).toBe(true); + expect(results[120]).toBe("rate_limited"); + }); +}); + +// ── Changing a role (§5.2) ────────────────────────────────────────── + +describe("setUserRole", () => { + it("promotes a student to SuperMaker and writes exactly one audit event", async () => { + const director = await asDirector(); + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + + expect(await setUserRole({ userId: target.id, role: "admin" })).toEqual({ + ok: true, + role: "admin", + }); + expect(await roleOf(target.id)).toBe("admin"); + + const events = await listAuditEvents(); + expect(events).toHaveLength(1); + expect(events[0]).toMatchObject({ + actorUserId: director.user.id, + action: "role.changed", + subjectType: "user", + subjectId: target.id, + // Both halves: "became an admin" is unanswerable later without the from. + detail: { from: "user", to: "admin" }, + }); + }); + + it("revalidates the page so the roster shows the change", async () => { + await asDirector(); + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + + await setUserRole({ userId: target.id, role: "admin" }); + + expect(vi.mocked(revalidatePath)).toHaveBeenCalledWith("/admin/users"); + }); + + it("treats a change to the role they already hold as a no-op with no event", async () => { + await asDirector(); + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + + expect(await setUserRole({ userId: target.id, role: "user" })).toEqual({ + ok: true, + role: "user", + }); + expect(await listAuditEvents()).toEqual([]); + }); + + it("refuses a role outside the vocabulary", async () => { + await asDirector(); + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + + expect(await setUserRole({ userId: target.id, role: "staff" })).toEqual({ + ok: false, + error: "invalid_role", + }); + expect(await roleOf(target.id)).toBe("user"); + }); + + it("refuses an id that names nobody", async () => { + await asDirector(); + + expect(await setUserRole({ userId: "nobody-here", role: "admin" })).toEqual({ + ok: false, + error: "unknown_user", + }); + }); + + it("refuses to demote an address on the super-admin floor, and says why", async () => { + await asDirector(); + const floor = await seedUser({ email: "founder@cornell.edu", role: "super_admin" }); + vi.stubEnv("AUTH_SUPER_ADMIN_EMAILS", "founder@cornell.edu"); + + expect(await setUserRole({ userId: floor.id, role: "user" })).toEqual({ + ok: false, + error: "protected_floor", + }); + // The row is untouched, which is the half that matters: a refusal that + // half-applied would be worse than no refusal at all. + expect(await roleOf(floor.id)).toBe("super_admin"); + expect(await listAuditEvents()).toEqual([]); + }); + + it("still allows *promoting* a floor address — the floor is a floor, not a freeze", async () => { + await asDirector(); + const floor = await seedUser({ email: "founder@cornell.edu", role: "super_admin" }); + vi.stubEnv("AUTH_SUPER_ADMIN_EMAILS", "founder@cornell.edu"); + + expect(await setUserRole({ userId: floor.id, role: "super_admin" })).toEqual({ + ok: true, + role: "super_admin", + }); + }); + + it("refuses the last director demoting themselves (spec §10)", async () => { + const director = await asDirector(); + + expect(await setUserRole({ userId: director.user.id, role: "user" })).toEqual({ + ok: false, + error: "last_super_admin", + }); + expect(await roleOf(director.user.id)).toBe("super_admin"); + }); + + it("allows a director to step down once somebody else holds the role", async () => { + const director = await asDirector(); + await seedUser({ email: "successor@cornell.edu", role: "super_admin" }); + + expect(await setUserRole({ userId: director.user.id, role: "admin" })).toEqual({ + ok: true, + role: "admin", + }); + expect(await roleOf(director.user.id)).toBe("admin"); + }); +}); + +// ── Banning (§5.2) ────────────────────────────────────────────────── + +describe("setUserBanned", () => { + it("bans with a reason, records it, and deletes the person's sessions", async () => { + const director = await asDirector(); + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + await signInAs(target); + + expect( + await setUserBanned({ userId: target.id, banned: true, reason: "Ignored the rules" }) + ).toEqual({ ok: true, banned: true }); + + const stored = await findUserById(target.id); + expect(stored).toMatchObject({ banned: true, banReason: "Ignored the rules" }); + + // The plugin deletes their sessions, so the ban bites mid-visit rather + // than at expiry — and `resolveIdentity` would refuse them regardless. + const db = await getDb(); + const rows = await db.select().from(session).where(eq(session.userId, target.id)); + expect(rows).toEqual([]); + + const events = await listAuditEvents(); + expect(events[0]).toMatchObject({ + actorUserId: director.user.id, + action: "user.banned", + subjectId: target.id, + detail: { banned: true, reason: "Ignored the rules" }, + }); + }); + + it("lifts a ban, recorded as the same action with `banned: false`", async () => { + await asDirector(); + const target = await seedUser({ + email: "student@cornell.edu", + role: "user", + banned: true, + }); + + expect(await setUserBanned({ userId: target.id, banned: false })).toEqual({ + ok: true, + banned: false, + }); + expect((await findUserById(target.id))?.banned).toBe(false); + + const events = await listAuditEvents(); + expect(events[0]).toMatchObject({ action: "user.banned", detail: { banned: false } }); + }); + + it("refuses to ban a floor address", async () => { + await asDirector(); + const floor = await seedUser({ email: "founder@cornell.edu", role: "super_admin" }); + vi.stubEnv("AUTH_SUPER_ADMIN_EMAILS", "founder@cornell.edu"); + + expect(await setUserBanned({ userId: floor.id, banned: true })).toEqual({ + ok: false, + error: "protected_floor", + }); + expect((await findUserById(floor.id))?.banned).toBe(false); + }); + + it("refuses to ban yourself", async () => { + const director = await asDirector(); + await seedUser({ email: "successor@cornell.edu", role: "super_admin" }); + + expect(await setUserBanned({ userId: director.user.id, banned: true })).toEqual({ + ok: false, + error: "self_ban", + }); + expect((await findUserById(director.user.id))?.banned).toBe(false); + }); + + it("allows banning another director — the caller is still one", async () => { + // There is deliberately no "last director" guard on the ban path: reaching + // it means the *caller* holds `users.manage`, so the lab keeps one. + await asDirector(); + const other = await seedUser({ email: "other@cornell.edu", role: "super_admin" }); + + expect(await setUserBanned({ userId: other.id, banned: true })).toEqual({ + ok: true, + banned: true, + }); + expect((await findUserById(other.id))?.banned).toBe(true); + }); + + it("is a no-op when the account is already in that state", async () => { + await asDirector(); + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + + expect(await setUserBanned({ userId: target.id, banned: false })).toEqual({ + ok: true, + banned: false, + }); + expect(await listAuditEvents()).toEqual([]); + }); +}); + +// ── The floor has to reach the plugin, not just `can()` (§3.4) ────── + +describe("a floor address whose row has not caught up", () => { + /** + * The regression this guards. `AUTH_SUPER_ADMIN_EMAILS` is applied in two + * places — `databaseHooks.user.create.before`, which only runs at first + * sign-in, and `identityFromSession`, which overrides the resolved role. The + * *plugin* reads `session.user.role` off the row and sees neither. So a floor + * address added after that person had already signed in, or a `super_admin` + * demoted by a restored backup, reached `/admin/users` with every control + * live (the app says they are a director) and every save returning `failed` + * (the plugin says they are a `user`) — the exact lock-out the floor exists + * to undo, with no hint in the message about why. + */ + async function asFloorAddressStoredAs(role: "user" | "admin") { + const signedIn = await signInAsNew({ + email: "founder@cornell.edu", + role, + name: "Fran Ounder", + }); + vi.stubEnv("AUTH_SUPER_ADMIN_EMAILS", "founder@cornell.edu"); + setMockHeaders({ cookie: signedIn.cookie }); + return signedIn; + } + + it("can still change a role, and the row catches up", async () => { + const caller = await asFloorAddressStoredAs("user"); + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + + expect(await setUserRole({ userId: target.id, role: "admin" })).toEqual({ + ok: true, + role: "admin", + }); + expect(await roleOf(target.id)).toBe("admin"); + // Reconciled rather than special-cased: the table now shows the role the + // app has been reporting all along. + expect(await roleOf(caller.user.id)).toBe("super_admin"); + }); + + it("can still ban somebody", async () => { + await asFloorAddressStoredAs("admin"); + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + + expect(await setUserBanned({ userId: target.id, banned: true })).toEqual({ + ok: true, + banned: true, + }); + expect((await findUserById(target.id))?.banned).toBe(true); + }); + + it("records the reconciliation as the role change it is", async () => { + const caller = await asFloorAddressStoredAs("user"); + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + + await setUserRole({ userId: target.id, role: "admin" }); + + const events = await listAuditEvents(); + const reconciliation = events.find((event) => event.subjectId === caller.user.id); + // Null actor: the environment did this, not a person who clicked something. + expect(reconciliation).toMatchObject({ + action: "role.changed", + actorUserId: null, + detail: { from: "user", to: "super_admin", reason: "super_admin_floor" }, + }); + }); + + it("reconciles once, not on every action", async () => { + const caller = await asFloorAddressStoredAs("user"); + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + + await setUserRole({ userId: target.id, role: "admin" }); + await setUserRole({ userId: target.id, role: "user" }); + + const forCaller = (await listAuditEvents()).filter( + (event) => event.subjectId === caller.user.id + ); + expect(forCaller).toHaveLength(1); + }); + + it("leaves a director who is not on the floor exactly as they were", async () => { + const director = await asDirector(); + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + + await setUserRole({ userId: target.id, role: "admin" }); + + // One event, for the target. Nothing was written about the caller. + expect((await listAuditEvents()).map((event) => event.subjectId)).toEqual([ + target.id, + ]); + expect(await roleOf(director.user.id)).toBe("super_admin"); + }); + + it("does not promote an ordinary user who merely shares a prefix with the floor", async () => { + // `isSuperAdminFloor` matches the whole normalized address; a reconciler + // that matched loosely would be a privilege escalation, not a repair. + const caller = await signInAsNew({ email: "founder2@cornell.edu", role: "user" }); + vi.stubEnv("AUTH_SUPER_ADMIN_EMAILS", "founder@cornell.edu"); + setMockHeaders({ cookie: caller.cookie }); + const target = await seedUser({ email: "student@cornell.edu", role: "user" }); + + expect(await setUserRole({ userId: target.id, role: "admin" })).toEqual({ + ok: false, + error: "not_permitted", + }); + expect(await roleOf(caller.user.id)).toBe("user"); + }); +}); diff --git a/v5/src/app/admin/users/actions.ts b/v5/src/app/admin/users/actions.ts new file mode 100644 index 0000000..883ccb9 --- /dev/null +++ b/v5/src/app/admin/users/actions.ts @@ -0,0 +1,342 @@ +"use server"; + +import { revalidatePath } from "next/cache"; +import { headers } from "next/headers"; +import { getAuth } from "../../../lib/auth/config"; +import { reconcileSuperAdminFloor } from "../../../lib/auth/floor-role"; +import { resolveIdentityFromHeaders, type Identity } from "../../../lib/auth/identity"; +import { can } from "../../../lib/auth/permissions"; +import { isSuperAdminFloor } from "../../../lib/auth/super-admins"; +import { recordAuditEvent, type NewAuditEvent } from "../../../lib/data/audit"; +import { countUsersWithRole, findUserById, type UserRecord } from "../../../lib/data/users"; +import { isOneOf, ROLES, type Role } from "../../../lib/db/schema/vocabulary"; +import { ADMIN_ACTION_TIER, rateLimitAsync } from "../../../lib/rate-limit"; +import { + ADMIN_USERS_PATH, + type AdminActionError, + type AdminActionResult, + type AdminActionWarning, +} from "./action-result"; + +/** + * The two writes `/admin/users` performs (data platform design spec §5.2, §8). + * + * **Each one checks its own permission.** A server action is a POST endpoint + * with a generated name: it is reachable without ever rendering the page that + * offers it, so nothing it receives — and nothing about the page that rendered + * the control — is evidence of anything (§8). The identity is resolved here, + * `users.manage` is checked here, and the limiter runs here. + * + * **The write goes through the admin plugin, the reads do not.** `set-role` + * and `ban-user` carry behaviour worth having (a ban deletes the person's + * sessions, so they are signed out mid-visit rather than on expiry), and they + * authorize against the same declaration `can()` does. The roster itself is + * read straight from Postgres — see `src/lib/data/users.ts`. + * + * **Refusals are values, not exceptions.** Each action answers + * `{ ok: false, error: }` and the client island renders the matching + * `next-intl` string. A thrown error in a server action reaches the browser as + * a digest and an error boundary, which is the wrong shape for "you cannot + * demote the floor address, and here is why" (§5.2). + * + * **And a change that lands without its audit event is a success with a + * warning, not a failure.** The two writes are two statements and only the + * first one is the change; see {@link record}. + */ + +/** + * Change one person's role. + * + * Refuses, in this order: an anonymous caller, the rate ceiling, a caller + * without `users.manage`, a role outside the vocabulary, an unknown target, the + * super-admin floor, and a demotion that would leave nobody holding + * `super_admin` at all (spec §10, "the last super admin demotes themselves"). + * + * A change to the role the person already has is a no-op that reports success + * and writes no audit event — the trail records changes, and "admin → admin" + * is not one. + */ +export async function setUserRole(input: { + userId: string; + role: string; +}): Promise { + const gate = await authorize(); + if (!gate.ok) return gate; + // `gateWarning` is the reconciliation's own audit gap, if it had one. It + // rides on every success below, including the ones that change nothing here: + // the caller's row still moved. + const { identity, warning: gateWarning } = gate; + + if (!isOneOf(ROLES, input.role)) return { ok: false, error: "invalid_role" }; + const role: Role = input.role; + + const target = await findUserById(input.userId); + if (!target) return { ok: false, error: "unknown_user" }; + if (target.role === role) return { ok: true, role, ...warn(gateWarning) }; + + const protection = await demotionProtection(target, role); + if (protection) return { ok: false, error: protection }; + + try { + const auth = await getAuth(); + if (!auth) return { ok: false, error: "failed" }; + await auth.api.setRole({ + body: { userId: target.id, role }, + headers: await requestHeaders(), + }); + } catch (err) { + // The plugin refuses with an `APIError`; anything else is a database or + // configuration problem. Either way the row did not change, and the caller + // is told that rather than being shown a success they did not get. + console.error("[admin/users] set-role failed", err); + return { ok: false, error: "failed" }; + } + + const recorded = await record({ + actorUserId: identity.userId, + action: "role.changed", + subjectType: "user", + subjectId: target.id, + // Both halves: "became an admin" is not answerable later without the + // "from", and that is the question an audit trail exists to answer. + detail: { from: target.role, to: role }, + }); + + revalidatePath(ADMIN_USERS_PATH); + return { ok: true, role, ...warn(gateWarning, recorded) }; +} + +/** + * Ban or unban one person. + * + * A ban deletes their sessions, so it bites immediately rather than on the next + * page load — and `resolveIdentity` refuses a banned user anyway, so even a + * cookie that outlived the sweep resolves to anonymous. + * + * The floor address cannot be banned, and neither can the last super admin; + * banning yourself is refused here so the message is ours (the plugin refuses + * it too, with an error code the page would have to translate). + */ +export async function setUserBanned(input: { + userId: string; + banned: boolean; + reason?: string; +}): Promise { + const gate = await authorize(); + if (!gate.ok) return gate; + const { identity, warning: gateWarning } = gate; + + const target = await findUserById(input.userId); + if (!target) return { ok: false, error: "unknown_user" }; + if (target.banned === input.banned) { + return { ok: true, banned: input.banned, ...warn(gateWarning) }; + } + + if (input.banned) { + // Self-ban first: the plugin refuses it too, but with an error code the + // page would have to translate, and this way the message is ours. + if (target.id === identity.userId) return { ok: false, error: "self_ban" }; + // No "last super admin" check here, deliberately. Reaching this line means + // somebody *else* holds `users.manage` — the caller — so banning this + // account cannot leave the lab without one. + if (isSuperAdminFloor(target.email)) return { ok: false, error: "protected_floor" }; + } + + const reason = (input.reason ?? "").trim() || undefined; + + try { + const auth = await getAuth(); + if (!auth) return { ok: false, error: "failed" }; + const requestedHeaders = await requestHeaders(); + if (input.banned) { + await auth.api.banUser({ + body: { userId: target.id, ...(reason ? { banReason: reason } : {}) }, + headers: requestedHeaders, + }); + } else { + await auth.api.unbanUser({ + body: { userId: target.id }, + headers: requestedHeaders, + }); + } + } catch (err) { + console.error("[admin/users] ban-user failed", err); + return { ok: false, error: "failed" }; + } + + const recorded = await record({ + actorUserId: identity.userId, + action: "user.banned", + subjectType: "user", + subjectId: target.id, + // `AUDIT_ACTIONS` has no `user.unbanned` (spec §4.11), so lifting a ban is + // the same action with `banned: false`. The alternative is a vocabulary + // that drifts from the spec, which is worse than a flag in the detail. + detail: { banned: input.banned, ...(reason ? { reason } : {}) }, + }); + + revalidatePath(ADMIN_USERS_PATH); + return { + ok: true, + banned: input.banned, + ...warn(gateWarning, recorded), + }; +} + +// ── The shared preamble ───────────────────────────────────────────── + +type Gate = + | { ok: true; identity: Identity; warning?: AdminActionWarning } + | { ok: false; error: AdminActionError }; + +/** + * Resolve the caller, bound their attempts, and check `users.manage`. + * + * Bounded *before* the permission check and the queries behind it (Article 4, + * §8: 120/min per user), and keyed on `rateLimitKey` — the user id when signed + * in, a hashed IP when not — so an anonymous prodder cannot spend an admin's + * allowance. + * + * The last step is the one that is not a refusal: the floor is written onto the + * caller's own row before either action calls the plugin. `can()` honours the + * floor and the plugin does not — see `lib/auth/floor-role.ts` — so without + * this a floor address whose row says `user` reaches the page with every + * control live and every save failing. It runs after the permission check, so + * only somebody the app already treats as a director can trigger it, and it is + * a no-op for everyone whose row already agrees. + */ +async function authorize(): Promise { + const identity = await resolveIdentityFromHeaders(); + + const { allowed } = await rateLimitAsync( + `admin-action:${identity.rateLimitKey}`, + ADMIN_ACTION_TIER + ); + if (!allowed) return { ok: false, error: "rate_limited" }; + + // Told apart on purpose: "sign in" is actionable and "you are not permitted" + // is not, and showing the wrong one of those is how a page feels broken. + if (identity.role === "anonymous") return { ok: false, error: "not_signed_in" }; + if (!can(identity, "users.manage")) return { ok: false, error: "not_permitted" }; + + let reconciliation; + try { + reconciliation = await reconcileSuperAdminFloor(identity); + } catch (err) { + // The write that follows depends on this having landed, so reporting + // "failed" is the honest answer — better than letting the plugin refuse + // for a reason the page cannot explain. + console.error("[admin/users] super-admin floor reconciliation failed", err); + return { ok: false, error: "failed" }; + } + + // The reconciliation may have promoted this caller — and lifted a ban — with + // no trail. That rides back as a warning on whatever the action goes on to + // do, because refusing here would deny a change the database has kept. + return { + ok: true, + identity, + ...(reconciliation.audited ? {} : { warning: AUDIT_WARNING }), + }; +} + +/** + * Why `target` may not be moved to `nextRole`, or null when they may. + * + * Only demotions are protected — promoting anybody, including a floor address + * that already holds the role, is always fine. Two guarantees, and they are + * not the same one: + * + * - **The floor.** An address in `AUTH_SUPER_ADMIN_EMAILS` resolves + * `super_admin` whatever its row says, so demoting it in the database would + * produce a row that disagrees with the running app — confusing rather than + * dangerous, and worth refusing plainly. + * - **The last super admin.** A deployment with no floor configured — a + * preview, a fork — can genuinely lock itself out, and this is the case spec + * §10 names. Banned super admins do not count towards "somebody is left": + * they resolve to anonymous and can undo nothing. + */ +async function demotionProtection( + target: UserRecord, + nextRole: Role +): Promise { + if (nextRole === "super_admin") return null; + + if (isSuperAdminFloor(target.email)) return "protected_floor"; + + if (target.role === "super_admin") { + const remaining = await countUsersWithRole("super_admin", { + excludeUserId: target.id, + }); + if (remaining === 0) return "last_super_admin"; + } + + return null; +} + +/** + * The incoming request's headers as a real `Headers`. + * + * Better Auth iterates what it is given and `next/headers` returns a read-only + * look-alike, so this copies rather than casts. Only the cookie is needed: + * `set-role` is `requireHeaders: true` and authenticates from the session. + */ +async function requestHeaders(): Promise { + const incoming = await headers(); + const copy = new Headers(); + const cookie = incoming.get("cookie"); + if (cookie) copy.set("cookie", cookie); + return copy; +} + +// ── Recording it ──────────────────────────────────────────────────── + +/** The one warning either action can carry. Named so the two cannot drift. */ +const AUDIT_WARNING: AdminActionWarning = "audit_unavailable"; + +/** + * Write the audit event, and say whether it landed. + * + * **The order is not negotiable and neither is the shape.** The event describes + * a change that has already committed — `auth.api.setRole` / `banUser` have + * returned — and `recordAuditEvent` throws on any database failure. With the + * Neon HTTP driver each statement is its own request, so a transient 5xx + * between the two is an ordinary outcome rather than an exotic one, and there + * is no transaction spanning them to roll back. + * + * Letting the throw propagate would reach the island as a rejected action, and + * both islands answer a rejection by restoring the previous value: the page + * would show the old role over a database holding the new one, which is exactly + * what `RoleSelect`'s comment says it never does. Swallowing it silently would + * leave a gap in the trail nobody was told about (spec §4.11). + * + * So the failure becomes a `warning` on a successful result: the row changed, + * the page says so, and it also says the change was not recorded. The console + * line is the operator's copy — it is the only place the event now exists. + */ +/** + * The warning half of a successful result, or nothing. + * + * Two audit writes can go missing on one action — the floor reconciliation's, + * before the action ran, and the action's own — and there is one warning for + * both, because the admin's question is the same either way: *did the trail + * record this?* Spread into the result so a success without a gap carries no + * `warning` key at all. + */ +function warn( + gateWarning: AdminActionWarning | undefined, + recorded = true +): { warning?: AdminActionWarning } { + const warning = gateWarning ?? (recorded ? undefined : AUDIT_WARNING); + return warning ? { warning } : {}; +} + +async function record(event: NewAuditEvent): Promise { + try { + await recordAuditEvent(event); + return true; + } catch (err) { + console.error("[admin/users] audit write failed after the change landed", err); + return false; + } +} diff --git a/v5/src/app/admin/users/page.tsx b/v5/src/app/admin/users/page.tsx new file mode 100644 index 0000000..1ca50ee --- /dev/null +++ b/v5/src/app/admin/users/page.tsx @@ -0,0 +1,58 @@ +import { getTranslations } from "next-intl/server"; +import { AdminNotice } from "../../../components/admin/AdminNotice"; +import { UsersTable } from "../../../components/admin/UsersTable"; +import { resolveIdentityFromHeaders } from "../../../lib/auth/identity"; +import { can } from "../../../lib/auth/permissions"; +import { listUsers } from "../../../lib/data/users"; +import { siteConfig } from "../../../lib/site-config"; +import { setUserBanned, setUserRole } from "./actions"; + +/** + * `/admin/users` — who is who, and how to change it (spec §5.2, §6). + * + * Super admin only: `users.manage` belongs to that role alone (§8). The layout + * above let a SuperMaker in — they hold other admin permissions — so the + * refusal for them happens here, and it says so rather than 404ing. + * + * Nothing on this page is cached. The roster is read per request, because the + * point of the whole phase is that a role change is visible immediately; a + * cached roster would show the change to everyone except the person who made + * it. The actions call `revalidatePath` for the same reason. + * + * The two server actions are imported here and handed to the table, which + * hands them to its client islands. That is what keeps `RoleSelect` free of + * `next/headers` and the rate limiter — see its own note. + */ + +export const metadata = { + title: `People — ${siteConfig.name}`, +}; + +export default async function AdminUsersPage() { + const t = await getTranslations("admin"); + const identity = await resolveIdentityFromHeaders(); + + if (!can(identity, "users.manage")) return ; + + const users = await listUsers(); + + return ( +
+
+

{t("eyebrow")}

+

{t("usersTitle")}

+ {/* No placeholder in this string: `/admin/page.tsx` renders the same + key without arguments, and a next-intl placeholder with no argument + renders literally (Article 6 — this has been a real bug here). */} +

{t("usersLede")}

+
+ + +
+ ); +} diff --git a/v5/src/app/api/admin/backup/route.ts b/v5/src/app/api/admin/backup/route.ts index ae4f170..0a6685f 100644 --- a/v5/src/app/api/admin/backup/route.ts +++ b/v5/src/app/api/admin/backup/route.ts @@ -1,3 +1,14 @@ +/** + * **SUPERSEDED — retired in Phase 3, kept on disk pending deletion approval.** + * + * `GET /api/cron/daily` replaces this route (data platform design spec §3.9): + * the source of truth is Postgres now, so the nightly job exports tables rather + * than dumping Notion, and `vercel.json`'s single cron entry points there. Both + * of this route's accepted callers (the cron bearer and the + * `ADMIN_REVALIDATE_SECRET` hand-trigger) were carried across. Nothing schedules + * or calls this handler any more. + */ + import { getBlobStore, isBlobConfigured } from "../../../../lib/blob"; import { getNotionEnvContract } from "../../../../lib/notion"; import { rateLimitAsync } from "../../../../lib/rate-limit"; diff --git a/v5/src/app/api/admin/revalidate/route.test.ts b/v5/src/app/api/admin/revalidate/route.test.ts index bfff88e..2870524 100644 --- a/v5/src/app/api/admin/revalidate/route.test.ts +++ b/v5/src/app/api/admin/revalidate/route.test.ts @@ -1,13 +1,12 @@ +// @vitest-environment node import { nextCacheMock } from "../../../../../test/mocks/next-cache"; vi.mock("next/cache", () => nextCacheMock()); import { revalidateTag } from "next/cache"; -import { - SESSION_COOKIE_NAME, - createSessionPayload, - signSession, -} from "../../../../lib/auth/session-cookie"; +import { resetAuthForTests } from "../../../../lib/auth/config"; +import { resetDbForTests } from "../../../../lib/db/client"; +import { signInAsNew } from "../../../../../test/utils/session"; import { POST } from "./route"; function makeRequest(headers: Record = {}) { @@ -85,34 +84,32 @@ function uniqueIp() { return `198.51.100.${counter}`; } -async function cookieFor(sub: string, email: string) { - const token = await signSession( - createSessionPayload({ sub, email, name: "Test Person" }), - AUTH_SECRET - ); - return `${SESSION_COOKIE_NAME}=${token}`; -} - function sessionRequest(cookie?: string, ip = uniqueIp()) { const headers: Record = { "x-forwarded-for": ip }; if (cookie) headers.cookie = cookie; return makeRequest(headers); } -describe("POST /api/admin/revalidate — staff session", () => { +describe("POST /api/admin/revalidate — a signed-in session", () => { beforeEach(() => { // The `next/cache` mock is a module-factory mock, so `restoreAllMocks` // leaves its call log alone; each test needs a clean one. vi.mocked(revalidateTag).mockClear(); + vi.stubEnv("DATABASE_URL", ""); vi.stubEnv("AUTH_SECRET", AUTH_SECRET); - // Unset on purpose: a staff session must not depend on the shared secret - // being configured, because the browser can never send it. + // Unset on purpose: a signed-in session must not depend on the shared + // secret being configured, because the browser can never send it. vi.stubEnv("ADMIN_REVALIDATE_SECRET", ""); + resetAuthForTests(); + }); + + afterEach(() => { + resetAuthForTests(); + resetDbForTests(); }); - it("accepts a staff session with no secret header at all", async () => { - vi.stubEnv("AUTH_STAFF_EMAILS", "niti@cornell.edu"); - const cookie = await cookieFor("sub-staff", "niti@cornell.edu"); + it("accepts an admin session with no secret header at all", async () => { + const { cookie } = await signInAsNew({ role: "admin" }); const res = await POST(sessionRequest(cookie)); @@ -122,16 +119,15 @@ describe("POST /api/admin/revalidate — staff session", () => { expect(vi.mocked(revalidateTag)).toHaveBeenCalledWith("projects", "minutes"); }); - it("accepts an admin session", async () => { - vi.stubEnv("AUTH_ADMIN_EMAILS", "isaac@cornell.edu"); - const cookie = await cookieFor("sub-admin", "isaac@cornell.edu"); + it("accepts a super admin session", async () => { + const { cookie } = await signInAsNew({ role: "super_admin" }); expect((await POST(sessionRequest(cookie))).status).toBe(200); expect(vi.mocked(revalidateTag)).toHaveBeenCalledTimes(2); }); - it("refuses a signed-in student — signing in is not staff", async () => { - const cookie = await cookieFor("sub-student", "ada@cornell.edu"); + it("refuses an ordinary signed-in user — signing in grants no tools.edit", async () => { + const { cookie } = await signInAsNew({ role: "user" }); const res = await POST(sessionRequest(cookie)); @@ -148,12 +144,9 @@ describe("POST /api/admin/revalidate — staff session", () => { expect(vi.mocked(revalidateTag)).not.toHaveBeenCalled(); }); - it("refuses a tampered session cookie rather than trusting its claims", async () => { - vi.stubEnv("AUTH_STAFF_EMAILS", "niti@cornell.edu"); - const cookie = await cookieFor("sub-forge", "niti@cornell.edu"); - const [name, token] = cookie.split("="); - const [payload, signature] = token.split("."); - const forged = `${name}=${payload}.${signature.startsWith("A") ? "B" : "A"}${signature.slice(1)}`; + it("refuses a tampered session cookie rather than trusting it", async () => { + const { cookie } = await signInAsNew({ role: "admin" }); + const forged = cookie.replace(/.$/, (c) => (c === "A" ? "B" : "A")); const res = await POST(sessionRequest(forged)); @@ -176,8 +169,7 @@ describe("POST /api/admin/revalidate — staff session", () => { }); it("is bounded: the ceiling refuses with Retry-After", async () => { - vi.stubEnv("AUTH_STAFF_EMAILS", "niti@cornell.edu"); - const cookie = await cookieFor("sub-flood", "niti@cornell.edu"); + const { cookie } = await signInAsNew({ role: "admin" }); const ip = uniqueIp(); for (let i = 0; i < 30; i += 1) { diff --git a/v5/src/app/api/admin/revalidate/route.ts b/v5/src/app/api/admin/revalidate/route.ts index 11cfad6..d419955 100644 --- a/v5/src/app/api/admin/revalidate/route.ts +++ b/v5/src/app/api/admin/revalidate/route.ts @@ -1,6 +1,6 @@ import { revalidateTag } from "next/cache"; import { resolveIdentity } from "../../../../lib/auth/identity"; -import { isAtLeast } from "../../../../lib/auth/roles"; +import { can } from "../../../../lib/auth/permissions"; import { rateLimitAsync } from "../../../../lib/rate-limit"; /** @@ -10,9 +10,9 @@ import { rateLimitAsync } from "../../../../lib/rate-limit"; * The catalog caches for a day now, so freshness comes from invalidation rather * than polling and *something has to invalidate*. Two callers do: * - * - **A signed-in `staff` or `admin` session**, which is what the Refresh - * control in the header uses. A browser cannot hold the shared secret, so - * without this branch the button could not exist. + * - **A signed-in session holding `tools.edit`** — an admin or a super admin — + * which is what the Refresh control in the header uses. A browser cannot hold + * the shared secret, so without this branch the button could not exist. * - **The `x-admin-secret` header**, unchanged, for the callers that have no * session: a Notion automation webhook, a cron job, `curl` during an incident. * @@ -22,10 +22,10 @@ import { rateLimitAsync } from "../../../../lib/rate-limit"; /** * Invalidation is cheap here but expensive on the next request: it forces a - * full Notion re-read. Bounded before that happens (Article 4), keyed per + * full catalogue re-read. Bounded before that happens (Article 4), keyed per * identity so one caller cannot spend another's allowance. Generous enough that - * a staff member correcting a run of rows never notices, and a webhook firing - * per row edit only sheds refreshes it would have made redundant anyway. + * an admin correcting a run of rows never notices, and a webhook firing per row + * edit only sheds refreshes it would have made redundant anyway. */ const REVALIDATE_TIER = { limit: 30, windowMs: 60_000 }; @@ -49,7 +49,7 @@ export async function POST(req: Request) { ); } - if (!isAtLeast(identity.role, "staff")) { + if (!can(identity, "tools.edit")) { const presented = req.headers.get("x-admin-secret"); // No session and no secret offered: nothing to check, and nothing about the // deployment's configuration is worth telling this caller. diff --git a/v5/src/app/api/auth/[...all]/route.test.ts b/v5/src/app/api/auth/[...all]/route.test.ts index e87298d..55edd54 100644 --- a/v5/src/app/api/auth/[...all]/route.test.ts +++ b/v5/src/app/api/auth/[...all]/route.test.ts @@ -7,14 +7,20 @@ */ import { GET, POST } from "@/app/api/auth/[...all]/route"; import { resetAuthForTests } from "@/lib/auth/config"; +import { resetDbForTests } from "@/lib/db/client"; const ORIGIN = "http://localhost:3000"; -function stubAuthEnv() { +function stubSessionEnv() { + vi.stubEnv("DATABASE_URL", ""); vi.stubEnv("AUTH_SECRET", "route-test-secret"); + vi.stubEnv("AUTH_BASE_URL", ORIGIN); +} + +function stubAuthEnv() { + stubSessionEnv(); vi.stubEnv("GOOGLE_CLIENT_ID", "client-id.apps.googleusercontent.com"); vi.stubEnv("GOOGLE_CLIENT_SECRET", "client-secret"); - vi.stubEnv("AUTH_BASE_URL", ORIGIN); } // The limiter is a per-process singleton keyed by hashed IP — give each test @@ -39,10 +45,16 @@ function authRequest( } beforeEach(() => { + vi.stubEnv("DATABASE_URL", ""); resetAuthForTests(); }); -describe("GET|POST /api/auth/* — not configured", () => { +afterEach(() => { + resetAuthForTests(); + resetDbForTests(); +}); + +describe("GET|POST /api/auth/* — no AUTH_SECRET", () => { it("answers 503 rather than throwing, so the rest of the app still works", async () => { const res = await GET(authRequest("/get-session")); expect(res.status).toBe(503); @@ -57,6 +69,28 @@ describe("GET|POST /api/auth/* — not configured", () => { }); }); +describe("GET|POST /api/auth/* — sessions but no Google", () => { + // How the E2E suite runs, and how a deployment looks before the OAuth client + // exists: real sessions, no way to start one through Google. + it("still answers get-session, so a seeded cookie works", async () => { + stubSessionEnv(); + const res = await GET(authRequest("/get-session")); + expect(res.status).toBe(200); + }); + + it("refuses social sign-in with the 503 the header renders as unconfigured", async () => { + stubSessionEnv(); + const res = await POST( + authRequest("/sign-in/social", { + method: "POST", + body: { provider: "google", callbackURL: "/" }, + }) + ); + expect(res.status).toBe(503); + expect((await res.json()).error).toMatch(/not configured/i); + }); +}); + describe("GET|POST /api/auth/* — configured", () => { it("hands the request to Better Auth", async () => { stubAuthEnv(); @@ -80,6 +114,77 @@ describe("GET|POST /api/auth/* — configured", () => { }); }); +describe("the admin plugin's own endpoints are not exposed", () => { + /** + * The regression this guards: the catch-all mounts every endpoint the admin + * plugin registers, so `/api/auth/admin/set-role` was a second way to change + * a role — one that writes no `audit_events` row, consults no super-admin + * floor, has no "last super admin" guard and is outside `ADMIN_ACTION_TIER`. + * Anyone holding a director's session cookie could have used it from the + * browser console on the site's own origin. + */ + async function seedDirectorCookie() { + const { signInAsNew } = await import("../../../../../test/utils/session"); + return signInAsNew({ email: "director@cornell.edu", role: "super_admin" }); + } + + it("refuses set-role even when a real super admin asks", async () => { + stubAuthEnv(); + const director = await seedDirectorCookie(); + const { seedUser } = await import("../../../../../test/utils/session"); + const target = await seedUser({ email: "student-plugin@cornell.edu", role: "user" }); + + const req = new Request(`${ORIGIN}/api/auth/admin/set-role`, { + method: "POST", + headers: { + "x-forwarded-for": uniqueIp(), + origin: ORIGIN, + "content-type": "application/json", + cookie: director.cookie, + }, + body: JSON.stringify({ userId: target.id, role: "super_admin" }), + }); + const res = await POST(req); + + expect(res.status).toBe(403); + expect((await res.json()).code).toBe("admin_api_not_exposed"); + + // The row is the assertion that matters: a 403 that still wrote would be + // worse than no check at all. + const { findUserById } = await import("@/lib/data/users"); + expect((await findUserById(target.id))?.role).toBe("user"); + }); + + it("refuses every other plugin path too, however it is spelled", async () => { + stubAuthEnv(); + for (const path of [ + "/admin/ban-user", + "/admin/update-user", + "/admin/remove-user", + "/admin/list-users", + "/admin/impersonate-user", + "/ADMIN/set-role", + "/%61dmin/set-role", + ]) { + const res = await POST(authRequest(path, { method: "POST", body: {} })); + expect(res.status, path).toBe(403); + } + }); + + it("refuses before asking whether auth is configured at all", async () => { + // No AUTH_SECRET: the answer is still "not exposed", not "not configured". + const res = await POST( + authRequest("/admin/set-role", { method: "POST", body: {} }) + ); + expect(res.status).toBe(403); + }); + + it("leaves the ordinary endpoints alone", async () => { + stubSessionEnv(); + expect((await GET(authRequest("/get-session"))).status).toBe(200); + }); +}); + describe("rate limiting", () => { it("refuses past the per-IP ceiling before ever calling Google", async () => { stubAuthEnv(); diff --git a/v5/src/app/api/auth/[...all]/route.ts b/v5/src/app/api/auth/[...all]/route.ts index db30953..5e6e708 100644 --- a/v5/src/app/api/auth/[...all]/route.ts +++ b/v5/src/app/api/auth/[...all]/route.ts @@ -1,23 +1,40 @@ -import { getAuth } from "../../../../lib/auth/config"; +import { AUTH_BASE_PATH, getAuth, hasGoogleEnv } from "../../../../lib/auth/config"; import { anonymousIdentity } from "../../../../lib/auth/identity"; import { checkRateLimit } from "../../../../lib/rate-limit"; /** - * `GET|POST /api/auth/*` — the Better Auth handler (auth design spec §3.1). + * `GET|POST /api/auth/*` — the Better Auth handler (data platform design spec + * §3.4). * * The route decides nothing. It rate-limits, then hands the request to the - * configured instance; the domain check, the session cookie, and the + * configured instance; the domain check, the session row, and the * rejected-domain redirect all live in `lib/auth/config.ts`. * - * When sign-in is not configured the endpoint answers 503 rather than throwing, - * so a deployment without Google credentials still serves the catalog and the - * assistant — sign-in unlocks, it does not gate the front door. + * Two things can be unconfigured, and they are now separate. Without + * `AUTH_SECRET` there is no instance at all. With a secret but no Google client + * there *is* one — the E2E suite runs exactly that way, with seeded sessions + * and no OAuth — but the social sign-in endpoint has nothing to start, so it + * answers 503 rather than a library error. 503 is what `sign-in-client.ts` + * reads as "this deployment has no sign-in set up" and renders as such. + * + * Either way the deployment still serves the catalogue and the assistant: + * sign-in unlocks, it does not gate the front door. + * + * **The one thing the route does decide is that the admin plugin's own HTTP + * endpoints are not part of the public surface.** See {@link isPluginAdminPath}. */ // `runtime` cannot be set when nextConfig.cacheComponents is enabled. // Default Node.js runtime is used. export const maxDuration = 15; +const NOT_CONFIGURED = { error: "Sign-in is not configured." }; + +const ADMIN_NOT_EXPOSED = { + error: "Account administration is not available over this API. Use /admin/users.", + code: "admin_api_not_exposed", +}; + async function handle(req: Request): Promise { // Always by IP: the whole point of these endpoints is that the caller has no // session yet (Article 4 — limit inbound before any outbound to Google). @@ -33,13 +50,78 @@ async function handle(req: Request): Promise { ); } - const auth = getAuth(); - if (!auth) { - return Response.json({ error: "Sign-in is not configured." }, { status: 503 }); + // Before the instance is even asked for: this refusal does not depend on + // configuration, and an unconfigured deployment should answer it the same way. + if (isPluginAdminPath(req)) { + return Response.json(ADMIN_NOT_EXPOSED, { status: 403 }); + } + + const auth = await getAuth(); + if (!auth) return Response.json(NOT_CONFIGURED, { status: 503 }); + if (isSocialSignIn(req) && !hasGoogleEnv()) { + return Response.json(NOT_CONFIGURED, { status: 503 }); } return auth.handler(req); } +/** True for `POST /api/auth/sign-in/social`, the one endpoint Google gates. */ +function isSocialSignIn(req: Request): boolean { + return normalizedPath(req) === `${AUTH_BASE_PATH}/sign-in/social`; +} + +/** + * True for anything the admin plugin mounts — `/api/auth/admin/set-role`, + * `/admin/ban-user`, `/admin/update-user`, and the rest. + * + * **Those endpoints are a second, weaker way to do everything `/admin/users` + * does.** The plugin authorizes them against the *stored* `user.role` and + * nothing else: no audit event is written (`recordAuditEvent` has exactly two + * call sites, both in `app/admin/users/actions.ts`), the super-admin floor is + * not consulted, the "last super admin" guard does not exist, and they are + * outside `ADMIN_ACTION_TIER`. A director's session cookie — borrowed laptop, + * XSS, an exfiltrated cookie — would therefore be able to change roles from the + * browser console leaving no trace in `audit_events`, which is the one hole an + * audit trail must not have (spec §4.11). + * + * So the app does not expose them. v5 calls `auth.api.setRole` / `banUser` / + * `unbanUser` **in process**, from the server actions that carry the guarantees; + * `auth.api.*` never travels through this handler, so refusing the HTTP paths + * costs the application nothing. Nothing in the app or the E2E suite calls + * `/api/auth/admin/*` — there is no Better Auth *client* in the codebase at all. + * + * 403 rather than 404 on purpose (Article 4): the endpoint is real and the + * caller is told why it will not answer, instead of being told a comfortable + * lie about what exists. + */ +function isPluginAdminPath(req: Request): boolean { + const prefix = `${AUTH_BASE_PATH}/admin`; + // Decoded and lower-cased before comparing, so `%61dmin` and `/ADMIN/` are + // the same path to this check as they may be to the router underneath. There + // is nothing else under `/api/auth` named `admin`, so matching loosely here + // can only ever over-refuse an endpoint that does not exist. + return normalizedPath(req).startsWith(prefix); +} + +/** + * The request's pathname, percent-decoded and lower-cased. `""` for a URL that + * will not parse or will not decode — neither can reach a real endpoint, and a + * path this function cannot read is not one to wave through. + */ +function normalizedPath(req: Request): string { + let pathname: string; + try { + pathname = new URL(req.url).pathname; + } catch { + return ""; + } + try { + pathname = decodeURIComponent(pathname); + } catch { + return ""; + } + return pathname.toLowerCase(); +} + export const GET = handle; export const POST = handle; diff --git a/v5/src/app/api/chat/rate-limit.route.test.ts b/v5/src/app/api/chat/rate-limit.route.test.ts index 5266036..b875b63 100644 --- a/v5/src/app/api/chat/rate-limit.route.test.ts +++ b/v5/src/app/api/chat/rate-limit.route.test.ts @@ -4,18 +4,15 @@ * * Unlike `route.test.ts`, this suite uses the **real** rate limiter and the * **real** `resolveIdentity`, so it exercises the thing that actually matters: - * an anonymous visitor and a signed-in student hitting the same endpoint from - * the same IP get different allowances, and the refusal offers a way forward. + * an anonymous visitor and a signed-in user hitting the same endpoint from the + * same IP get different allowances, and the refusal offers a way forward. * * The model is still stubbed at the `streamText` boundary, and the catalogue * comes from the demo-seeded PGlite database — no network (Art. 3). */ -import { - SESSION_COOKIE_NAME, - createSessionPayload, - signSession, -} from "@/lib/auth/session-cookie"; +import { resetAuthForTests } from "@/lib/auth/config"; import { resetDbForTests } from "@/lib/db/client"; +import { signInAsNew } from "../../../../test/utils/session"; const AUTH_SECRET = "chat-ceiling-test-secret"; @@ -65,12 +62,10 @@ function uniqueIp() { return `192.0.2.${counter}`; } -async function studentCookie(sub: string, email = "student@cornell.edu") { - const token = await signSession( - createSessionPayload({ sub, email, name: "Ada" }), - AUTH_SECRET - ); - return `${SESSION_COOKIE_NAME}=${token}`; +/** A seeded `user` session — a real row and the cookie that addresses it. */ +async function userCookie(): Promise { + const { cookie } = await signInAsNew({ role: "user", name: "Ada" }); + return cookie; } function chatRequest({ ip, cookie }: { ip: string; cookie?: string }): Request { @@ -91,6 +86,11 @@ function chatRequest({ ip, cookie }: { ip: string; cookie?: string }): Request { beforeEach(() => { vi.stubEnv("AUTH_SECRET", AUTH_SECRET); vi.stubEnv("DATABASE_URL", ""); + resetAuthForTests(); +}); + +afterEach(() => { + resetAuthForTests(); }); afterAll(() => { @@ -154,19 +154,19 @@ describe("POST /api/chat — anonymous ceiling", () => { }); describe("POST /api/chat — signed in", () => { - it("gets the higher student ceiling from the same IP that was exhausted", async () => { + it("gets the higher signed-in ceiling from the same IP that was exhausted", async () => { const ip = uniqueIp(); for (let i = 0; i < 8; i += 1) await POST(chatRequest({ ip })); expect((await POST(chatRequest({ ip }))).status).toBe(429); - const cookie = await studentCookie("sub-signed-in-1"); + const cookie = await userCookie(); for (let i = 0; i < 20; i += 1) { expect((await POST(chatRequest({ ip, cookie }))).status).toBe(200); } }); it("keys on the user, so changing IP neither resets nor escapes the ceiling", async () => { - const cookie = await studentCookie("sub-roaming"); + const cookie = await userCookie(); const first = await POST(chatRequest({ ip: uniqueIp(), cookie })); expect(first.status).toBe(200); @@ -174,12 +174,12 @@ describe("POST /api/chat — signed in", () => { for (let i = 0; i < 59; i += 1) { expect((await POST(chatRequest({ ip: uniqueIp(), cookie }))).status).toBe(200); } - // 61st message overall: the student ceiling, reached despite the IP churn. + // 61st message overall: the signed-in ceiling, despite the IP churn. expect((await POST(chatRequest({ ip: uniqueIp(), cookie }))).status).toBe(429); }); it("refuses without offering sign-in to someone already signed in", async () => { - const cookie = await studentCookie("sub-at-ceiling"); + const cookie = await userCookie(); for (let i = 0; i < 60; i += 1) await POST(chatRequest({ ip: uniqueIp(), cookie })); const res = await POST(chatRequest({ ip: uniqueIp(), cookie })); @@ -191,10 +191,8 @@ describe("POST /api/chat — signed in", () => { }); it("falls back to the anonymous ceiling when the cookie is tampered with", async () => { - const cookie = await studentCookie("sub-tampered"); - const [name, token] = cookie.split("="); - const [payload, sig] = token.split("."); - const forged = `${name}=${payload}.${sig.startsWith("A") ? "B" : "A"}${sig.slice(1)}`; + const cookie = await userCookie(); + const forged = cookie.replace(/.$/, (c) => (c === "A" ? "B" : "A")); const ip = uniqueIp(); const statuses: number[] = []; diff --git a/v5/src/app/api/chat/route.test.ts b/v5/src/app/api/chat/route.test.ts index 9cab52f..65762b7 100644 --- a/v5/src/app/api/chat/route.test.ts +++ b/v5/src/app/api/chat/route.test.ts @@ -46,9 +46,8 @@ vi.mock("ai", async (importOriginal) => { const mocks = vi.hoisted(() => ({ rateLimitAsync: vi.fn(), checkRateLimit: vi.fn(), - createMaintenanceLog: vi.fn(), })); -const { checkRateLimit, createMaintenanceLog } = mocks; +const { checkRateLimit } = mocks; // ── Mock the rate limiter (default: allowed, set in beforeEach) ────── // The route calls the identity-keyed `checkRateLimit`; the rest of the module @@ -64,14 +63,9 @@ vi.mock("@/lib/rate-limit", async (importOriginal) => { }; }); -// ── Mock the one Notion call left in the route's tools ─────────────── -// `report_issue` still files its ticket in Notion (spec §9, Phase 3). Every -// other export stays real: nothing else in this request path reads Notion, and -// a full replacement would have to track every import in the capability layer. -vi.mock("@/lib/notion", async (importOriginal) => { - const actual = await importOriginal(); - return { ...actual, createMaintenanceLog: mocks.createMaintenanceLog }; -}); +// No Notion mock: since Phase 3 nothing in this request path reads or writes +// Notion. `report_issue` files its ticket into the demo-seeded PGlite database +// and the assertions below read the row back out. // next/cache is imported by catalog.ts ("use cache" / cacheTag / cacheLife). vi.mock("next/cache", () => ({ @@ -83,7 +77,6 @@ vi.mock("next/cache", () => ({ import { eq, inArray } from "drizzle-orm"; import { POST } from "@/app/api/chat/route"; import { getDb, resetDbForTests } from "@/lib/db/client"; -import { DEMO_FORM_4_UNIT_NOTION_PAGE_ID } from "@/lib/db/demo-seed"; import { attachments, maintenanceLogs, @@ -91,11 +84,8 @@ import { tools as toolsTable, units, } from "@/lib/db/schema/index"; -import { - SESSION_COOKIE_NAME, - createSessionPayload, - signSession, -} from "@/lib/auth/session-cookie"; +import { resetAuthForTests } from "@/lib/auth/config"; +import { signInAsNew } from "../../../../test/utils/session"; /** * The catalogue, the units and the resources all come from the demo-seeded @@ -175,6 +165,7 @@ const userMessage = (text: string) => ({ beforeEach(() => { captured.args = undefined; vi.stubEnv("DATABASE_URL", ""); + resetAuthForTests(); // Undo any `vi.stubGlobal("fetch", …)` from a prior PDF test (the shared // setup file does not call vi.unstubAllGlobals). vi.unstubAllGlobals(); @@ -184,9 +175,8 @@ beforeEach(() => { limit: 60, windowMs: 60 * 60_000, retryAfterSeconds: 3600, - role: "student", + role: "user", }); - createMaintenanceLog.mockReset(); }); afterEach(async () => { @@ -338,14 +328,17 @@ describe("report_issue.execute", () => { return captured.args.tools; } - it("returns { success:true, ticket_id } when createMaintenanceLog resolves", async () => { - createMaintenanceLog.mockResolvedValueOnce({ - id: "created-page-1", - createdTime: "2024-09-01T10:00:00.000Z", - lastEditedTime: "2024-09-01T10:00:00.000Z", - fields: { title: "Bed not leveling" }, - }); + /** The ticket the tool call actually wrote. */ + async function ticket(id: string) { + const db = await getDb(); + const [row] = await db + .select() + .from(maintenanceLogs) + .where(eq(maintenanceLogs.id, id)); + return row; + } + it("writes an open issue_report and returns its id", async () => { const tools = await getTools(); const result = await tools.report_issue.execute({ title: "Bed not leveling", @@ -354,25 +347,16 @@ describe("report_issue.execute", () => { }); expect(result.success).toBe(true); - expect(result.ticket_id).toBe("created-page-1"); - expect(createMaintenanceLog).toHaveBeenCalledTimes(1); - expect(createMaintenanceLog).toHaveBeenCalledWith( - expect.objectContaining({ - title: "Bed not leveling", - type: "Issue Report", - status: "Open", - }) - ); + const row = await ticket(result.ticket_id); + expect(row).toMatchObject({ + title: "Bed not leveling", + type: "issue_report", + priority: "medium", + status: "open", + }); }); it("links the resolved unit when a known unit_label is supplied", async () => { - createMaintenanceLog.mockResolvedValueOnce({ - id: "created-page-2", - createdTime: "2024-09-01T10:00:00.000Z", - lastEditedTime: "2024-09-01T10:00:00.000Z", - fields: { title: "x" }, - }); - const tools = await getTools(); const result = await tools.report_issue.execute({ title: "Resin leak", @@ -381,86 +365,68 @@ describe("report_issue.execute", () => { priority: "High", }); - // The caller is told the Postgres uuid the catalogue resolved; the ticket, - // still filed in Notion, carries the unit's imported Notion page id — the - // only id that database's `unit` relation can address. + // The catalogue id and the ticket's `unit_id` are the same Postgres uuid — + // there is no translation left between them. const form4A = await unitId("Form 4 // A"); expect(result.success).toBe(true); expect(result.unit_resolved).toEqual({ id: form4A, label: "Form 4 // A" }); - expect(createMaintenanceLog).toHaveBeenCalledWith( - expect.objectContaining({ unit: [DEMO_FORM_4_UNIT_NOTION_PAGE_ID] }) - ); + expect((await ticket(result.ticket_id)).unitId).toBe(form4A); }); - it("records the verified session name and email for a signed-in student", async () => { - // The end-to-end proof of the pass-through: a real signed cookie on the - // request, through resolveIdentity, onto the CapabilityCtx, into the write. + it("records the verified session name and email for a signed-in user", async () => { + // The end-to-end proof of the pass-through: a real session row and the + // cookie that addresses it, through resolveIdentity, onto the + // CapabilityCtx, into the write. vi.stubEnv("AUTH_SECRET", "chat-route-test-secret"); - const token = await signSession( - createSessionPayload({ - sub: "google-sub-1", - email: "ada@cornell.edu", - name: "Ada Lovelace", - }), - "chat-route-test-secret" - ); - createMaintenanceLog.mockResolvedValueOnce({ - id: "created-page-3", - createdTime: "2024-09-01T10:00:00.000Z", - lastEditedTime: "2024-09-01T10:00:00.000Z", - fields: { title: "x" }, + resetAuthForTests(); + const reporter = await signInAsNew({ + email: "ada@cornell.edu", + name: "Ada Lovelace", }); await POST( - chatRequest( - { messages: [userMessage("hi")] }, - { cookie: `${SESSION_COOKIE_NAME}=${token}` } - ) + chatRequest({ messages: [userMessage("hi")] }, { cookie: reporter.cookie }) ); // The assistant is told who it is talking to, and told not to ask. expect(captured.args.system).toContain("Ada Lovelace"); expect(captured.args.system).not.toContain("ada@cornell.edu"); - await captured.args.tools.report_issue.execute({ + const result = await captured.args.tools.report_issue.execute({ title: "Resin leak", description: "Leaking resin", priority: "High", reported_by: "Somebody Else", }); - expect(createMaintenanceLog).toHaveBeenCalledWith( - expect.objectContaining({ - reported_by: "Ada Lovelace", - reporter_email: "ada@cornell.edu", - }) - ); + expect(await ticket(result.ticket_id)).toMatchObject({ + reportedByName: "Ada Lovelace", + reportedByEmail: "ada@cornell.edu", + reportedByUserId: reporter.user.id, + }); }); it("records no email and the supplied name for an anonymous reporter", async () => { - createMaintenanceLog.mockResolvedValueOnce({ - id: "created-page-4", - createdTime: "2024-09-01T10:00:00.000Z", - lastEditedTime: "2024-09-01T10:00:00.000Z", - fields: { title: "x" }, - }); - const tools = await getTools(); - await tools.report_issue.execute({ + const result = await tools.report_issue.execute({ title: "Resin leak", description: "Leaking resin", priority: "High", reported_by: "Grace Hopper", }); - const fields = createMaintenanceLog.mock.calls[0][0]; - expect(fields.reported_by).toBe("Grace Hopper"); - expect(fields.reporter_email).toBeUndefined(); + const row = await ticket(result.ticket_id); + expect(row.reportedByName).toBe("Grace Hopper"); + expect(row.reportedByEmail).toBeNull(); }); - it("returns { success:false, error } when createMaintenanceLog rejects", async () => { - createMaintenanceLog.mockRejectedValueOnce(new Error("Notion is down")); - + it("returns { success:false, error } when the write fails, and files nothing", async () => { + const logged = vi.spyOn(console, "error").mockImplementation(() => {}); const tools = await getTools(); + // A configured database nobody can reach — the case that must never come + // back as "logged your ticket". + vi.stubEnv("DATABASE_URL", "postgres://user:hunter2@127.0.0.1:1/none"); + resetDbForTests(); + const result = await tools.report_issue.execute({ title: "Broken", description: "It is broken", @@ -468,7 +434,14 @@ describe("report_issue.execute", () => { }); expect(result.success).toBe(false); - expect(result.error).toBe("Notion is down"); + expect(result.ticket_id).toBeUndefined(); + expect(result.error).not.toMatch(/hunter2|postgres|ECONNREFUSED/i); + expect(logged).toHaveBeenCalled(); + + // Put the demo substrate back before the file's own cleanup runs: the + // stub is still in force until vitest's global afterEach clears it. + vi.stubEnv("DATABASE_URL", ""); + resetDbForTests(); }); }); @@ -617,24 +590,18 @@ describe("PDF manual collection (focused tool)", () => { }); }); -// ── Adding equipment is staff-only (auth spec amendment 2026-09-14) ── +// ── Adding equipment needs `tools.add` (spec §3.5) ─────────────────── describe("POST /api/chat — who may add equipment", () => { const INTAKE_TOOLS = ["research_tool", "propose_listing", "create_tool"]; const SECRET = "chat-route-test-secret"; const ASK = "I'd like to add new equipment to the inventory."; - async function postAs(email: string, name: string) { + /** Post as a seeded session in `role`. No Google, no env roster. */ + async function postAs(role: "user" | "admin" | "super_admin", name: string) { vi.stubEnv("AUTH_SECRET", SECRET); - const token = await signSession( - createSessionPayload({ sub: `sub-${email}`, email, name }), - SECRET - ); - await POST( - chatRequest( - { messages: [userMessage(ASK)] }, - { cookie: `${SESSION_COOKIE_NAME}=${token}` } - ) - ); + resetAuthForTests(); + const { cookie } = await signInAsNew({ role, name }); + await POST(chatRequest({ messages: [userMessage(ASK)] }, { cookie })); } it("gives an anonymous visitor no intake tools, and tells the assistant why", async () => { @@ -647,8 +614,8 @@ describe("POST /api/chat — who may add equipment", () => { expect(captured.args.system).not.toContain("act as an intake agent"); }); - it("gives a signed-in student no intake tools either", async () => { - await postAs("ada@cornell.edu", "Ada Lovelace"); + it("gives an ordinary signed-in user no intake tools either", async () => { + await postAs("user", "Ada Lovelace"); for (const name of INTAKE_TOOLS) { expect(captured.args.tools).not.toHaveProperty(name); @@ -656,9 +623,9 @@ describe("POST /api/chat — who may add equipment", () => { expect(captured.args.system).toContain("limited to lab staff"); }); - it("gives staff the intake tools and the full intake instructions", async () => { - vi.stubEnv("AUTH_STAFF_EMAILS", "niti@cornell.edu"); - await postAs("niti@cornell.edu", "Niti Parikh"); + it("gives an admin the intake tools and the full intake instructions", async () => { + // The role comes from the `user` row. `AUTH_STAFF_EMAILS` is retired. + await postAs("admin", "Niti Parikh"); for (const name of INTAKE_TOOLS) { expect(captured.args.tools).toHaveProperty(name); @@ -685,7 +652,7 @@ describe("POST /api/chat — photos reach the model", () => { parts: [ { type: "text", - text: "what printer is this?\n\n[Attached photos: file_upload_id=fu_1 name=plate.jpg]", + text: "what printer is this?\n\n[Attached photos: attachment_id=3f2504e0-4f89-41d3-9a0c-0305e82c3301 name=plate.jpg]", }, { type: "file", @@ -710,4 +677,40 @@ describe("POST /api/chat — photos reach the model", () => { ]) ); }); + + it("still shows the model a photo whose hint entry is malformed", async () => { + // The hint is assembled by the client and re-sent verbatim on every turn, + // so a truncated one must degrade to "no id" rather than throwing the + // request away — the model can still look at the picture. + const res = await POST( + chatRequest({ + messages: [ + { + id: "1", + role: "user", + parts: [ + { + type: "text", + text: "what is this?\n\n[Attached photos: name=plate.jpg; attachment_id=]", + }, + { + type: "file", + mediaType: "image/jpeg", + filename: "plate.jpg", + url: "data:image/jpeg;base64,AAAA", + }, + ], + }, + ], + }) + ); + + expect(res.status).toBe(200); + const user = captured.args.messages.find((m: any) => m.role === "user"); + expect(user.content).toEqual( + expect.arrayContaining([ + expect.objectContaining({ type: "file", mediaType: "image/jpeg" }), + ]) + ); + }); }); diff --git a/v5/src/app/api/chat/route.ts b/v5/src/app/api/chat/route.ts index d0735d7..4ca2ca4 100644 --- a/v5/src/app/api/chat/route.ts +++ b/v5/src/app/api/chat/route.ts @@ -24,7 +24,7 @@ import { siteConfig } from "../../../lib/site-config"; import { chatModel } from "../../../lib/model"; import { CAPABILITIES, - capabilitiesForRole, + capabilitiesForIdentity, composeChat, } from "../../../lib/capabilities"; import type { @@ -85,8 +85,8 @@ export async function POST(req: Request) { // Convert the UI messages, attach any server-fetched manuals, and surface the // uploaded photos for this turn both to the model (image bytes — design spec - // §6.1) and to the capability layer (file_upload ids the intake `create_tool` - // re-uses to attach the same photo to the new Notion page). + // §6.1) and to the capability layer (`attachments.id`s a write such as + // `report_issue` claims onto the row it creates). const baseMessages = await convertToModelMessages(messages); const attachments = collectAttachments(baseMessages); if (attachments.length > 0) { @@ -125,10 +125,10 @@ export async function POST(req: Request) { // them). web_fetch keeps the focused-tool domain allow-list. // // The registry is composed as this caller may use it: a capability whose - // minimum role they do not meet contributes no tools, only a note on why - // (auth spec amendment 2026-09-14). + // required permission they do not hold contributes no tools, only a note + // on why (spec §3.5). const { tools: capabilityTools, system } = composeChat( - capabilitiesForRole(CAPABILITIES, identity.role), + capabilitiesForIdentity(CAPABILITIES, identity), ctx, { tools, focusedTool: focused, locale } ); @@ -231,12 +231,13 @@ function describeChatError(error: unknown): string { /** * Reconstruct the uploaded photos for this turn into {@link UploadedImage}s. * - * The chat client uploads each photo to Notion and appends a text hint - * (`[Attached photos: file_upload_id= name=; ...]`) to the user - * message; for vision it also includes the image bytes as image/file parts so - * Claude can see them. We pair the hint entries (which carry the durable Notion - * `file_upload_id` the intake `create_tool` re-uses) with the inline image bytes - * (the `dataUrl` the model sees) from the latest user message, in order. + * The chat client uploads each photo to Blob through `POST /api/uploads` and + * appends a text hint (`[Attached photos: attachment_id= name=; + * ...]`) to the user message; for vision it also includes the image bytes as + * image/file parts so Claude can see them. We pair the hint entries (which + * carry the durable `attachments.id` a write later claims) with the inline + * image bytes (the `dataUrl` the model sees) from the latest user message, in + * order. */ function collectAttachments(messages: ModelMessage[]): UploadedImage[] { const lastUser = [...messages] @@ -250,7 +251,7 @@ function collectAttachments(messages: ModelMessage[]): UploadedImage[] { if (hints.length === 0 && images.length === 0) return []; - // Pair hints (file_upload_id + name) with inline image bytes by position. Some + // Pair hints (attachment_id + name) with inline image bytes by position. Some // entries may have only one side: a hint without bytes still feeds the intake // layer; bytes without a hint still let the model see the photo. const count = Math.max(hints.length, images.length); @@ -259,7 +260,7 @@ function collectAttachments(messages: ModelMessage[]): UploadedImage[] { const hint = hints[i]; const image = images[i]; attachments.push({ - file_upload_id: hint?.file_upload_id ?? "", + attachmentId: hint?.attachmentId ?? "", name: hint?.name ?? image?.name ?? `photo-${i + 1}`, contentType: image?.contentType ?? "image/jpeg", dataUrl: image?.dataUrl, @@ -348,11 +349,11 @@ function imageDataToUrl( const PHOTO_HINT_RE = /\[Attached photos:\s*([^\]]+)\]/i; interface PhotoHint { - file_upload_id: string; + attachmentId: string; name: string; } -/** Parse the `[Attached photos: file_upload_id=… name=…; …]` hint into entries. */ +/** Parse the `[Attached photos: attachment_id=… name=…; …]` hint into entries. */ function parsePhotoHints(text: string): PhotoHint[] { const block = text.match(PHOTO_HINT_RE); if (!block) return []; @@ -361,11 +362,11 @@ function parsePhotoHints(text: string): PhotoHint[] { .map((entry) => entry.trim()) .filter(Boolean) .map((entry) => { - const id = entry.match(/file_upload_id=(\S+)/)?.[1] ?? ""; + const id = entry.match(/attachment_id=(\S+)/)?.[1] ?? ""; const name = entry.match(/name=([^;]+?)\s*$/)?.[1]?.trim() ?? ""; - return { file_upload_id: id, name }; + return { attachmentId: id, name }; }) - .filter((hint) => hint.file_upload_id || hint.name); + .filter((hint) => hint.attachmentId || hint.name); } // ── Helpers (focused tool / manuals) ─────────────────────────────── diff --git a/v5/src/app/api/cron/daily/route.test.ts b/v5/src/app/api/cron/daily/route.test.ts new file mode 100644 index 0000000..a4314d9 --- /dev/null +++ b/v5/src/app/api/cron/daily/route.test.ts @@ -0,0 +1,231 @@ +// @vitest-environment node + +// Vercel Blob is the one service MSW cannot stand in for, so it is mocked at +// the seam `blob.ts` exists to provide. `vi.hoisted` because the `vi.mock` +// factory runs before module scope exists. +const blob = vi.hoisted(() => ({ + configured: { value: true }, + put: vi.fn(), + putUpload: vi.fn(), + list: vi.fn(), + del: vi.fn(), +})); + +vi.mock("../../../../lib/blob", () => ({ + isBlobConfigured: () => blob.configured.value, + getBlobStore: () => ({ + put: blob.put, + putUpload: blob.putUpload, + list: blob.list, + del: blob.del, + }), +})); + +import { getDb, resetDbForTests } from "@/lib/db/client"; +import { attachments, tools } from "@/lib/db/schema/index"; +import { GET } from "./route"; + +/** + * The daily cron against the demo-seeded PGlite database. Nothing leaves the + * process and no credential is real. + */ + +const CRON_SECRET = "cron-s3cret"; +const ADMIN_SECRET = "admin-s3cret"; +const HOUR = 60 * 60 * 1000; + +beforeEach(async () => { + vi.stubEnv("DATABASE_URL", ""); + vi.stubEnv("CRON_SECRET", CRON_SECRET); + vi.stubEnv("ADMIN_REVALIDATE_SECRET", ""); + vi.stubEnv("BLOB_READ_WRITE_TOKEN", "vercel_blob_rw_test"); + + blob.configured.value = true; + blob.put.mockReset().mockResolvedValue({ pathname: "written" }); + blob.list.mockReset().mockResolvedValue([]); + blob.del.mockReset().mockResolvedValue(undefined); + + const db = await getDb(); + await db.delete(attachments); +}); + +afterAll(() => { + resetDbForTests(); +}); + +// The in-memory limiter is a per-process singleton keyed by IP. +let ipCounter = 0; +function uniqueIp() { + ipCounter += 1; + return `203.0.113.${ipCounter}`; +} + +function cronRequest(headers: Record = {}) { + return new Request("http://localhost/api/cron/daily", { + method: "GET", + headers: { "x-forwarded-for": uniqueIp(), ...headers }, + }); +} + +function authorized() { + return cronRequest({ authorization: `Bearer ${CRON_SECRET}` }); +} + +/** The body handed to `store.put`, parsed. */ +function writtenFile() { + return JSON.parse(blob.put.mock.calls[0][1] as string); +} + +describe("GET /api/cron/daily — authorization", () => { + it("refuses with 503 when no secret is configured at all", async () => { + vi.stubEnv("CRON_SECRET", ""); + vi.stubEnv("ADMIN_REVALIDATE_SECRET", ""); + + const res = await GET(cronRequest()); + + expect(res.status).toBe(503); + expect((await res.json()).error).toContain("CRON_SECRET"); + // Unconfigured is not open: nothing is read and nothing is written. + expect(blob.put).not.toHaveBeenCalled(); + }); + + it("returns 403 when the bearer is missing", async () => { + const res = await GET(cronRequest()); + expect(res.status).toBe(403); + expect(blob.put).not.toHaveBeenCalled(); + }); + + it("returns 403 when the bearer is wrong", async () => { + const res = await GET(cronRequest({ authorization: "Bearer nope" })); + expect(res.status).toBe(403); + }); + + it("accepts a Vercel Cron request carrying Authorization: Bearer $CRON_SECRET", async () => { + const res = await GET(authorized()); + expect(res.status).toBe(200); + }); + + it("still accepts the documented hand-trigger header", async () => { + // `docs/deploy.md` tells an operator to run the backup once by hand after + // the first deploy; folding the job into the cron route must not quietly + // remove that affordance. + vi.stubEnv("ADMIN_REVALIDATE_SECRET", ADMIN_SECRET); + + const res = await GET(cronRequest({ "x-admin-secret": ADMIN_SECRET })); + + expect(res.status).toBe(200); + }); +}); + +describe("GET /api/cron/daily — the backup stage", () => { + it("writes one JSON export of Postgres, whose tool count matches the seed", async () => { + const res = await GET(authorized()); + const body = await res.json(); + + expect(res.status).toBe(200); + expect(body.ok).toBe(true); + expect(body.backup.pathname).toMatch(/^backups\/\d{4}-\d{2}-\d{2}\.json$/); + + const db = await getDb(); + const seeded = await db.select().from(tools); + expect(writtenFile().tables.tools.rowCount).toBe(seeded.length); + expect(body.backup.tables.tools).toBe(seeded.length); + }); + + it("prunes an over-retention backup and leaves an unrecognised pathname alone", async () => { + blob.list.mockResolvedValue([ + { pathname: "backups/2020-01-01.json", uploadedAt: "" }, + { pathname: "uploads/project/lamp-Xa9k2.png", uploadedAt: "" }, + ]); + + const body = await (await GET(authorized())).json(); + + expect(body.backup.pruned).toEqual(["backups/2020-01-01.json"]); + }); + + it("returns 500 naming the stage when the backup write fails", async () => { + blob.put.mockRejectedValueOnce(new Error("blob down")); + + const res = await GET(authorized()); + const body = await res.json(); + + // A backup that fails quietly is worse than no backup — this has to land + // in the cron log as a failed invocation. + expect(res.status).toBe(500); + expect(body.stage).toBe("backup"); + expect(body.error).toContain("blob down"); + }); +}); + +describe("GET /api/cron/daily — the cleanup stage", () => { + async function upload( + ageHours: number, + overrides: Partial = {} + ) { + const db = await getDb(); + const [row] = await db + .insert(attachments) + .values({ + blobPathname: `uploads/chat/${crypto.randomUUID()}.png`, + access: "private", + createdAt: new Date(Date.now() - ageHours * HOUR), + ...overrides, + }) + .returning({ id: attachments.id }); + return row.id; + } + + it("sweeps an unclaimed upload older than 24 hours and reports what it did", async () => { + await upload(25); + + const body = await (await GET(authorized())).json(); + + expect(body.cleanup).toMatchObject({ + orphans: 1, + blobsDeleted: 1, + rowsDeleted: 1, + }); + }); + + it("leaves a fresh upload and a claimed one alone", async () => { + const db = await getDb(); + const [tool] = await db.select({ id: tools.id }).from(tools).limit(1); + await upload(1); + // `owner_id` is what marks a file as claimed; an old one still survives. + await upload(48, { ownerType: "tool", ownerId: tool.id }); + + const body = await (await GET(authorized())).json(); + + expect(body.cleanup.orphans).toBe(0); + expect(await db.select().from(attachments)).toHaveLength(2); + }); + + it("returns 500 naming the stage, but reports the backup that did land", async () => { + await upload(25); + // The backup's own `del` (the prune) succeeds; the cleanup's fails. + blob.del.mockResolvedValueOnce(undefined).mockRejectedValueOnce( + new Error("delete failed") + ); + + const res = await GET(authorized()); + const body = await res.json(); + + expect(res.status).toBe(500); + expect(body.stage).toBe("cleanup"); + // Today's data is safe; only the sweep needs attention. Whoever reads the + // log has to be able to tell those apart. + expect(body.backup.pathname).toMatch(/^backups\//); + }); +}); + +describe("GET /api/cron/daily — blob not configured", () => { + it("refuses with 503 naming the variable rather than running half the job", async () => { + blob.configured.value = false; + + const res = await GET(authorized()); + + expect(res.status).toBe(503); + expect((await res.json()).error).toContain("BLOB_READ_WRITE_TOKEN"); + expect(blob.put).not.toHaveBeenCalled(); + }); +}); diff --git a/v5/src/app/api/cron/daily/route.ts b/v5/src/app/api/cron/daily/route.ts new file mode 100644 index 0000000..406e8c8 --- /dev/null +++ b/v5/src/app/api/cron/daily/route.ts @@ -0,0 +1,124 @@ +import { getBlobStore, isBlobConfigured } from "../../../../lib/blob"; +import { runBackup } from "../../../../lib/cron/backup"; +import { runCleanup } from "../../../../lib/cron/cleanup"; +import { rateLimitAsync } from "../../../../lib/rate-limit"; +import { resolveIdentity } from "../../../../lib/auth/identity"; + +/** + * `GET /api/cron/daily` — the one scheduled job (data platform design spec + * §3.9). + * + * It replaces `GET /api/admin/backup`, which dumped Notion. Hobby allows a + * cron at most once a day, so everything nightly shares this one entry in + * `vercel.json`: + * + * 1. **Backup** — a JSON export of every Postgres table to a private blob, + * kept 30 days. + * 2. **Cleanup** — uploads nobody claimed within 24 hours, removed from Blob + * and from `attachments`. + * + * Mirror pushes (§3.8) and pending-tool expiry (§4.10) join this list in later + * phases; neither has a writer yet. + * + * **Nothing here fails quietly.** Both stages report, and either one failing + * makes the whole invocation non-200 so it shows in Vercel's cron log as + * failed. A backup that silently stopped running is the thing this route was + * built to prevent. + */ + +// `runtime` cannot be set when nextConfig.cacheComponents is enabled. +// Default Node.js runtime is used. +export const maxDuration = 60; + +/** + * Two accepted callers, carried over from `/api/admin/backup` rather than + * quietly dropped: Vercel Cron, which sends `Authorization: Bearer + * $CRON_SECRET`, and a person holding `ADMIN_REVALIDATE_SECRET` — the + * hand-trigger `docs/deploy.md` documents for the first run after a deploy. + * + * With neither secret set the route is **unconfigured, not open**. An + * unauthenticated endpoint that dumps every student email is not a state to + * degrade into, so it refuses and says which variable is missing (Article 4). + */ +type AuthResult = "ok" | "unconfigured" | "forbidden"; + +function authorize(req: Request): AuthResult { + const cronSecret = process.env.CRON_SECRET; + const adminSecret = process.env.ADMIN_REVALIDATE_SECRET; + if (!cronSecret && !adminSecret) return "unconfigured"; + + if (cronSecret && req.headers.get("authorization") === `Bearer ${cronSecret}`) { + return "ok"; + } + if (adminSecret && req.headers.get("x-admin-secret") === adminSecret) { + return "ok"; + } + return "forbidden"; +} + +function message(error: unknown): string { + return error instanceof Error ? error.message : "unknown error"; +} + +export async function GET(req: Request) { + const auth = authorize(req); + if (auth === "unconfigured") { + return Response.json( + { ok: false, error: "cron is not configured: CRON_SECRET is not set" }, + { status: 503 } + ); + } + if (auth === "forbidden") { + return Response.json({ ok: false, error: "forbidden" }, { status: 403 }); + } + + // Article 4: bound the expensive work before doing any of it. The route is + // secret-gated, so this is the second line — a leaked secret in a loop would + // otherwise be one full database dump per request. Generous enough that a + // daily cron plus a few manual retries never trips it. + const identity = await resolveIdentity(req); + const { allowed } = await rateLimitAsync(`cron:${identity.rateLimitKey}`, { + limit: 10, + windowMs: 60 * 60_000, + }); + if (!allowed) { + return Response.json( + { ok: false, error: "Too many requests. Please slow down." }, + { status: 429, headers: { "Retry-After": "3600" } } + ); + } + + if (!isBlobConfigured()) { + return Response.json( + { ok: false, error: "BLOB_READ_WRITE_TOKEN is not set" }, + { status: 503 } + ); + } + + const store = getBlobStore(); + + let backup: Awaited>; + try { + backup = await runBackup(store); + } catch (error) { + console.error("[cron] backup failed:", error); + return Response.json( + { ok: false, stage: "backup", error: message(error) }, + { status: 500 } + ); + } + + // Cleanup runs second and reports separately: today's data is already safe, + // so a sweep that fails is worth a failed invocation but not a lost backup — + // whoever reads the log needs to be able to tell those apart. + try { + const cleanup = await runCleanup(store); + return Response.json({ ok: true, backup, cleanup }); + } catch (error) { + console.error("[cron] cleanup failed:", error); + return Response.json( + { ok: false, stage: "cleanup", backup, error: message(error) }, + { status: 500 } + ); + } +} diff --git a/v5/src/app/api/flags/route.test.ts b/v5/src/app/api/flags/route.test.ts index 4ce6754..ad661e2 100644 --- a/v5/src/app/api/flags/route.test.ts +++ b/v5/src/app/api/flags/route.test.ts @@ -1,28 +1,33 @@ // @vitest-environment node -import { http, HttpResponse } from "msw"; -import { server } from "../../../../test/msw/server"; -import { DB_IDS } from "../../../../test/msw/handlers"; import { nextCacheMock } from "../../../../test/mocks/next-cache"; // The route reaches the catalog (for tool resolution) which imports next/cache. vi.mock("next/cache", () => nextCacheMock()); import { getCatalogTools } from "@/lib/catalog"; -import { resetDbForTests } from "@/lib/db/client"; -import { DEMO_FORM_4_NOTION_PAGE_ID } from "@/lib/db/demo-seed"; +import { getDb, resetDbForTests } from "@/lib/db/client"; +import { feedback, tools as toolsTable } from "@/lib/db/schema/index"; +import { resetAuthForTests } from "@/lib/auth/config"; +import { signInAsNew } from "../../../../test/utils/session"; import { POST } from "./route"; -const NOTION = "https://api.notion.com/v1"; +/** + * `POST /api/flags` against the demo-seeded PGlite database, with **no Notion + * environment stubbed anywhere** — the write is local now, so there is no + * credential this route could be missing (Article 3). + */ + +const AUTH_SECRET = "flags-route-test-secret"; -// The route resolves the flagged tool against the demo-seeded PGlite database -// (`DATABASE_URL` unset). Its id is a Postgres uuid minted at seed time, so it -// is looked up rather than hard-coded. let FORM_4_ID = ""; beforeEach(async () => { vi.stubEnv("DATABASE_URL", ""); + resetAuthForTests(); const tools = await getCatalogTools(); FORM_4_ID = tools.find((tool) => tool.slug === "form-4")?.id ?? ""; + const db = await getDb(); + await db.delete(feedback); }); afterAll(() => { @@ -37,15 +42,15 @@ function uniqueIp() { return `198.51.100.${ipCounter}`; } -function stubFlagsEnv() { - vi.stubEnv("NOTION_API_KEY", "secret_test"); - vi.stubEnv("NOTION_DB_FLAGS", DB_IDS.flags); -} - -function flagRequest(body: unknown, ip = uniqueIp()) { +function flagRequest(body: unknown, ip = uniqueIp(), cookie?: string) { + const headers: Record = { + "content-type": "application/json", + "x-forwarded-for": ip, + }; + if (cookie) headers.cookie = cookie; return new Request("http://localhost/api/flags", { method: "POST", - headers: { "content-type": "application/json", "x-forwarded-for": ip }, + headers, body: typeof body === "string" ? body : JSON.stringify(body), }) as never; } @@ -59,108 +64,117 @@ function validPayload(overrides: Record = {}) { }; } -/** Record every Notion write the route provokes, per database. */ -function captureNotionWrites() { - const creates: Array<{ parent: { database_id: string }; properties: Record }> = - []; - const patches: string[] = []; - server.use( - http.post(`${NOTION}/pages`, async ({ request }) => { - const body = (await request.json()) as (typeof creates)[number]; - creates.push(body); - return HttpResponse.json({ - object: "page", - id: "flag-page-1", - created_time: "2026-07-29T10:00:00.000Z", - last_edited_time: "2026-07-29T10:00:00.000Z", - properties: body.properties, - }); - }), - http.patch(`${NOTION}/pages/:id`, ({ params }) => { - patches.push(params.id as string); - return HttpResponse.json({ object: "page", id: params.id }); - }) - ); - return { creates, patches }; +/** + * A seeded session — a `user` row, a `session` row and the cookie Better Auth + * would have set. The only way to reach the route as a signed-in person + * without a network call (Article 3). + */ +async function signedIn(email: string, name: string) { + vi.stubEnv("AUTH_SECRET", AUTH_SECRET); + resetAuthForTests(); + return signInAsNew({ email, name }); } -describe("POST /api/flags", () => { - it("creates a Flags row with status New and returns 201", async () => { - stubFlagsEnv(); - const { creates } = captureNotionWrites(); +/** Every correction currently in the table. */ +async function storedRows() { + const db = await getDb(); + return db.select().from(feedback); +} +describe("POST /api/flags", () => { + it("inserts a feedback row with status `new` and returns 201", async () => { const res = await POST(flagRequest(validPayload({ reporter: "Ada" }))); expect(res.status).toBe(201); - expect(await res.json()).toEqual({ id: "flag-page-1" }); - expect(creates).toHaveLength(1); - expect(creates[0].properties).toMatchObject({ - status: { select: { name: "New" } }, - field_flagged: { select: { name: "location" } }, - // The relation addresses the tool's Notion page, not its Postgres id. - tool: { relation: [{ id: DEMO_FORM_4_NOTION_PAGE_ID }] }, + const rows = await storedRows(); + expect(rows).toHaveLength(1); + expect(await res.json()).toEqual({ id: rows[0].id }); + expect(rows[0]).toMatchObject({ + toolId: FORM_4_ID, + fieldFlagged: "location", + status: "new", + reporterName: "Ada", + issueDescription: "This lives in the Resin Bench, not the Wood Shop.", }); }); // The assertion that matters (spec §10): a flag is inert. - it("never writes to the Tools database", async () => { - stubFlagsEnv(); - const { creates, patches } = captureNotionWrites(); + it("never writes to the tools table", async () => { + const db = await getDb(); + const before = await db.select().from(toolsTable); await POST(flagRequest(validPayload())); - expect(creates).toHaveLength(1); - expect(creates[0].parent.database_id).toBe(DB_IDS.flags); - expect( - creates.some((create) => create.parent.database_id === DB_IDS.tools) - ).toBe(false); - // Nor does it edit the tool page it refers to. - expect(patches).toEqual([]); + expect(await db.select().from(toolsTable)).toEqual(before); }); - it("rejects an invalid field_flagged with 400 and writes nothing", async () => { - stubFlagsEnv(); - const { creates } = captureNotionWrites(); + it("resolves the tool by slug as well as by uuid", async () => { + const res = await POST(flagRequest(validPayload({ tool_id: "form-4" }))); + + expect(res.status).toBe(201); + expect((await storedRows())[0].toolId).toBe(FORM_4_ID); + }); + + it("records the reporter's email and id from a session, never from the body", async () => { + const reporter = await signedIn("ada@cornell.edu", "Ada Lovelace"); + + const res = await POST( + flagRequest( + validPayload({ reporter_email: "dean@cornell.edu" }), + uniqueIp(), + reporter.cookie + ) + ); + expect(res.status).toBe(201); + const [row] = await storedRows(); + expect(row.reporterEmail).toBe("ada@cornell.edu"); + expect(row.reporterUserId).toBe(reporter.user.id); + expect(JSON.stringify(row)).not.toContain("dean@cornell.edu"); + }); + + it("files an anonymous correction with no email at all", async () => { + const res = await POST( + flagRequest(validPayload({ reporter_email: "dean@cornell.edu" })) + ); + + // Anonymous reporting stays the intended default (§8), and a client may + // not assert who it is. + expect(res.status).toBe(201); + const [row] = await storedRows(); + expect(row.reporterEmail).toBeNull(); + expect(row.reporterUserId).toBeNull(); + }); + + it("rejects an invalid field_flagged with 400 and writes nothing", async () => { const res = await POST(flagRequest(validPayload({ field_flagged: "price" }))); expect(res.status).toBe(400); expect(await res.json()).toEqual({ code: "invalid_input" }); - expect(creates).toHaveLength(0); + expect(await storedRows()).toHaveLength(0); }); it("rejects an empty description with 400", async () => { - stubFlagsEnv(); const res = await POST(flagRequest(validPayload({ issue_description: " " }))); expect(res.status).toBe(400); expect(await res.json()).toEqual({ code: "invalid_input" }); + expect(await storedRows()).toHaveLength(0); }); it("rejects a body that is not JSON with 400", async () => { - stubFlagsEnv(); const res = await POST(flagRequest("not json at all")); expect(res.status).toBe(400); expect(await res.json()).toEqual({ code: "invalid_input" }); }); it("returns 404 for a tool that is not in the catalog", async () => { - stubFlagsEnv(); const res = await POST(flagRequest(validPayload({ tool_id: "tool-nope" }))); expect(res.status).toBe(404); expect(await res.json()).toEqual({ code: "unknown_tool" }); - }); - - it("returns 503 when the Flags database is not configured", async () => { - vi.stubEnv("NOTION_API_KEY", ""); - vi.stubEnv("NOTION_DB_FLAGS", ""); - const res = await POST(flagRequest(validPayload())); - expect(res.status).toBe(503); - expect(await res.json()).toEqual({ code: "not_configured" }); + expect(await storedRows()).toHaveLength(0); }); it("returns 429 after the fifth report from one IP", async () => { - stubFlagsEnv(); - captureNotionWrites(); const ip = uniqueIp(); for (let attempt = 1; attempt <= 5; attempt += 1) { @@ -172,31 +186,27 @@ describe("POST /api/flags", () => { expect(limited.status).toBe(429); expect(limited.headers.get("Retry-After")).toBe("3600"); expect(await limited.json()).toEqual({ code: "rate_limited" }); + // Rate limiting happens before any database work (Article 4). + expect(await storedRows()).toHaveLength(5); }); - it("returns 502 on a Notion failure without leaking the Notion error", async () => { - stubFlagsEnv(); + it("returns 502 on a database failure without leaking the driver's error", async () => { const logged = vi.spyOn(console, "error").mockImplementation(() => {}); - server.use( - http.post(`${NOTION}/pages`, () => - HttpResponse.json( - { - object: "error", - code: "validation_error", - message: "property 'field_flagged' does not exist", - }, - { status: 400 } - ) - ) - ); + vi.stubEnv("DATABASE_URL", "postgres://user:hunter2@127.0.0.1:1/none"); + resetDbForTests(); const res = await POST(flagRequest(validPayload())); expect(res.status).toBe(502); const body = await res.text(); expect(body).toBe(JSON.stringify({ code: "write_failed" })); - expect(body).not.toMatch(/validation_error|does not exist|property/i); + expect(body).not.toMatch(/hunter2|postgres|ECONNREFUSED/i); // The detail is still visible to operators in the server log (Art. 4). expect(logged).toHaveBeenCalled(); + + // Put the demo substrate back before the file's own cleanup runs: the + // stub is still in force until vitest's global afterEach clears it. + vi.stubEnv("DATABASE_URL", ""); + resetDbForTests(); }); }); diff --git a/v5/src/app/api/flags/route.ts b/v5/src/app/api/flags/route.ts index 05072f8..3d9ff71 100644 --- a/v5/src/app/api/flags/route.ts +++ b/v5/src/app/api/flags/route.ts @@ -9,8 +9,8 @@ import { resolveIdentity } from "../../../lib/auth/identity"; /** * `POST /api/flags` — the form path for "report a correction" (design spec - * 2026-07-29 §3, §9.2). It owns nothing: validation and the Notion write both - * live in the `flags` capability, so this route and the assistant's + * 2026-07-29 §3, §9.2). It owns nothing: validation and the write both live in + * the `flags` capability, so this route and the assistant's * `report_correction` tool share one code path (constitution Art. 2). * * Responses carry a machine-readable `code` rather than a prose message — @@ -25,6 +25,10 @@ export const maxDuration = 15; /** 5 per hour per IP — tighter than chat, looser than project submission (§8). */ const RATE_LIMIT = { limit: 5, windowMs: 60 * 60_000 }; +// `not_configured` is unreachable since the write moved to Postgres — there is +// no credential left that could be missing — but the code stays declared here +// and in `FlagButton`'s message map rather than being retired across three +// files for nothing. const STATUS_BY_CODE: Record = { invalid_input: 400, unknown_tool: 404, @@ -61,14 +65,18 @@ export async function POST(req: NextRequest) { const parsed = parseCorrectionReport(payload); if (!parsed.ok) return fail(parsed.code); - // `reporter_email` is only ever written from the server-resolved session - // above — a client may not assert its own identity. Anonymous reporting stays - // the intended default (§8): an unauthenticated caller simply has no email, - // and the report is filed without one. + // `reporter_email` and `reporter_user_id` are only ever written from the + // server-resolved session above — a client may not assert its own identity. + // Anonymous reporting stays the intended default (§8): an unauthenticated + // caller simply has no email, and the report is filed without one. const result = await submitCorrection( parsed.report, identity.email - ? { name: identity.name ?? undefined, email: identity.email } + ? { + name: identity.name ?? undefined, + email: identity.email, + userId: identity.userId ?? undefined, + } : undefined ); if (!result.ok) return fail(result.code); diff --git a/v5/src/app/api/identity/route.test.ts b/v5/src/app/api/identity/route.test.ts index 71d7af2..e76f028 100644 --- a/v5/src/app/api/identity/route.test.ts +++ b/v5/src/app/api/identity/route.test.ts @@ -1,16 +1,20 @@ +// @vitest-environment node /** - * `GET /api/identity` — the projection the header reads (auth design spec §6). + * `GET /api/identity` — the projection the header reads (spec §3.5, auth spec §6). * - * Uses the real `resolveIdentity` and the real limiter; the only thing minted - * here is a session cookie, signed exactly the way the callback signs one. No - * network, no OAuth (Article 3). + * Uses the real `resolveIdentity` and the real limiter against real session + * rows in PGlite. No network, no OAuth (Article 3): `signInAsNew` seeds the + * row and mints the cookie Better Auth would have set. */ -import { - SESSION_COOKIE_NAME, - createSessionPayload, - signSession, -} from "@/lib/auth/session-cookie"; import { GET } from "@/app/api/identity/route"; +import { resetAuthForTests } from "@/lib/auth/config"; +import { resetDbForTests } from "@/lib/db/client"; +import { + BETTER_AUTH_SESSION_COOKIE, + seedUser, + signInAs, + signInAsNew, +} from "../../../../test/utils/session"; const AUTH_SECRET = "identity-route-test-secret"; @@ -22,14 +26,6 @@ function uniqueIp() { return `203.0.113.${counter}`; } -async function cookieFor(sub: string, email: string, name: string | null) { - const token = await signSession( - createSessionPayload({ sub, email, name }), - AUTH_SECRET - ); - return `${SESSION_COOKIE_NAME}=${token}`; -} - function identityRequest({ ip = uniqueIp(), cookie }: { ip?: string; cookie?: string } = {}) { const headers: Record = { "x-forwarded-for": ip }; if (cookie) headers.cookie = cookie; @@ -37,7 +33,14 @@ function identityRequest({ ip = uniqueIp(), cookie }: { ip?: string; cookie?: st } beforeEach(() => { + vi.stubEnv("DATABASE_URL", ""); vi.stubEnv("AUTH_SECRET", AUTH_SECRET); + resetAuthForTests(); +}); + +afterEach(() => { + resetAuthForTests(); + resetDbForTests(); }); describe("GET /api/identity", () => { @@ -48,24 +51,33 @@ describe("GET /api/identity", () => { expect(await res.json()).toEqual({ role: "anonymous", name: null }); }); - it("returns the role and display name of a signed-in student", async () => { - const cookie = await cookieFor("sub-1", "ada@cornell.edu", "Ada Lovelace"); + it("returns the role and display name of a signed-in user", async () => { + const { cookie } = await signInAsNew({ + email: "ada@cornell.edu", + name: "Ada Lovelace", + }); const res = await GET(identityRequest({ cookie })); expect(res.status).toBe(200); - expect(await res.json()).toEqual({ role: "student", name: "Ada Lovelace" }); + expect(await res.json()).toEqual({ role: "user", name: "Ada Lovelace" }); }); - it("reflects the staff role from AUTH_STAFF_EMAILS", async () => { - vi.stubEnv("AUTH_STAFF_EMAILS", "niti@cornell.edu"); - const cookie = await cookieFor("sub-2", "niti@cornell.edu", "Niti"); - - expect((await (await GET(identityRequest({ cookie }))).json()).role).toBe("staff"); + it("reports the role from the database, for every stored role", async () => { + // What the header reads to decide whether to show Add and Refresh. The + // env lists that used to answer this question are gone. + for (const role of ["user", "admin", "super_admin"] as const) { + const { cookie } = await signInAsNew({ role }); + const body = await (await GET(identityRequest({ cookie }))).json(); + expect(body.role).toBe(role); + } }); it("never returns the email address — the header only needs a name", async () => { - const cookie = await cookieFor("sub-3", "ada@cornell.edu", "Ada Lovelace"); + const { cookie } = await signInAsNew({ + email: "ada@cornell.edu", + name: "Ada Lovelace", + }); const body = await (await GET(identityRequest({ cookie }))).json(); @@ -75,10 +87,8 @@ describe("GET /api/identity", () => { }); it("degrades to anonymous on a tampered cookie rather than throwing", async () => { - const cookie = await cookieFor("sub-4", "ada@cornell.edu", "Ada"); - const [name, token] = cookie.split("="); - const [payload, signature] = token.split("."); - const forged = `${name}=${payload}.${signature.startsWith("A") ? "B" : "A"}${signature.slice(1)}`; + const { cookie } = await signInAsNew(); + const forged = cookie.replace(/.$/, (c) => (c === "A" ? "B" : "A")); const res = await GET(identityRequest({ cookie: forged })); @@ -86,24 +96,31 @@ describe("GET /api/identity", () => { expect((await res.json()).role).toBe("anonymous"); }); - it("degrades to anonymous on an expired cookie", async () => { - const token = await signSession( - createSessionPayload( - { sub: "sub-5", email: "ada@cornell.edu", name: "Ada" }, - Date.now() - 60_000, - 1 // one-second lifetime, already spent - ), - AUTH_SECRET - ); + it("degrades to anonymous on an expired session", async () => { + const { cookie } = await signInAsNew({}, { expiresInSeconds: -60 }); - const res = await GET( - identityRequest({ cookie: `${SESSION_COOKIE_NAME}=${token}` }) - ); + const res = await GET(identityRequest({ cookie })); expect(res.status).toBe(200); expect((await res.json()).role).toBe("anonymous"); }); + it("degrades to anonymous for a banned user", async () => { + const banned = await seedUser({ banned: true, banReason: "spam" }); + const { cookie } = await signInAs(banned); + + expect((await (await GET(identityRequest({ cookie }))).json()).role).toBe( + "anonymous" + ); + }); + + it("degrades to anonymous when the cookie names no session at all", async () => { + const cookie = `${BETTER_AUTH_SESSION_COOKIE}=not-a-real-token`; + expect((await (await GET(identityRequest({ cookie }))).json()).role).toBe( + "anonymous" + ); + }); + it("is never cached — it is per-request and per-person", async () => { const res = await GET(identityRequest()); expect(res.headers.get("cache-control")).toContain("no-store"); diff --git a/v5/src/app/api/projects/route.test.ts b/v5/src/app/api/projects/route.test.ts index c9a79a6..53cc57e 100644 --- a/v5/src/app/api/projects/route.test.ts +++ b/v5/src/app/api/projects/route.test.ts @@ -1,38 +1,76 @@ // @vitest-environment node -import { http, HttpResponse } from "msw"; - -import { server } from "../../../../test/msw/server"; -import { - SESSION_COOKIE_NAME, - createSessionPayload, - signSession, -} from "@/lib/auth/session-cookie"; +import { eq } from "drizzle-orm"; + +import { resetAuthForTests } from "@/lib/auth/config"; +import { signInAsNew } from "../../../../test/utils/session"; import { nextCacheMock } from "../../../../test/mocks/next-cache"; -// The route translates catalogue ids to Notion page ids through Postgres, and -// that module's neighbours import cacheTag/cacheLife. +// The route's neighbours in `src/lib/data` pull in modules that import +// cacheTag/cacheLife. vi.mock("next/cache", () => nextCacheMock()); import { getCatalogTools } from "@/lib/catalog"; -import { resetDbForTests } from "@/lib/db/client"; -import { DEMO_FORM_4_NOTION_PAGE_ID } from "@/lib/db/demo-seed"; +import { getPublishedProjects } from "@/lib/projects"; +import { getDb, resetDbForTests } from "@/lib/db/client"; +import { attachments, projectTools, projects } from "@/lib/db/schema/index"; + +/** + * `POST /api/projects` against the demo-seeded PGlite database, with **no + * environment variables stubbed** — the submission is a Postgres row now, so + * there is no credential this route could be missing (Article 3). + */ -const NOTION = "https://api.notion.com/v1"; -const PROJECTS_DB = "db-projects"; const AUTH_SECRET = "projects-route-test-secret"; -// `tools_used` is a Notion relation, but the form now submits catalogue ids — -// Postgres uuids minted at seed time. The demo seed is the substrate here -// (`DATABASE_URL` unset): the Form 4 came from Notion and has a page id, the -// Trotec did not and has none. +// The form submits catalogue ids — Postgres uuids minted at seed time — so +// they are resolved rather than hard-coded. let FORM_4_ID = ""; let TROTEC_ID = ""; +// The signed-in student every test submits as unless it says otherwise. +let defaultSession: Awaited>; +let sessionCounter = 0; + +/** + * The route warns when a submission's photos did not attach, and most payloads + * here carry a photo id no `attachments` row answers to — so the warning is the + * expected case, not a surprise. Captured rather than printed, and read back + * with `vi.mocked(console.warn)` where it is the thing under test. + * `vi.restoreAllMocks()` in the global setup puts the real console back after + * every test. + */ +function warnings() { + return vi.mocked(console.warn); +} + beforeEach(async () => { + vi.spyOn(console, "warn").mockImplementation(() => {}); vi.stubEnv("DATABASE_URL", ""); + vi.stubEnv("AUTH_SECRET", AUTH_SECRET); + resetAuthForTests(); const tools = await getCatalogTools(); FORM_4_ID = tools.find((tool) => tool.slug === "form-4")?.id ?? ""; TROTEC_ID = tools.find((tool) => tool.slug === "trotec-speedy-400")?.id ?? ""; + + const db = await getDb(); + // The demo seed ships one published sample project; clearing the table keeps + // each test's assertions about "the submission" unambiguous. + await db.delete(attachments); + await db.delete(projects); + + // Since Phase 4 this route requires sign-in (spec §5.5), so the *default* + // caller is a signed-in student. A fresh account per test on purpose: the + // limiter keys a signed-in caller by user id, so one test's ten requests + // must not be another's, the way `uniqueIp()` already isolates anonymous ones. + sessionCounter += 1; + defaultSession = await signInAsNew({ + email: `ada-${sessionCounter}@cornell.edu`, + name: "Ada Lovelace", + }); +}); + +afterEach(() => { + resetAuthForTests(); }); afterAll(() => { @@ -50,21 +88,20 @@ function uniqueIp() { return `10.1.0.${ipCounter}`; } -function stubProjectsEnv() { - vi.stubEnv("NOTION_API_KEY", "secret_test"); - vi.stubEnv("NOTION_DB_PROJECTS", PROJECTS_DB); -} - interface SubmitOptions { ip?: string; raw?: string; - cookie?: string; + /** A different session, or `null` to submit as an anonymous visitor. */ + cookie?: string | null; } // The route only uses `getClientIp(req)` (headers), the session cookie, and // `req.json()`, so a plain Request is enough; it's cast at the call site because // the signature asks for a NextRequest. -function submitRequest(payload: unknown, { ip, raw, cookie }: SubmitOptions = {}) { +function submitRequest(payload: unknown, options: SubmitOptions = {}) { + const { ip, raw } = options; + const cookie = + "cookie" in options ? options.cookie : defaultSession.cookie; const headers: Record = { "content-type": "application/json", "x-forwarded-for": ip ?? uniqueIp(), @@ -78,23 +115,17 @@ function submitRequest(payload: unknown, { ip, raw, cookie }: SubmitOptions = {} } /** - * A session cookie signed exactly the way the OAuth callback signs one — the - * only way to reach the route as a signed-in student without a network call - * (Article 3). + * A seeded session — a `user` row, a `session` row and the cookie Better Auth + * would have set. The only way to reach the route as a signed-in person + * without a network call (Article 3). */ -async function cookieFor(email: string, name: string | null) { - vi.stubEnv("AUTH_SECRET", AUTH_SECRET); - const token = await signSession( - createSessionPayload({ sub: `sub-${email}`, email, name }), - AUTH_SECRET - ); - return `${SESSION_COOKIE_NAME}=${token}`; +async function signedIn(email: string, name: string) { + return signInAsNew({ email, name }); } function validPayload(overrides: Record = {}) { return { title: "Lamp from scrap plywood", - author: "Ada Lovelace", body: "Cut on the laser, glued, sanded.", tools: [FORM_4_ID], materials: ["Plywood"], @@ -103,34 +134,6 @@ function validPayload(overrides: Record = {}) { }; } -type NotionCreateBody = { - parent?: { database_id?: string }; - properties?: Record; -}; - -/** - * Capture the body of the Notion page-create call. Returns a getter that is - * `undefined` when the route never reached Notion — which is itself the - * assertion for every rejected payload. - */ -function captureNotionCreate() { - const calls: NotionCreateBody[] = []; - server.use( - http.post(`${NOTION}/pages`, async ({ request }) => { - const body = (await request.json()) as NotionCreateBody; - calls.push(body); - return HttpResponse.json({ - object: "page", - id: "created-project-1", - created_time: "2024-09-01T10:00:00.000Z", - last_edited_time: "2024-09-01T10:00:00.000Z", - properties: body.properties ?? {}, - }); - }) - ); - return calls; -} - async function loadRoute() { return import("./route"); } @@ -140,41 +143,49 @@ async function post(req: Request) { return POST(req as never); } -// ── Configuration ─────────────────────────────────────────────────── - -describe("POST /api/projects (not configured)", () => { - it("returns 503 with a clear message and never calls Notion", async () => { - vi.stubEnv("NOTION_API_KEY", ""); - vi.stubEnv("NOTION_DB_PROJECTS", ""); - - const res = await post(submitRequest(validPayload())); +/** Every project row currently in the table. */ +async function storedProjects() { + const db = await getDb(); + return db.select().from(projects); +} - expect(res.status).toBe(503); - const body = await res.json(); - expect(body.error).toMatch(/not configured/i); - }); -}); +/** An uploaded-but-unattached file, as `POST /api/uploads` will leave one. */ +async function upload(): Promise { + const db = await getDb(); + const [row] = await db + .insert(attachments) + .values({ + blobPathname: `uploads/${crypto.randomUUID()}.png`, + access: "public", + publicUrl: `https://blob.test/${crypto.randomUUID()}.png`, + contentType: "image/png", + }) + .returning({ id: attachments.id }); + return row.id; +} // ── The moderation gate (Article 5) ───────────────────────────────── describe("POST /api/projects (drafts by default)", () => { - it("creates the page with published explicitly false", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); - + it("records one unpublished row and answers with its id and slug", async () => { const res = await post(submitRequest(validPayload())); expect(res.status).toBe(201); - expect(await res.json()).toEqual({ id: "created-project-1" }); - expect(calls).toHaveLength(1); - expect(calls[0].parent?.database_id).toBe(PROJECTS_DB); - expect(calls[0].properties?.published).toEqual({ checkbox: false }); + const rows = await storedProjects(); + expect(rows).toHaveLength(1); + expect(await res.json()).toEqual({ + id: rows[0].id, + slug: rows[0].slug, + // The photo counts ride along on every submission — see the "photos that + // did not attach" block below for why they are there. + photosSubmitted: 1, + photosAttached: 0, + }); + expect(rows[0].published).toBe(false); + expect(rows[0].slug).toBe("lamp-from-scrap-plywood"); }); it("IGNORES published:true from the client — the submission stays a draft", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); - const res = await post( submitRequest( validPayload({ published: true, Published: true, fields: { published: true } }) @@ -183,17 +194,22 @@ describe("POST /api/projects (drafts by default)", () => { expect(res.status).toBe(201); // The single most important assertion in this feature: nothing a client - // sends may publish a project. Staff tick the box in Notion, or it stays + // sends may publish a project. Staff publish it in the app, or it stays // invisible. - expect(calls[0].properties?.published).toEqual({ checkbox: false }); - expect(JSON.stringify(calls[0])).not.toContain('"checkbox":true'); + const [row] = await storedProjects(); + expect(row.published).toBe(false); + expect(row.publishedAt).toBeNull(); + expect(row.publishedBy).toBeNull(); }); - it("writes the submitted fields through to Notion", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); + it("does not appear in the gallery, which is what 'draft' actually means", async () => { + await post(submitRequest(validPayload())); + + expect(await getPublishedProjects()).toEqual([]); + }); - await post( + it("writes the submitted fields through to the row", async () => { + const res = await post( submitRequest( validPayload({ link: "https://example.com/lamp", @@ -202,175 +218,246 @@ describe("POST /api/projects (drafts by default)", () => { ) ); - const properties = calls[0].properties as Record; - expect(properties.title).toEqual({ - title: [{ text: { content: "Lamp from scrap plywood" } }], - }); - expect(properties.author).toEqual({ - rich_text: [{ text: { content: "Ada Lovelace" } }], - }); - expect(properties.link).toEqual({ url: "https://example.com/lamp" }); - // Each catalogue id is translated to the tool's Notion page; the Trotec has - // no imported page, so it drops out rather than failing the whole write. - expect(properties.tools_used).toEqual({ - relation: [{ id: DEMO_FORM_4_NOTION_PAGE_ID }], - }); - expect(properties.materials).toEqual({ multi_select: [{ name: "Plywood" }] }); - // Photos go in as file_upload references from /api/upload-notion. - expect(properties.photos).toEqual({ - files: [ - { - type: "file_upload", - file_upload: { id: "file-upload-1" }, - name: "cover.png", - }, - ], - }); - }); -}); - -// ── Verified authorship (spec §4, §5) ─────────────────────────────── - -describe("POST /api/projects (author identity)", () => { - it("records author_email from the session of a signed-in student", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); - const cookie = await cookieFor("ada@cornell.edu", "Ada Lovelace"); - - const res = await post(submitRequest(validPayload(), { cookie })); - expect(res.status).toBe(201); - const properties = calls[0].properties as Record; - expect(properties.author_email).toEqual({ email: "ada@cornell.edu" }); - // The byline comes from the session too — the form makes the field - // read-only, and this is what makes that guarantee real. - expect(properties.author).toEqual({ - rich_text: [{ text: { content: "Ada Lovelace" } }], - }); + const [row] = await storedProjects(); + expect(row.title).toBe("Lamp from scrap plywood"); + expect(row.authorName).toBe("Ada Lovelace"); + expect(row.body).toBe("Cut on the laser, glued, sanded."); + expect(row.link).toBe("https://example.com/lamp"); + expect(row.materials).toEqual(["Plywood"]); + + const db = await getDb(); + const links = await db + .select() + .from(projectTools) + .where(eq(projectTools.projectId, row.id)); + expect(links.map((link) => link.toolId).sort()).toEqual([FORM_4_ID, TROTEC_ID].sort()); }); - it("uses the session name even when the body claims a different author", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); - const cookie = await cookieFor("ada@cornell.edu", "Ada Lovelace"); + it("claims the uploaded photos onto the project, cover first", async () => { + const cover = await upload(); + const second = await upload(); const res = await post( - submitRequest(validPayload({ author: "Somebody Else" }), { cookie }) + submitRequest( + validPayload({ + photos: [ + { id: cover, name: "cover.png" }, + { id: second, name: "detail.png" }, + ], + }) + ) ); expect(res.status).toBe(201); - expect((calls[0].properties as Record).author).toEqual({ - rich_text: [{ text: { content: "Ada Lovelace" } }], - }); + const [row] = await storedProjects(); + const db = await getDb(); + const photos = await db + .select() + .from(attachments) + .where(eq(attachments.ownerId, row.id)) + .orderBy(attachments.position); + expect(photos.map((photo) => photo.id)).toEqual([cover, second]); + expect(photos.every((photo) => photo.ownerType === "project")).toBe(true); }); - it("records no author_email for an anonymous submission, which still succeeds", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); + it("records the submission even when no photo id matches an upload", async () => { + // The write-up is worth more than the photos (Article 4) — and until + // `/api/uploads` lands, the ids the form holds are Notion file_upload ids + // that no `attachments` row answers to. + const res = await post(submitRequest(validPayload())); + + expect(res.status).toBe(201); + expect(await storedProjects()).toHaveLength(1); + }); + it("gives a second submission with the same title its own slug", async () => { + await post(submitRequest(validPayload())); const res = await post(submitRequest(validPayload())); - // Anonymous submission is deliberate (spec §5) — the ISAM demo and any - // student who has not signed in must still be able to contribute. expect(res.status).toBe(201); - const properties = calls[0].properties as Record; - expect(properties).not.toHaveProperty("author_email"); - expect(JSON.stringify(calls[0])).not.toContain("author_email"); - expect(properties.author).toEqual({ - rich_text: [{ text: { content: "Ada Lovelace" } }], - }); + expect((await res.json()).slug).toBe("lamp-from-scrap-plywood-2"); }); +}); + +// ── Photos that did not attach (Article 4) ────────────────────────── - it("IGNORES author_email supplied in the body by an anonymous caller", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); +describe("POST /api/projects (photos that did not attach)", () => { + it("reports both counts so the form can tell the student what landed", async () => { + const kept = await upload(); const res = await post( submitRequest( validPayload({ - author_email: "dean@cornell.edu", - authorEmail: "dean@cornell.edu", + photos: [ + { id: kept, name: "cover.png" }, + // Uploaded, then swept by the nightly cron while the tab sat open: + // uuid-shaped, and nothing answers to it any more. + { id: crypto.randomUUID(), name: "detail.png" }, + ], }) ) ); expect(res.status).toBe(201); - // A client may not assert who it is: no session, no verified author. - expect(JSON.stringify(calls[0])).not.toContain("dean@cornell.edu"); - expect(calls[0].properties).not.toHaveProperty("author_email"); + expect(await res.json()).toMatchObject({ photosSubmitted: 2, photosAttached: 1 }); }); - it("IGNORES author_email in the body when a session says something else", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); - const cookie = await cookieFor("ada@cornell.edu", "Ada Lovelace"); + it("says none attached rather than thanking the student for pictures nobody has", async () => { + // The exact shape of the reviewed bug: three photos uploaded yesterday, + // `runCleanup` deleted the unclaimed rows overnight, the write-up lands and + // the pictures do not. A 201 that says nothing about it is a quiet lie. + const stale = Array.from({ length: 3 }, (_, i) => ({ + id: crypto.randomUUID(), + name: `photo-${i}.png`, + })); + + const res = await post(submitRequest(validPayload({ photos: stale }))); + + expect(res.status).toBe(201); + expect(await res.json()).toMatchObject({ photosSubmitted: 3, photosAttached: 0 }); + // And staff can find it afterwards, the way the maintenance path already + // logs its own lost photos. + expect(warnings()).toHaveBeenCalledWith( + expect.stringContaining("saved without 3 of its 3 photo(s)") + ); + // The write-up itself is still there — losing the photos never costs the + // student the submission. + expect(await storedProjects()).toHaveLength(1); + }); + + it("claims nothing and says so when the photos belong to somebody else", async () => { + // An already-claimed attachment is not annexable (`owner_id is null` is part + // of the WHERE), so the count tells the truth here too. + const someoneElses = await upload(); + await post(submitRequest(validPayload({ photos: [{ id: someoneElses, name: "a.png" }] }))); const res = await post( - submitRequest(validPayload({ author_email: "dean@cornell.edu" }), { cookie }) + submitRequest(validPayload({ photos: [{ id: someoneElses, name: "a.png" }] })) ); expect(res.status).toBe(201); - expect((calls[0].properties as Record).author_email).toEqual({ - email: "ada@cornell.edu", + expect(await res.json()).toMatchObject({ photosSubmitted: 1, photosAttached: 0 }); + }); + + it("stays quiet when every photo attached, and when none was sent", async () => { + const first = await upload(); + const both = await post( + submitRequest(validPayload({ photos: [{ id: first, name: "cover.png" }] })) + ); + expect(await both.json()).toMatchObject({ photosSubmitted: 1, photosAttached: 1 }); + + const none = await post(submitRequest(validPayload({ photos: [] }))); + expect(await none.json()).toMatchObject({ photosSubmitted: 0, photosAttached: 0 }); + + // Nothing was lost in either case, so nothing is logged about it. + expect(warnings()).not.toHaveBeenCalled(); + }); +}); + +// ── Signing in is the gate (spec §5.5) ────────────────────────────── + +describe("POST /api/projects (sign-in required)", () => { + it("answers 401 to an anonymous submission and writes nothing", async () => { + const res = await post(submitRequest(validPayload(), { cookie: null })); + + expect(res.status).toBe(401); + // 401, not 403: signing in is something the visitor can actually do, and + // the browser tells them so. + expect(await res.json()).toMatchObject({ code: "sign_in_required" }); + expect(await storedProjects()).toHaveLength(0); + }); + + it("answers 401 to a forged cookie rather than trusting it", async () => { + const forged = defaultSession.cookie.replace(/.$/, (c) => (c === "A" ? "B" : "A")); + + const res = await post(submitRequest(validPayload(), { cookie: forged })); + + expect(res.status).toBe(401); + expect(await storedProjects()).toHaveLength(0); + }); + + it("answers 401 to a banned account — a ban resolves to anonymous", async () => { + const banned = await signInAsNew({ + email: "banned@cornell.edu", + name: "Ben Banned", + banned: true, }); - expect(JSON.stringify(calls[0])).not.toContain("dean@cornell.edu"); + + const res = await post(submitRequest(validPayload(), { cookie: banned.cookie })); + + expect(res.status).toBe(401); + expect(await storedProjects()).toHaveLength(0); }); - it("still refuses published:true from a signed-in client", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); - const cookie = await cookieFor("ada@cornell.edu", "Ada Lovelace"); + it("accepts a signed-in student — `projects.submit` is what signing in grants", async () => { + const res = await post(submitRequest(validPayload())); + expect(res.status).toBe(201); + }); + + it("accepts a SuperMaker and a director too", async () => { + for (const role of ["admin", "super_admin"] as const) { + const { cookie } = await signInAsNew({ + email: `${role}@cornell.edu`, + name: "Staff Person", + role, + }); + const res = await post(submitRequest(validPayload(), { cookie })); + expect(res.status).toBe(201); + } + }); +}); + +// ── Verified authorship (spec §4, §5) ─────────────────────────────── + +describe("POST /api/projects (author identity)", () => { + it("records the author id from the session", async () => { + const author = await signedIn("author@cornell.edu", "Ada Lovelace"); + + const res = await post(submitRequest(validPayload(), { cookie: author.cookie })); + + expect(res.status).toBe(201); + const [row] = await storedProjects(); + expect(row.authorUserId).toBe(author.user.id); + expect(row.createdBy).toBe(author.user.id); + // The byline comes from the session too — the form stopped offering the + // field, and this is what makes that guarantee real rather than cosmetic. + expect(row.authorName).toBe("Ada Lovelace"); + }); + + it("IGNORES an author supplied in the body", async () => { + const { cookie } = await signedIn("author@cornell.edu", "Ada Lovelace"); const res = await post( - submitRequest(validPayload({ published: true }), { cookie }) + submitRequest(validPayload({ author: "Somebody Else" }), { cookie }) ); expect(res.status).toBe(201); - // Signing in verifies who submitted; it does not publish anything. - expect(calls[0].properties?.published).toEqual({ checkbox: false }); - expect(JSON.stringify(calls[0])).not.toContain('"checkbox":true'); - }); - - it("records the submission without the email when Notion has no author_email property", async () => { - stubProjectsEnv(); - const cookie = await cookieFor("ada@cornell.edu", "Ada Lovelace"); - const bodies: NotionCreateBody[] = []; - server.use( - http.post(`${NOTION}/pages`, async ({ request }) => { - const body = (await request.json()) as NotionCreateBody; - bodies.push(body); - if (body.properties?.author_email) { - return HttpResponse.json( - { - object: "error", - status: 400, - code: "validation_error", - message: "author_email is not a property that exists", - }, - { status: 400 } - ); - } - return HttpResponse.json({ - object: "page", - id: "created-project-1", - created_time: "2024-09-01T10:00:00.000Z", - last_edited_time: "2024-09-01T10:00:00.000Z", - properties: body.properties ?? {}, - }); - }) + expect((await storedProjects())[0].authorName).toBe("Ada Lovelace"); + }); + + it("IGNORES author_email supplied in the body", async () => { + const res = await post( + submitRequest( + validPayload({ + author_email: "dean@cornell.edu", + authorEmail: "dean@cornell.edu", + }) + ) ); - const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); - const res = await post(submitRequest(validPayload(), { cookie })); + expect(res.status).toBe(201); + // A client may not assert who it is, and `projects` has no email column to + // put one in even if it could. + expect(JSON.stringify(await storedProjects())).not.toContain("dean@cornell.edu"); + }); + + it("still refuses published:true from a signed-in client", async () => { + const res = await post(submitRequest(validPayload({ published: true }))); - // A column a person has not added yet must not cost a student their - // write-up (Article 4 — fail toward stale, not toward wrong). expect(res.status).toBe(201); - expect(bodies).toHaveLength(2); - expect(bodies[1].properties).not.toHaveProperty("author_email"); - expect(bodies[1].properties?.published).toEqual({ checkbox: false }); - // And the misconfiguration is loud in the logs. - expect(warn).toHaveBeenCalled(); + // Signing in verifies who submitted; it does not publish anything. + expect((await storedProjects())[0].published).toBe(false); }); }); @@ -378,58 +465,44 @@ describe("POST /api/projects (author identity)", () => { describe("POST /api/projects (validation)", () => { it("rejects malformed JSON with 400", async () => { - stubProjectsEnv(); - captureNotionCreate(); - const res = await post(submitRequest(null, { raw: "{not json" })); expect(res.status).toBe(400); expect((await res.json()).error).toMatch(/invalid json/i); + expect(await storedProjects()).toHaveLength(0); }); it.each([ ["title", { title: "" }], - ["author", { author: " " }], ["body", { body: "" }], ])("rejects a submission missing %s with 400", async (_field, overrides) => { - stubProjectsEnv(); - const calls = captureNotionCreate(); - const res = await post(submitRequest(validPayload(overrides))); expect(res.status).toBe(400); expect((await res.json()).error).toMatch(/required/i); - expect(calls).toHaveLength(0); + expect(await storedProjects()).toHaveLength(0); }); it("rejects an oversized write-up with 400", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); - const res = await post( submitRequest(validPayload({ body: "x".repeat(20_001) })) ); expect(res.status).toBe(400); expect((await res.json()).error).toMatch(/too long/i); - expect(calls).toHaveLength(0); + expect(await storedProjects()).toHaveLength(0); }); it("accepts a write-up right at the size limit", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); - const res = await post( submitRequest(validPayload({ body: "x".repeat(20_000) })) ); expect(res.status).toBe(201); - expect(calls).toHaveLength(1); + expect(await storedProjects()).toHaveLength(1); }); it("rejects more than 8 photos with 400 rather than silently dropping them", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); const photos = Array.from({ length: 9 }, (_, i) => ({ id: `file-upload-${i}`, name: `photo-${i}.png`, @@ -439,55 +512,52 @@ describe("POST /api/projects (validation)", () => { expect(res.status).toBe(400); expect((await res.json()).error).toMatch(/8 photos/i); - expect(calls).toHaveLength(0); + expect(await storedProjects()).toHaveLength(0); }); it("accepts exactly 8 photos", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); - const photos = Array.from({ length: 8 }, (_, i) => ({ - id: `file-upload-${i}`, - name: `photo-${i}.png`, - })); + const ids = await Promise.all(Array.from({ length: 8 }, () => upload())); + const photos = ids.map((id, i) => ({ id, name: `photo-${i}.png` })); const res = await post(submitRequest(validPayload({ photos }))); expect(res.status).toBe(201); - expect(calls[0].properties?.photos).toHaveProperty("files"); - expect((calls[0].properties?.photos as { files: unknown[] }).files).toHaveLength(8); + const [row] = await storedProjects(); + const db = await getDb(); + const owned = await db + .select() + .from(attachments) + .where(eq(attachments.ownerId, row.id)); + expect(owned).toHaveLength(8); }); it("rejects more than 20 tools with 400", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); const tools = Array.from({ length: 21 }, (_, i) => `tool-${i}`); const res = await post(submitRequest(validPayload({ tools }))); expect(res.status).toBe(400); expect((await res.json()).error).toMatch(/20 tools/i); - expect(calls).toHaveLength(0); + expect(await storedProjects()).toHaveLength(0); }); it("accepts exactly 20 tools", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); const tools = Array.from({ length: 20 }, () => crypto.randomUUID()); const res = await post(submitRequest(validPayload({ tools }))); // The cap is what is under test: twenty is not one too many. None of these - // ids is a real tool, so nothing survives the Notion-page translation and - // the submission is filed without a relation rather than refused. + // ids names a real tool, so the submission is filed with no links rather + // than refused. expect(res.status).toBe(201); - expect(calls).toHaveLength(1); - expect(calls[0].properties?.tools_used).toBeUndefined(); + const [row] = await storedProjects(); + const db = await getDb(); + expect( + await db.select().from(projectTools).where(eq(projectTools.projectId, row.id)) + ).toEqual([]); }); it("ignores non-string entries in tools and materials", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); - const res = await post( submitRequest( validPayload({ tools: [FORM_4_ID, 7, null], materials: [{}, "Plywood"] }) @@ -495,12 +565,14 @@ describe("POST /api/projects (validation)", () => { ); expect(res.status).toBe(201); - expect(calls[0].properties?.tools_used).toEqual({ - relation: [{ id: DEMO_FORM_4_NOTION_PAGE_ID }], - }); - expect(calls[0].properties?.materials).toEqual({ - multi_select: [{ name: "Plywood" }], - }); + const [row] = await storedProjects(); + expect(row.materials).toEqual(["Plywood"]); + const db = await getDb(); + const links = await db + .select() + .from(projectTools) + .where(eq(projectTools.projectId, row.id)); + expect(links.map((link) => link.toolId)).toEqual([FORM_4_ID]); }); }); @@ -513,27 +585,21 @@ describe("POST /api/projects (link validation)", () => { "data:text/html;base64,PHNjcmlwdD5hbGVydCgxKTwvc2NyaXB0Pg==", "file:///etc/passwd", "not a url at all", - ])("rejects %s with 400 and never reaches Notion", async (link) => { - stubProjectsEnv(); - const calls = captureNotionCreate(); - + ])("rejects %s with 400 and writes nothing", async (link) => { const res = await post(submitRequest(validPayload({ link }))); expect(res.status).toBe(400); expect((await res.json()).error).toMatch(/http/i); - expect(calls).toHaveLength(0); + expect(await storedProjects()).toHaveLength(0); }); it.each(["https://example.com/lamp", "http://example.com/lamp"])( "accepts %s", async (link) => { - stubProjectsEnv(); - const calls = captureNotionCreate(); - const res = await post(submitRequest(validPayload({ link }))); expect(res.status).toBe(201); - expect(calls[0].properties?.link).toEqual({ url: link }); + expect((await storedProjects())[0].link).toBe(link); } ); }); @@ -542,8 +608,6 @@ describe("POST /api/projects (link validation)", () => { describe("POST /api/projects (rate limiting)", () => { it("returns 429 with Retry-After once the limiter says no", async () => { - stubProjectsEnv(); - const calls = captureNotionCreate(); vi.resetModules(); vi.doMock("@/lib/rate-limit", async () => { const actual = await vi.importActual( @@ -561,21 +625,17 @@ describe("POST /api/projects (rate limiting)", () => { expect(res.status).toBe(429); expect(res.headers.get("Retry-After")).toBe("60"); expect((await res.json()).error).toMatch(/too many requests/i); - // Rate limiting happens before the expensive outbound call (Article 4). - expect(calls).toHaveLength(0); + // Rate limiting happens before any database work (Article 4). + expect(await storedProjects()).toHaveLength(0); vi.doUnmock("@/lib/rate-limit"); vi.resetModules(); }); - it("starts refusing the same IP once its window is exhausted", async () => { - stubProjectsEnv(); - captureNotionCreate(); - const ip = uniqueIp(); - + it("starts refusing the same person once their window is exhausted", async () => { const statuses: number[] = []; for (let i = 0; i < 12; i += 1) { - const res = await post(submitRequest(validPayload(), { ip })); + const res = await post(submitRequest(validPayload())); statuses.push(res.status); } @@ -583,47 +643,101 @@ describe("POST /api/projects (rate limiting)", () => { expect(statuses).toContain(429); // Once refused, it stays refused for the rest of the window. expect(statuses.at(-1)).toBe(429); + expect(await storedProjects()).toHaveLength(10); }); - it("does not penalize a different IP", async () => { - stubProjectsEnv(); - captureNotionCreate(); - const noisy = uniqueIp(); + it("keys a signed-in caller by who they are, not where they are", async () => { + // Every request below comes from a different address. A signed-in caller + // is keyed on their user id, so moving networks does not buy a fresh + // allowance — and, the other way round, sharing a campus NAT does not + // spend somebody else's. + const statuses: number[] = []; for (let i = 0; i < 12; i += 1) { - await post(submitRequest(validPayload(), { ip: noisy })); + const res = await post(submitRequest(validPayload(), { ip: uniqueIp() })); + statuses.push(res.status); } - const res = await post(submitRequest(validPayload(), { ip: uniqueIp() })); + expect(statuses.at(-1)).toBe(429); + }); + + it("does not penalize a different person", async () => { + for (let i = 0; i < 12; i += 1) { + await post(submitRequest(validPayload())); + } + + const other = await signInAsNew({ email: "quiet@cornell.edu", name: "Quiet Person" }); + const res = await post(submitRequest(validPayload(), { cookie: other.cookie })); expect(res.status).toBe(201); }); + + it("bounds an anonymous caller by IP, before telling them to sign in", async () => { + const ip = uniqueIp(); + + const statuses: number[] = []; + for (let i = 0; i < 12; i += 1) { + const res = await post(submitRequest(validPayload(), { ip, cookie: null })); + statuses.push(res.status); + } + + // The ordering is the point (Article 4): the limiter runs before the + // sign-in check, so an unauthenticated flood is shed rather than answered. + expect(statuses[0]).toBe(401); + expect(statuses.at(-1)).toBe(429); + }); }); -// ── Notion failure ────────────────────────────────────────────────── - -describe("POST /api/projects (Notion failure)", () => { - it("returns 502 and does not leak the raw Notion error", async () => { - stubProjectsEnv(); - const notionMessage = - "validation_error: body.properties.tools_used[0].id is not a valid uuid (db-projects, token secret_test)"; - server.use( - http.post(`${NOTION}/pages`, () => - HttpResponse.json( - { object: "error", status: 400, code: "validation_error", message: notionMessage }, - { status: 400 } - ) - ) - ); +// ── Database failure ──────────────────────────────────────────────── + +describe("POST /api/projects (database failure)", () => { + it("returns 502 and does not leak the driver's error when the write fails", async () => { const error = vi.spyOn(console, "error").mockImplementation(() => {}); + vi.resetModules(); + vi.doMock("@/lib/data/projects", async () => { + const actual = + await vi.importActual("@/lib/data/projects"); + return { + ...actual, + createProjectSubmission: vi.fn(async () => { + throw new Error("connect ECONNREFUSED postgres://user:hunter2@127.0.0.1:1/none"); + }), + }; + }); + const { POST } = await import("./route"); - const res = await post(submitRequest(validPayload())); + const res = await POST(submitRequest(validPayload()) as never); expect(res.status).toBe(502); const raw = await res.text(); - expect(raw).not.toContain(notionMessage); - expect(raw).not.toContain("validation_error"); - expect(raw).not.toContain("secret_test"); + expect(raw).not.toMatch(/hunter2|postgres|ECONNREFUSED/i); expect(JSON.parse(raw).error).toMatch(/try again/i); // The detail is still logged server-side. expect(error).toHaveBeenCalled(); + + vi.doUnmock("@/lib/data/projects"); + vi.resetModules(); + }); + + it("degrades to 401 when the session store itself is unreachable", async () => { + // Documented consequence of Phase 4's ordering, not an accident: + // `resolveIdentity` treats an unreachable database as "nobody is signed + // in" so that public pages keep serving (Article 4), and this route checks + // sign-in before it writes. A signed-in student therefore sees "sign in to + // share a project" during an outage rather than "submission failed" — a + // worse sentence than it could be, but it still leaks nothing and still + // writes nothing. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + vi.stubEnv("DATABASE_URL", "postgres://user:hunter2@127.0.0.1:1/none"); + resetDbForTests(); + resetAuthForTests(); + + const res = await post(submitRequest(validPayload())); + + expect(res.status).toBe(401); + expect(await res.text()).not.toMatch(/hunter2|ECONNREFUSED/i); + + warn.mockRestore(); + vi.stubEnv("DATABASE_URL", ""); + resetDbForTests(); + resetAuthForTests(); }); }); diff --git a/v5/src/app/api/projects/route.ts b/v5/src/app/api/projects/route.ts index 1aefd9a..0ddf4bc 100644 --- a/v5/src/app/api/projects/route.ts +++ b/v5/src/app/api/projects/route.ts @@ -1,13 +1,30 @@ import { NextRequest } from "next/server"; -import { - createProject, - hasProjectsEnv, - type ProjectWriteFields, -} from "../../../lib/notion"; -import { notionPageIdsForTools } from "../../../lib/data/notion-ids"; -import type { ProjectRecord } from "../../../lib/types"; +import { createProjectSubmission } from "../../../lib/data/projects"; import { rateLimitAsync } from "../../../lib/rate-limit"; import { resolveIdentity } from "../../../lib/auth/identity"; +import { can } from "../../../lib/auth/permissions"; + +/** + * `POST /api/projects` — a student's project write-up. + * + * Since Phase 3 the submission is a `projects` row plus its `project_tools` + * links (data platform spec §3.10, §4.10), written by + * `src/lib/data/projects.ts` in one transaction. Nothing here decides anything + * about publication: `createProjectSubmission` takes no `published` argument, + * so there is no field a client could send that would put a write-up in the + * gallery (Article 5). + * + * **Phase 4 made sign-in a requirement here** (spec §5.5) — the one place in + * the app where it is. Browsing, searching, chatting and reporting a problem + * are all still anonymous; submitting is not, because a project carries a + * byline into a public gallery and "who wrote this" has to be something the + * server knows rather than something the request claimed. `projects.submit` is + * held by every signed-in role, so the gate is sign-in, not seniority. + * + * There is no "not configured" state any more. The database is always there — + * Neon in production, PGlite when `DATABASE_URL` is unset — so a submission + * either lands or reports that it did not. + */ // `runtime` cannot be set when nextConfig.cacheComponents is enabled. // Default Node.js runtime is used. @@ -19,12 +36,13 @@ const MAX_TOOLS = 20; const MAX_MATERIALS = 20; /** - * What a submission may send. There is deliberately no `author_email` here: - * the verified author comes from the session and nowhere else. + * What a submission may send. There is deliberately no `author` and no + * `author_email`: since Phase 4 the byline and the author id both come from the + * session, and a field the server ignores is a field somebody will eventually + * believe in. */ interface ProjectPayload { title?: unknown; - author?: unknown; body?: unknown; link?: unknown; tools?: unknown; @@ -32,11 +50,6 @@ interface ProjectPayload { photos?: unknown; } -interface PhotoUpload { - id: string; - name: string; -} - function asString(value: unknown): string { return typeof value === "string" ? value.trim() : ""; } @@ -50,16 +63,21 @@ function asStringArray(value: unknown, max: number): string[] { .slice(0, max); } -function asPhotoUploads(value: unknown): PhotoUpload[] { +/** + * The upload ids out of the `photos` array. The `name` each entry also carries + * is the client's own label for its preview; the filename staff see comes from + * the `attachments` row the upload wrote, not from the request body. + */ +function asPhotoIds(value: unknown): string[] { if (!Array.isArray(value)) return []; return value .filter( - (item): item is { id: string; name?: string } => + (item): item is { id: string } => typeof item === "object" && item !== null && typeof (item as { id?: unknown }).id === "string" ) - .map((item) => ({ id: item.id, name: asString(item.name) || "upload" })) + .map((item) => item.id) .slice(0, MAX_PHOTOS); } @@ -73,7 +91,7 @@ function isValidUrl(value: string): boolean { } export async function POST(req: NextRequest) { - // Rate limit before any expensive work (Notion page create). + // Rate limit before any expensive work (Article 4). const identity = await resolveIdentity(req); const { allowed } = await rateLimitAsync(`projects:${identity.rateLimitKey}`, { limit: 10, @@ -86,10 +104,20 @@ export async function POST(req: NextRequest) { ); } - if (!hasProjectsEnv()) { + // Told apart on purpose (spec §5.5). 401 means "sign in and this will work", + // which is true for everybody with an institutional address; 403 would mean + // "signing in will not help", which is only true for a banned account — and + // a banned account resolves to anonymous, so it lands on the 401 too. + if (identity.role === "anonymous") { + return Response.json( + { error: "Sign in to share a project.", code: "sign_in_required" }, + { status: 401 } + ); + } + if (!can(identity, "projects.submit")) { return Response.json( - { error: "Project submissions are not configured yet." }, - { status: 503 } + { error: "Your account cannot submit projects.", code: "forbidden" }, + { status: 403 } ); } @@ -101,20 +129,22 @@ export async function POST(req: NextRequest) { } const title = asString(payload.title); - // The session wins over whatever the client typed: when the server knows who - // is submitting, the byline is theirs. The form makes the field read-only for - // a signed-in student, and this is what makes that guarantee real rather than - // cosmetic. Anonymous submission keeps working — the typed name is used. - const author = asString(identity.name) || asString(payload.author); + // **The byline is the session's, full stop.** `payload.author` is no longer + // read at all: the form stopped offering the field (spec §5.5), and leaving + // the fallback in would mean a request that simply omitted its cookie could + // still choose its own byline. A signed-in account with no display name gets + // a null byline, which the gallery renders as "Anonymous" — an account we + // know but cannot name, which is the truth. + const author = asString(identity.name); const body = asString(payload.body); const link = asString(payload.link); const tools = asStringArray(payload.tools, MAX_TOOLS); const materials = asStringArray(payload.materials, MAX_MATERIALS); - const photoUploads = asPhotoUploads(payload.photos); + const photoIds = asPhotoIds(payload.photos); - if (!title || !author || !body) { + if (!title || !body) { return Response.json( - { error: "Title, author, and a write-up are required." }, + { error: "A title and a write-up are required." }, { status: 400 } ); } @@ -145,28 +175,52 @@ export async function POST(req: NextRequest) { ); } - // `tools` arrives as catalogue ids, which are Postgres uuids since the read - // path moved (spec §3.10), while `tools_used` is a Notion relation. Translate - // through `tools.notion_page_id` and drop what does not resolve: Notion - // rejects the whole page for one unknown relation id, and losing a student's - // whole write-up over a tool link is the wrong way to fail (Article 4). - const toolPageIds = await notionPageIdsForTools(tools); - try { - const record = await submitProject({ + const record = await createProjectSubmission({ title, - author, body, - // Server-resolved only (spec §4). `payload.author_email` is never read — - // a client may not assert who it is. Anonymous stays a first-class path: - // no session, no email, submission still succeeds. - author_email: identity.email || undefined, - link: link || undefined, - tools_used: toolPageIds, + authorName: author || null, + // Server-resolved only (spec §4). Nothing in the request body reaches + // either of these — a client may not assert who it is — and after the + // gate above `identity.userId` is always a real `user.id`, which the + // `created_by` foreign key now requires anyway. + authorUserId: identity.userId, + link: link || null, materials, - photo_uploads: photoUploads, + // Unknown ids are dropped inside the write rather than refused here: a + // stale catalogue id in a form that has been open a while must not cost + // a student their write-up (Article 4). + toolIds: tools, + photoAttachmentIds: photoIds, }); - return Response.json({ id: record.id }, { status: 201 }); + // Photos offered but not all of them claimed: say so rather than let the + // student believe the gallery will show a picture that is not there + // (Article 4). `createProjectSubmission` returns the count for exactly this + // reason, and the sibling write path — `report_issue` in + // `src/lib/capabilities/maintenance.ts` — already tells the student the same + // thing, so answering 201 and dropping the count on the floor here was the + // odd one out. The usual cause is time: an upload nobody claims is deleted + // by the nightly cron after 24 hours, so a form left open overnight submits + // ids that no `attachments` row answers to any more. + const photosLost = photoIds.length - record.photosAttached; + if (photosLost > 0) { + console.warn( + `[projects] submission ${record.id} saved without ${photosLost} of its ${photoIds.length} photo(s) — no unclaimed attachment matched the ids supplied` + ); + } + // The id is what the form has always been handed back; the slug rides + // along for the admin page that will publish it. The two counts are the + // form's evidence — it renders the "saved without your photos" line off + // the server's answer rather than assuming its own uploads stuck. + return Response.json( + { + id: record.id, + slug: record.slug, + photosSubmitted: photoIds.length, + photosAttached: record.photosAttached, + }, + { status: 201 } + ); } catch (err) { console.error("Project submission failed", err); return Response.json( @@ -175,42 +229,3 @@ export async function POST(req: NextRequest) { ); } } - -/** - * Create the project, retrying once without `author_email` if Notion refuses - * that property. - * - * `author_email` is a new Email column a person has to add to the Projects - * database by hand (spec §4) — Notion has no migrations and rejects any write - * naming a property that does not exist. Losing a student's write-up to a - * missing column is the wrong way to fail: record the project, drop the email, - * and make the misconfiguration loud in the logs (Article 4 — fail toward - * stale, not toward wrong). Mirrors `report_issue`'s `reporter_email` fallback. - */ -async function submitProject(fields: ProjectWriteFields): Promise { - try { - return await createProject(fields); - } catch (err) { - if (!fields.author_email || !isUnknownPropertyError(err, "author_email")) { - throw err; - } - console.warn( - "[projects] Notion rejected `author_email` — recording the submission without it. Add the Email property to the Projects database (projects spec §4).", - err - ); - const withoutEmail = { ...fields }; - delete withoutEmail.author_email; - return createProject(withoutEmail); - } -} - -/** - * Does this look like Notion refusing an unknown property? `projectsRequest` - * throws `Notion API : `, and a schema mismatch is a 400 whose - * body names the offending property. Narrow on both so a 401 or a network blip - * still surfaces as the failure it is. - */ -function isUnknownPropertyError(err: unknown, property: string): boolean { - const message = err instanceof Error ? err.message : String(err); - return message.includes("400") && message.includes(property); -} diff --git a/v5/src/app/api/upload-notion/route.ts b/v5/src/app/api/upload-notion/route.ts index 68fc9bf..795a880 100644 --- a/v5/src/app/api/upload-notion/route.ts +++ b/v5/src/app/api/upload-notion/route.ts @@ -1,3 +1,13 @@ +/** + * **SUPERSEDED — retired in Phase 3, kept on disk pending deletion approval.** + * + * `POST /api/uploads` replaces this route (data platform design spec §3.3). + * Uploads go to Vercel Blob and are recorded in `attachments`; nothing in the + * app calls this handler any more. It is left in place only because deletions + * are approved separately — do not wire anything new to it, and do not treat + * its `file_upload_id` response as a live contract. + */ + import { NextRequest } from "next/server"; import { rateLimitAsync } from "../../../lib/rate-limit"; import { resolveIdentity } from "../../../lib/auth/identity"; diff --git a/v5/src/app/api/uploads/route.test.ts b/v5/src/app/api/uploads/route.test.ts new file mode 100644 index 0000000..4008c1f --- /dev/null +++ b/v5/src/app/api/uploads/route.test.ts @@ -0,0 +1,409 @@ +// @vitest-environment node + +// Vercel Blob is the one service here MSW cannot stand in for (the SDK talks to +// a signed API and would need a real token), so it is mocked at the seam +// `blob.ts` exists to provide — the same pattern the backup route test uses. +// `vi.hoisted` because the `vi.mock` factory runs before module scope exists. +const blob = vi.hoisted(() => ({ + configured: { value: true }, + putUpload: vi.fn(), + put: vi.fn(), + list: vi.fn(), + del: vi.fn(), +})); + +vi.mock("../../../lib/blob", () => ({ + isBlobConfigured: () => blob.configured.value, + getBlobStore: () => ({ + put: blob.put, + putUpload: blob.putUpload, + list: blob.list, + del: blob.del, + }), +})); + +import { resetAuthForTests } from "@/lib/auth/config"; +import { getDb, resetDbForTests } from "@/lib/db/client"; +import { attachments } from "@/lib/db/schema/index"; +import { signInAsNew } from "../../../../test/utils/session"; +import { POST } from "./route"; + +/** + * `POST /api/uploads` against the demo-seeded PGlite database with the Blob + * seam stubbed. No environment variable is set beyond the token flag and a + * test-only `AUTH_SECRET`, and no request leaves the process (Article 3). + */ + +const AUTH_SECRET = "uploads-route-test-secret"; + +const STORED = { + pathname: "uploads/project/lamp-Xa9k2.png", + url: "https://store.public.blob.vercel-storage.com/uploads/project/lamp-Xa9k2.png", +}; + +// A public `kind` needs a permission now, so the default caller for those tests +// is a signed-in student. Fresh per test: the limiter keys a signed-in caller +// by user id, so one test's fifteen uploads must not be another's — the same +// isolation `uniqueIp()` gives the anonymous ones. +let student: Awaited>; +let studentCounter = 0; + +beforeEach(async () => { + vi.stubEnv("DATABASE_URL", ""); + vi.stubEnv("AUTH_SECRET", AUTH_SECRET); + vi.stubEnv("BLOB_READ_WRITE_TOKEN", "vercel_blob_rw_test"); + resetAuthForTests(); + blob.configured.value = true; + blob.putUpload.mockReset().mockResolvedValue(STORED); + blob.del.mockReset().mockResolvedValue(undefined); + + const db = await getDb(); + await db.delete(attachments); + + studentCounter += 1; + student = await signInAsNew({ email: `ada-${studentCounter}@cornell.edu` }); +}); + +afterEach(() => { + resetAuthForTests(); +}); + +afterAll(() => { + resetDbForTests(); +}); + +// The in-memory limiter is a per-process singleton keyed by IP, so each test +// gets its own IP rather than inheriting a spent window. +let ipCounter = 0; +function uniqueIp() { + ipCounter += 1; + return `192.0.2.${ipCounter}`; +} + +function imageFile(bytes = 3, name = "lamp.png", type = "image/png") { + return new File([new Uint8Array(bytes)], name, { type }); +} + +interface UploadOptions { + kind?: string; + ip?: string; + /** A different session, or `null` to upload as an anonymous visitor. */ + cookie?: string | null; +} + +function uploadRequest(file: File | null, options: UploadOptions = {}) { + const form = new FormData(); + if (file) form.append("file", file); + if (options.kind) form.append("kind", options.kind); + // Signed in as the default student unless the test says otherwise: a public + // `kind` requires a permission, and most of these tests use one. + const cookie = "cookie" in options ? options.cookie : student.cookie; + const headers: Record = { + "x-forwarded-for": options.ip ?? uniqueIp(), + }; + if (cookie) headers.cookie = cookie; + return new Request("http://localhost/api/uploads", { + method: "POST", + headers, + body: form, + }) as never; +} + +async function rows() { + const db = await getDb(); + return db.select().from(attachments); +} + +/** A signed-in SuperMaker — the role that holds `tools.add` and `tools.edit`. */ +async function asSuperMaker() { + studentCounter += 1; + return signInAsNew({ email: `maker-${studentCounter}@cornell.edu`, role: "admin" }); +} + +describe("POST /api/uploads — degrading honestly", () => { + it("refuses with a machine-readable code when BLOB_READ_WRITE_TOKEN is unset", async () => { + blob.configured.value = false; + + const res = await POST(uploadRequest(imageFile())); + + expect(res.status).toBe(503); + // The client translates the code; the prose is for a log, not a student. + expect((await res.json()).code).toBe("blob_not_configured"); + }); + + it("never invents an attachment or touches the store when it cannot save", async () => { + blob.configured.value = false; + + const res = await POST(uploadRequest(imageFile())); + const body = await res.json(); + + // The failure mode this route exists to avoid: an id handed back for a file + // that was never stored, which a later claim would silently drop. + expect(body.attachmentId).toBeUndefined(); + expect(blob.putUpload).not.toHaveBeenCalled(); + expect(await rows()).toHaveLength(0); + }); +}); + +describe("POST /api/uploads — validation", () => { + it("rejects a request with no file", async () => { + const res = await POST(uploadRequest(null)); + expect(res.status).toBe(400); + expect((await res.json()).error).toBe("Missing file"); + }); + + it("rejects an empty file", async () => { + const res = await POST(uploadRequest(imageFile(0))); + expect(res.status).toBe(400); + expect((await res.json()).error).toBe("Empty file"); + }); + + it("rejects a non-image", async () => { + const res = await POST( + uploadRequest(imageFile(3, "notes.txt", "text/plain")) + ); + expect(res.status).toBe(400); + expect(blob.putUpload).not.toHaveBeenCalled(); + }); + + it("rejects an image over 18 MB", async () => { + const res = await POST(uploadRequest(imageFile(19 * 1024 * 1024))); + expect(res.status).toBe(400); + expect((await res.json()).error).toContain("18MB"); + }); + + it("accepts a PDF only for a resource upload", async () => { + const pdf = () => imageFile(3, "manual.pdf", "application/pdf"); + const maker = await asSuperMaker(); + + const rejected = await POST(uploadRequest(pdf(), { kind: "project" })); + expect(rejected.status).toBe(400); + + const accepted = await POST( + uploadRequest(pdf(), { kind: "resource", cookie: maker.cookie }) + ); + expect(accepted.status).toBe(200); + }); + + it("rejects a resource PDF over 20 MB", async () => { + const maker = await asSuperMaker(); + const res = await POST( + uploadRequest( + imageFile(21 * 1024 * 1024, "manual.pdf", "application/pdf"), + { kind: "resource", cookie: maker.cookie } + ) + ); + expect(res.status).toBe(400); + expect((await res.json()).error).toContain("20MB"); + }); +}); + +describe("POST /api/uploads — the happy path", () => { + it("stores the file and records an UNOWNED attachments row", async () => { + const res = await POST(uploadRequest(imageFile(), { kind: "project" })); + const body = await res.json(); + + expect(res.status).toBe(200); + expect(body.attachmentId).toMatch( + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i + ); + + const [row] = await rows(); + // Unowned on purpose: the project this photo belongs to does not exist yet. + expect(row).toMatchObject({ + id: body.attachmentId, + ownerType: null, + ownerId: null, + blobPathname: STORED.pathname, + access: "public", + contentType: "image/png", + originalFilename: "lamp.png", + }); + }); + + it("records the pathname the store chose, not the one it was asked for", async () => { + await POST(uploadRequest(imageFile(), { kind: "project" })); + + const [prefix] = blob.putUpload.mock.calls[0]; + expect(prefix).toBe("uploads/project/"); + const [row] = await rows(); + expect(row.blobPathname).toBe(STORED.pathname); + }); + + it("returns a preview URL for a public upload", async () => { + const res = await POST(uploadRequest(imageFile(), { kind: "project" })); + expect((await res.json()).previewUrl).toBe(STORED.url); + }); + + it("stores a maintenance photo PRIVATELY and returns no preview URL", async () => { + const res = await POST(uploadRequest(imageFile(), { kind: "maintenance" })); + + // §3.3: maintenance photos may show people. A private blob has no URL an + // unauthenticated viewer can follow, so the client keeps showing its own + // object-URL preview instead. + expect(blob.putUpload.mock.calls[0][2]).toBe("private"); + expect((await res.json()).previewUrl).toBeNull(); + expect((await rows())[0]).toMatchObject({ access: "private", publicUrl: null }); + }); + + it("treats an unrecognised kind as a chat upload rather than failing", async () => { + const res = await POST(uploadRequest(imageFile(), { kind: "nonsense" })); + + expect(res.status).toBe(200); + expect(blob.putUpload.mock.calls[0][0]).toBe("uploads/chat/"); + }); + + it("uploads anonymously — no sign-in is required to report a broken machine", async () => { + await POST(uploadRequest(imageFile(), { kind: "maintenance", cookie: null })); + expect((await rows())[0].uploadedBy).toBeNull(); + }); + + it("records the uploader when there is one", async () => { + await POST(uploadRequest(imageFile(), { kind: "project" })); + expect((await rows())[0].uploadedBy).toBe(student.user.id); + }); +}); + +describe("POST /api/uploads — who may ask for a public URL", () => { + /** + * The regression this guards: `kind` came straight off the request body and + * was the only thing deciding public vs private, so + * `curl -F kind=resource -F file=@anything.pdf .../api/uploads` with no + * cookie got back a permanent, world-readable Blob URL for a 20 MB PDF the + * lab never agreed to host — claimed by nothing, and swept only if still + * unclaimed 24 hours later. Every surface that *consumes* a public kind + * already requires sign-in, so no legitimate flow ever needed this. + */ + const publicKinds = ["project", "tool", "resource"] as const; + + it.each(publicKinds)("refuses an anonymous caller asking for kind=%s", async (kind) => { + const res = await POST(uploadRequest(imageFile(), { kind, cookie: null })); + + expect(res.status).toBe(401); + expect((await res.json()).code).toBe("sign_in_required"); + // Nothing was written: no blob, no row, no id handed back. + expect(blob.putUpload).not.toHaveBeenCalled(); + expect(await rows()).toHaveLength(0); + }); + + it("refuses a signed-in student asking for a catalogue image", async () => { + // 403, not 401: signing in is what they already did, and it did not help. + const res = await POST(uploadRequest(imageFile(), { kind: "tool" })); + + expect(res.status).toBe(403); + expect((await res.json()).code).toBe("forbidden"); + expect(blob.putUpload).not.toHaveBeenCalled(); + }); + + it("lets a SuperMaker upload a catalogue image and a manual", async () => { + const maker = await asSuperMaker(); + + for (const kind of ["tool", "resource"] as const) { + const res = await POST( + uploadRequest(imageFile(), { kind, cookie: maker.cookie }) + ); + expect(res.status, kind).toBe(200); + } + }); + + it("still lets an anonymous student send a chat or maintenance photo", async () => { + for (const kind of ["chat", "maintenance"] as const) { + const res = await POST( + uploadRequest(imageFile(), { kind, cookie: null }) + ); + expect(res.status, kind).toBe(200); + // Private, so there is no public URL to hand out in the first place — + // which is what makes leaving these two open safe. + expect((await res.json()).previewUrl).toBeNull(); + } + }); + + it("refuses on the permission, not on the size", async () => { + const res = await POST( + uploadRequest(imageFile(19 * 1024 * 1024), { kind: "resource", cookie: null }) + ); + // 401 rather than the 400 the oversize check would give: the caller is told + // they may not ask at all, which is the more useful of the two answers. + // + // It is not free, though. `req.formData()` has already parsed the body by + // the time the permission is checked, so an anonymous caller can still make + // the server read 19 MB; only the 15/min-per-IP limiter bounds that. Nothing + // is stored and no URL comes back, which is what the check is for — moving + // it ahead of the parse would be a separate change. + expect(res.status).toBe(401); + }); + + it("does not let an unrecognised kind smuggle in a public upload", async () => { + // `nonsense` falls back to `chat`, which is private and open — so the + // fallback cannot be used to reach a public URL without a permission. + const res = await POST( + uploadRequest(imageFile(), { kind: "nonsense", cookie: null }) + ); + + expect(res.status).toBe(200); + expect(blob.putUpload.mock.calls[0][2]).toBe("private"); + expect((await res.json()).previewUrl).toBeNull(); + }); +}); + +describe("POST /api/uploads — failures leave nothing behind", () => { + it("answers 502 when the blob write fails, and records nothing", async () => { + blob.putUpload.mockRejectedValueOnce(new Error("network down")); + + const res = await POST(uploadRequest(imageFile())); + + expect(res.status).toBe(502); + expect(await rows()).toHaveLength(0); + }); + + it("deletes the stored blob when the attachments insert fails", async () => { + // Staged by pointing the row's `access` at a value the CHECK constraint + // rejects, so the insert fails the way a real constraint violation would. + blob.putUpload.mockResolvedValueOnce({ + pathname: "uploads/project/x.png", + url: "x", + }); + const db = await getDb(); + const insert = vi + .spyOn(db, "insert") + .mockImplementation(() => { + throw new Error("insert failed"); + }); + + try { + const res = await POST(uploadRequest(imageFile(), { kind: "project" })); + + expect(res.status).toBe(502); + // Otherwise the bytes sit in the store forever: the cron only sweeps + // files that have a row to find them by. + expect(blob.del).toHaveBeenCalledWith(["uploads/project/x.png"]); + } finally { + insert.mockRestore(); + } + }); +}); + +describe("POST /api/uploads — rate limiting", () => { + it("answers 429 after 15 uploads in a minute from one caller", async () => { + const ip = uniqueIp(); + for (let i = 0; i < 15; i += 1) { + const ok = await POST(uploadRequest(imageFile(), { kind: "project", ip })); + expect(ok.status).toBe(200); + } + + const res = await POST(uploadRequest(imageFile(), { kind: "project", ip })); + expect(res.status).toBe(429); + expect(res.headers.get("Retry-After")).toBe("60"); + }); + + it("refuses before reading the body, so a flood costs no bytes", async () => { + const ip = uniqueIp(); + for (let i = 0; i < 15; i += 1) { + await POST(uploadRequest(imageFile(), { kind: "project", ip })); + } + blob.putUpload.mockClear(); + + await POST(uploadRequest(imageFile(), { kind: "project", ip })); + expect(blob.putUpload).not.toHaveBeenCalled(); + }); +}); diff --git a/v5/src/app/api/uploads/route.ts b/v5/src/app/api/uploads/route.ts new file mode 100644 index 0000000..c2cdbb8 --- /dev/null +++ b/v5/src/app/api/uploads/route.ts @@ -0,0 +1,248 @@ +import { NextRequest } from "next/server"; +import { getBlobStore, isBlobConfigured, type BlobAccess } from "../../../lib/blob"; +import { createAttachment } from "../../../lib/data/attachments"; +import { rateLimitAsync } from "../../../lib/rate-limit"; +import { resolveIdentity, type Identity } from "../../../lib/auth/identity"; +import { can, type Permission } from "../../../lib/auth/permissions"; + +/** + * `POST /api/uploads` — the one upload route (data platform design spec §3.3, + * §4.7). It replaces `/api/upload-notion`, which pushed bytes into a Notion + * `file_upload` session and handed back a Notion handle. + * + * What a caller gets back is now an `attachmentId`: a row in `attachments` + * owned by nothing yet. The write that follows — a ticket, a project + * submission — *claims* those ids onto itself, and the daily cron deletes + * anything still unclaimed after 24 hours. That two-step exists because at + * upload time the record the photo belongs to has not been written: the student + * is still typing it. + * + * **Access is decided here, not by the client.** A maintenance photo may show a + * person and is written privately, so it comes back with `previewUrl: null` and + * the client shows the local `URL.createObjectURL` preview it already holds. A + * project or tool photo is about to appear on a public page, so it is public + * and its URL comes back. + * + * **And so is who may ask for which.** The client still picks `kind`, so `kind` + * alone cannot be the whole of the decision: see {@link KIND_POLICY}. + * + * **With no `BLOB_READ_WRITE_TOKEN` this route refuses and says so** (Article + * 4). It does not invent an id, and it does not pretend the file was stored: + * a student who is told their photo is attached, and whose photo is not, is + * worse off than one who is told photos are unavailable today. + */ + +// `runtime` cannot be set when nextConfig.cacheComponents is enabled. +// Default Node.js runtime is used. +export const maxDuration = 30; + +/** + * Kept identical to `/api/upload-notion`'s: the same anonymous students file + * the same maintenance photos through it, and tightening the limit as a side + * effect of changing the storage backend would be a behaviour change nobody + * asked for. + */ +const RATE_LIMIT = { limit: 15, windowMs: 60_000 }; + +/** What the upload is for. Decides both the access and where it is filed. */ +const KINDS = ["chat", "maintenance", "project", "tool", "resource"] as const; +type UploadKind = (typeof KINDS)[number]; +const DEFAULT_KIND: UploadKind = "chat"; + +const MAX_IMAGE_BYTES = 18 * 1024 * 1024; +const MAX_PDF_BYTES = 20 * 1024 * 1024; + +/** + * What each kind costs the caller: the blob's access, and the permission the + * surface that consumes it requires. + * + * **A public `kind` is a request for a permanent, world-readable URL**, and the + * client chooses `kind`. So a route that read `kind` and nothing else handed an + * unauthenticated caller a place to host arbitrary images — and, with + * `kind=resource`, 20 MB PDFs — on the lab's Blob account, claimed by nothing + * and swept only if still unclaimed 24 hours later. The route's own comment + * said access was decided here; it was in fact decided by the request body. + * + * The rule now is that the permission matches the surface the file is destined + * for, and it is the permission that surface already enforces: + * + * - **`project`** — `projects.submit`. `POST /api/projects` has required sign-in + * since Phase 4 (spec §5.5), so an anonymous project photo belongs to no + * submission anybody can make. + * - **`tool`** / **`resource`** — `tools.add` and `tools.edit`. Catalogue images + * and manuals come from intake and the admin surfaces, both of which are + * already gated on exactly these. + * - **`chat`** and **`maintenance`** stay open, because they are the two the lab + * deliberately lets an anonymous student use — photograph a machine to ask + * what it is, photograph a broken one to report it (§3.3). Both are written + * **private**: no public URL exists to hand back, and the row is the only way + * to reach the bytes. That is what makes leaving them open safe, and it is why + * the two lists are the same list. + * + * Maintenance photos are the private case §3.3 names — they may show people and + * are admin-only to read. Chat photos follow them for the same reason. + */ +const KIND_POLICY: Record< + UploadKind, + { access: BlobAccess; permission: Permission | null } +> = { + chat: { access: "private", permission: null }, + maintenance: { access: "private", permission: null }, + project: { access: "public", permission: "projects.submit" }, + tool: { access: "public", permission: "tools.add" }, + resource: { access: "public", permission: "tools.edit" }, +}; + +function isKind(value: string): value is UploadKind { + return (KINDS as readonly string[]).includes(value); +} + +/** + * Why this caller may not upload this kind, or null when they may. + * + * 401 and 403 are told apart the way `POST /api/projects` tells them apart: + * 401 means "sign in and this will work", which is true of every institutional + * address for a project photo; 403 means signing in will not help, which is the + * honest answer for a student asking to write a catalogue image. + */ +function uploadRefusal( + identity: Identity, + kind: UploadKind +): { status: number; code: string; error: string } | null { + const { permission } = KIND_POLICY[kind]; + if (!permission) return null; + + if (identity.role === "anonymous") { + return { + status: 401, + code: "sign_in_required", + error: "Sign in to upload this kind of file.", + }; + } + if (!can(identity, permission)) { + return { + status: 403, + code: "forbidden", + error: "Your account cannot upload this kind of file.", + }; + } + return null; +} + +export async function POST(req: NextRequest) { + // Rate limit before reading a multipart body or touching Blob (Article 4). + const identity = await resolveIdentity(req); + const { allowed } = await rateLimitAsync(`upload:${identity.rateLimitKey}`, RATE_LIMIT); + if (!allowed) { + return Response.json( + { error: "Too many requests. Please slow down." }, + { status: 429, headers: { "Retry-After": "60" } } + ); + } + + // Checked before the body is read: with no store there is nowhere for the + // bytes to go, and reading 18 MB to then refuse is work for nothing. + if (!isBlobConfigured()) { + return Response.json( + { + code: "blob_not_configured", + error: "File uploads are unavailable: BLOB_READ_WRITE_TOKEN is not set.", + }, + { status: 503 } + ); + } + + let form: FormData; + try { + form = await req.formData(); + } catch { + return Response.json({ error: "Invalid form data" }, { status: 400 }); + } + + const rawKind = String(form.get("kind") ?? ""); + const kind: UploadKind = isKind(rawKind) ? rawKind : DEFAULT_KIND; + + // Before the file is looked at, let alone written: a caller who may not ask + // for this kind learns so without the lab storing anything on their behalf. + const refusal = uploadRefusal(identity, kind); + if (refusal) { + const { status, ...body } = refusal; + return Response.json(body, { status }); + } + + const file = form.get("file"); + if (!(file instanceof File)) { + return Response.json({ error: "Missing file" }, { status: 400 }); + } + if (file.size === 0) { + return Response.json({ error: "Empty file" }, { status: 400 }); + } + + const type = file.type || ""; + const isImage = type.startsWith("image/"); + // PDFs are for resources only — a manual. Accepting one on a chat or + // maintenance upload would put an arbitrary document behind a public URL for + // no feature that asks for it (§3.3). + const isResourcePdf = type === "application/pdf" && kind === "resource"; + + if (!isImage && !isResourcePdf) { + return Response.json( + { error: "Only image uploads are supported" }, + { status: 400 } + ); + } + + const maxBytes = isImage ? MAX_IMAGE_BYTES : MAX_PDF_BYTES; + if (file.size > maxBytes) { + return Response.json( + { error: `File too large (max ${Math.round(maxBytes / (1024 * 1024))}MB)` }, + { status: 400 } + ); + } + + const access = KIND_POLICY[kind].access; + const store = getBlobStore(); + + let stored: { pathname: string; url: string }; + try { + stored = await store.putUpload(`uploads/${kind}/`, file, access); + } catch (err) { + console.error("[uploads] blob write failed", err); + return Response.json({ error: "Upload failed" }, { status: 502 }); + } + + let attachmentId: string; + try { + const created = await createAttachment({ + blobPathname: stored.pathname, + access, + // A private blob has no URL an unauthenticated viewer can follow, so + // recording one would be a lie the catalogue would later render. + publicUrl: access === "public" ? stored.url : null, + contentType: type, + sizeBytes: file.size, + originalFilename: file.name || "upload", + uploadedBy: identity.userId, + }); + attachmentId = created.id; + } catch (err) { + console.error("[uploads] attachment insert failed", err); + // The bytes landed but nothing points at them, so they would sit in the + // store forever — the cron only sweeps files that *have* a row. Remove + // them here so a failed upload leaves nothing behind. + try { + await store.del([stored.pathname]); + } catch (cleanupErr) { + console.error("[uploads] orphaned blob cleanup failed", cleanupErr); + } + return Response.json({ error: "Upload failed" }, { status: 502 }); + } + + return Response.json({ + attachmentId, + previewUrl: access === "public" ? stored.url : null, + name: file.name || "upload", + contentType: type, + size: file.size, + }); +} diff --git a/v5/src/components/AdminLink.test.tsx b/v5/src/components/AdminLink.test.tsx new file mode 100644 index 0000000..143212a --- /dev/null +++ b/v5/src/components/AdminLink.test.tsx @@ -0,0 +1,53 @@ +import { ADMIN_HREF, AdminLink } from "./AdminLink"; +import { render, screen } from "../../test/utils/render"; + +vi.mock("next/link", () => ({ + __esModule: true, + default: ({ + href, + children, + ...rest + }: { + href: string; + children: React.ReactNode; + }) => ( + + {children} + + ), +})); + +/** + * Who sees the way into `/admin`. The refusal behind it is the `/admin` layout + * (`canReachAdmin` again, server-side) — hiding this link is presentation. + */ + +// en.json: nav.admin = "ADMIN". +describe("AdminLink — who sees it", () => { + it("renders nothing while identity is still resolving", () => { + const { container } = render(); + expect(container).toBeEmptyDOMElement(); + }); + + it("renders nothing for an anonymous visitor", () => { + const { container } = render(); + expect(container).toBeEmptyDOMElement(); + }); + + it("renders nothing for an ordinary signed-in user", () => { + // Signing in unlocks submitting a project and nothing else — there is no + // admin surface a student holds a permission for. + const { container } = render(); + expect(container).toBeEmptyDOMElement(); + }); + + it("renders for a SuperMaker, whose queues live behind the same link", () => { + render(); + expect(screen.getByRole("link", { name: "ADMIN" })).toHaveAttribute("href", ADMIN_HREF); + }); + + it("renders for a director", () => { + render(); + expect(screen.getByRole("link", { name: "ADMIN" })).toBeInTheDocument(); + }); +}); diff --git a/v5/src/components/AdminLink.tsx b/v5/src/components/AdminLink.tsx new file mode 100644 index 0000000..1cf40cd --- /dev/null +++ b/v5/src/components/AdminLink.tsx @@ -0,0 +1,36 @@ +"use client"; + +import Link from "next/link"; +import { useTranslations } from "next-intl"; +import { canReachAdmin } from "../lib/auth/permissions"; +import type { Role } from "../lib/auth/roles"; + +/** + * The way into `/admin`, in the header (spec §6). + * + * Shown to anyone holding *any* admin-surface permission, not only a super + * admin: a SuperMaker's queues arrive in later phases and land behind this same + * link, and one entry point that grows is better than a link that appears when + * Phase 5 merges. + * + * **Hiding is presentation.** `/admin`'s layout resolves the identity itself + * and refuses anyone without one of these permissions; rendering `null` here + * only spares everyone else a link into a refusal. Same contract as + * `RefreshCatalogButton`, and the same reason `role` may be `undefined` — the + * header asks `/api/identity` after mount, and until it answers there is + * nothing to show. + */ + +export const ADMIN_HREF = "/admin"; + +export function AdminLink({ role }: { role: Role | undefined }) { + const t = useTranslations("nav"); + + if (!canReachAdmin({ role })) return null; + + return ( + + {t("admin")} + + ); +} diff --git a/v5/src/components/ChatFab.test.tsx b/v5/src/components/ChatFab.test.tsx index 9fb7d99..0bff1c1 100644 --- a/v5/src/components/ChatFab.test.tsx +++ b/v5/src/components/ChatFab.test.tsx @@ -487,9 +487,9 @@ describe("ChatFab — pending tool-call status", () => { // ── Photo upload + attachment hint ───────────────────────────────── // -// Selecting an image uploads it to /api/upload-notion, shows a removable -// preview, and on submit appends a parseable [Attached photos: …] hint that -// the chat route turns into report_issue photo_uploads. +// Selecting an image uploads it to /api/uploads, shows a removable preview, +// and on submit appends a parseable [Attached photos: …] hint that the chat +// route turns into report_issue photo_attachment_ids. describe("ChatFab — photo uploads", () => { let origCreate: typeof URL.createObjectURL; let origRevoke: typeof URL.revokeObjectURL; @@ -507,12 +507,16 @@ describe("ChatFab — photo uploads", () => { vi.unstubAllGlobals(); }); - it("uploads an image and includes its file_upload hint in the sent message", async () => { + it("uploads an image and includes its attachment hint in the sent message", async () => { const user = userEvent.setup(); const fetchMock = vi.fn( async () => new Response( - JSON.stringify({ file_upload_id: "fu_123", name: "broken.png" }), + JSON.stringify({ + attachmentId: "3f2504e0-4f89-41d3-9a0c-0305e82c3301", + previewUrl: null, + name: "broken.png", + }), { status: 200, headers: { "content-type": "application/json" } } ) ); @@ -536,7 +540,7 @@ describe("ChatFab — photo uploads", () => { await screen.findByRole("button", { name: "Remove broken.png" }) ).toBeInTheDocument(); expect(fetchMock).toHaveBeenCalledWith( - "/api/upload-notion", + "/api/uploads", expect.objectContaining({ method: "POST" }) ); @@ -548,7 +552,7 @@ describe("ChatFab — photo uploads", () => { const arg = sendMessage.mock.calls[0][0] as { text: string }; expect(arg.text).toContain("the printer is broken"); expect(arg.text).toContain( - "[Attached photos: file_upload_id=fu_123 name=broken.png]" + "[Attached photos: attachment_id=3f2504e0-4f89-41d3-9a0c-0305e82c3301 name=broken.png]" ); }); @@ -583,6 +587,41 @@ describe("ChatFab — photo uploads", () => { ).not.toBeInTheDocument(); }); + it("says photo uploads are unavailable when no blob store is configured", async () => { + const user = userEvent.setup(); + vi.stubGlobal( + "fetch", + vi.fn( + async () => + new Response(JSON.stringify({ code: "blob_not_configured" }), { + status: 503, + headers: { "content-type": "application/json" }, + }) + ) + ); + render(); + + await user.click( + screen.getByRole("button", { name: "Open MakerLab assistant" }) + ); + await user.upload( + document.querySelector('input[type="file"]') as HTMLInputElement, + new File([new Uint8Array([1])], "bed.png", { type: "image/png" }) + ); + + // Translated, not the route's English prose — a student never sees an + // English-only string (Article 6). + expect( + await screen.findByText( + "Photo uploads are unavailable right now. You can still send your message without a photo." + ) + ).toBeInTheDocument(); + // And the conversation is still usable without one. + expect( + screen.getByRole("textbox", { name: "Ask the lab console" }) + ).toBeEnabled(); + }); + async function attachPhotoAndSend( user: ReturnType, message: string @@ -592,7 +631,11 @@ describe("ChatFab — photo uploads", () => { vi.fn( async () => new Response( - JSON.stringify({ file_upload_id: "fu_123", name: "plate.jpg" }), + JSON.stringify({ + attachmentId: "3f2504e0-4f89-41d3-9a0c-0305e82c3302", + previewUrl: null, + name: "plate.jpg", + }), { status: 200, headers: { "content-type": "application/json" } } ) ) @@ -617,7 +660,7 @@ describe("ChatFab — photo uploads", () => { } it("sends the photo itself with the message, so the model can see it", async () => { - // Intake spec §6.1: the Notion upload is the record and the downscaled copy + // Intake spec §6.1: the stored upload is the record and the downscaled copy // is what the model looks at. Both go out on the same message. downscaleForVision.mockClear(); downscaleForVision.mockResolvedValue("data:image/jpeg;base64,SMALL"); @@ -632,7 +675,7 @@ describe("ChatFab — photo uploads", () => { files?: unknown[]; }; expect(arg.text).toContain( - "[Attached photos: file_upload_id=fu_123 name=plate.jpg]" + "[Attached photos: attachment_id=3f2504e0-4f89-41d3-9a0c-0305e82c3302 name=plate.jpg]" ); expect(arg.files).toEqual([ { @@ -652,7 +695,7 @@ describe("ChatFab — photo uploads", () => { expect(sendMessage).toHaveBeenCalledWith({ text: expect.stringContaining( - "[Attached photos: file_upload_id=fu_123 name=plate.jpg]" + "[Attached photos: attachment_id=3f2504e0-4f89-41d3-9a0c-0305e82c3302 name=plate.jpg]" ), }); }); diff --git a/v5/src/components/ChatFab.tsx b/v5/src/components/ChatFab.tsx index 96a8586..8ed5c81 100644 --- a/v5/src/components/ChatFab.tsx +++ b/v5/src/components/ChatFab.tsx @@ -221,7 +221,8 @@ const SPEECH_STORE = { interface PendingPhoto { key: string; - file_upload_id: string; + /** `attachments.id` from `POST /api/uploads` — a Postgres uuid. */ + attachmentId: string; name: string; previewUrl: string; /** Downscaled copy the model sees; absent when the browser could not encode it. */ @@ -351,16 +352,24 @@ export function ChatFab() { try { const form = new FormData(); form.append("file", file); - // The Notion upload is the record; the downscaled copy is what the + form.append("kind", "chat"); + // The stored upload is the record; the downscaled copy is what the // model looks at (intake spec §6.1). They run side by side, and a // photo the browser cannot encode still uploads. const [res, dataUrl] = await Promise.all([ - fetch("/api/upload-notion", { + fetch("/api/uploads", { method: "POST", body: form, }), downscaleForVision(file), ]); + if (res.status === 503) { + // No Blob store is configured, so there is nowhere to keep the + // photo. Say so in the visitor's language and let them send the + // message anyway — a report without a picture still beats no + // report (Article 4). + throw new Error(t("uploadsUnavailable")); + } if (!res.ok) { const body = (await res.json().catch(() => null)) as | { error?: string } @@ -368,15 +377,18 @@ export function ChatFab() { throw new Error(body?.error || "Upload failed"); } const data = (await res.json()) as { - file_upload_id: string; + attachmentId: string; name: string; }; setPendingPhotos((prev) => [ ...prev, { - key: `${data.file_upload_id}-${Date.now()}-${Math.random()}`, - file_upload_id: data.file_upload_id, + key: `${data.attachmentId}-${Date.now()}-${Math.random()}`, + attachmentId: data.attachmentId, name: data.name, + // A chat photo is stored privately (it may show a person), so + // the response carries no URL — the local object URL made above + // is the preview, and always was. previewUrl, dataUrl: dataUrl ?? undefined, }, @@ -545,7 +557,7 @@ export function ChatFab() { if (pendingPhotos.length > 0) { const hint = pendingPhotos .map( - (p) => `file_upload_id=${p.file_upload_id} name=${p.name}` + (p) => `attachment_id=${p.attachmentId} name=${p.name}` ) .join("; "); outgoing = `${text}\n\n[Attached photos: ${hint}]`; diff --git a/v5/src/components/PrimaryNav.test.tsx b/v5/src/components/PrimaryNav.test.tsx index 13209b9..308100f 100644 --- a/v5/src/components/PrimaryNav.test.tsx +++ b/v5/src/components/PrimaryNav.test.tsx @@ -156,7 +156,7 @@ describe("PrimaryNav — sign-in control", () => { }); it("shows the first name and a sign-out control once signed in", async () => { - fetchIdentity.mockResolvedValue({ role: "student", name: "Ada Lovelace" }); + fetchIdentity.mockResolvedValue({ role: "user", name: "Ada Lovelace" }); render(); expect(await screen.findByText("Ada")).toBeInTheDocument(); @@ -171,7 +171,7 @@ describe("PrimaryNav — sign-in control", () => { }); it("names the signed-in state for screen readers", async () => { - fetchIdentity.mockResolvedValue({ role: "staff", name: "Niti Parikh" }); + fetchIdentity.mockResolvedValue({ role: "admin", name: "Niti Parikh" }); render(); expect(await screen.findByLabelText("Signed in as Niti")).toHaveTextContent( @@ -180,7 +180,7 @@ describe("PrimaryNav — sign-in control", () => { }); it("renders no avatar image in either state (technical-schematic system)", async () => { - fetchIdentity.mockResolvedValue({ role: "student", name: "Ada Lovelace" }); + fetchIdentity.mockResolvedValue({ role: "user", name: "Ada Lovelace" }); const { container } = render(); await screen.findByText("Ada"); @@ -188,7 +188,7 @@ describe("PrimaryNav — sign-in control", () => { }); it("still offers sign-out when Google supplied no display name", async () => { - fetchIdentity.mockResolvedValue({ role: "student", name: null }); + fetchIdentity.mockResolvedValue({ role: "user", name: null }); render(); expect( @@ -208,7 +208,7 @@ describe("PrimaryNav — sign-in control", () => { it("signs out through the shared helper", async () => { const user = userEvent.setup(); - fetchIdentity.mockResolvedValue({ role: "student", name: "Ada Lovelace" }); + fetchIdentity.mockResolvedValue({ role: "user", name: "Ada Lovelace" }); render(); await user.click(await screen.findByRole("button", { name: "SIGN OUT" })); @@ -283,8 +283,8 @@ describe("PrimaryNav — staff refresh control", () => { fetchIdentity.mockResolvedValue(null); }); - it("offers the refresh control to staff", async () => { - fetchIdentity.mockResolvedValue({ role: "staff", name: "Niti Parikh" }); + it("offers the refresh control to an admin", async () => { + fetchIdentity.mockResolvedValue({ role: "admin", name: "Niti Parikh" }); render(); expect( @@ -292,8 +292,8 @@ describe("PrimaryNav — staff refresh control", () => { ).toBeInTheDocument(); }); - it("offers the refresh control to admins", async () => { - fetchIdentity.mockResolvedValue({ role: "admin", name: "Isaac Steinberg" }); + it("offers the refresh control to a super admin", async () => { + fetchIdentity.mockResolvedValue({ role: "super_admin", name: "Isaac Steinberg" }); render(); expect( @@ -301,8 +301,8 @@ describe("PrimaryNav — staff refresh control", () => { ).toBeInTheDocument(); }); - it("does not show it to a signed-in student", async () => { - fetchIdentity.mockResolvedValue({ role: "student", name: "Ada Lovelace" }); + it("does not show it to an ordinary signed-in user", async () => { + fetchIdentity.mockResolvedValue({ role: "user", name: "Ada Lovelace" }); render(); await screen.findByRole("button", { name: "SIGN OUT" }); @@ -322,8 +322,8 @@ describe("PrimaryNav — staff refresh control", () => { }); // en.json: nav.add = "ADD", nav.addAria = "Add new equipment to the inventory". -// Adding equipment is staff-only (auth spec amendment 2026-09-14); the chat -// enforces it server-side, so this only asserts the entry point's visibility. +// Adding equipment needs `tools.add` (spec §3.5); the chat enforces it +// server-side, so this only asserts the entry point's visibility. describe("PrimaryNav — add equipment", () => { const ADD = "Add new equipment to the inventory"; @@ -334,8 +334,8 @@ describe("PrimaryNav — add equipment", () => { }); it.each([ - ["staff", "Niti Parikh"], - ["admin", "Isaac Steinberg"], + ["admin", "Niti Parikh"], + ["super_admin", "Isaac Steinberg"], ] as const)("offers it to %s", async (role, name) => { fetchIdentity.mockResolvedValue({ role, name }); render(); @@ -345,8 +345,8 @@ describe("PrimaryNav — add equipment", () => { ); }); - it("does not offer it to a signed-in student", async () => { - fetchIdentity.mockResolvedValue({ role: "student", name: "Ada Lovelace" }); + it("does not offer it to an ordinary signed-in user", async () => { + fetchIdentity.mockResolvedValue({ role: "user", name: "Ada Lovelace" }); render(); await screen.findByRole("button", { name: "SIGN OUT" }); diff --git a/v5/src/components/PrimaryNav.tsx b/v5/src/components/PrimaryNav.tsx index 31d7562..3162246 100644 --- a/v5/src/components/PrimaryNav.tsx +++ b/v5/src/components/PrimaryNav.tsx @@ -5,6 +5,7 @@ import { usePathname } from "next/navigation"; import { useEffect, useState } from "react"; import { useTranslations } from "next-intl"; import { useChatLauncher } from "./ChatLauncherContext"; +import { AdminLink } from "./AdminLink"; import { RefreshCatalogButton } from "./RefreshCatalogButton"; import { canAddEquipment } from "../lib/capabilities/access"; import { siteConfig } from "../lib/site-config"; @@ -96,10 +97,14 @@ export function PrimaryNav({ noticeDurationMs = SIGN_IN_NOTICE_MS }: { noticeDur {t(link.key)} ))} - {/* Adding equipment is staff-only (auth spec amendment 2026-09-14), so the - entry point waits for an identity that may use it. The chat enforces - the same rule server-side; hiding the button is only presentation. */} - {canAddEquipment(identity?.role) ? ( + {/* Adding equipment needs `tools.add` (spec §3.5), so the entry point + waits for an identity that holds it. The chat enforces the same + declaration server-side; hiding the button is only presentation. */} + {/* The way into `/admin`, for anyone holding an admin-surface permission + (spec §6). Like every other control here it is presentation: the + layout behind it resolves the identity again and refuses. */} + + {canAddEquipment(identity) ? ( /* Same nav-action chrome as Report: an action, not a page. */ + + ) : null} + - - {/* Outside the