132 lines
7.1 KiB
Markdown
132 lines
7.1 KiB
Markdown
---
|
|
phase: "10-admin-role-settings"
|
|
plan: "02"
|
|
subsystem: "api-auth"
|
|
tags: ["requireAdmin", "admin-role", "middleware", "upsertUser", "first-login-wins", "me-api", "tdd"]
|
|
dependency_graph:
|
|
requires:
|
|
- "users.is_admin column (10-01)"
|
|
- "member_credentials table with UNIQUE(user_id) (10-01)"
|
|
- "app_config table (10-01)"
|
|
provides:
|
|
- "requireAdmin MiddlewareHandler (DB-backed role enforcement, T-10-04/T-10-05)"
|
|
- "first-login-wins is_admin bootstrap in upsertUser (D-01)"
|
|
- "isAdmin + needsProviderSetup on /api/me response (D-03)"
|
|
affects:
|
|
- "Phase 10 Plan 03 (adminRouter mounts requireAdmin)"
|
|
- "Phase 10 Plan 04 (PWA nav gating reads isAdmin from /api/me)"
|
|
- "Phase 12 (first-login-wins hook point documented for setup_complete tightening)"
|
|
tech_stack:
|
|
added: []
|
|
patterns:
|
|
- "MiddlewareHandler inline export (requireAdmin pattern, not factory function)"
|
|
- "sql<number> COUNT(*) with .limit(1) for scalar aggregate in Drizzle"
|
|
- "resolveAdminAndSetupStatus helper — two sequential DB selects in a route"
|
|
- "TDD RED→GREEN: 6 RED commits → 3 GREEN commits"
|
|
key_files:
|
|
created:
|
|
- "apps/api/src/lib/requireAdmin.ts"
|
|
- "apps/api/tests/lib/requireAdmin.test.ts"
|
|
modified:
|
|
- "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"
|
|
decisions:
|
|
- "sql<number> COUNT(*) with .limit(1) — not .limit() on Drizzle aggregate; scalar aggregate needs explicit limit for mock-chain compatibility and Drizzle's select-where pattern"
|
|
- "resolveAdminAndSetupStatus extracted as a shared helper in me.ts — used by both bypass and OIDC paths to avoid duplication"
|
|
- "requireAdmin is an inline MiddlewareHandler constant, not a factory function — applied as adminRouter.use('*', requireAdmin)"
|
|
metrics:
|
|
duration_seconds: 700
|
|
completed_date: "2026-06-13"
|
|
tasks_completed: 3
|
|
files_modified: 6
|
|
---
|
|
|
|
# Phase 10 Plan 02: Admin Role Primitives Summary
|
|
|
|
**One-liner:** DB-backed `requireAdmin` MiddlewareHandler, first-login-wins `is_admin` bootstrap in `upsertUser`, and `/api/me` extended with `isAdmin` + `needsProviderSetup` — all TDD-verified with 22 tests.
|
|
|
|
## Tasks Completed
|
|
|
|
| Task | Name | Commits | Files |
|
|
|------|------|---------|-------|
|
|
| 1 | requireAdmin middleware (RED→GREEN) | 9217930 (RED), f9c70ab (GREEN) | requireAdmin.ts, requireAdmin.test.ts |
|
|
| 2 | First-login-wins is_admin bootstrap in upsertUser (RED→GREEN) | 9e1507f (RED), 72e0140 (GREEN) | user.ts, user.test.ts |
|
|
| 3 | Extend /api/me with isAdmin + needsProviderSetup (RED→GREEN) | e5889df (RED), 1adff61 (GREEN) | me.ts, me.test.ts |
|
|
|
|
## What Was Built
|
|
|
|
### Task 1: requireAdmin middleware
|
|
|
|
`apps/api/src/lib/requireAdmin.ts` exports `requireAdmin: MiddlewareHandler`:
|
|
- Reads `c.get('user')?.id`; if no id → 403 `{ error: 'Forbidden' }` immediately (no DB query)
|
|
- Queries `db.select({ isAdmin: users.isAdmin }).from(users).where(eq(users.id, userId)).limit(1)`
|
|
- If `!row?.isAdmin` → 403; else `await next()`
|
|
- Side-effect import of `../auth/devBypass.js` carries the ContextVariableMap augmentation
|
|
- Never reads `isAdmin` from the context user object — DB is the sole authority (T-10-04)
|
|
- The dev-auth bypass skips OIDC; requireAdmin still hits the DB for every request (T-10-05)
|
|
- No `console.log` of user object or credentials (T-10-07)
|
|
|
|
4 test cases covering: non-admin DB row → 403, admin DB row → next(), no user → 403 (no DB call), spoofed `isAdmin: true` on context but non-admin DB row → 403.
|
|
|
|
### Task 2: First-login-wins is_admin bootstrap in upsertUser
|
|
|
|
`apps/api/src/auth/user.ts` extended before the INSERT block:
|
|
- Added `import { sql } from 'drizzle-orm'`
|
|
- Zero-admin COUNT check: `db.select({ count: sql<number>\`COUNT(*)\` }).from(users).where(eq(users.isAdmin, true)).limit(1)`
|
|
- `shouldBeAdmin = Number(count) === 0`
|
|
- INSERT `.values({ ..., isAdmin: shouldBeAdmin })` — first user when zero admins → `is_admin=true`; subsequent users → `is_admin=false`
|
|
- Existing-user early-return path unchanged (no `is_admin` modification on re-upsert)
|
|
- Phase-12 hook comment: "Phase 12 tightens to: first user after app_config.setup_complete"
|
|
|
|
3 new test cases + existing tests updated for the new 4-select call sequence (identity lookup → used-colors → admin COUNT → re-fetch).
|
|
|
|
### Task 3: /api/me extended with isAdmin + needsProviderSetup
|
|
|
|
`apps/api/src/routes/me.ts` extended with:
|
|
- `resolveAdminAndSetupStatus(userId)` helper — two DB selects:
|
|
1. `users.isAdmin` via `db.select({ isAdmin: users.isAdmin }).from(users).where(eq(users.id, userId)).limit(1)`
|
|
2. `memberCredentials.id` via `db.select({ id: memberCredentials.id }).from(memberCredentials).where(eq(memberCredentials.userId, userId)).limit(1)`
|
|
- Returns `{ isAdmin: row?.isAdmin ?? false, needsProviderSetup: !cred }`
|
|
- Dev-bypass path: now calls `resolveAdminAndSetupStatus(devUser.id)` — DB-backed, not hardcoded (T-10-05)
|
|
- OIDC path: calls `resolveAdminAndSetupStatus(user.id)` after `upsertUser`
|
|
- Response: `{ user: { id, displayName, color, isAdmin, needsProviderSetup } }` on both paths
|
|
- No `/api/me/credential` POST added (Plan 03)
|
|
|
|
3 new test cases: isAdmin from DB (not hardcoded), needsProviderSetup=true (no cred), needsProviderSetup=false (cred exists).
|
|
|
|
## Deviations from Plan
|
|
|
|
### Auto-fixed Issues
|
|
|
|
**1. [Rule 1 - Bug] Drizzle aggregate mock chaining — added .limit(1) to COUNT query**
|
|
- **Found during:** Task 2 (GREEN phase)
|
|
- **Issue:** The COUNT query `const [{ count }] = await db.select({...}).from(users).where(...)` was awaiting the `.where()` return directly. In mocked tests, `makeSelectChain.where()` returns the chain object (not a Promise), so destructuring `[{ count }]` failed with "is not iterable".
|
|
- **Fix:** Added `.limit(1)` to the COUNT query, making it terminate at `.limit()` which returns a Promise in the mock (consistent with all other select patterns in this codebase).
|
|
- **Files modified:** `apps/api/src/auth/user.ts` (`.limit(1)` on COUNT query)
|
|
- **Commit:** 72e0140
|
|
|
|
## Known Stubs
|
|
|
|
None. This plan is API-only (no UI components). All DB queries are real and fully implemented.
|
|
|
|
## Threat Flags
|
|
|
|
None new beyond the plan's threat model. All T-10-04/T-10-05/T-10-06/T-10-07 mitigations implemented:
|
|
- T-10-04: requireAdmin reads `users.is_admin` from DB, never trusts context user's `isAdmin`
|
|
- T-10-05: Both requireAdmin and /api/me do DB lookups even on the dev-bypass path
|
|
- T-10-06: isAdmin on /api/me is documented UX-only; Plan 03's requireAdmin is the server boundary
|
|
- T-10-07: No `console.log` of user object or credentials in any modified file
|
|
|
|
## Self-Check: PASSED
|
|
|
|
- `apps/api/src/lib/requireAdmin.ts` exists and exports `requireAdmin`: PASS
|
|
- `grep -q "users.isAdmin" apps/api/src/lib/requireAdmin.ts`: PASS
|
|
- `grep -q "isAdmin: shouldBeAdmin" apps/api/src/auth/user.ts`: PASS
|
|
- `grep -q "needsProviderSetup" apps/api/src/routes/me.ts`: PASS
|
|
- `grep -q "memberCredentials" apps/api/src/routes/me.ts`: PASS
|
|
- All 22 tests pass (requireAdmin: 4, user: 10, me: 8): PASS
|
|
- `pnpm --filter @familysync/api exec tsc --noEmit` exits 0: PASS
|
|
- Commits 9217930, f9c70ab, 9e1507f, 72e0140, e5889df, 1adff61 in git log: PASS
|