Files
2026-06-18 22:21:38 -04:00

16 KiB
Raw Permalink Blame History

phase, plan, type, wave, depends_on, files_modified, autonomous, requirements, must_haves
phase plan type wave depends_on files_modified autonomous requirements must_haves
10-admin-role-settings 02 tdd 2
10-01
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
true
ADMIN-03
truths artifacts key_links
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
path provides exports min_lines
apps/api/src/lib/requireAdmin.ts MiddlewareHandler that reads c.get('user').id, looks up users.is_admin in the DB, 403s non-admins
requireAdmin
15
path provides contains
apps/api/src/auth/user.ts upsertUser extended with first-login-wins is_admin bootstrap (zero-admins → first user is admin) isAdmin
path provides contains
apps/api/src/routes/me.ts /api/me response extended with isAdmin + needsProviderSetup (both dev-bypass and OIDC paths) needsProviderSetup
from to via pattern
apps/api/src/lib/requireAdmin.ts users.is_admin Drizzle select where eq(users.id, userId) users.isAdmin
from to via pattern
apps/api/src/routes/me.ts member_credentials needsProviderSetup = no member_credentials row for the user memberCredentials
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.

<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/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 Task 1: requireAdmin middleware (RED→GREEN→REFACTOR) apps/api/src/lib/requireAdmin.ts, apps/api/tests/lib/requireAdmin.test.ts - 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 - 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). 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. cd /home/luc/Projects/familysync && pnpm --filter @familysync/api test -- requireAdmin 2>&1 | tail -15 - `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. requireAdmin guard exists, DB-backed, 403s non-admins, tests green. Task 2: First-login-wins is_admin bootstrap in upsertUser (RED→GREEN→REFACTOR) apps/api/src/auth/user.ts, apps/api/tests/auth/user.test.ts - apps/api/src/auth/user.ts (the file being modified — the full `upsertUser` function lines 76128: the existing-row early-return path, the color assignment, the INSERT `.values({...}).$returningId()` block lines 112122) - 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") - 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). 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\`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. cd /home/luc/Projects/familysync && pnpm --filter @familysync/api test -- user 2>&1 | tail -15 - 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. First-login-wins bootstrap writes is_admin on first insert, member-count-agnostic, Phase-12-safe, tests green. Task 3: Extend /api/me with isAdmin + needsProviderSetup (RED→GREEN→REFACTOR) apps/api/src/routes/me.ts, apps/api/tests/routes/me.test.ts - apps/api/src/routes/me.ts (the file being modified — the dev-bypass short-circuit lines 3143 returning `{ user: { id, displayName, color } }`, the OIDC path lines 6075 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) - 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. 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. cd /home/luc/Projects/familysync && pnpm --filter @familysync/api test -- me 2>&1 | tail -15 - `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. /api/me exposes isAdmin + needsProviderSetup on both paths, DB-backed, tests green.

<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>
- `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`.

<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>
Create `.planning/phases/10-admin-role-settings/10-02-SUMMARY.md` when done.