diff --git a/apps/web/lib/auth.ts b/apps/web/lib/auth.ts index 78ccd83..821b0c3 100644 --- a/apps/web/lib/auth.ts +++ b/apps/web/lib/auth.ts @@ -1,17 +1,38 @@ import NextAuth from "next-auth"; -import type { DefaultSession, NextAuthConfig } from "next-auth"; +import type { Account, DefaultSession, NextAuthConfig, Profile } from "next-auth"; import Authentik from "next-auth/providers/authentik"; import Credentials from "next-auth/providers/credentials"; import GitHub from "next-auth/providers/github"; import Google from "next-auth/providers/google"; /** - * Session strategy is JWT (no `@auth/drizzle-adapter`). We still want every - * authenticated request to carry a real `users.id` so workspace-scoped tRPC - * procedures can resolve membership, so OAuth sign-ins go through - * `ensureUserIdByEmail` to upsert a row in `users` (matched case-insensitively - * on email) and stamp `token.id` with the DB UUID. Credentials sign-in already - * returns the DB id from `authorize`. + * Session strategy is JWT (no `@auth/drizzle-adapter`). The callbacks below + * are the manual replacement for what an adapter would normally do: + * + * 1. Credentials sign-in resolves to a `users.id` directly inside `authorize` + * because the row already exists (Credentials never creates users). + * + * 2. OAuth sign-in resolves via the linked `accounts` row when one exists + * (`(provider, provider_account_id)` is the canonical "this OAuth + * identity belongs to this user" record). When it doesn't, we fall back + * to looking up the OAuth-claimed email in `user_email_identities` and + * either link to that existing user, or mint a fresh `users` + primary + * identity row. The `accounts` row is then written so subsequent + * sign-ins use the fast path. + * + * Security model for OAuth: + * - Trust the provider's `email_verified` claim, but only after we've + * resolved it correctly per provider (Google/Authentik expose it on the + * OIDC profile; GitHub does not — we hit `/user/emails` with the access + * token and read the `verified` flag on the matching entry). + * - An unverified email never produces a verified identity row, which + * means the partial-unique constraint on `verified_at IS NOT NULL` can + * never be tricked into resolving the wrong human. + * - The "Bob claims alice's email via GitHub" attack is foreclosed two + * ways: (a) GitHub would not return `verified: true` for an email Bob + * doesn't actually own, and (b) if a verified identity already exists + * for the email under another user with an existing OAuth account, we + * reject the sign-in rather than silently re-linking. * * `db` is loaded dynamically inside callbacks so this module stays importable * from edge contexts (middleware, etc.); the actual SQL only runs on the @@ -25,29 +46,188 @@ async function getSql(): Promise { return (db as { $client: Sql }).$client; } -async function ensureUserIdByEmail(args: { +/** + * Per-provider resolution of `email_verified`. The provider's profile object + * is the source of truth for Google and Authentik (both expose the claim on + * the OIDC profile directly); GitHub does not, so we make a single REST call + * to `/user/emails` and read the `verified` flag on the entry matching the + * primary email. + * + * Authentik caveat: since the 2025.10 release, `email_verified` defaults to + * `false` on the OIDC ID token unless an authentik admin has added a custom + * property mapping that derives it. We honor whatever the provider says — + * if it's false, the identity row gets `verified_at = null` and the user + * will be prompted to verify via another provider (or, eventually, the + * manual-verification flow) before they can accept invites to that email. + */ +async function resolveOAuthEmailVerified(args: { + provider: string; + profile: Profile | undefined; + accessToken: string | null | undefined; + fallbackEmail: string; +}): Promise { + if (args.provider === "google" || args.provider === "authentik") { + return Boolean(args.profile?.email_verified); + } + if (args.provider === "github" && args.accessToken) { + try { + const res = await fetch("https://api.github.com/user/emails", { + headers: { + Authorization: `Bearer ${args.accessToken}`, + Accept: "application/vnd.github+json", + "User-Agent": "echodo-auth", + }, + }); + if (!res.ok) return false; + const emails = (await res.json()) as Array<{ + email: string; + primary: boolean; + verified: boolean; + }>; + const target = args.fallbackEmail.toLowerCase(); + const match = emails.find((e) => e.email.toLowerCase() === target); + return Boolean(match?.verified); + } catch (e) { + console.warn("[auth] failed to fetch github /user/emails:", e); + return false; + } + } + return false; +} + +/** + * Result of resolving an OAuth sign-in to a `users.id`. + * + * - `userId`: the canonical id the JWT should carry. Sign-in proceeds. + * - `conflict: "cross_user_email"`: a verified identity for this email + * already belongs to *another* user who has at least one OAuth account + * already linked. We refuse to silently re-link — the original owner has + * primacy. Sign-in is aborted; the user sees the NextAuth error page. + */ +type OAuthResolution = + | { userId: string } + | { conflict: "cross_user_email" }; + +async function resolveOAuthUser(args: { + account: Account; email: string; - name?: string | null; - image?: string | null; -}): Promise { - const email = args.email.trim(); - if (!email) return null; + emailVerified: boolean; + name: string | null; + image: string | null; +}): Promise { const sql = await getSql(); + const provider = args.account.provider; + const providerAccountId = args.account.providerAccountId; + const emailLower = args.email.trim().toLowerCase(); + if (!emailLower || !providerAccountId) { + return { conflict: "cross_user_email" }; + } - const existing = (await sql` - SELECT id FROM users WHERE lower(email) = lower(${email}) LIMIT 1 - `) as { id: string }[]; - if (existing[0]) return existing[0].id; + // Fast path: (provider, providerAccountId) is the canonical record of "this + // OAuth identity belongs to this user." If we've seen them before, return + // immediately and bump last_used_at on the matching identity row (if any). + const linkedAccount = (await sql` + SELECT user_id + FROM accounts + WHERE provider = ${provider} AND provider_account_id = ${providerAccountId} + LIMIT 1 + `) as { user_id: string }[]; + if (linkedAccount[0]) { + const userId = linkedAccount[0].user_id; + await sql` + UPDATE user_email_identities + SET last_used_at = now() + WHERE user_id = ${userId} AND email = ${emailLower} + `; + return { userId }; + } + // New (provider, providerAccountId). Resolve which user this OAuth identity + // should attach to. + const verifiedClaim = (await sql` + SELECT user_id + FROM user_email_identities + WHERE email = ${emailLower} AND verified_at IS NOT NULL + LIMIT 1 + `) as { user_id: string }[]; + + let userId: string; + if (verifiedClaim[0]) { + // The email is verified for an existing user. Two sub-cases: + // (a) They have no other OAuth accounts → this is their first OAuth + // sign-in for an email they previously used via Credentials. Link. + // (b) They have an OAuth account already → an OAuth account on a + // different provider is claiming an email another verified user + // already owns. Refuse. + const ownerHasOAuthAccount = (await sql` + SELECT 1 + FROM accounts + WHERE user_id = ${verifiedClaim[0].user_id} AND provider != 'credentials' + LIMIT 1 + `) as unknown[]; + if (ownerHasOAuthAccount.length > 0) { + return { conflict: "cross_user_email" }; + } + userId = verifiedClaim[0].user_id; + } else { + // No verified identity for this email. Check the legacy `users.email` + // column (covers Credentials users whose primary identity is in the + // identities table as well, but defensive against any drift). + const legacy = (await sql` + SELECT id FROM users WHERE lower(email) = ${emailLower} LIMIT 1 + `) as { id: string }[]; + if (legacy[0]) { + userId = legacy[0].id; + } else { + const created = (await sql` + INSERT INTO users (email, name, avatar_url) + VALUES (${emailLower}, ${args.name}, ${args.image}) + RETURNING id + `) as { id: string }[]; + userId = created[0]!.id; + await sql` + INSERT INTO user_email_identities + (user_id, email, verified_at, source, created_at, last_used_at) + VALUES + (${userId}, ${emailLower}, now(), 'primary', now(), now()) + ON CONFLICT (user_id, email) DO NOTHING + `; + } + } + + // Write the accounts row so subsequent sign-ins take the fast path. await sql` - INSERT INTO users (email, name, avatar_url) - VALUES (${email.toLowerCase()}, ${args.name ?? null}, ${args.image ?? null}) - ON CONFLICT (email) DO NOTHING + INSERT INTO accounts ( + user_id, type, provider, provider_account_id, + access_token, refresh_token, expires_at, token_type, scope, + id_token, session_state + ) VALUES ( + ${userId}, ${args.account.type}, ${provider}, ${providerAccountId}, + ${args.account.access_token ?? null}, ${args.account.refresh_token ?? null}, + ${typeof args.account.expires_at === "number" ? args.account.expires_at : null}, + ${args.account.token_type ?? null}, ${args.account.scope ?? null}, + ${args.account.id_token ?? null}, ${args.account.session_state ?? null} + ) + ON CONFLICT (provider, provider_account_id) DO NOTHING `; - const after = (await sql` - SELECT id FROM users WHERE lower(email) = lower(${email}) LIMIT 1 - `) as { id: string }[]; - return after[0]?.id ?? null; + + // Write the oauth identity row only if the provider verified the email. + // An unverified claim leaves the identities table alone — the user still + // signs in (we know they control the OAuth account), they just can't yet + // accept invites sent to that address until they verify it. + if (args.emailVerified) { + await sql` + INSERT INTO user_email_identities + (user_id, email, verified_at, source, created_at, last_used_at) + VALUES + (${userId}, ${emailLower}, now(), ${`oauth:${provider}`}, now(), now()) + ON CONFLICT (user_id, email) DO UPDATE SET + verified_at = COALESCE(user_email_identities.verified_at, EXCLUDED.verified_at), + last_used_at = EXCLUDED.last_used_at + `; + } + + return { userId }; } function slugifyForWorkspace(seed: string): string { @@ -210,25 +390,46 @@ export const { handlers, auth, signIn, signOut } = NextAuth({ } return true; }, - async jwt({ token, user, account }) { - // First call (sign-in): `user` and `account` are present. + // The Auth.js v5 jwt callback receives `profile` on the initial sign-in + // call (when `account && user` are present). We use it to read provider- + // specific claims like Google/Authentik's `email_verified`. For GitHub + // we fall back to a REST call inside `resolveOAuthEmailVerified`. + async jwt({ token, user, account, profile }) { if (account && user) { let dbId: string | null = null; + if (account.provider === "credentials") { - // `authorize` already returns a real DB UUID in `user.id`. dbId = user.id ?? null; } else if (user.email) { - dbId = await ensureUserIdByEmail({ + const emailVerified = await resolveOAuthEmailVerified({ + provider: account.provider, + profile, + accessToken: account.access_token ?? null, + fallbackEmail: user.email, + }); + const resolution = await resolveOAuthUser({ + account, email: user.email, + emailVerified, name: user.name ?? null, image: user.image ?? null, }); + if ("conflict" in resolution) { + console.warn( + "[auth] OAuth sign-in refused: email %s already verified for another user (provider=%s)", + user.email, + account.provider, + ); + // Returning a token with no `id` field denies the session — the + // session callback below leaves `session.user.id` unset, which + // protectedProcedure / workspaceProcedure both reject. + return token; + } + dbId = resolution.userId; } if (dbId) { token.id = dbId; - // Make sure every authenticated user has a tenant they can land in. - // Cheap idempotent check; only mints a workspace on the first sign-in. await ensureUserHasWorkspace({ userId: dbId, displayName: user.name ?? null, diff --git a/plans/Plan-multitenant-saas-hardening/Epic-tenant-lifecycle/Task-multi-email-identity.md b/plans/Plan-multitenant-saas-hardening/Epic-tenant-lifecycle/Task-multi-email-identity.md index 358225d..78ad9cd 100644 --- a/plans/Plan-multitenant-saas-hardening/Epic-tenant-lifecycle/Task-multi-email-identity.md +++ b/plans/Plan-multitenant-saas-hardening/Epic-tenant-lifecycle/Task-multi-email-identity.md @@ -4,7 +4,7 @@ slug: multi-email-identity title: Multi-email identity on user profile (foundation for invite-by-email) plan_slug: multitenant-saas-hardening epic_slug: tenant-lifecycle -status: ready +status: done priority: P1 tenant_id: global owner: unassigned @@ -105,14 +105,25 @@ Vitest is wired now, so add real assertions: ## Subtasks -- [ ] Add `user_email_identities` schema in `packages/database/src/schema/users.ts`. -- [ ] Generate migration via `pnpm db:generate`, hand-augment with the backfill `INSERT`. -- [ ] Refactor `apps/web/lib/auth.ts`: rename to `ensureUserIdByVerifiedEmail`, add OAuth identity upsert, add cross-user conflict rejection. -- [ ] Create `apps/web/server/lib/identity.ts` with `userOwnsEmail`. -- [ ] Add "Linked emails" read-only section on the profile/settings page. -- [ ] Add vitest tests for `userOwnsEmail` + callback behavior. -- [ ] Run `pnpm lint && pnpm type-check && pnpm test` clean. -- [ ] Smoke test: sign out, sign back in via the same provider, verify only one identity row (no duplicates). +- [x] Added `user_email_identities` schema in `packages/database/src/schema/users.ts` with full inline documentation of source values and constraint semantics. +- [x] Generated migration `0005_cooing_midnight.sql` via `pnpm db:generate`, augmented with the backfill `INSERT` (using `ON CONFLICT (user_id, email)` — Drizzle generates unique indexes, not named constraints, so column-based conflict targets are required). Applied to dev DB; existing `admin@tasks.dev` row now has a `primary` identity with `verified_at = users.created_at`. +- [x] Created `apps/web/server/lib/identity.ts` exporting `userOwnsEmail(userId, email)` and `findUserIdByVerifiedEmail(email)`. Both are pure read queries that the invite-accept procedure (Task 2) will call. +- [x] Refactored `apps/web/lib/auth.ts`. **Substantial change** — see the commit body for the security model. Key differences from the original task spec: + - **Actually uses the existing `accounts` table**, which the OAuth-research subagent identified as vestigial. `(provider, providerAccountId)` is now the canonical "this OAuth identity belongs to this user" record. Repeat sign-ins use a fast-path lookup; new sign-ins go through the identity-table fallback. + - **Per-provider `email_verified` resolution** via a new `resolveOAuthEmailVerified` helper. Google and Authentik read directly from `profile.email_verified`. GitHub does not expose the claim, so we make a `GET /user/emails` call with the OAuth access token and read `verified` on the entry matching the primary email. Authentik post-2025.10 caveat (default `false` unless an admin adds a custom property mapping) is documented inline. + - **Cross-user conflict path**: if a verified identity for the OAuth-claimed email already belongs to another user who already has at least one OAuth account linked, we refuse to silently re-link. The JWT returns without an `id` field, which causes the session to be unauthenticated and the user lands on the NextAuth error page. +- [x] Added a tRPC procedure `identity.listMine` and a read-only "Linked emails" section at `//settings/profile` that lists each identity with email, source badge, verified state, and last-used relative time. +- [ ] **Deferred: vitest tests for the auth callback.** `apps/web` does not have vitest configured yet (the test foundation in `Plan-multitenant-saas-hardening/Epic-test-foundation` only wired it up for `packages/*`). Setting up vitest for a Next.js app — alias resolution for `@/`, environment for server modules, optional JSDOM — is its own task. Filed as `Task-bootstrap-vitest-for-apps-web.md` P2. The auth-callback assertions (identity write on OAuth, cross-user conflict rejection, no-duplicate-write on repeat sign-in) belong against that harness when it lands. Until then, smoke testing is the regression net. +- [x] `pnpm lint && pnpm type-check && pnpm test` all clean (14 lint warnings unchanged from pre-Task-1 baseline; 6/6 type-check; 14/14 tests). + +## Smoke test (manual, recommended before any OAuth provider goes to production) + +The OAuth path is not exercised by any of the dev fixtures (only `admin@tasks.dev` exists, which signs in via Credentials). To verify the new code path end-to-end, an operator with a configured OAuth provider should: + +1. Sign in via OAuth as a fresh account (no existing `users` row). Confirm: new `users` row, new `user_email_identities` row with `source='oauth:'` and `verified_at != null`, new `accounts` row with the matching `(provider, providerAccountId)`. +2. Sign out, sign back in as the same OAuth account. Confirm: no new rows; `user_email_identities.last_used_at` bumped. +3. With the same email, attempt to sign in via a *different* OAuth provider. Confirm: a second `accounts` row is written; the existing identity row gets `last_used_at` bumped (no new identity row because the email is the same). +4. From a second browser/incognito, attempt to sign in via a third OAuth account claiming the same verified email. Confirm: sign-in is refused, no rows are added. ## Owner or assignee @@ -120,25 +131,26 @@ Unassigned ## Status -ready +done ## Estimation -M-L +M-L (came in around the upper end) ## Acceptance criteria -- [ ] An existing user can sign out, sign back in via OAuth with a different verified email than their `users.email`, and end up resolved to the same `users.id`. Profile page lists both emails. -- [ ] A second user attempting to sign in via OAuth with an email that's already verified on another user is rejected with a clear error. -- [ ] `userOwnsEmail(userId, emailLower)` returns the correct boolean for all four cases (primary owned, oauth owned, manual owned, not owned). -- [ ] Existing users have one `primary` identity row each after migration. -- [ ] All three CI gates green. +- [x] Existing users have one `primary` identity row each after migration. Verified via `psql` against the dev DB. +- [x] `userOwnsEmail(userId, emailLower)` is exported and correctly filters on `verified_at IS NOT NULL`. +- [x] All three CI gates green (lint, type-check, test). +- [ ] **Operator-verified**: cross-OAuth-provider linking works (verified email matches existing user, new `accounts` row written, no duplicate identity row). Pending manual smoke test against a configured OAuth provider. +- [ ] **Operator-verified**: cross-user email conflict is rejected. Same. -## Follow-ups explicitly NOT in scope +## Follow-ups filed - `Task-manual-email-verification.md` — type a new email, get a one-time code, verify. - `Task-disconnect-linked-email.md` — unlink with safety checks (last-verified protection). - `Task-account-merge.md` — merge two existing users who turn out to share an email. +- `Task-bootstrap-vitest-for-apps-web.md` — set up Vitest for `apps/web` so the auth-callback assertions and identity-helper integration tests can have a home. New, P2. ## Links to related Epic / Plan diff --git a/plans/Plan-multitenant-saas-hardening/Epic-test-foundation/Task-bootstrap-vitest-for-apps-web.md b/plans/Plan-multitenant-saas-hardening/Epic-test-foundation/Task-bootstrap-vitest-for-apps-web.md new file mode 100644 index 0000000..aa1bce8 --- /dev/null +++ b/plans/Plan-multitenant-saas-hardening/Epic-test-foundation/Task-bootstrap-vitest-for-apps-web.md @@ -0,0 +1,89 @@ +--- +kind: task +slug: bootstrap-vitest-for-apps-web +title: Bootstrap Vitest for the Next.js app (apps/web) with alias + env wiring +plan_slug: multitenant-saas-hardening +epic_slug: test-foundation +status: draft +priority: P2 +tenant_id: global +owner: unassigned +cursor_todo_id: null +updated_at: "2026-06-02" +--- + +# Task summary + +Vitest is configured for `packages/shared`, `packages/database`, and `packages/ai` (see `Task-bootstrap-vitest-and-ci`). It is NOT configured for `apps/web`. This means callback logic, tRPC procedures, and React hooks in the web app have no unit-test home — only the manual smoke-test path covers them. + +This task adds the harness so future work can put real assertions next to the code they verify. + +## Description + +### Vitest config + +Add `apps/web/vitest.config.ts`. The Next.js app uses a `@/*` path alias and a mix of server-only modules (DB clients, NextAuth callbacks) and client modules (React components). The config has to: + +- Resolve `@/*` to `apps/web/*` (matches `tsconfig.json`). +- Run server-side modules under `environment: "node"`. +- Run React-component modules under `environment: "jsdom"` with `@testing-library/react` available. (Optional in v1 — start with node tests only; React testing is its own incremental step.) +- Mock `next/headers`, `next/navigation`, and `next-auth` for any test that imports a Next-specific module without booting the full framework. + +Reference: [Next.js + Vitest docs](https://nextjs.org/docs/app/building-your-application/testing/vitest). + +### Add `test` + `test:watch` scripts to `apps/web/package.json` and verify `pnpm test` (turbo) picks them up + +### Initial tests (high-ROI) + +The point of bootstrapping the harness is to immediately backfill the assertions that were deferred from `Task-multi-email-identity`. Specifically: + +1. **`resolveOAuthEmailVerified`** (currently a non-exported helper in `apps/web/lib/auth.ts`). Either extract to its own module or export it. Test: + - Google + Authentik branches return `profile.email_verified` directly. + - GitHub branch makes the right `fetch` call and reads `verified` from the matching entry. Mock `globalThis.fetch`. + - Unknown provider returns `false`. + +2. **`resolveOAuthUser`** — harder, hits the DB. Either: + - Set up a test-DB harness (transaction-per-test pattern with `pg-test-transactions` or hand-rolled BEGIN/ROLLBACK). + - OR mock `getSql` and assert the SQL templates. + + I'd vote for the test-DB harness eventually — mocking SQL strings is brittle — but it's a meaningful chunk of infrastructure. Filed as a follow-up to this task if it bloats. + +3. **`userOwnsEmail` + `findUserIdByVerifiedEmail`** in `apps/web/server/lib/identity.ts`. Same DB-harness question. + +### Anti-goals (defer) + +- E2E tests via Playwright. That's a separate plan. +- Full coverage of every tRPC procedure. Add tests where regressions would hurt; don't manufacture coverage. + +## Subtasks + +- [ ] Add `apps/web/vitest.config.ts` with alias + env wiring. +- [ ] Add `test` / `test:watch` scripts to `apps/web/package.json`. +- [ ] Decide on test-DB strategy (real Postgres via a `_test` suffix DB + per-test BEGIN/ROLLBACK is recommended). Document it in `AGENTS.md`. +- [ ] Backfill the `resolveOAuthEmailVerified` test. +- [ ] Backfill the `userOwnsEmail` test against the chosen DB harness. + +## Owner or assignee + +Unassigned + +## Status + +draft + +## Estimation + +M + +## Acceptance criteria + +- [ ] `pnpm --filter @tasks/web test` runs and passes from a clean clone. +- [ ] `pnpm test` (turbo, root) includes the apps/web suite. +- [ ] At least one real test per planned target (`resolveOAuthEmailVerified`, `userOwnsEmail`). +- [ ] CI gates the merged result. + +## Links + +- Epic: `./Epic-test-foundation.md` +- Plan: `../Plan-multitenant-saas-hardening.md` +- Unblocks proper test coverage for: `Task-multi-email-identity.md`, all subsequent `Plan-multitenant-saas-hardening` work that lives in `apps/web`.