232 lines
17 KiB
Markdown
232 lines
17 KiB
Markdown
---
|
|
phase: 19-local-auth-no-oidc-mode
|
|
plan: 02
|
|
type: tdd
|
|
wave: 2
|
|
depends_on: ["19-01"]
|
|
files_modified:
|
|
- apps/api/src/routes/admin.ts
|
|
- apps/api/src/routes/me.ts
|
|
- apps/api/src/auth/linkOidc.ts
|
|
- apps/api/tests/routes/admin.test.ts
|
|
- apps/api/tests/routes/me.test.ts
|
|
autonomous: true
|
|
requirements: [AUTH-LOCAL-07, AUTH-LOCAL-08, AUTH-LOCAL-09, AUTH-LOCAL-10, AUTH-LOCAL-17]
|
|
|
|
must_haves:
|
|
truths:
|
|
- "An admin can create a local member (users row + local_credentials row with a hashed initial password) in one transaction"
|
|
- "Creating a member with an already-used username returns 409, not a 500 or a partial insert"
|
|
- "An admin can reset any local member's password without knowing the current one"
|
|
- "A user can change their own password only after verifying their current password"
|
|
- "GET /api/me returns hasLocalCredential so the PWA knows whether to show Change-password / Link-OIDC"
|
|
- "Linking an OIDC identity binds iss+sub to the current user and deletes their local_credentials row; a conflicting iss+sub is rejected (409) and no local row is deleted"
|
|
- "No password or Zod received-value is ever echoed in any response or log on these routes"
|
|
artifacts:
|
|
- path: "apps/api/src/auth/linkOidc.ts"
|
|
provides: "linkOidcToUser(userId, iss, sub) — binds identity + deletes local cred, 409 on conflict"
|
|
exports: ["linkOidcToUser", "OidcLinkConflictError"]
|
|
min_lines: 25
|
|
- path: "apps/api/src/routes/admin.ts"
|
|
provides: "POST /members + POST /members/:id/password + hasLocalCredential in GET /members"
|
|
contains: "members"
|
|
- path: "apps/api/src/routes/me.ts"
|
|
provides: "POST /password + POST /link-oidc + hasLocalCredential in GET /"
|
|
contains: "hasLocalCredential"
|
|
key_links:
|
|
- from: "apps/api/src/routes/admin.ts"
|
|
to: "apps/api/src/auth/localCredentials.ts"
|
|
via: "hashPassword on create-member and reset-password"
|
|
pattern: "hashPassword"
|
|
- from: "apps/api/src/routes/me.ts"
|
|
to: "apps/api/src/auth/localCredentials.ts"
|
|
via: "verifyPassword(current) then hashPassword(new) on self-change"
|
|
pattern: "verifyPassword"
|
|
- from: "apps/api/src/auth/linkOidc.ts"
|
|
to: "apps/api/src/db/schema.ts"
|
|
via: "uniq_oidc_identity preflight SELECT + UPDATE users + DELETE local_credentials"
|
|
pattern: "localCredentials"
|
|
---
|
|
|
|
<objective>
|
|
Build the backend account-management surface for local auth: admin-creates-member, admin-reset-password, self-change-password, the `hasLocalCredential` signal on `/api/me`, and the OIDC-link binding helper (`linkOidcToUser`) that the middleware plan's `/callback` will invoke.
|
|
|
|
Purpose: These are API endpoints with defined request/response contracts and high security stakes (credential creation, password reset, identity binding) — TDD candidates. The OIDC-link binding is extracted into a standalone `linkOidc.ts` helper so the middleware plan (19-03) can call it from `/callback` without this plan and that plan touching the same file.
|
|
|
|
Output: extended `admin.ts` + `me.ts`, new `linkOidc.ts` helper, extended `admin.test.ts` + `me.test.ts`.
|
|
|
|
Derived REQ-IDs covered: AUTH-LOCAL-07 (admin create member, D-10), AUTH-LOCAL-08 (admin reset, D-11), AUTH-LOCAL-09 (self-change, D-11), AUTH-LOCAL-10 (OIDC-link, D-12), AUTH-LOCAL-17 (hasLocalCredential).
|
|
</objective>
|
|
|
|
<execution_context>
|
|
@$HOME/.claude/gsd-core/workflows/execute-plan.md
|
|
@$HOME/.claude/gsd-core/templates/summary.md
|
|
</execution_context>
|
|
|
|
<context>
|
|
@.planning/PROJECT.md
|
|
@.planning/ROADMAP.md
|
|
@.planning/STATE.md
|
|
@.planning/phases/19-local-auth-no-oidc-mode/19-RESEARCH.md
|
|
@.planning/phases/19-local-auth-no-oidc-mode/19-PATTERNS.md
|
|
@.planning/phases/19-local-auth-no-oidc-mode/19-CONTEXT.md
|
|
@.planning/phases/19-local-auth-no-oidc-mode/19-01-SUMMARY.md
|
|
</context>
|
|
|
|
<tasks>
|
|
|
|
<task type="tdd" tdd="true">
|
|
<name>Task 1: Admin create-member + reset-password + hasLocalCredential on GET /members</name>
|
|
<read_first>
|
|
- apps/api/src/routes/admin.ts (requireAdmin guard at line ~42; noEchoHook lines ~70-74; POST /credentials lines ~112-132; GET /members lines ~83-102; db.transaction in PUT /calendars/:id/shared lines ~170-183)
|
|
- apps/api/tests/routes/admin.test.ts (existing admin route test patterns + mock setup)
|
|
- apps/api/src/auth/localCredentials.ts (hashPassword — from 19-01)
|
|
- apps/api/src/db/schema.ts (users, localCredentials, COLOR_PALETTE for member color)
|
|
- .planning/phases/19-local-auth-no-oidc-mode/19-PATTERNS.md §apps/api/src/routes/admin.ts (LEFT JOIN extension + 409 pattern + transaction)
|
|
- .planning/phases/19-local-auth-no-oidc-mode/19-UI-SPEC.md Surface 11A/11B (field names, copy, validation: passwords-match, min-8-char)
|
|
</read_first>
|
|
<files>apps/api/src/routes/admin.ts, apps/api/tests/routes/admin.test.ts</files>
|
|
<behavior>
|
|
- RED first: extend apps/api/tests/routes/admin.test.ts asserting:
|
|
- Test 1: POST /api/admin/members { displayName, username, initialPassword } → 201, inserts a users row + a local_credentials row whose hash verifies against the initial password
|
|
- Test 2: POST /api/admin/members with a username already in local_credentials → 409, no new users row created (transaction rolled back)
|
|
- Test 3: POST /api/admin/members/:id/password { newPassword } → 200, the stored hash now verifies the new password; current password NOT required
|
|
- Test 4: a non-admin caller gets 403 on both routes (requireAdmin already covers it — assert it)
|
|
- Test 5: GET /api/admin/members returns hasLocalCredential:true for a member with a local_credentials row, false otherwise
|
|
- Run; confirm FAIL.
|
|
</behavior>
|
|
<action>
|
|
Extend apps/api/src/routes/admin.ts (do NOT move the `adminRouter.use('*', requireAdmin)` first statement). Reuse the existing `noEchoHook`. Add `POST /members` with a zValidator json schema `{ displayName: string min1, username: string min1 max128, initialPassword: string min8 }` + noEchoHook: in a `db.transaction`, INSERT users (displayName, color from COLOR_PALETTE round-robin or existing color-assignment helper), then INSERT local_credentials (user_id, username, passwordHash via hashPassword(initialPassword)); on a username uniqueness violation return `c.json({ error: 'Username already in use' }, 409)`. Add `POST /members/:id/password` with schema `{ newPassword: string min8 }` + noEchoHook: verify the target user exists and has a local_credentials row (404 if not), UPDATE local_credentials SET password_hash = hashPassword(newPassword) WHERE user_id = :id. Extend the existing `GET /members` query with a `.leftJoin(localCredentials, eq(localCredentials.userId, users.id))` and map `hasLocalCredential: row.localCredId !== null` into each member object alongside the existing `hasCredential`. Use the established error-response pattern (known error → 4xx; unexpected → console.error without body + 503). Never log request bodies. Run the suite — GREEN.
|
|
</action>
|
|
<verify>
|
|
<automated>pnpm --filter @familysync/api test tests/routes/admin.test.ts</automated>
|
|
</verify>
|
|
<acceptance_criteria>
|
|
- `pnpm --filter @familysync/api test tests/routes/admin.test.ts` exits 0, all new tests green
|
|
- Source assertion: `grep -c "requireAdmin" apps/api/src/routes/admin.ts` >= 1 and the `.use('*', requireAdmin)` line remains the first router statement
|
|
- Source assertion: `grep -c "hashPassword" apps/api/src/routes/admin.ts` >= 1
|
|
- Source assertion: `grep -c "noEchoHook" apps/api/src/routes/admin.ts` >= 1 used on both new POST routes
|
|
- Behavior: duplicate-username create returns 409 and leaves the users table unchanged (transaction rollback verified in Test 2)
|
|
</acceptance_criteria>
|
|
<done>Admin can create local members (atomic users+local_credentials), reset member passwords, and GET /members reports hasLocalCredential; non-admin is 403; no password echo.</done>
|
|
</task>
|
|
|
|
<task type="tdd" tdd="true">
|
|
<name>Task 2: Self-change password + hasLocalCredential on GET /api/me</name>
|
|
<read_first>
|
|
- apps/api/src/routes/me.ts (resolveUserId lines ~74-86; meNoEchoHook lines ~164-168; resolveAdminAndSetupStatus lines ~50-66; POST /credential lines ~154-202; response shape lines ~93-139)
|
|
- apps/api/tests/routes/me.test.ts (existing me-route test patterns)
|
|
- apps/api/src/auth/localCredentials.ts (verifyPassword + hashPassword)
|
|
- .planning/phases/19-local-auth-no-oidc-mode/19-PATTERNS.md §apps/api/src/routes/me.ts (resolveAdminAndSetupStatus extension + change-password route)
|
|
- .planning/phases/19-local-auth-no-oidc-mode/19-UI-SPEC.md Surface 12 (current/new/confirm fields, error copy)
|
|
</read_first>
|
|
<files>apps/api/src/routes/me.ts, apps/api/tests/routes/me.test.ts</files>
|
|
<behavior>
|
|
- RED first: extend apps/api/tests/routes/me.test.ts asserting:
|
|
- Test 1: POST /api/me/password { currentPassword, newPassword } with correct current → 200, stored hash now verifies newPassword
|
|
- Test 2: wrong currentPassword → 401, hash unchanged
|
|
- Test 3: user with no local_credentials row → 404
|
|
- Test 4: GET /api/me includes hasLocalCredential (true when a row exists, false otherwise) alongside isAdmin/needsProviderSetup
|
|
- Run; confirm FAIL.
|
|
</behavior>
|
|
<action>
|
|
Extend apps/api/src/routes/me.ts. Reuse `resolveUserId` and the existing `meNoEchoHook`. Add `POST /password` with zValidator json `{ currentPassword: string min1, newPassword: string min8 }` + meNoEchoHook: resolveUserId (401 if null); SELECT the user's local_credentials row (404 if none); `verifyPassword(cred.passwordHash, currentPassword)` → 401 `{ error: 'Current password incorrect' }` on false; else UPDATE local_credentials SET password_hash = hashPassword(newPassword) WHERE user_id. Extend `resolveAdminAndSetupStatus` (or the GET / handler) to also SELECT whether a local_credentials row exists for the user and include `hasLocalCredential: boolean` in the GET /api/me response object next to isAdmin and needsProviderSetup. Use the established error pattern; never log the body. Run the suite — GREEN.
|
|
</action>
|
|
<verify>
|
|
<automated>pnpm --filter @familysync/api test tests/routes/me.test.ts</automated>
|
|
</verify>
|
|
<acceptance_criteria>
|
|
- `pnpm --filter @familysync/api test tests/routes/me.test.ts` exits 0, all new tests green
|
|
- Source assertion: `grep -c "verifyPassword" apps/api/src/routes/me.ts` >= 1
|
|
- Source assertion: `grep -c "hasLocalCredential" apps/api/src/routes/me.ts` >= 1
|
|
- Behavior: wrong current password returns 401 and leaves the hash unchanged (Test 2)
|
|
</acceptance_criteria>
|
|
<done>Self password-change verifies current then updates; GET /api/me exposes hasLocalCredential; no echo.</done>
|
|
</task>
|
|
|
|
<task type="tdd" tdd="true">
|
|
<name>Task 3: linkOidcToUser helper + POST /api/me/link-oidc initiation</name>
|
|
<!-- planner-discipline-allow: email -->
|
|
<!-- planner-discipline-allow: never uses email -->
|
|
<read_first>
|
|
- apps/api/src/auth/user.ts (upsertUser — identity = oidc_iss+oidc_sub never email, D-10; the strictness the link helper must replicate)
|
|
- apps/api/src/db/schema.ts (users.oidcIss/oidcSub, uniq_oidc_identity index line ~63; localCredentials)
|
|
- apps/api/src/routes/me.ts (resolveUserId; route registration style)
|
|
- .planning/phases/19-local-auth-no-oidc-mode/19-RESEARCH.md §OIDC-Link Flow + §Common Pitfalls 6 (iss+sub uniqueness; 409; state param)
|
|
- .planning/phases/19-local-auth-no-oidc-mode/19-UI-SPEC.md Surface 13 (link copy + 409 post-redirect error)
|
|
</read_first>
|
|
<files>apps/api/src/auth/linkOidc.ts, apps/api/src/routes/me.ts, apps/api/tests/routes/me.test.ts</files>
|
|
<behavior>
|
|
- RED first: extend apps/api/tests/routes/me.test.ts asserting:
|
|
- Test 1: linkOidcToUser(userId, iss, sub) where no other user holds iss+sub → UPDATEs users.oidc_iss/oidc_sub for userId AND DELETEs that user's local_credentials row
|
|
- Test 2: linkOidcToUser when iss+sub already belongs to a DIFFERENT user → throws OidcLinkConflictError AND the target user's local_credentials row is NOT deleted (binding aborted before any write)
|
|
- Test 3: POST /api/me/link-oidc (authenticated) → returns a redirect target / authorization-code initiation payload that encodes the current userId in signed state (assert the response shape only; the actual OIDC redirect is exercised by 19-03's /callback)
|
|
- Run; confirm FAIL.
|
|
</behavior>
|
|
<action>
|
|
Create apps/api/src/auth/linkOidc.ts exporting `class OidcLinkConflictError extends Error` and async `linkOidcToUser(userId: number, iss: string, sub: string): Promise<void>`: preflight SELECT users WHERE oidc_iss=iss AND oidc_sub=sub LIMIT 1 — if a row exists with id !== userId, throw OidcLinkConflictError (do NOT write anything). Otherwise run a db.transaction: UPDATE users SET oidc_iss=iss, oidc_sub=sub, claimed=true WHERE id=userId; DELETE FROM local_credentials WHERE user_id=userId. Bind by iss+sub only — never email (D-10/D-12). The uniq_oidc_identity DB constraint is the safety net behind the preflight (Pitfall 6).
|
|
|
|
In apps/api/src/routes/me.ts add `POST /link-oidc`: resolveUserId (401 if null); produce the OIDC authorization-code initiation with a signed `state` encoding `{ linkUserId: userId, nonce }` so the 19-03 /callback can read it. Return the initiation payload/redirect target the PWA needs (Surface 13 "Continue with OIDC"). Do not perform the binding here — the binding happens in /callback (19-03) via linkOidcToUser. Run the suite — GREEN.
|
|
</action>
|
|
<verify>
|
|
<automated>pnpm --filter @familysync/api test tests/routes/me.test.ts</automated>
|
|
</verify>
|
|
<acceptance_criteria>
|
|
- `pnpm --filter @familysync/api test tests/routes/me.test.ts` exits 0, link tests green
|
|
- Source assertion: linkOidc.ts performs a preflight SELECT before the UPDATE (grep for the conflict check) and throws OidcLinkConflictError on a foreign iss+sub
|
|
- Behavior: Test 2 confirms NO local_credentials deletion occurs on conflict
|
|
- Source assertion: `grep -ci "email" apps/api/src/auth/linkOidc.ts` == 0 (binding never uses email — D-10)
|
|
</acceptance_criteria>
|
|
<done>linkOidcToUser binds iss+sub and drops the local credential atomically, 409-equivalent on conflict with no partial write; /api/me/link-oidc initiates the signed-state OIDC redirect.</done>
|
|
</task>
|
|
|
|
</tasks>
|
|
|
|
<threat_model>
|
|
## Trust Boundaries
|
|
|
|
| Boundary | Description |
|
|
|----------|-------------|
|
|
| client → /api/admin/* | untrusted member-management input crosses here; gated by requireAdmin |
|
|
| client → /api/me/* | self-service password/link input; gated by session (resolveUserId) |
|
|
| OIDC token → users row | iss+sub binding crosses an external-identity boundary |
|
|
|
|
## STRIDE Threat Register (ASVS L1, block on high)
|
|
|
|
| Threat ID | Category | Component | Disposition | Mitigation Plan |
|
|
|-----------|----------|-----------|-------------|-----------------|
|
|
| T-19-05 | Elevation of Privilege | POST /api/admin/members | mitigate | requireAdmin router guard; integration test asserts 403 for non-admin (V4) |
|
|
| T-19-06 | Information Disclosure | password in Zod error | mitigate | noEchoHook on every credential route; no console.log of bodies (V5; RESEARCH Pitfall 3) |
|
|
| T-19-07 | Elevation of Privilege | self-change password | mitigate | verifyPassword(current) required before update; resolveUserId from session not body (V4) |
|
|
| T-19-08 | Elevation of Privilege | OIDC-link account takeover | mitigate | preflight iss+sub uniqueness → conflict aborts before any write; uniq_oidc_identity DB constraint backstop (RESEARCH Pitfall 6) |
|
|
| T-19-09 | Tampering | OIDC-link CSRF | mitigate | userId carried in signed OIDC `state` (nonce); binding only for the state-encoded user |
|
|
| T-19-10 | Tampering | partial insert on create-member failure | mitigate | db.transaction wraps users + local_credentials; 409 rolls back (RESEARCH Pitfall 5) |
|
|
</threat_model>
|
|
|
|
<verification>
|
|
- `pnpm --filter @familysync/api test` green (admin + me suites)
|
|
- `pnpm --filter @familysync/api typecheck` exits 0
|
|
- No password or Zod received-value appears in any response body or log (noEchoHook everywhere)
|
|
</verification>
|
|
|
|
<success_criteria>
|
|
- AUTH-LOCAL-07/08: admin create + reset member passwords (atomic, 409 on dup, 403 for non-admin)
|
|
- AUTH-LOCAL-09: self-change requires correct current password
|
|
- AUTH-LOCAL-10: OIDC-link binds iss+sub + drops local cred, conflict aborts cleanly
|
|
- AUTH-LOCAL-17: GET /api/me exposes hasLocalCredential
|
|
</success_criteria>
|
|
|
|
<artifacts_produced>
|
|
## Artifacts this phase produces (Plan 02)
|
|
- Route: `POST /api/admin/members` (admin create local member)
|
|
- Route: `POST /api/admin/members/:id/password` (admin reset)
|
|
- Route: `POST /api/me/password` (self-change)
|
|
- Route: `POST /api/me/link-oidc` (OIDC-link initiation, signed state)
|
|
- Function: `linkOidcToUser(userId, iss, sub)` + `OidcLinkConflictError` (apps/api/src/auth/linkOidc.ts)
|
|
- Response field: `hasLocalCredential` on GET /api/me and GET /api/admin/members
|
|
</artifacts_produced>
|
|
|
|
<output>
|
|
Create `.planning/phases/19-local-auth-no-oidc-mode/19-02-SUMMARY.md` when done
|
|
</output>
|