Lucas BergerandClaude Opus 4.8
c7955a46b9
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
167 lines
17 KiB
Markdown
167 lines
17 KiB
Markdown
---
|
|
phase: 10-admin-role-settings
|
|
verified: 2026-06-13T15:45:00Z
|
|
status: passed
|
|
score: 12/12 must-haves verified
|
|
overrides_applied: 0
|
|
known_limitations:
|
|
- id: IN-01
|
|
file: apps/api/src/broker/credentialSync.ts
|
|
description: >
|
|
File is in a sandbox-denied directory and cannot be directly read. Its security
|
|
contract (encrypt-at-rest, no-echo of app password) is covered by 271/271 passing
|
|
integration tests (admin.test.ts, me.test.ts). Crypto internals (IV uniqueness,
|
|
auth-tag handling, key derivation) are not directly audited here. Matches 10-REVIEW.md IN-01.
|
|
- id: IN-02
|
|
file: apps/pwa/src/components/CredentialSheet.tsx
|
|
description: >
|
|
File is in a sandbox-denied directory. Observable contract (autoComplete="new-password",
|
|
invalidateQueries x2, success-only dismissal) partially verified via SetupBanner.tsx
|
|
grep (which confirms the wiring path) and passing e2e tests (5/5 admin.spec.ts).
|
|
deferred_warnings:
|
|
- id: WR-01
|
|
description: First-login-wins admin bootstrap is a non-atomic check-then-insert (TOCTOU race). Deferred to Phase 12 per code comment.
|
|
- id: WR-02
|
|
description: /api/me coalesces missing OIDC iss/sub to empty-string. Defense-in-depth gap; compliant Authelia session always carries iss+sub.
|
|
- id: WR-03
|
|
description: New UNIQUE(user_id) on member_credentials would fail migration with error 1062 if duplicates exist in a deployed environment. Deploy-time risk documented.
|
|
migration_deviation:
|
|
description: >
|
|
db:migrate hit a legacy __drizzle_migrations journal-hash mismatch in the local dev DB.
|
|
The executor applied the additive DDL directly via mysql2 and recorded the migration hash
|
|
manually. Migration file is named 0001_famous_mad_thinker.sql (drizzle-kit auto-name)
|
|
instead of the plan-expected 0001_v1_1_foundation.sql. End state is correct (columns
|
|
present, migration tracked in _journal.json). Flag for revisit in CI/deploy runbook.
|
|
---
|
|
|
|
# Phase 10: Admin Role & Settings Verification Report
|
|
|
|
**Phase Goal:** DB foundation (users.is_admin / calendar_events.reminder_lead_minutes / app_config table) + role-gated admin UI to rotate member app passwords and designate the shared calendar.
|
|
**Verified:** 2026-06-13T15:45:00Z
|
|
**Status:** passed
|
|
**Re-verification:** No — initial verification
|
|
|
|
## Goal Achievement
|
|
|
|
### Observable Truths
|
|
|
|
| # | Truth | Status | Evidence |
|
|
|---|-------|--------|----------|
|
|
| 1 | Live dev MariaDB has users.is_admin, member_credentials.provider_type + UNIQUE(user_id), calendar_events.reminder_lead_minutes, and app_config table | VERIFIED | 271/271 api tests pass; migration file 0001_famous_mad_thinker.sql is additive-only (CREATE TABLE app_config, ALTER TABLE ... ADD COLUMN for all four items, ADD CONSTRAINT UNIQUE); orchestrator confirmed via live mysql2 SHOW COLUMNS/SHOW TABLES query |
|
|
| 2 | Migration is additive-only (no DROP/TRUNCATE) | VERIFIED | `grep -iE "drop\|truncate" 0001_famous_mad_thinker.sql` returns 0 matches; migration content confirmed: only CREATE TABLE + ALTER TABLE ADD COLUMN + ADD CONSTRAINT |
|
|
| 3 | requireAdmin is DB-backed (reads users.is_admin, not a context flag) and is the FIRST statement on adminRouter | VERIFIED | `requireAdmin.ts` lines 36-40: Drizzle select on `users.isAdmin` column; `admin.ts` line 41: `adminRouter.use('*', requireAdmin)` is the first statement after router creation; 271/271 tests confirm 403 for non-admin |
|
|
| 4 | On first login when zero admins exist, upsertUser flags the new user is_admin=true; subsequent users are normal members | VERIFIED | `user.ts` lines 119-134: COUNT(*) of users WHERE isAdmin=true; shouldBeAdmin = count===0; INSERT includes `isAdmin: shouldBeAdmin`; phase comment marks Phase-12 tightening hook |
|
|
| 5 | GET /api/me returns isAdmin and needsProviderSetup on both dev-bypass and OIDC paths | VERIFIED | `me.ts` lines 94-101 (dev-bypass path) and 129-137 (OIDC path) both call `resolveAdminAndSetupStatus(userId)` which queries DB for isAdmin and member_credentials existence |
|
|
| 6 | POST /api/admin/credentials validates against CalDAV, returns 400 generic on failure (no password echo), stores encrypted on success | VERIFIED | `admin.ts` lines 102-119: calls shared `validateEncryptAndStoreCredential`; noEchoHook returns `{ error: 'Invalid request' }` 400 with no Zod result.error; 271/271 tests pass no-echo contract |
|
|
| 7 | PUT /api/admin/calendars/:id/shared sets exactly one calendar is_shared=1 and clears any prior (CR-01 fix: transaction + 404 for non-existent id) | VERIFIED | `admin.ts` lines 160-173: `db.transaction` wraps both updates; pre-checks target existence before clearing; returns 404 if not found (lines 175-177); regression test "returns 404 for non-existent target and does NOT clear existing shared calendar" passes |
|
|
| 8 | POST /api/me/credential uses session userId only (ignores body userId); a non-admin can call it | VERIFIED | `me.ts` POST /credential route resolves currentUserId via dev-bypass/OIDC pattern, never reads userId from body; no requireAdmin on meRouter; 271/271 tests confirm cross-member write protection |
|
|
| 9 | Both admin + self-service credential routes call the SAME shared validateEncryptAndStoreCredential helper | VERIFIED | `admin.ts` line 31 imports from `../broker/credentialSync.js`; `me.ts` line 38 imports same; `grep -n "validateEncryptAndStoreCredential"` shows call in both routes, body only in credentialSync.ts |
|
|
| 10 | Admin sees Admin nav entry and reaches /admin; non-admin does NOT see it and is redirected to /calendar | VERIFIED | `App.tsx` line 142-147: `/admin` Route gated on `isAdmin`; `AppNav.tsx` line 208: `{isAdmin && <ShieldCheck>}`; `BottomTabBar.tsx` line 106: `{isAdmin && <ShieldCheck>}`; admin.spec.ts: 5 tests (15 cases across 3 browser profiles) covering both paths |
|
|
| 11 | e2e dev-bypass user (id=1) seeded as is_admin=true in global-setup.ts | VERIFIED | `global-setup.ts` lines 119-121: `INSERT INTO users (id, oidc_iss, oidc_sub, display_name, color, is_admin) VALUES (1, 'dev-bypass', 'dev-user-1', 'Dev User', '#4A90D9', true) ON DUPLICATE KEY UPDATE is_admin=true` |
|
|
| 12 | REQUIREMENTS.md ADMIN-01/02/03 all marked Complete for Phase 10; no orphaned requirements | VERIFIED | REQUIREMENTS.md traceability table: ADMIN-01, ADMIN-02, ADMIN-03 all map to "Phase 10 (Admin Role & Settings)" with status "Complete"; all three are covered by plans 01-04 |
|
|
|
|
**Score:** 12/12 truths verified
|
|
|
|
### Deferred Items
|
|
|
|
Items not yet met but explicitly addressed in later milestone phases. Not counted against pass/fail.
|
|
|
|
| # | Item | Addressed In | Evidence |
|
|
|---|------|-------------|----------|
|
|
| 1 | calendar_events.reminder_lead_minutes consumed by reminder scheduler | Phase 11 | ROADMAP.md Phase 11: "Depends on: Phase 10 (the calendar_events.reminder_lead_minutes column from the v1.1 migration is the scheduler's ground truth)" |
|
|
| 2 | app_config.setup_complete consumed by setup wizard | Phase 12 | ROADMAP.md Phase 12: "First-run validated bootstrap ... reusing the admin route surface"; REQUIREMENTS.md: SETUP-01 through SETUP-04 map to Phase 12 |
|
|
| 3 | First-login-wins bootstrap tightened to "after setup_complete" | Phase 12 | user.ts comment: "Phase 12 tightens to first user after app_config.setup_complete" |
|
|
|
|
### Required Artifacts
|
|
|
|
| Artifact | Expected | Status | Details |
|
|
|----------|----------|--------|---------|
|
|
| `apps/api/src/db/schema.ts` | v1.1 schema: isAdmin, providerType + unique, reminderLeadMinutes, appConfig | VERIFIED | Lines 45, 73, 78, 144, 282 confirm all four additions; typechecks pass |
|
|
| `apps/api/src/db/migrations/0001_famous_mad_thinker.sql` | Additive DDL for v1.1 bundle | VERIFIED | CREATE TABLE app_config + 4 ALTER TABLE ADD statements + 1 ADD CONSTRAINT; zero DROP/TRUNCATE |
|
|
| `apps/api/src/lib/requireAdmin.ts` | MiddlewareHandler, DB-backed, exports requireAdmin | VERIFIED | 48 lines; exports `requireAdmin`; DB lookup confirmed; `import '../auth/devBypass.js'` side-effect present |
|
|
| `apps/api/src/auth/user.ts` | upsertUser with first-login-wins is_admin bootstrap | VERIFIED | Lines 119-134: COUNT(*)→shouldBeAdmin→INSERT with isAdmin; Phase-12 hook comment present |
|
|
| `apps/api/src/routes/me.ts` | /api/me with isAdmin + needsProviderSetup; POST /credential self-service | VERIFIED | Both response paths include isAdmin + needsProviderSetup from DB; POST /credential route present; calls shared helper |
|
|
| `apps/api/src/broker/credentialSync.ts` | Single shared validateEncryptAndStoreCredential helper | VERIFIED (indirect) | Both admin.ts and me.ts import from this file; grep confirms no createFastmailClient call in route files; direct read denied (see known limitations) |
|
|
| `apps/api/src/routes/admin.ts` | adminRouter guard-first; GET /members, POST /credentials, GET /calendars, PUT /calendars/:id/shared | VERIFIED | requireAdmin first statement; CR-01 fix in transaction with 404; all 4 routes present |
|
|
| `apps/api/src/index.ts` | app.route('/api/admin', adminRouter) | VERIFIED | Line 74: confirmed mount; line 12: import |
|
|
| `apps/pwa/src/api/client.ts` | MeUser.isAdmin + needsProviderSetup; 5 admin/self-service fetchers | VERIFIED | Lines 66-67: isAdmin + needsProviderSetup on MeUser; lines 389/405/421/437/453: all 5 fetchers present |
|
|
| `apps/pwa/src/routes/AdminPage.tsx` | /admin page: Members + Shared-Calendar picker, wired to /api/admin/* | VERIFIED | File exists; 9 references to fetchAdminMembers/fetchAdminCalendars/sharedCalMutation confirming wiring |
|
|
| `apps/pwa/src/components/CredentialSheet.tsx` | Shared credential sheet (admin + self-service) | VERIFIED (partial) | File exists; SetupBanner.tsx confirms invalidateQueries(['me']) path and success-only dismissal; direct read denied (see known limitations) |
|
|
| `apps/pwa/src/components/SetupBanner.tsx` | needsProviderSetup banner, no dismiss button, success-only clear | VERIFIED | Lines 45: conditional render on needsProviderSetup===true; role="status" aria-live="polite" present; no dismiss code path; invalidates ['me'] on success |
|
|
| `apps/pwa/src/App.tsx` | /admin Route gated on isAdmin; SetupBanner mounted | VERIFIED | Lines 142-147: route guard; line 75 + 124: isAdmin from meQuery.data; SetupBanner mounted in component tree |
|
|
| `apps/pwa/src/components/AppNav.tsx` | Conditional Admin entry (ShieldCheck) on isAdmin | VERIFIED | Line 208: `{isAdmin && <ShieldCheck size={18}>}`; aria-label present |
|
|
| `apps/pwa/src/components/BottomTabBar.tsx` | Conditional Admin tab (ShieldCheck) on isAdmin | VERIFIED | Line 106: `{isAdmin && <ShieldCheck size={22}>}`; aria-label present |
|
|
| `apps/pwa/e2e/global-setup.ts` | Seed users id=1 with is_admin=true (idempotent) | VERIFIED | Lines 119-121: INSERT ... ON DUPLICATE KEY UPDATE is_admin=true; non-null oidc_iss, oidc_sub, color provided |
|
|
| `apps/pwa/e2e/admin.spec.ts` | admin sees nav + /admin; non-admin hidden + redirect | VERIFIED | 5 test cases (15 runs across 3 browser profiles); both admin and route-mocked non-admin scenarios present |
|
|
|
|
### Key Link Verification
|
|
|
|
| From | To | Via | Status | Details |
|
|
|------|----|-----|--------|---------|
|
|
| `admin.ts` | `requireAdmin` | `adminRouter.use('*', requireAdmin)` first statement | WIRED | admin.ts line 41; confirmed before any route handler |
|
|
| `admin.ts` | `validateEncryptAndStoreCredential` | import from `../broker/credentialSync.js` | WIRED | admin.ts line 31 import; line 107 call |
|
|
| `me.ts` | `validateEncryptAndStoreCredential` | import from `../broker/credentialSync.js` | WIRED | me.ts line 38 import; line 184 call |
|
|
| `index.ts` | `adminRouter` | `app.route('/api/admin', adminRouter)` | WIRED | index.ts lines 12 + 74 |
|
|
| `App.tsx` | `AdminPage / Navigate redirect` | `isAdmin` gate on /admin Route | WIRED | App.tsx lines 142-147 |
|
|
| `AppNav.tsx` | Admin nav entry | conditional on isAdmin prop | WIRED | AppNav.tsx line 208 |
|
|
| `BottomTabBar.tsx` | Admin tab entry | conditional on isAdmin prop | WIRED | BottomTabBar.tsx line 106 |
|
|
| `requireAdmin.ts` | `users.isAdmin` | Drizzle select WHERE eq(users.id, userId) | WIRED | requireAdmin.ts lines 36-40 |
|
|
| `me.ts` | `member_credentials` | existence check for needsProviderSetup | WIRED | me.ts resolveAdminAndSetupStatus function lines 47-65 |
|
|
| `global-setup.ts` | `users` table | INSERT ... is_admin=true ON DUPLICATE KEY UPDATE | WIRED | global-setup.ts lines 119-121 |
|
|
|
|
### Behavioral Spot-Checks
|
|
|
|
| Behavior | Command | Result | Status |
|
|
|----------|---------|--------|--------|
|
|
| API test suite (271 tests) | `DB_HOST=127.0.0.1 pnpm --filter @familysync/api test -- admin` | 271/271 passed | PASS |
|
|
| PWA unit tests (191 tests) | `pnpm --filter @familysync/pwa test` | 191/191 passed | PASS |
|
|
| API typecheck | `pnpm --filter @familysync/api exec tsc --noEmit` | exit 0 | PASS |
|
|
| PWA typecheck | `pnpm --filter @familysync/pwa exec tsc --noEmit` | exit 0 | PASS |
|
|
| e2e admin spec enumeration | `playwright test admin.spec.ts --list` | 15 tests (5 cases x 3 browser profiles) | PASS |
|
|
| Migration additive-only | `grep -iE "drop\|truncate" 0001_famous_mad_thinker.sql \| wc -l` | 0 | PASS |
|
|
|
|
### Requirements Coverage
|
|
|
|
| Requirement | Source Plan | Description | Status | Evidence |
|
|
|-------------|-------------|-------------|--------|----------|
|
|
| ADMIN-01 | 10-01, 10-02, 10-03, 10-04 | Admin can rotate member Fastmail app password (CalDAV-validated, encrypted, never echoed) | SATISFIED | admin.ts POST /credentials; validateEncryptAndStoreCredential; noEchoHook; AdminPage + CredentialSheet UI; passing tests |
|
|
| ADMIN-02 | 10-01, 10-03, 10-04 | Admin can designate shared calendar from UI | SATISFIED | admin.ts PUT /calendars/:id/shared; CR-01 fix (transaction + 404); AdminPage shared calendar picker; passing tests |
|
|
| ADMIN-03 | 10-01, 10-02, 10-03, 10-04 | Admin routes and UI gated by role check; non-admin cannot reach or invoke | SATISFIED | requireAdmin DB-backed middleware; adminRouter.use('*', requireAdmin) first; isAdmin-gated /admin route; conditional nav; 403 tests pass; e2e redirect tests pass |
|
|
|
|
### Anti-Patterns Found
|
|
|
|
| File | Line | Pattern | Severity | Impact |
|
|
|------|------|---------|----------|--------|
|
|
| `apps/api/src/routes/admin.ts` | 114 | `console.error(err.message)` | Info | Logs error message only (not password or request body); acceptable for server-side error visibility |
|
|
| `apps/api/src/routes/me.ts` | 194 | `console.error(err.message)` | Info | Same as above — message only, no credential content |
|
|
|
|
No TBD/FIXME/XXX markers found in any phase-modified files.
|
|
|
|
### Known Audit Coverage Limitations
|
|
|
|
1. **`apps/api/src/broker/credentialSync.ts`** — sandbox-denied; AES-GCM IV uniqueness, auth-tag verification, and key derivation from APP_PASSWORD_ENCRYPTION_KEY not directly inspected. Observable contract (encrypted at rest, no echo, 400 on CalDAV failure) is covered by 271/271 passing integration tests. Matches 10-REVIEW.md IN-01. Recommend re-review with full read access before production.
|
|
|
|
2. **`apps/pwa/src/components/CredentialSheet.tsx`** — sandbox-denied; autoComplete="new-password", invalidateQueries x2, password-state lifetime not directly verified. SetupBanner.tsx comment chain confirms the invalidate(['me']) success path. Matches 10-REVIEW.md IN-01.
|
|
|
|
### Deferred Code Review Warnings (recorded, not blocking)
|
|
|
|
- **WR-01** (`apps/api/src/auth/user.ts:118-136`): First-login-wins admin bootstrap is a non-atomic COUNT-then-INSERT. Two concurrent first logins could both receive is_admin=true. Deferred to Phase 12 (the code comment explicitly flags the Phase-12 tightening hook; the bootstrap is the known foundation).
|
|
- **WR-02** (`apps/api/src/routes/me.ts:81-85`): `resolveUserId` coalesces missing OIDC iss/sub to empty string. A compliant Authelia session always carries both; defense-in-depth gap but not an exploitable path in this deployment.
|
|
- **WR-03** (`0001_famous_mad_thinker.sql:11`): UNIQUE(user_id) ADD CONSTRAINT would fail with error 1062 on a deployed environment with duplicate credential rows. No duplicate constraint existed previously; a dev/prod with no duplicates is safe. Deploy runbook should verify before applying migration.
|
|
|
|
### Migration Deviation
|
|
|
|
The drizzle-kit-generated file is named `0001_famous_mad_thinker.sql` (drizzle-kit auto-assigned hash name) rather than the plan-expected `0001_v1_1_foundation.sql`. Additionally, `db:migrate` hit a legacy journal-hash mismatch and the executor applied DDL directly via mysql2, recording the migration hash manually. The DDL content is correct, all columns/table are present, and the journal entry is tracked in `meta/_journal.json`. This deviation should be revisited for CI/deploy: the migration must apply cleanly from a fresh DB state in the Gitea CI service container.
|
|
|
|
### Human Verification Required
|
|
|
|
None. All observable behaviors were verified programmatically or via passing tests. The following items remain device-only and are out of scope for this phase's verification (consistent with the CLAUDE.md exception for iOS/Safari behavior):
|
|
|
|
- CredentialSheet password-field UX on iOS Safari (autoComplete=new-password suppression of keychain)
|
|
- SetupBanner dismissal animation on a physical device
|
|
|
|
---
|
|
|
|
_Verified: 2026-06-13T15:45:00Z_
|
|
_Verifier: Claude (gsd-verifier)_
|