From 94c62e213e6b51af4dc84e95f25e9e5dd1e1c6f4 Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Sat, 13 Jun 2026 15:46:09 -0400 Subject: [PATCH] docs(phase-10): complete phase execution --- .planning/ROADMAP.md | 2 +- .planning/STATE.md | 9 +- .../10-admin-role-settings/10-VERIFICATION.md | 166 ++++++++++++++++++ 3 files changed, 172 insertions(+), 5 deletions(-) create mode 100644 .planning/phases/10-admin-role-settings/10-VERIFICATION.md diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index 9a9efbd..d90819a 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -369,7 +369,7 @@ Plans: | 7. Mobile Test Harness | v1.1 | 4/4 | Complete | 2026-06-11 | | 8. Gitea CI | v1.1 | 4/4 | Complete | 2026-06-11 | | 9. Faster Write-Back | v1.1 | 2/2 | Complete | 2026-06-12 | -| 10. Admin Role & Settings | v1.1 | 4/4 | Complete | 2026-06-13 | +| 10. Admin Role & Settings | v1.1 | 4/4 | Complete | 2026-06-13 | | 11. Per-Event Reminders | v1.1 | 0/? | Not started | - | | 12. Initial Setup Wizard | v1.1 | 0/? | Not started | - | | 13. Real Lint Gate (ESLint) | v1.1 | 3/3 | Complete | 2026-06-12 | diff --git a/.planning/STATE.md b/.planning/STATE.md index e410229..29bcb3a 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -4,7 +4,7 @@ milestone: v1.1 milestone_name: Operability & Polish status: verifying stopped_at: "Completed Phase 10 Plan 02 (admin role primitives: requireAdmin, upsertUser is_admin, /api/me isAdmin+needsProviderSetup)" -last_updated: "2026-06-13T19:27:40.449Z" +last_updated: "2026-06-13T19:45:55.920Z" last_activity: 2026-06-13 progress: total_phases: 20 @@ -25,8 +25,8 @@ See: .planning/PROJECT.md (updated 2026-06-10) ## Current Position -Phase: 10 (admin-role-settings) — EXECUTING -Plan: 4 of 4 +Phase: 13 +Plan: Not started Status: Phase complete — ready for verification Last activity: 2026-06-13 @@ -38,7 +38,7 @@ Done 2026-06-12. Gitea branch protection on `main` now requires EXACTLY `CI / fa **Velocity:** -- Total plans completed: 39 +- Total plans completed: 43 - Average duration: - - Total execution time: 0 hours @@ -54,6 +54,7 @@ Done 2026-06-12. Gitea branch protection on `main` now requires EXACTLY `CI / fa | 15 | 3 | - | - | | 09 | 2 | - | - | | 16 | 6 | - | - | +| 10 | 4 | - | - | **Recent Trend:** diff --git a/.planning/phases/10-admin-role-settings/10-VERIFICATION.md b/.planning/phases/10-admin-role-settings/10-VERIFICATION.md new file mode 100644 index 0000000..53a27e6 --- /dev/null +++ b/.planning/phases/10-admin-role-settings/10-VERIFICATION.md @@ -0,0 +1,166 @@ +--- +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 && }`; `BottomTabBar.tsx` line 106: `{isAdmin && }`; 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 && }`; aria-label present | +| `apps/pwa/src/components/BottomTabBar.tsx` | Conditional Admin tab (ShieldCheck) on isAdmin | VERIFIED | Line 106: `{isAdmin && }`; 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)_