17 KiB
17 KiB
phase, plan, type, wave, depends_on, files_modified, autonomous, requirements, must_haves
| phase | plan | type | wave | depends_on | files_modified | autonomous | requirements | must_haves | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 19-local-auth-no-oidc-mode | 02 | tdd | 2 |
|
|
true |
|
|
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).
<execution_context> @$HOME/.claude/gsd-core/workflows/execute-plan.md @$HOME/.claude/gsd-core/templates/summary.md </execution_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 Task 1: Admin create-member + reset-password + hasLocalCredential on GET /members - 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) apps/api/src/routes/admin.ts, apps/api/tests/routes/admin.test.ts - 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. 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. pnpm --filter @familysync/api test tests/routes/admin.test.ts - `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) Admin can create local members (atomic users+local_credentials), reset member passwords, and GET /members reports hasLocalCredential; non-admin is 403; no password echo. Task 2: Self-change password + hasLocalCredential on GET /api/me - 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) apps/api/src/routes/me.ts, apps/api/tests/routes/me.test.ts - 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. 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. pnpm --filter @familysync/api test tests/routes/me.test.ts - `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) Self password-change verifies current then updates; GET /api/me exposes hasLocalCredential; no echo. Task 3: linkOidcToUser helper + POST /api/me/link-oidc initiation - 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) apps/api/src/auth/linkOidc.ts, apps/api/src/routes/me.ts, apps/api/tests/routes/me.test.ts - 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. Create apps/api/src/auth/linkOidc.ts exporting `class OidcLinkConflictError extends Error` and async `linkOidcToUser(userId: number, iss: string, sub: string): Promise`: 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.
<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> |
<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:
hasLocalCredentialon GET /api/me and GET /api/admin/members </artifacts_produced>