Lucas Berger
b24fbbfde7
- 10-01 v1.1 DB foundation migration + dev-bypass admin seed - 10-02 requireAdmin guard + first-login-wins + /api/me extension (TDD) - 10-03 adminRouter credentials/shared-calendar + member self-service (TDD) - 10-04 PWA /admin route + nav gating + CredentialSheet + SetupBanner - filled 10-VALIDATION Per-Task Verification Map (Nyquist compliant) - finalized ROADMAP Phase 10 plan list
205 lines
16 KiB
Markdown
205 lines
16 KiB
Markdown
---
|
||
phase: 10-admin-role-settings
|
||
plan: 02
|
||
type: tdd
|
||
wave: 2
|
||
depends_on: ["10-01"]
|
||
files_modified:
|
||
- apps/api/src/lib/requireAdmin.ts
|
||
- apps/api/tests/lib/requireAdmin.test.ts
|
||
- apps/api/src/auth/user.ts
|
||
- apps/api/tests/auth/user.test.ts
|
||
- apps/api/src/routes/me.ts
|
||
- apps/api/tests/routes/me.test.ts
|
||
autonomous: true
|
||
requirements: [ADMIN-03]
|
||
must_haves:
|
||
truths:
|
||
- "requireAdmin returns 403 for an authenticated non-admin user and calls next() for an admin user (role read from the DB, never a client flag)"
|
||
- "On first login when zero admins exist, upsertUser flags the new user is_admin=true; subsequent users are normal members"
|
||
- "GET /api/me returns isAdmin and needsProviderSetup for both the dev-bypass path and the OIDC path"
|
||
artifacts:
|
||
- path: "apps/api/src/lib/requireAdmin.ts"
|
||
provides: "MiddlewareHandler that reads c.get('user').id, looks up users.is_admin in the DB, 403s non-admins"
|
||
exports: ["requireAdmin"]
|
||
min_lines: 15
|
||
- path: "apps/api/src/auth/user.ts"
|
||
provides: "upsertUser extended with first-login-wins is_admin bootstrap (zero-admins → first user is admin)"
|
||
contains: "isAdmin"
|
||
- path: "apps/api/src/routes/me.ts"
|
||
provides: "/api/me response extended with isAdmin + needsProviderSetup (both dev-bypass and OIDC paths)"
|
||
contains: "needsProviderSetup"
|
||
key_links:
|
||
- from: "apps/api/src/lib/requireAdmin.ts"
|
||
to: "users.is_admin"
|
||
via: "Drizzle select where eq(users.id, userId)"
|
||
pattern: "users\\.isAdmin"
|
||
- from: "apps/api/src/routes/me.ts"
|
||
to: "member_credentials"
|
||
via: "needsProviderSetup = no member_credentials row for the user"
|
||
pattern: "memberCredentials"
|
||
---
|
||
|
||
<objective>
|
||
Build the server-side admin role primitives that ADMIN-03 depends on: the `requireAdmin` MiddlewareHandler (DB-backed role check, always server-enforced), the first-login-wins `is_admin` bootstrap in `upsertUser` (D-01), and the `/api/me` extension exposing `isAdmin` + `needsProviderSetup` (D-03) for both the dev-bypass and OIDC code paths. TDD: each behavior has a defined input→output contract, so write the failing test first.
|
||
|
||
Purpose: `requireAdmin` is the single security boundary for every `/api/admin/*` route (Plan 03 mounts it). `isAdmin` on `/api/me` drives PWA nav gating (Plan 04, UX-only). `needsProviderSetup` drives the member self-service banner (Plan 04). First-login-wins is written so Phase 12 can later tighten it to "first login after setup_complete" without a rewrite.
|
||
Output: New `requireAdmin.ts` + tests, extended `user.ts` + tests, extended `me.ts` + tests.
|
||
</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/10-admin-role-settings/10-CONTEXT.md
|
||
@.planning/phases/10-admin-role-settings/10-RESEARCH.md
|
||
@.planning/phases/10-admin-role-settings/10-PATTERNS.md
|
||
@.planning/phases/10-admin-role-settings/10-01-SUMMARY.md
|
||
</context>
|
||
|
||
<tasks>
|
||
|
||
<task type="tdd" tdd="true">
|
||
<name>Task 1: requireAdmin middleware (RED→GREEN→REFACTOR)</name>
|
||
<files>apps/api/src/lib/requireAdmin.ts, apps/api/tests/lib/requireAdmin.test.ts</files>
|
||
<read_first>
|
||
- apps/api/src/auth/devBypass.ts (MiddlewareHandler signature + the `c.get('user')`/`c.set('user', DEV_USER)` pattern + the `ContextVariableMap` augmentation, line ~46; the side-effect import idiom)
|
||
- apps/api/src/lib/ (sibling lib modules — e.g. listAccess.ts — for the lib-file import/style conventions)
|
||
- apps/api/tests/lib/ + apps/api/tests/auth/devBypass.test.ts (existing test idioms: how a Hono app/middleware is exercised, how c.get('user') is stubbed, how DB is reached in tests per [[api-integration-test-db]])
|
||
- apps/api/src/db/schema.ts users.isAdmin (from Plan 01) + apps/api/src/db/client.ts (the `db` export)
|
||
- .planning/phases/10-admin-role-settings/10-PATTERNS.md §`apps/api/src/lib/requireAdmin.ts` (the exact MiddlewareHandler shape, the DB lookup excerpt, the `import '../auth/devBypass.js'` side-effect import) + 10-RESEARCH.md §Pattern 1 (Pitfall 9) + §Pitfall 3
|
||
</read_first>
|
||
<behavior>
|
||
- Test (RED): an authenticated user whose DB row has is_admin=false → requireAdmin responds 403 `{ error: 'Forbidden' }` and does NOT call next().
|
||
- Test: an authenticated user whose DB row has is_admin=true → requireAdmin calls next() (request proceeds).
|
||
- Test: no resolved user on context (c.get('user') undefined) → 403 (never throws).
|
||
- Test: the role is read from the DB (users.is_admin), NOT from any value on c.get('user') — a context user object claiming isAdmin=true but with a non-admin DB row is still 403 (defence: bypass only skips OIDC, not the DB check).
|
||
</behavior>
|
||
<action>
|
||
Create `apps/api/src/lib/requireAdmin.ts` exporting `requireAdmin: MiddlewareHandler` (per 10-PATTERNS.md excerpt): read `c.get('user')?.id`; if no id → `c.json({ error: 'Forbidden' }, 403)`; else `db.select({ isAdmin: users.isAdmin }).from(users).where(eq(users.id, userId)).limit(1)`; if `!row?.isAdmin` → 403; else `await next()`. Include the `import '../auth/devBypass.js'` side-effect import for the ContextVariableMap augmentation. NEVER log the user object or any credential. Write `apps/api/tests/lib/requireAdmin.test.ts` FIRST (the four behaviors above), confirm RED, then implement to GREEN. Follow the real-DB test conventions in [[api-integration-test-db]] (tests live in tests/, DB_HOST=127.0.0.1 override) if the test exercises the live DB; otherwise stub the db module.
|
||
</action>
|
||
<verify>
|
||
<automated>cd /home/luc/Projects/familysync && pnpm --filter @familysync/api test -- requireAdmin 2>&1 | tail -15</automated>
|
||
</verify>
|
||
<acceptance_criteria>
|
||
- `apps/api/src/lib/requireAdmin.ts` exports `requireAdmin` typed as `MiddlewareHandler`.
|
||
- The 403 response body is `{ error: 'Forbidden' }` (HTTP 403) for a non-admin authenticated user.
|
||
- The role decision reads `users.isAdmin` from the DB (`grep -q "users.isAdmin" apps/api/src/lib/requireAdmin.ts`); it does NOT branch on a property of `c.get('user')` other than `.id`.
|
||
- No `console.log`/`console.error` of the user object or credentials in the file.
|
||
- `pnpm --filter @familysync/api test -- requireAdmin` passes all four cases.
|
||
</acceptance_criteria>
|
||
<done>requireAdmin guard exists, DB-backed, 403s non-admins, tests green.</done>
|
||
</task>
|
||
|
||
<task type="tdd" tdd="true">
|
||
<name>Task 2: First-login-wins is_admin bootstrap in upsertUser (RED→GREEN→REFACTOR)</name>
|
||
<files>apps/api/src/auth/user.ts, apps/api/tests/auth/user.test.ts</files>
|
||
<read_first>
|
||
- apps/api/src/auth/user.ts (the file being modified — the full `upsertUser` function lines 76–128: the existing-row early-return path, the color assignment, the INSERT `.values({...}).$returningId()` block lines 112–122)
|
||
- apps/api/tests/auth/user.test.ts (existing upsertUser test idioms — how it seeds/asserts DB state, the real-DB test setup)
|
||
- apps/api/src/db/schema.ts users.isAdmin (from Plan 01)
|
||
- .planning/phases/10-admin-role-settings/10-PATTERNS.md §`apps/api/src/auth/user.ts` (the zero-admin COUNT check + `isAdmin: shouldBeAdmin` in `.values()`, the `import { sql }` addition) + 10-RESEARCH.md §Pattern 5 (Phase-12-safe first-login-wins, the "zero admins exist" check that P12 tightens to "after setup_complete")
|
||
</read_first>
|
||
<behavior>
|
||
- Test (RED): upsertUser inserting a brand-new user when the users table has ZERO admins → the inserted row has is_admin=true.
|
||
- Test: upsertUser inserting a new user when an admin already exists → the inserted row has is_admin=false.
|
||
- Test: upsertUser for an EXISTING user (oidc_iss+oidc_sub already present) → is_admin is NOT changed by the upsert (the early-return path is untouched; promotion/demotion is not this function's job).
|
||
</behavior>
|
||
<action>
|
||
In `apps/api/src/auth/user.ts`, before the INSERT in `upsertUser` (after color assignment), add a zero-admin check (per 10-PATTERNS.md excerpt): `db.select({ count: sql<number>\`COUNT(*)\` }).from(users).where(eq(users.isAdmin, true))`; `shouldBeAdmin = Number(count) === 0`; pass `isAdmin: shouldBeAdmin` in the INSERT `.values({...})`. Add `import { sql } from 'drizzle-orm'` if absent. Leave the existing-user early-return path unchanged (do NOT toggle is_admin for existing users). Add a comment marking this as the Phase-12 hook point: "first user when zero admins exist (D-01); Phase 12 tightens to first user after app_config.setup_complete". Write the three test cases in `apps/api/tests/auth/user.test.ts` FIRST, confirm RED, implement to GREEN.
|
||
</action>
|
||
<verify>
|
||
<automated>cd /home/luc/Projects/familysync && pnpm --filter @familysync/api test -- user 2>&1 | tail -15</automated>
|
||
</verify>
|
||
<acceptance_criteria>
|
||
- A new user inserted with zero pre-existing admins has `is_admin=true`; with an existing admin, `is_admin=false`.
|
||
- The existing-user early-return path does not modify is_admin (test asserts unchanged).
|
||
- `grep -q "isAdmin" apps/api/src/auth/user.ts` and the INSERT `.values()` includes `isAdmin`.
|
||
- A comment in `user.ts` names the Phase-12 tightening hook (first login after setup_complete).
|
||
- `pnpm --filter @familysync/api test -- user` passes all cases.
|
||
</acceptance_criteria>
|
||
<done>First-login-wins bootstrap writes is_admin on first insert, member-count-agnostic, Phase-12-safe, tests green.</done>
|
||
</task>
|
||
|
||
<task type="tdd" tdd="true">
|
||
<name>Task 3: Extend /api/me with isAdmin + needsProviderSetup (RED→GREEN→REFACTOR)</name>
|
||
<files>apps/api/src/routes/me.ts, apps/api/tests/routes/me.test.ts</files>
|
||
<read_first>
|
||
- apps/api/src/routes/me.ts (the file being modified — the dev-bypass short-circuit lines 31–43 returning `{ user: { id, displayName, color } }`, the OIDC path lines 60–75 returning the resolved user; both must add isAdmin + needsProviderSetup)
|
||
- apps/api/tests/routes/me.test.ts (existing /api/me test idioms — both dev-bypass and OIDC response assertions)
|
||
- apps/api/src/db/schema.ts users.isAdmin + memberCredentials (from Plan 01) + apps/api/src/db/client.ts (`db`)
|
||
- apps/api/src/auth/user.ts (the resolved `user` shape returned by upsertUser — confirm it now carries isAdmin after Task 2; if not selected, me.ts must select users.isAdmin itself)
|
||
- .planning/phases/10-admin-role-settings/10-PATTERNS.md §`apps/api/src/routes/me.ts` (the needsProviderSetup lookup excerpt: `db.select({id: memberCredentials.id}).from(memberCredentials).where(eq(memberCredentials.userId, userId)).limit(1)` → `needsProviderSetup = !cred`) + 10-RESEARCH.md §Code Examples "/api/me Response Extension" + §Open Questions #3 (needsProviderSetup lives on /api/me)
|
||
</read_first>
|
||
<behavior>
|
||
- Test (RED): dev-bypass path (DEV_USER id=1) → response `user` includes `isAdmin` (looked up from the DB row for id=1, NOT hardcoded) and `needsProviderSetup` (true iff no member_credentials row for id=1).
|
||
- Test: OIDC path → response `user` includes `isAdmin` (from the resolved users row) and `needsProviderSetup` (member_credentials existence for that user id).
|
||
- Test: a user WITH a member_credentials row → needsProviderSetup=false; a user WITHOUT one → needsProviderSetup=true.
|
||
</behavior>
|
||
<action>
|
||
In `apps/api/src/routes/me.ts`, extend BOTH response paths to include `isAdmin` and `needsProviderSetup`. The dev-bypass path currently short-circuits without a DB lookup — it MUST now query `users.isAdmin` for id=1 (same lookup as requireAdmin) rather than hardcoding, and compute `needsProviderSetup` via the member_credentials existence check (per 10-PATTERNS.md excerpt). The OIDC path uses the resolved user's isAdmin + the same member_credentials existence check. Add the `eq`/`db`/`users`/`memberCredentials` imports as needed. Do NOT add the self-service `/api/me/credential` POST endpoint here — that belongs to Plan 03. Write the three test cases in `apps/api/tests/routes/me.test.ts` FIRST, confirm RED, implement to GREEN.
|
||
</action>
|
||
<verify>
|
||
<automated>cd /home/luc/Projects/familysync && pnpm --filter @familysync/api test -- me 2>&1 | tail -15</automated>
|
||
</verify>
|
||
<acceptance_criteria>
|
||
- `GET /api/me` response `user` object contains `isAdmin` (boolean) and `needsProviderSetup` (boolean) on BOTH the dev-bypass and OIDC paths.
|
||
- The dev-bypass path's isAdmin is read from the DB (`grep -q "memberCredentials" apps/api/src/routes/me.ts` and isAdmin not hardcoded `true`/`false` in the dev path).
|
||
- needsProviderSetup is true exactly when no member_credentials row exists for the user.
|
||
- No `/api/me/credential` POST route added in this plan.
|
||
- `pnpm --filter @familysync/api test -- me` passes all cases.
|
||
</acceptance_criteria>
|
||
<done>/api/me exposes isAdmin + needsProviderSetup on both paths, DB-backed, tests green.</done>
|
||
</task>
|
||
|
||
</tasks>
|
||
|
||
<artifacts_this_phase_produces>
|
||
New symbols/files created by this plan (excluded from drift verification):
|
||
- `apps/api/src/lib/requireAdmin.ts` exporting `requireAdmin` (MiddlewareHandler)
|
||
- `apps/api/tests/lib/requireAdmin.test.ts`
|
||
- first-login-wins `is_admin` bootstrap branch in `upsertUser` (`apps/api/src/auth/user.ts`)
|
||
- `isAdmin` + `needsProviderSetup` fields on the `/api/me` response (`apps/api/src/routes/me.ts`)
|
||
- test additions in `apps/api/tests/auth/user.test.ts` and `apps/api/tests/routes/me.test.ts`
|
||
</artifacts_this_phase_produces>
|
||
|
||
<threat_model>
|
||
## Trust Boundaries
|
||
|
||
| Boundary | Description |
|
||
|----------|-------------|
|
||
| client → /api/admin/* (via requireAdmin) | untrusted authenticated request must be proven admin server-side before any admin handler runs |
|
||
| /api/me → browser | the isAdmin flag crosses to the client for UX gating only; never the security boundary |
|
||
|
||
## STRIDE Threat Register
|
||
|
||
| Threat ID | Category | Component | Disposition | Mitigation Plan |
|
||
|-----------|----------|-----------|-------------|-----------------|
|
||
| T-10-04 | Elevation of Privilege | non-admin invoking admin-gated logic | mitigate | requireAdmin reads users.is_admin from the DB and 403s non-admins (Task 1); decision never trusts a client-supplied or context-attached isAdmin claim, only the DB row |
|
||
| T-10-05 | Elevation of Privilege | DEV_AUTH_BYPASS bypasses the role check | mitigate | requireAdmin and /api/me both DB-lookup users.is_admin even on the bypass path (the bypass skips OIDC, not the DB check); the bypass admin row comes only from the guarded dev seed (Plan 01 Task 3) |
|
||
| T-10-06 | Spoofing/EoP | client trusting its own isAdmin to reach admin features | mitigate | isAdmin on /api/me is documented and used as UX-only; the server-side 403 (requireAdmin, Plan 03) is the real boundary on every /api/admin/* request |
|
||
| T-10-07 | Information Disclosure | logging the resolved user / claims | mitigate | requireAdmin and me.ts must not console.log the user object or any credential (acceptance grep) |
|
||
| T-10-SC | Tampering | npm/pip/cargo installs | mitigate | No new packages this phase (RESEARCH Package Legitimacy Audit); no install task |
|
||
</threat_model>
|
||
|
||
<verification>
|
||
- `pnpm --filter @familysync/api test -- requireAdmin && pnpm --filter @familysync/api test -- user && pnpm --filter @familysync/api test -- me` all pass.
|
||
- `pnpm --filter @familysync/api exec tsc --noEmit` passes (per [[vitest-passes-tsc-fails]], run tsc separately — vitest stays green on type errors).
|
||
- `grep -q "users.isAdmin" apps/api/src/lib/requireAdmin.ts`.
|
||
</verification>
|
||
|
||
<success_criteria>
|
||
- requireAdmin 403s authenticated non-admins, admits admins, DB-backed (ADMIN-03 server enforcement; supports Success Criteria 1 & 4).
|
||
- First-login-wins writes is_admin on first insert, role-agnostic / member-count-agnostic (Success Criterion 4).
|
||
- /api/me exposes isAdmin + needsProviderSetup on both paths for downstream PWA gating + self-service.
|
||
</success_criteria>
|
||
|
||
<output>
|
||
Create `.planning/phases/10-admin-role-settings/10-02-SUMMARY.md` when done.
|
||
</output>
|