From a944dcd881e03e575dd10cba1b3e911598ee4060 Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Sat, 13 Jun 2026 14:07:22 -0400 Subject: [PATCH] docs(10): revise phase plan per plan-checker feedback (3 blockers, 2 warnings) --- .../10-admin-role-settings/10-01-PLAN.md | 28 +++--- .../10-admin-role-settings/10-03-PLAN.md | 86 ++++++++++++------- .../10-admin-role-settings/10-04-PLAN.md | 31 +++++-- .../10-admin-role-settings/10-VALIDATION.md | 17 ++-- 4 files changed, 100 insertions(+), 62 deletions(-) diff --git a/.planning/phases/10-admin-role-settings/10-01-PLAN.md b/.planning/phases/10-admin-role-settings/10-01-PLAN.md index 946b015..5080ac7 100644 --- a/.planning/phases/10-admin-role-settings/10-01-PLAN.md +++ b/.planning/phases/10-admin-role-settings/10-01-PLAN.md @@ -94,27 +94,27 @@ Output: Edited `schema.ts`, a generated `0001_v1_1_foundation.sql` migration fil - apps/api/src/db/migrations/0000_baseline.sql (existing migration format — `--> statement-breakpoint` between DDL statements; the format the generated file must follow) - apps/api/drizzle.config.ts (migration output dir + env-driven DB credentials) - - apps/api/package.json (the `db:generate` / `db:migrate` scripts) + - apps/api/package.json (the `db:generate` / `db:migrate` scripts — `db:generate` = `drizzle-kit generate`, `db:migrate` = `drizzle-kit migrate`) - .planning/phases/10-admin-role-settings/10-RESEARCH.md §Pattern 3 (generate+migrate workflow, exact commands, DB_HOST=127.0.0.1 dev override) + §Pitfall 4 (why never push) - - MEMORY note [[drizzle-mariadb-push-unsafe]] context in 10-CONTEXT.md Claude's Discretion (generate+migrate, never push) + - MEMORY note [[drizzle-mariadb-push-unsafe]] context in 10-CONTEXT.md Claude's Discretion (generate+migrate, never push) + [[api-integration-test-db]] (DB_HOST=127.0.0.1 + .env creds for a reachable dev MariaDB) - BLOCKING — this must run AFTER Task 1 (schema.ts complete) and BEFORE any plan that reads the new columns. Bring up the dev MariaDB if not already bound on 3306 (per [[dev-stack-bringup]]: dev compose override exposes 3306). Then: - 1. Run `pnpm --filter @familysync/api db:generate` to produce `apps/api/src/db/migrations/0001_v1_1_foundation.sql` (drizzle-kit names it; the actual filename may differ — commit whatever drizzle-kit emits as the next sequential migration) and update `meta/_journal.json`. DO NOT hand-write the SQL. - 2. INSPECT the generated SQL: it MUST be only `ALTER TABLE ... ADD COLUMN` / `ADD UNIQUE` / `CREATE TABLE` statements (additive). If it contains any `DROP TABLE`, `DROP COLUMN`, or `TRUNCATE`, STOP — that is the false-destructive-diff trap; do NOT apply it, and do NOT fall back to `db:push`. Re-derive from schema.ts. - 3. Apply with `DB_HOST=127.0.0.1` (+ dev DB_USER/DB_PASSWORD/DB_NAME from .env): `cd apps/api && set -a; source ../../.env; set +a; DB_HOST=127.0.0.1 pnpm db:migrate`. NEVER `db:push`. - 4. Verify the live columns/table exist via a mysql2 query (not just tsc): assert `is_admin` on `users`, `provider_type` + the unique index on `member_credentials`, `reminder_lead_minutes` on `calendar_events`, and the `app_config` table. + BLOCKING — this must run AFTER Task 1 (schema.ts complete) and BEFORE any plan that reads the new columns. You MUST actually RUN both commands in this task; describing them is not enough, and tsc/build passing is NOT sufficient proof that the migration ran (Drizzle types come from schema.ts regardless of whether the DB was migrated). Bring up the dev MariaDB if not already bound on 3306 (per [[dev-stack-bringup]]: dev compose override exposes 3306). Then: + 1. RUN `pnpm --filter @familysync/api db:generate` to produce the next sequential migration `.sql` under `apps/api/src/db/migrations/` (drizzle-kit names it `0001_v1_1_foundation.sql` or a similar sequential name — commit whatever drizzle-kit emits) and update `meta/_journal.json`. DO NOT hand-write the SQL. + 2. INSPECT the generated SQL: it MUST be only `ALTER TABLE ... ADD COLUMN` / `ADD UNIQUE` / `CREATE TABLE` statements (additive). If it contains any `DROP TABLE`, `DROP COLUMN`, or `TRUNCATE`, STOP — that is the false-destructive-diff trap ([[drizzle-mariadb-push-unsafe]]); do NOT apply it, and do NOT fall back to `db:push`. Re-derive from schema.ts. + 3. RUN `pnpm --filter @familysync/api db:migrate` against the reachable dev DB with `DB_HOST=127.0.0.1` + the dev DB_USER/DB_PASSWORD/DB_NAME/DB_PORT from `.env` (per [[api-integration-test-db]]): `set -a; source .env; set +a; DB_HOST=127.0.0.1 pnpm --filter @familysync/api db:migrate`. NEVER `db:push` / `drizzle-kit push`. + 4. Verify the live columns/table exist via a mysql2 query (not just tsc): assert `is_admin` on `users`, `provider_type` + the unique index on `member_credentials`, `reminder_lead_minutes` on `calendar_events`, and the `app_config` table each return a row. cd /home/luc/Projects/familysync && set -a; source .env 2>/dev/null; set +a; DB_HOST=127.0.0.1 node -e "const m=require('mysql2/promise');(async()=>{const c=await m.createConnection({host:'127.0.0.1',port:Number(process.env.DB_PORT||3306),user:process.env.DB_USER||'familysync',password:process.env.DB_PASSWORD||'',database:process.env.DB_NAME||'familysync'});const[u]=await c.query(\"SHOW COLUMNS FROM users LIKE 'is_admin'\");const[mc]=await c.query(\"SHOW COLUMNS FROM member_credentials LIKE 'provider_type'\");const[ce]=await c.query(\"SHOW COLUMNS FROM calendar_events LIKE 'reminder_lead_minutes'\");const[ac]=await c.query(\"SHOW TABLES LIKE 'app_config'\");if(u.length&&mc.length&&ce.length&&ac.length){console.log('MIGRATION OK');process.exit(0)}console.error('MISSING',{u:u.length,mc:mc.length,ce:ce.length,ac:ac.length});process.exit(1)})().catch(e=>{console.error(e.message);process.exit(1)})" - - A new migration `.sql` file exists under `apps/api/src/db/migrations/` (sequential after `0000_baseline.sql`) and `meta/_journal.json` references it. - - The generated SQL contains NO `DROP TABLE`, `DROP COLUMN`, or `TRUNCATE` statement: `grep -v '^--' | grep -ciE 'drop (table|column)|truncate'` returns 0. - - The live dev MariaDB query above prints `MIGRATION OK` and exits 0 — `users.is_admin`, `member_credentials.provider_type`, the `member_credentials` unique on `user_id`, `calendar_events.reminder_lead_minutes`, and the `app_config` table all exist. - - `db:push` was NOT run (no push in command history for this task). + - Both commands were actually RUN this task: `pnpm --filter @familysync/api db:generate` produced a new migration `.sql` file under `apps/api/src/db/migrations/` (sequential after `0000_baseline.sql`) with a matching `meta/_journal.json` entry, and `pnpm --filter @familysync/api db:migrate` (with `DB_HOST=127.0.0.1` + `.env` creds) applied it to the live dev DB. tsc/build passing is explicitly NOT accepted as proof. + - The generated migration `.sql` contains NO `DROP`/`TRUNCATE` statement: `grep -v '^--' | grep -ciE 'drop (table|column)|truncate'` returns 0 (guards the false-destructive-diff trap). + - The live dev MariaDB now has the columns/table: the mysql2 query above prints `MIGRATION OK` and exits 0 — `SHOW COLUMNS FROM users LIKE 'is_admin'`, `SHOW COLUMNS FROM member_credentials LIKE 'provider_type'`, `SHOW COLUMNS FROM calendar_events LIKE 'reminder_lead_minutes'`, and `SHOW TABLES LIKE 'app_config'` each return a row, plus the `member_credentials` unique on `user_id` exists. + - `db:push` / `drizzle-kit push` was NOT run (no push in command history for this task). - The v1.1 migration is generated (additive-only), committed, and applied to the live dev DB; all new columns/table verified present by a real DB query. + The v1.1 migration is generated (additive-only) by an actual db:generate run, committed, and applied to the live dev DB by an actual db:migrate run; all new columns/table verified present by a real DB query (not tsc). @@ -175,7 +175,7 @@ This plan creates the following new symbols/files (excluded from drift verificat - `pnpm --filter @familysync/api exec tsc --noEmit` passes (schema typechecks). -- The live DB query in Task 2 prints `MIGRATION OK`. +- The live DB query in Task 2 prints `MIGRATION OK` (proof the migration actually ran — tsc alone is NOT proof). - The generated migration SQL is additive-only (no DROP/TRUNCATE). - `grep -c is_admin apps/pwa/e2e/global-setup.ts` >= 1. @@ -190,3 +190,5 @@ This plan creates the following new symbols/files (excluded from drift verificat Create `.planning/phases/10-admin-role-settings/10-01-SUMMARY.md` when done. + + diff --git a/.planning/phases/10-admin-role-settings/10-03-PLAN.md b/.planning/phases/10-admin-role-settings/10-03-PLAN.md index 12146cb..19eeefc 100644 --- a/.planning/phases/10-admin-role-settings/10-03-PLAN.md +++ b/.planning/phases/10-admin-role-settings/10-03-PLAN.md @@ -6,6 +6,7 @@ wave: 3 depends_on: ["10-01", "10-02"] files_modified: - apps/api/src/broker/outboxWorker.ts + - apps/api/src/broker/credentialSync.ts - apps/api/src/routes/admin.ts - apps/api/src/routes/me.ts - apps/api/src/index.ts @@ -18,11 +19,16 @@ must_haves: - "POST /api/admin/credentials validates against CalDAV (PROPFIND), 400 on bad credential with NO submitted password in the body, 200 + encrypted store on success; never logs/echoes the password" - "PUT /api/admin/calendars/:id/shared sets exactly one calendar is_shared=1 and clears any prior shared calendar" - "POST /api/me/credential sets only the current user's credential (ignores any userId in the body); a non-admin cannot POST /api/admin/credentials" + - "Both POST /api/admin/credentials and POST /api/me/credential call ONE shared validateEncryptAndStoreCredential helper (no duplicated validate/encrypt/store logic)" artifacts: - path: "apps/api/src/routes/admin.ts" provides: "adminRouter guarded by requireAdmin (.use('*', ...) first); GET /members, POST /credentials, GET /calendars, PUT /calendars/:id/shared" exports: ["adminRouter"] min_lines: 60 + - path: "apps/api/src/broker/credentialSync.ts" + provides: "shared validateEncryptAndStoreCredential(userId, fastmailEmail, appPassword, providerType) helper used by BOTH admin + self-service paths" + exports: ["validateEncryptAndStoreCredential"] + contains: "validateEncryptAndStoreCredential" - path: "apps/api/src/routes/me.ts" provides: "POST /api/me/credential member-scoped self-service (currentUserId only)" contains: "credential" @@ -35,9 +41,13 @@ must_haves: via: "adminRouter.use('*', requireAdmin) as the first statement (Pitfall 9)" pattern: "adminRouter\\.use\\('\\*', requireAdmin\\)" - from: "apps/api/src/routes/admin.ts" - to: "broker validate→encrypt→sync" - via: "createFastmailClient + fetchCalendars, encryptPassword, exported triggerTargetedResync/loadClientForUser" - pattern: "encryptPassword" + to: "validateEncryptAndStoreCredential" + via: "import from ../broker/credentialSync.js (shared validate→encrypt→sync path)" + pattern: "validateEncryptAndStoreCredential" + - from: "apps/api/src/routes/me.ts" + to: "validateEncryptAndStoreCredential" + via: "import from ../broker/credentialSync.js (same helper, currentUserId)" + pattern: "validateEncryptAndStoreCredential" - from: "apps/api/src/index.ts" to: "adminRouter" via: "app.route('/api/admin', adminRouter)" @@ -45,10 +55,10 @@ must_haves: --- -Build the admin API surface (ADMIN-01 credential rotation + ADMIN-02 shared-calendar designation), gated by `requireAdmin` (ADMIN-03), plus the member-scoped self-service credential endpoint (D-07), all sharing one validate→encrypt→initial-sync path. Promote the broker's private resync helpers to exports so both the admin and self-service paths reuse them. TDD: the credential and guard contracts have precise input→output behavior (403 / 400-no-echo / 200), so write the failing tests first. +Build the admin API surface (ADMIN-01 credential rotation + ADMIN-02 shared-calendar designation), gated by `requireAdmin` (ADMIN-03), plus the member-scoped self-service credential endpoint (D-07), all sharing ONE `validateEncryptAndStoreCredential` validate→encrypt→initial-sync helper. Promote the broker's private resync helpers to exports so the shared helper can reuse them. TDD: the credential and guard contracts have precise input→output behavior (403 / 400-no-echo / 200), so write the failing tests first. -Purpose: This is the single shared credential + shared-calendar surface (`/api/admin/credentials`, `/api/admin/calendars/:id/shared`) — Phase 12 MUST reuse it, not duplicate it into `/api/setup/*`. The self-service endpoint is the member-scoped counterpart of admin rotation. -Output: Exported broker helpers, new `admin.ts` router, the `/api/me/credential` self-service endpoint, the index.ts mount, and integration tests covering the Pitfall 7 (no-echo) and Pitfall 9 (403) hard checks. +Purpose: This is the single shared credential + shared-calendar surface (`/api/admin/credentials`, `/api/admin/calendars/:id/shared`) — Phase 12 MUST reuse it, not duplicate it into `/api/setup/*`. The self-service endpoint is the member-scoped counterpart of admin rotation, and it MUST call the exact same credential helper to avoid divergence. +Output: Exported broker helpers, a new shared `credentialSync.ts` helper, the new `admin.ts` router, the `/api/me/credential` self-service endpoint, the index.ts mount, and integration tests covering the Pitfall 7 (no-echo) and Pitfall 9 (403) hard checks. @@ -79,7 +89,7 @@ Output: Exported broker helpers, new `admin.ts` router, the `/api/me/credential` - .planning/phases/10-admin-role-settings/10-PATTERNS.md §`apps/api/src/broker/outboxWorker.ts` (add `export` to both functions; the post-credential-save full sync uses loadClientForUser → fetchCalendars → syncCalendar per davCal) + 10-RESEARCH.md §Pattern 4 + §Open Questions #2 (full per-member poll after a fresh credential save — no known calendarUrl yet) + Assumptions A1 - Add the `export` keyword to `loadClientForUser` and `triggerTargetedResync` in `apps/api/src/broker/outboxWorker.ts` so the admin + self-service credential routes can reuse them (per 10-PATTERNS.md). Do NOT change their bodies or the outbox drain cycle (A1: standalone fetch+sync helpers, no coupling to the drain loop). After a FRESH credential save there is no known calendarUrl, so the credential routes will call `loadClientForUser(userId)` → `client.fetchCalendars()` → `syncCalendar(...)` per returned DAV calendar (the full per-member poll, mirroring poller.ts) rather than `triggerTargetedResync` — but export both for flexibility. Confirm existing broker tests still pass (no behavior change). NEVER reintroduce node-cron ([[node-cron-skips-in-long-running-process]]) — these helpers are setInterval-driven callers' utilities, untouched. + Add the `export` keyword to `loadClientForUser` and `triggerTargetedResync` in `apps/api/src/broker/outboxWorker.ts` so the shared `credentialSync.ts` helper (Task 2) can reuse them (per 10-PATTERNS.md). Do NOT change their bodies or the outbox drain cycle (A1: standalone fetch+sync helpers, no coupling to the drain loop). After a FRESH credential save there is no known calendarUrl, so the shared helper will call `loadClientForUser(userId)` → `client.fetchCalendars()` → `syncCalendar(...)` per returned DAV calendar (the full per-member poll, mirroring poller.ts) rather than `triggerTargetedResync` — but export both for flexibility. Confirm existing broker tests still pass (no behavior change). NEVER reintroduce node-cron ([[node-cron-skips-in-long-running-process]]) — these helpers are setInterval-driven callers' utilities, untouched. cd /home/luc/Projects/familysync && grep -E "^export (async )?function (loadClientForUser|triggerTargetedResync)" apps/api/src/broker/outboxWorker.ts && pnpm --filter @familysync/api test -- outbox 2>&1 | tail -8 @@ -93,8 +103,8 @@ Output: Exported broker helpers, new `admin.ts` router, the `/api/me/credential` - Task 2: adminRouter — guard + members + credentials + shared-calendar (RED→GREEN→REFACTOR) - apps/api/src/routes/admin.ts, apps/api/src/index.ts, apps/api/tests/routes/admin.test.ts + Task 2: Shared credentialSync helper + adminRouter (guard + members + credentials + shared-calendar) (RED→GREEN→REFACTOR) + apps/api/src/broker/credentialSync.ts, apps/api/src/routes/admin.ts, apps/api/src/index.ts, apps/api/tests/routes/admin.test.ts - apps/api/src/routes/push.ts (closest analog: Hono sub-router, zValidator, resolveUserId, ContextVariableMap side-effect import; the subscribeSchema zValidator usage lines ~60–67) - apps/api/src/routes/events.ts (Drizzle leftJoin + where SELECT lines 165–177; the 400-not-422 zValidator convention; onDuplicateKeyUpdate upsert idiom) @@ -107,34 +117,43 @@ Output: Exported broker helpers, new `admin.ts` router, the `/api/me/credential` - Test (RED, Pitfall 9): GET /api/admin/members as a non-admin authenticated user → 403. As an admin → 200 with a list of members each carrying credential status (has credential / not). Integration test imports `app`, never adminRouter directly. - - Test (Pitfall 7): POST /api/admin/credentials with an INVALID app password (CalDAV PROPFIND fails) → 400, and the response body contains NONE of the submitted password value (assert the exact submitted string is absent from the body) and no Zod `received`/`issues`/`value` field. + - Test (Pitfall 7, validation→400 mapping): POST /api/admin/credentials with an INVALID app password (CalDAV PROPFIND fails) → 400, and the response body contains NONE of the submitted password value (assert the exact submitted string is absent from the body) and no Zod `received`/`issues`/`value` field. + - Test (Pitfall 7, all failure modes map to one generic 400): a malformed/bad-email payload that makes `createFastmailClient` throw, AND a network/connection error before PROPFIND, BOTH return `{ error: 'Invalid request' }` with status 400 (same generic shape as a PROPFIND auth failure) and never echo the submitted password. - Test: POST /api/admin/credentials with a VALID credential (CalDAV PROPFIND succeeds) → 200; the stored member_credentials.encrypted_password is NOT the plaintext (encryptPassword applied); response never echoes the password; initial sync is triggered (fire-and-forget). - Test (Pitfall 9): POST /api/admin/credentials as a non-admin → 403. - Test (ADMIN-02, Pitfall 7-adjacent): PUT /api/admin/calendars/:id/shared as admin → exactly one calendar has is_shared=1 afterward (the target), any prior shared calendar cleared. As non-admin → 403. - Test: GET /api/admin/calendars as admin → 200 list of synced calendars (id, name, is_shared). As non-admin → 403. - Create `apps/api/src/routes/admin.ts` exporting `adminRouter = new Hono()` with `adminRouter.use('*', requireAdmin)` as the VERY FIRST statement (Pitfall 9). Add the side-effect import `'../auth/devBypass.js'`. Routes (paths are planner's call per D — use these): + First create the SHARED helper `apps/api/src/broker/credentialSync.ts` exporting ONE function `validateEncryptAndStoreCredential(userId: number, fastmailEmail: string, appPassword: string, providerType: string)`. This is the single source of the validate→encrypt→store→initial-sync path; both `/api/admin/credentials` (Task 2) and `/api/me/credential` (Task 3) MUST import and call it — do NOT inline this logic in admin.ts or me.ts. The helper: + 1. Wraps BOTH `createFastmailClient(fastmailEmail, appPassword)` AND `await client.fetchCalendars()` in ONE try/catch. ANY throw — bad email, malformed input, network/connection error, PROPFIND/auth failure — is treated identically as a credential-validation failure. Signal this to the caller as a single generic outcome (throw a typed `CredentialValidationError` or return a discriminated failure) that the routes map to `{ error: 'Invalid request' }` 400. NEVER include the submitted password (or any Zod/error detail) in the failure path. + 2. On success: `encryptPassword(appPassword)` → upsert `member_credentials` via `onDuplicateKeyUpdate` (uses the Plan-01 UNIQUE(user_id)) with the given `providerType`. + 3. Then fire-and-forget the initial full per-member sync (`loadClientForUser(userId)` → `fetchCalendars()` → `syncCalendar` per davCal — the helpers exported in Task 1). + NEVER `console.log` the password, the request body, or `c.req.valid('json')` from anywhere in this path. + + Then create `apps/api/src/routes/admin.ts` exporting `adminRouter = new Hono()` with `adminRouter.use('*', requireAdmin)` as the VERY FIRST statement (Pitfall 9). Add the side-effect import `'../auth/devBypass.js'`. Routes (paths are planner's call per D — use these): - `GET /members`: SELECT users LEFT JOIN member_credentials → return id, displayName, color, hasCredential (boolean). Feeds UI-SPEC Surface 2. - - `POST /credentials`: `zValidator('json', credentialSchema, noEchoHook)` where credentialSchema = `{ userId: number().int().positive(), providerType: literal('caldav'), fastmailEmail: string().email().max(256), appPassword: string().min(1).max(500) }` and noEchoHook returns `c.json({ error: 'Invalid request' }, 400)` (NEVER `c.json(result.error, ...)`). Handler: validate via `createFastmailClient(email, appPassword)` + `client.fetchCalendars()` (throws on auth failure → return 400 generic, NO password in body); on success `encryptPassword(appPassword)` → upsert member_credentials via `onDuplicateKeyUpdate` (uses the Plan-01 UNIQUE(user_id)) with providerType 'caldav'; then fire-and-forget the initial full per-member sync (loadClientForUser → fetchCalendars → syncCalendar per davCal) and return 200. NEVER `console.log` the body or `c.req.valid('json')`. + - `POST /credentials`: `zValidator('json', credentialSchema, noEchoHook)` where credentialSchema = `{ userId: number().int().positive(), providerType: literal('caldav'), fastmailEmail: string().email().max(256), appPassword: string().min(1).max(500) }` and noEchoHook returns `c.json({ error: 'Invalid request' }, 400)` (NEVER `c.json(result.error, ...)`). Handler: call `validateEncryptAndStoreCredential(body.userId, body.fastmailEmail, body.appPassword, body.providerType)`; on the helper's validation-failure outcome return `c.json({ error: 'Invalid request' }, 400)` (no password in body); on success return 200. NEVER duplicate the createFastmailClient/fetchCalendars/encrypt logic here. - `GET /calendars`: SELECT calendars (id, displayName, isShared). Feeds UI-SPEC Surface 5. - `PUT /calendars/:id/shared`: exclusive update (Pattern 7) — `db.update(calendars).set({isShared:false}).where(eq(calendars.isShared,true))` then `db.update(calendars).set({isShared:true}).where(eq(calendars.id, targetId))` (D-06 single-select). Return 200. - Mount in `apps/api/src/index.ts`: `app.route('/api/admin', adminRouter)` after the existing route block (no extra app-level middleware — the guard lives inside the router). Write `apps/api/tests/routes/admin.test.ts` FIRST with all six behaviors (import `app`), confirm RED, implement to GREEN. Mock/stub CalDAV (createFastmailClient/fetchCalendars) for the validation outcomes to avoid live Fastmail calls in CI (per [[dev-data-user1-no-calendars]] — route-mocks for credential paths). + Mount in `apps/api/src/index.ts`: `app.route('/api/admin', adminRouter)` after the existing route block (no extra app-level middleware — the guard lives inside the router). Write `apps/api/tests/routes/admin.test.ts` FIRST with all the behaviors above (import `app`), confirm RED, implement to GREEN. Mock/stub CalDAV (createFastmailClient/fetchCalendars) for the validation outcomes — including the throw-on-createFastmailClient and network-error cases — to avoid live Fastmail calls in CI (per [[dev-data-user1-no-calendars]] — route-mocks for credential paths). cd /home/luc/Projects/familysync && pnpm --filter @familysync/api test -- admin 2>&1 | tail -20 + - `apps/api/src/broker/credentialSync.ts` exports exactly one `validateEncryptAndStoreCredential` and is the only place the createFastmailClient + fetchCalendars + encryptPassword + upsert + initial-sync sequence appears (`grep -rl "createFastmailClient" apps/api/src/routes/` returns nothing — that logic lives only in credentialSync.ts). + - createFastmailClient failures (bad email / malformed input / network error) AND fetchCalendars (PROPFIND/auth) failures BOTH return 400 with body `{ error: 'Invalid request' }`, and the submitted password string appears nowhere in the response or logs. - `apps/api/src/routes/admin.ts` first statement after router creation is `adminRouter.use('*', requireAdmin)` — `grep -nA1 "new Hono()" apps/api/src/routes/admin.ts` shows the `.use('*', requireAdmin)` immediately after. - `apps/api/src/index.ts` contains `app.route('/api/admin', adminRouter)`. - GET /api/admin/members returns 403 for a non-admin authenticated user (integration test importing `app`). - A 400 response from POST /api/admin/credentials with a bad credential contains NO submitted password value and no Zod `received`/`issues` field (test asserts the exact submitted string absent). - On a valid credential, the persisted member_credentials.encrypted_password != the plaintext (encryptPassword applied) and 200 is returned. - After PUT /api/admin/calendars/:id/shared, exactly one calendar row has is_shared=1. - - No `console.log`/`console.error` of request bodies in `apps/api/src/routes/admin.ts` (`grep -ciE "console\.(log|error)\(.*(body|valid|password)" apps/api/src/routes/admin.ts` returns 0). - - `pnpm --filter @familysync/api test -- admin` passes all six behaviors. + - No `console.log`/`console.error` of request bodies in `apps/api/src/routes/admin.ts` or `apps/api/src/broker/credentialSync.ts` (`grep -ciE "console\.(log|error)\(.*(body|valid|password)" apps/api/src/routes/admin.ts apps/api/src/broker/credentialSync.ts` returns 0). + - `pnpm --filter @familysync/api test -- admin` passes all behaviors. - adminRouter exists, guard-first, mounted; members/credentials/calendars/shared routes behave per contract; no-echo + 403 hard checks green. + credentialSync.ts holds the single shared validate→encrypt→sync helper; adminRouter exists, guard-first, mounted; members/credentials/calendars/shared routes behave per contract; all credential-validation failures map to one generic 400; no-echo + 403 hard checks green. @@ -142,31 +161,31 @@ Output: Exported broker helpers, new `admin.ts` router, the `/api/me/credential` apps/api/src/routes/me.ts, apps/api/tests/routes/admin.test.ts - apps/api/src/routes/me.ts (the file being modified — the meRouter export, the resolveUserId/dev-bypass + OIDC user resolution already present; the isAdmin/needsProviderSetup response added in Plan 02) - - apps/api/src/routes/admin.ts (from Task 2 — reuse the SAME credentialSchema shape minus userId, the SAME noEchoHook, the SAME validate→encrypt→sync sequence; extract any shared helper rather than duplicating the validate/encrypt logic) - - apps/api/src/broker/crypto.ts + client.ts + outboxWorker.ts (the shared path) + - apps/api/src/broker/credentialSync.ts (from Task 2 — the SHARED validateEncryptAndStoreCredential helper this route MUST call; do NOT re-implement validate/encrypt/store) + - apps/api/src/routes/admin.ts (from Task 2 — reuse the SAME credentialSchema shape minus userId, the SAME noEchoHook, and the SAME 400-mapping convention) - .planning/phases/10-admin-role-settings/10-PATTERNS.md §Shared Patterns "resolveUserId" + 10-RESEARCH.md §Pattern 4, §Pitfall 6 (cross-member write — endpoint MUST use currentUserId from session, NEVER a body userId), §Architectural Responsibility Map (needsProviderSetup) + §UI-SPEC Surface 4 (self-service onboarding) - Test (RED, Pitfall 6): POST /api/me/credential as user A with a body that includes `userId` for user B → the credential is written to user A (currentUserId), NOT user B; the body userId is ignored. - Test: POST /api/me/credential with a valid credential → 200, stored encrypted for the current user, needsProviderSetup becomes false on the next /api/me; initial sync triggered. - - Test (Pitfall 7): POST /api/me/credential with a bad credential → 400 generic, no password echoed. + - Test (Pitfall 7): POST /api/me/credential with a bad credential → 400 generic `{ error: 'Invalid request' }`, no password echoed (same helper, same 400 mapping as admin). - Test: the endpoint does NOT require admin (a normal member can set their own credential) but is still behind the auth guard (unauthenticated → 401 from the outer band). - Add `POST /credential` to the meRouter in `apps/api/src/routes/me.ts` (final path `/api/me/credential`), member-scoped. Schema = the admin credentialSchema WITHOUT `userId` (`{ providerType: literal('caldav'), fastmailEmail, appPassword }`) + the SAME `noEchoHook`. The handler resolves `currentUserId` via the existing resolveUserId/dev-bypass pattern and ALWAYS writes to that id — it MUST NOT read a userId from the body (Pitfall 6). Reuse the SAME validate→encrypt→initial-sync path as admin Task 2 (extract a shared helper, e.g. `validateEncryptAndStoreCredential(userId, email, password)`, to avoid divergence — D-07 "identical path"). Add the self-service test cases to `apps/api/tests/routes/admin.test.ts` (or a sibling me-credential test — planner's call; keep them with the credential-surface tests). Write tests FIRST, confirm RED, implement to GREEN. + Add `POST /credential` to the meRouter in `apps/api/src/routes/me.ts` (final path `/api/me/credential`), member-scoped. Schema = the admin credentialSchema WITHOUT `userId` (`{ providerType: literal('caldav'), fastmailEmail, appPassword }`) + the SAME `noEchoHook`. The handler resolves `currentUserId` via the existing resolveUserId/dev-bypass pattern and ALWAYS writes to that id — it MUST NOT read a userId from the body (Pitfall 6). It MUST call the SAME shared helper from Task 2: `validateEncryptAndStoreCredential(currentUserId, body.fastmailEmail, body.appPassword, body.providerType)` (import from `../broker/credentialSync.js`). Do NOT duplicate the validate/encrypt/store/sync sequence — admin passes the target member's userId from the route/body, self-service passes the authenticated currentUserId, but both call the identical helper with identical argument order (D-07 "identical path"). Map the helper's validation-failure outcome to `c.json({ error: 'Invalid request' }, 400)`; on success 200. Add the self-service test cases to `apps/api/tests/routes/admin.test.ts` (keep them with the credential-surface tests). Write tests FIRST, confirm RED, implement to GREEN. cd /home/luc/Projects/familysync && pnpm --filter @familysync/api test -- credential 2>&1 | tail -15 - `apps/api/src/routes/me.ts` adds a `POST /credential` route; the final mounted path is `/api/me/credential`. + - `validateEncryptAndStoreCredential` is defined once in `apps/api/src/broker/credentialSync.ts`; BOTH `apps/api/src/routes/admin.ts` (POST /credentials) and `apps/api/src/routes/me.ts` (POST /credential) import and call it with identical arguments — admin passes the target member's userId from the route/body, self-service passes the authenticated currentUserId, never a body userId. `grep -rc "validateEncryptAndStoreCredential" apps/api/src/routes/admin.ts apps/api/src/routes/me.ts` shows a call in each (and the function body exists only in credentialSync.ts). - A POST to /api/me/credential with a body `userId` for another user writes ONLY to the current session user (test proves the other user's credential is untouched). - - The self-service path reuses the same validate→encrypt→store logic as the admin path (shared helper; no duplicated encrypt/PROPFIND block) — `grep` shows a single shared function called by both routes. - - A bad credential returns 400 generic with no echoed password. + - A bad credential returns 400 generic `{ error: 'Invalid request' }` with no echoed password. - A normal (non-admin) member can succeed on /api/me/credential (no requireAdmin on this route). - `pnpm --filter @familysync/api test -- credential` passes all cases. - Member self-service credential endpoint exists, member-scoped (no cross-member write), shares the admin validate→encrypt→sync path, tests green. + Member self-service credential endpoint exists, member-scoped (no cross-member write), calls the SAME shared validateEncryptAndStoreCredential helper as the admin path, tests green. @@ -174,11 +193,11 @@ Output: Exported broker helpers, new `admin.ts` router, the `/api/me/credential` New symbols/files created by this plan (excluded from drift verification): - `export` on `loadClientForUser` + `triggerTargetedResync` in `apps/api/src/broker/outboxWorker.ts` +- `apps/api/src/broker/credentialSync.ts` exporting the single shared `validateEncryptAndStoreCredential(userId, fastmailEmail, appPassword, providerType)` helper (validate→encrypt→store→initial-sync; the only place createFastmailClient + fetchCalendars + encryptPassword + upsert live) - `apps/api/src/routes/admin.ts` exporting `adminRouter` with `GET /members`, `POST /credentials`, `GET /calendars`, `PUT /calendars/:id/shared` - `requireAdmin` applied as `adminRouter.use('*', requireAdmin)` (consumes the Plan-02 guard) - `app.route('/api/admin', adminRouter)` mount in `apps/api/src/index.ts` -- `POST /api/me/credential` member-scoped self-service endpoint in `apps/api/src/routes/me.ts` -- shared `validateEncryptAndStoreCredential` helper (admin + self-service) +- `POST /api/me/credential` member-scoped self-service endpoint in `apps/api/src/routes/me.ts` (calls the shared helper) - `apps/api/tests/routes/admin.test.ts` (+ self-service credential test cases) @@ -197,10 +216,10 @@ New symbols/files created by this plan (excluded from drift verification): | Threat ID | Category | Component | Disposition | Mitigation Plan | |-----------|----------|-----------|-------------|-----------------| | T-10-08 | Elevation of Privilege | non-admin hitting /api/admin/* | mitigate | adminRouter.use('*', requireAdmin) FIRST (Pitfall 9); integration tests import `app` and assert 403 on every admin route for a non-admin | -| T-10-09 | Information Disclosure | app password echoed in a Zod/validation error | mitigate | noEchoHook returns `{ error: 'Invalid request' }` with no result.error; test asserts the submitted password string is absent from any 400 body (Pitfall 7) | -| T-10-10 | Information Disclosure | app password logged | mitigate | No console.log of body/valid()/password in admin.ts or the me credential route (acceptance grep == 0) | -| T-10-11 | Information Disclosure | plaintext credential at rest | mitigate | encryptPassword (AES-256-GCM via crypto.ts) applied before the DB write; test asserts stored value != plaintext; no new crypto written | -| T-10-12 | Elevation of Privilege / IDOR | member self-service writes another member's credential | mitigate | /api/me/credential always uses currentUserId from the session and ignores any body userId (Pitfall 6); test proves the other user's row is untouched | +| T-10-09 | Information Disclosure | app password echoed in a Zod/validation error | mitigate | noEchoHook returns `{ error: 'Invalid request' }` with no result.error; ALL credential-validation failures (createFastmailClient throw, network error, PROPFIND/auth failure) map to one generic 400 in the shared helper; test asserts the submitted password string is absent from any 400 body (Pitfall 7) | +| T-10-10 | Information Disclosure | app password logged | mitigate | No console.log of body/valid()/password in admin.ts, me.ts, or credentialSync.ts (acceptance grep == 0) | +| T-10-11 | Information Disclosure | plaintext credential at rest | mitigate | encryptPassword (AES-256-GCM via crypto.ts) applied in the shared helper before the DB write; test asserts stored value != plaintext; no new crypto written | +| T-10-12 | Elevation of Privilege / IDOR | member self-service writes another member's credential | mitigate | /api/me/credential always passes currentUserId from the session to the shared helper and ignores any body userId (Pitfall 6); test proves the other user's row is untouched | | T-10-13 | IDOR | admin rotating an arbitrary member's credential | accept | D-05 explicitly allows an admin to rotate ANY member's credential; this is gated by requireAdmin and is the intended capability (the self-service path remains member-scoped) | | T-10-SC | Tampering | npm/pip/cargo installs | mitigate | No new packages this phase (RESEARCH Package Legitimacy Audit); no install task | @@ -209,17 +228,20 @@ New symbols/files created by this plan (excluded from drift verification): - `pnpm --filter @familysync/api test -- admin && pnpm --filter @familysync/api test -- credential && pnpm --filter @familysync/api test -- outbox` all pass. - `pnpm --filter @familysync/api exec tsc --noEmit` passes (run tsc separately per [[vitest-passes-tsc-fails]]). - `grep -A1 "new Hono()" apps/api/src/routes/admin.ts` shows `.use('*', requireAdmin)` first. -- `grep -ciE "console\.(log|error)\(.*(body|valid|password)" apps/api/src/routes/admin.ts` == 0. +- `grep -ciE "console\.(log|error)\(.*(body|valid|password)" apps/api/src/routes/admin.ts apps/api/src/broker/credentialSync.ts` == 0. +- `validateEncryptAndStoreCredential` is imported and called by both admin.ts and me.ts; its body exists only in credentialSync.ts (no duplicated createFastmailClient/encrypt block in the routes). - ADMIN-01: admin can rotate any member's credential, CalDAV-validated, encrypted, never echoed/logged (Success Criterion 2). - ADMIN-02: admin sets exactly one shared calendar via the API (Success Criterion 3). - ADMIN-03: every /api/admin/* route 403s non-admins (Success Criterion 1); guard inside the sub-router (Pitfall 9). -- D-07: member self-service credential, member-scoped, same path. -- Single shared surface — no /api/setup/* duplication (Phase 12 reuses these routes). +- D-07: member self-service credential, member-scoped, SAME shared helper path (no divergence). +- Single shared surface — no /api/setup/* duplication (Phase 12 reuses these routes + the shared helper). Create `.planning/phases/10-admin-role-settings/10-03-SUMMARY.md` when done. + + diff --git a/.planning/phases/10-admin-role-settings/10-04-PLAN.md b/.planning/phases/10-admin-role-settings/10-04-PLAN.md index 5a8058a..14eb651 100644 --- a/.planning/phases/10-admin-role-settings/10-04-PLAN.md +++ b/.planning/phases/10-admin-role-settings/10-04-PLAN.md @@ -20,6 +20,7 @@ must_haves: - "An admin sees an Admin nav entry, reaches /admin, can list members + credential status, rotate a member credential via the sheet, and pick the shared calendar" - "A non-admin never sees the Admin nav entry and is redirected from /admin to /calendar" - "A member with needsProviderSetup=true sees the self-service SetupBanner and can add their own credential via the same sheet (member-scoped)" + - "After a successful credential save the SetupBanner clears (needsProviderSetup→false via ['me'] invalidation), there being no dismiss button" artifacts: - path: "apps/pwa/src/routes/AdminPage.tsx" provides: "/admin page: Members section + Shared-Calendar picker, wired to /api/admin/*" @@ -108,8 +109,11 @@ Output: Extended client.ts, AdminPage.tsx, CredentialSheet.tsx, SetupBanner.tsx, - .planning/phases/10-admin-role-settings/10-UI-SPEC.md Surface 3 (credential sheet: heading variants, member subtitle, password field type="password"/autocomplete="new-password"/never pre-filled, helper text + Fastmail app-password link target="_blank" rel="noopener noreferrer", "Validating against CalDAV…" Loader2 spinner, failure copy, Save/Cancel actions) + Surface 4 (self-service banner: KeyRound icon, copy, "Set up now" CTA, no X/dismiss) + the Copywriting Contract (exact strings) + Accessibility Contracts + Color/Typography/Spacing (all via var(--token), 44px touch targets) - .planning/phases/10-admin-role-settings/10-PATTERNS.md §`apps/pwa/src/components/CredentialSheet.tsx` + §`apps/pwa/src/components/SetupBanner.tsx` (structural + mutation + style excerpts) + §Shared Patterns "CSS token inline style pattern" + + - SetupBanner dismissal (success-only): there is NO dismiss/X button per UI-SPEC Surface 4 — the ONLY way the banner clears is a successful credential save. After a successful save the CredentialSheet mutation's `onSuccess` invalidates the `['me']` query → /api/me refetches → `needsProviderSetup` becomes `false` → SetupBanner unmounts on the next rerender. A test/behavior assertion: given `needsProviderSetup=true` the banner renders; after a successful save (mocked) that flips /api/me to `needsProviderSetup=false`, the banner is no longer in the DOM. (No interaction other than success clears it.) + - Build `CredentialSheet.tsx` (shared by admin rotation AND self-service per D-07) following the SettingsSheet bottom-sheet pattern: props for mode (admin-rotate / admin-add / self-service), target member (admin) or current user (self-service), open/close. Render the heading variant per UI-SPEC Copywriting Contract ("Rotate Credential" / "Add Credential" / "Add your calendar credential"), the member-name subtitle, a `type="password" autoComplete="new-password"` field NEVER pre-filled, the helper text with the Fastmail app-password link (exact URL + copy from UI-SPEC, opens in new tab), the "Validating against CalDAV…" inline state (Loader2 size 16) during the mutation, the failure error copy on a CalDAV 400, and Cancel (ghost) + Save Credential (accent-filled) actions. Use a TanStack `useMutation` that calls `saveCredential` (admin) or `saveMyCredential` (self-service) and on success invalidates `['admin','members']` + `['me']` (so needsProviderSetup refreshes) and closes the sheet. Build `SetupBanner.tsx` following PermissionDeniedBanner: render only when `meQuery.data?.user.needsProviderSetup === true`, `role="status" aria-live="polite"`, KeyRound icon, the exact heading/body/CTA copy, "Set up now" opening the CredentialSheet in self-service mode; NO dismiss button (it clears when needsProviderSetup becomes false). All styling via `var(--token)`; every interactive element minWidth/minHeight 44px. Never log/echo the password. + Build `CredentialSheet.tsx` (shared by admin rotation AND self-service per D-07) following the SettingsSheet bottom-sheet pattern: props for mode (admin-rotate / admin-add / self-service), target member (admin) or current user (self-service), open/close. Render the heading variant per UI-SPEC Copywriting Contract ("Rotate Credential" / "Add Credential" / "Add your calendar credential"), the member-name subtitle, a `type="password" autoComplete="new-password"` field NEVER pre-filled, the helper text with the Fastmail app-password link (exact URL + copy from UI-SPEC, opens in new tab), the "Validating against CalDAV…" inline state (Loader2 size 16) during the mutation, the failure error copy on a CalDAV 400, and Cancel (ghost) + Save Credential (accent-filled) actions. Use a TanStack `useMutation` that calls `saveCredential` (admin) or `saveMyCredential` (self-service) and on success invalidates `['admin','members']` + `['me']` (so needsProviderSetup refreshes and the SetupBanner clears) and closes the sheet. Build `SetupBanner.tsx` following PermissionDeniedBanner: render only when `meQuery.data?.user.needsProviderSetup === true`, `role="status" aria-live="polite"`, KeyRound icon, the exact heading/body/CTA copy, "Set up now" opening the CredentialSheet in self-service mode; NO dismiss button (it clears ONLY when needsProviderSetup becomes false after a successful save — the success-only dismissal behavior above). All styling via `var(--token)`; every interactive element minWidth/minHeight 44px. Never log/echo the password. cd /home/luc/Projects/familysync && pnpm --filter @familysync/pwa exec tsc --noEmit 2>&1 | tail -8 && grep -l "autocomplete=\"new-password\"\|autoComplete=\"new-password\"" apps/pwa/src/components/CredentialSheet.tsx @@ -119,11 +123,12 @@ Output: Extended client.ts, AdminPage.tsx, CredentialSheet.tsx, SetupBanner.tsx, - The helper text contains the Fastmail app-password link with `target="_blank"` and `rel="noopener noreferrer"`. - The success mutation invalidates both `['admin','members']` and `['me']` (`grep -c "invalidateQueries" apps/pwa/src/components/CredentialSheet.tsx` >= 2). - `SetupBanner.tsx` renders conditionally on `needsProviderSetup`, uses `role="status"`/`aria-live`, and has NO dismiss/X button. + - SetupBanner success-only dismissal holds: when /api/me reports `needsProviderSetup=false` (the state after a successful save invalidates `['me']`), the banner does not render — there is no code path that hides it other than the `needsProviderSetup` flag flipping to false. - Exact UI-SPEC Copywriting Contract strings are present (e.g. "Set up your calendar", "Validating against CalDAV…", "Save Credential"). - No hard-coded color/spacing px except the 44px/48px touch-target minimums; values reference `var(--...)`. - `pnpm --filter @familysync/pwa exec tsc --noEmit` exits 0. - CredentialSheet (admin + self-service) and SetupBanner match the UI-SPEC contract, accessible, token-styled, typecheck clean. + CredentialSheet (admin + self-service) and SetupBanner match the UI-SPEC contract, accessible, token-styled, success-only banner dismissal wired via ['me'] invalidation, typecheck clean. @@ -134,12 +139,18 @@ Output: Extended client.ts, AdminPage.tsx, CredentialSheet.tsx, SetupBanner.tsx, - apps/pwa/src/routes/ListsIndex.tsx (page-level component analog for AdminPage: TanStack Query + sections layout) - apps/pwa/src/components/AppNav.tsx (NavLink + Lucide pattern lines 14–15, the DesktopNav section) + apps/pwa/src/components/BottomTabBar.tsx (tab pattern lines 27–98) — add the conditional Admin entry (ShieldCheck) to both - apps/pwa/e2e/calendar.spec.ts + layout.spec.ts + e2e/README.md (existing spec idioms, selectors, the dev-bypass admin user seeded in global-setup as id=1 is_admin=true from Plan 01) - - .claude/skills/playwright-cli/SKILL.md (how to drive the global playwright-cli binary for the browser verification) + - .claude/skills/playwright-cli/SKILL.md (how to drive the global playwright-cli binary for the supplementary browser verification) - .planning/phases/10-admin-role-settings/10-UI-SPEC.md Surface 1 (/admin page: AppNav persistent, ShieldCheck size 18, content maxWidth 640px centered desktop, var(--space-12) vertical padding, "Admin Settings" heading) + Surface 2 (Members section) + Surface 5 (Shared Calendar picker: radio group, "Currently shared" label, two-tap Save, empty state) + Copywriting Contract - .planning/phases/10-admin-role-settings/10-PATTERNS.md §`apps/pwa/src/routes/AdminPage.tsx` + §`apps/pwa/src/App.tsx` + §Shared Patterns "NavLink + Lucide icon" - Build `AdminPage.tsx` (analog ListsIndex): "Admin Settings" heading (18px/600), a MEMBERS section listing members from `fetchAdminMembers` (avatar swatch + name + credential status badge per UI-SPEC Surface 2 + a "Rotate"/"Add credential" button opening CredentialSheet in admin mode for that member), and a SHARED CALENDAR section (Surface 5) using `fetchAdminCalendars`: an exclusive single-select radio group (D-06), the "Currently shared" label on the active one, a two-tap "Save" button (disabled until selection differs) calling `setSharedCalendar`, and the empty state ("No calendars synced yet") when none synced. Desktop: centered column maxWidth 640px. In `App.tsx`: add the `/admin` Route (D-02: new dedicated gated route) gated by `meQuery.data?.user.isAdmin ? : ` — the client redirect is UX-only per D-03 (the server-side 403 from Plan 03 is the real boundary); add a loading gate so an in-flight meQuery doesn't flash-redirect (planner's call per 10-PATTERNS.md note); mount `` above the calendar content (renders only on needsProviderSetup). In `AppNav.tsx` (desktop) and `BottomTabBar.tsx` (mobile): add an Admin entry (ShieldCheck icon, `aria-label="Admin settings"`) rendered ONLY when `meQuery.data?.user.isAdmin === true` (D-03 UX gating). Write `apps/pwa/e2e/admin.spec.ts`: with the dev-bypass admin user (seeded id=1 is_admin=true), assert the Admin nav entry is visible and /admin renders "Admin Settings" + the Members section. Add a non-admin assertion by route-mocking GET /api/me to `isAdmin:false` (per [[dev-data-user1-no-calendars]] route-mock idiom and the lists.spec page.route precedent) and asserting the Admin nav entry is absent and /admin redirects to /calendar. THEN run a playwright-cli browser check against the dev stack to confirm the guard + nav gating + sheet open behaviors interactively (use the playwright-cli skill — this replaces a human-verify checkpoint since it is desktop-Chromium-drivable per CLAUDE.md). All styling via `var(--token)`; 44px touch targets. + Build `AdminPage.tsx` (analog ListsIndex): "Admin Settings" heading (18px/600), a MEMBERS section listing members from `fetchAdminMembers` (avatar swatch + name + credential status badge per UI-SPEC Surface 2 + a "Rotate"/"Add credential" button opening CredentialSheet in admin mode for that member), and a SHARED CALENDAR section (Surface 5) using `fetchAdminCalendars`: an exclusive single-select radio group (D-06), the "Currently shared" label on the active one, a two-tap "Save" button (disabled until selection differs) calling `setSharedCalendar`, and the empty state ("No calendars synced yet") when none synced. Desktop: centered column maxWidth 640px. In `App.tsx`: add the `/admin` Route (D-02: new dedicated gated route) gated by `meQuery.data?.user.isAdmin ? : ` — the client redirect is UX-only per D-03 (the server-side 403 from Plan 03 is the real boundary); add a loading gate so an in-flight meQuery doesn't flash-redirect (planner's call per 10-PATTERNS.md note); mount `` above the calendar content (renders only on needsProviderSetup). In `AppNav.tsx` (desktop) and `BottomTabBar.tsx` (mobile): add an Admin entry (ShieldCheck icon, `aria-label="Admin settings"`) rendered ONLY when `meQuery.data?.user.isAdmin === true` (D-03 UX gating). + + Verification has two layers, kept distinct: + 1. AUTOMATED GATE (the verify command): write `apps/pwa/e2e/admin.spec.ts` and run `pnpm --filter @familysync/pwa test:e2e -- admin`. With the dev-bypass admin user (seeded id=1 is_admin=true), assert the Admin nav entry is visible and /admin renders "Admin Settings" + the Members section. Add a non-admin assertion by route-mocking GET /api/me to `isAdmin:false` (per [[dev-data-user1-no-calendars]] route-mock idiom and the lists.spec page.route precedent) and asserting the Admin nav entry is absent and /admin redirects to /calendar. THIS e2e SPEC IS THE GATE. + 2. SUPPLEMENTARY (not the gate): run an interactive `playwright-cli` check against the running dev stack at the `/admin` route to confirm the guard redirect + nav gating + CredentialSheet opens — drive the global `/usr/local/bin/playwright-cli` binary per `.claude/skills/playwright-cli/SKILL.md` (navigate to /admin as the dev-bypass admin, confirm "Admin Settings" + open the credential sheet; then with a route-mocked non-admin confirm the redirect to /calendar). Record the playwright-cli observations in the SUMMARY. This is an optional supplementary confirmation; if the dev stack is not up it does not block the plan — the e2e spec is the binding proof. + + All styling via `var(--token)`; 44px touch targets. cd /home/luc/Projects/familysync && pnpm --filter @familysync/pwa exec tsc --noEmit 2>&1 | tail -6 && pnpm --filter @familysync/pwa test:e2e -- admin 2>&1 | tail -20 @@ -149,10 +160,10 @@ Output: Extended client.ts, AdminPage.tsx, CredentialSheet.tsx, SetupBanner.tsx, - `AppNav.tsx` and `BottomTabBar.tsx` render the Admin entry (ShieldCheck, `aria-label="Admin settings"`) ONLY when isAdmin is true (`grep -c "ShieldCheck" apps/pwa/src/components/AppNav.tsx apps/pwa/src/components/BottomTabBar.tsx` >= 2). - `AdminPage.tsx` renders "Admin Settings", a MEMBERS list, and a SHARED CALENDAR exclusive radio group with a two-tap Save and an empty state. - `apps/pwa/e2e/admin.spec.ts` asserts: admin sees the nav entry + reaches /admin; a non-admin (route-mocked isAdmin:false) does NOT see it and /admin redirects to /calendar. - - `pnpm --filter @familysync/pwa test:e2e -- admin` passes; the playwright-cli interactive check confirms the guard + nav gating (recorded in the SUMMARY). + - The e2e spec (`pnpm --filter @familysync/pwa test:e2e -- admin`) passes and IS the gate; the playwright-cli interactive check is a supplementary confirmation (guard + nav gating + sheet open) recorded in the SUMMARY, not the binding proof. - `pnpm --filter @familysync/pwa exec tsc --noEmit` exits 0. - /admin route gated, AdminPage wired to the admin API, conditional nav entries + SetupBanner mounted, e2e + playwright-cli verification green. + /admin route gated, AdminPage wired to the admin API, conditional nav entries + SetupBanner mounted, e2e gate green; playwright-cli supplementary check recorded in the SUMMARY. @@ -189,17 +200,19 @@ New symbols/files created by this plan (excluded from drift verification): - `pnpm --filter @familysync/pwa exec tsc --noEmit` passes. -- `pnpm --filter @familysync/pwa test:e2e -- admin` passes (admin sees nav + /admin; non-admin redirected, no nav entry). -- playwright-cli interactive check confirms the guard + nav gating + sheet open (recorded in SUMMARY). +- `pnpm --filter @familysync/pwa test:e2e -- admin` passes (admin sees nav + /admin; non-admin redirected, no nav entry) — this is the binding gate. +- playwright-cli interactive check confirms the guard + nav gating + sheet open (supplementary, recorded in SUMMARY). - Run the full CI fast-checks gate locally before declaring done (lint + typecheck + test + format:check + md:lint + PWA tests) per [[feedback-run-full-ci-gate-before-push]]. - Admin sees the Admin section, lists members + credential status, rotates a credential via the sheet, picks the shared calendar (Success Criteria 1, 2, 3). - Non-admin never sees the entry and is redirected from /admin (Success Criterion 1; client-side UX over the server 403). -- needsProviderSetup member sees the SetupBanner and can self-serve their own credential (D-07). +- needsProviderSetup member sees the SetupBanner and can self-serve their own credential; the banner clears on a successful save (no dismiss button) (D-07). Create `.planning/phases/10-admin-role-settings/10-04-SUMMARY.md` when done. + + diff --git a/.planning/phases/10-admin-role-settings/10-VALIDATION.md b/.planning/phases/10-admin-role-settings/10-VALIDATION.md index f2033c1..fe052f0 100644 --- a/.planning/phases/10-admin-role-settings/10-VALIDATION.md +++ b/.planning/phases/10-admin-role-settings/10-VALIDATION.md @@ -44,17 +44,17 @@ Notes: | Task ID | Plan | Wave | Requirement | Threat Ref | Secure Behavior | Test Type | Automated Command | File Exists | Status | |---------|------|------|-------------|------------|-----------------|-----------|-------------------|-------------|--------| | 10-01-01 | 01 | 1 | ADMIN-01/02/03 | T-10-01 | schema additive only; no destructive DDL | typecheck | `pnpm --filter @familysync/api exec tsc --noEmit` | ✅ existing | ⬜ pending | -| 10-01-02 | 01 | 1 | ADMIN-01/02/03 | T-10-01 | generate+migrate (never push); live DB columns verified by query | integration (DB) | mysql2 `SHOW COLUMNS`/`SHOW TABLES` assert (see plan verify) | ✅ existing | ⬜ pending | +| 10-01-02 | 01 | 1 | ADMIN-01/02/03 | T-10-01 | db:generate + db:migrate actually RUN (never push); migration SQL additive-only; live DB columns verified by mysql2 query (tsc NOT proof) | integration (DB) | mysql2 `SHOW COLUMNS`/`SHOW TABLES` assert (see plan verify) | ✅ existing | ⬜ pending | | 10-01-03 | 01 | 1 | ADMIN-03 | T-10-03 | dev-bypass admin row scoped to guarded dev/e2e DB only | source | `grep -c is_admin apps/pwa/e2e/global-setup.ts` | ✅ existing | ⬜ pending | | 10-02-01 | 02 | 2 | ADMIN-03 | T-10-04/05/06/07 | requireAdmin 403s non-admin; role from DB not client flag | unit | `pnpm --filter @familysync/api test -- requireAdmin` | ❌ W0 → `apps/api/tests/lib/requireAdmin.test.ts` | ⬜ pending | | 10-02-02 | 02 | 2 | ADMIN-03 | T-10-04 | first-login-wins is_admin; member-count-agnostic | integration (DB) | `pnpm --filter @familysync/api test -- user` | ✅ `apps/api/tests/auth/user.test.ts` (extend) | ⬜ pending | | 10-02-03 | 02 | 2 | ADMIN-03 | T-10-06/07 | /api/me exposes isAdmin + needsProviderSetup (UX-only flag) | integration | `pnpm --filter @familysync/api test -- me` | ✅ `apps/api/tests/routes/me.test.ts` (extend) | ⬜ pending | | 10-03-01 | 03 | 3 | ADMIN-01 | T-10-SC | broker helpers exported; bodies unchanged | unit | `pnpm --filter @familysync/api test -- outbox` | ✅ `apps/api/tests/broker/` | ⬜ pending | -| 10-03-02 | 03 | 3 | ADMIN-01/02/03 | T-10-08/09/10/11/13 | guard-first 403; no password echo/log; encrypted at rest; exclusive is_shared | integration | `pnpm --filter @familysync/api test -- admin` | ❌ W0 → `apps/api/tests/routes/admin.test.ts` | ⬜ pending | -| 10-03-03 | 03 | 3 | ADMIN-01 | T-10-09/10/12 | self-service member-scoped; no cross-member write; no echo | integration | `pnpm --filter @familysync/api test -- credential` | ❌ W0 → in `apps/api/tests/routes/admin.test.ts` | ⬜ pending | +| 10-03-02 | 03 | 3 | ADMIN-01/02/03 | T-10-08/09/10/11/13 | guard-first 403; ALL credential-validation failures → one generic `{error:'Invalid request'}` 400 (no password echo/log); shared credentialSync helper; encrypted at rest; exclusive is_shared | integration | `pnpm --filter @familysync/api test -- admin` | ❌ W0 → `apps/api/tests/routes/admin.test.ts` | ⬜ pending | +| 10-03-03 | 03 | 3 | ADMIN-01 | T-10-09/10/12 | self-service member-scoped; no cross-member write; calls SAME shared validateEncryptAndStoreCredential helper; no echo | integration | `pnpm --filter @familysync/api test -- credential` | ❌ W0 → in `apps/api/tests/routes/admin.test.ts` | ⬜ pending | | 10-04-01 | 04 | 4 | ADMIN-01/02/03 | T-10-14 | MeUser flags UX-only; self-service payload has no userId | typecheck | `pnpm --filter @familysync/pwa exec tsc --noEmit` | ✅ existing | ⬜ pending | -| 10-04-02 | 04 | 4 | ADMIN-01 | T-10-15/16 | password never pre-filled; autocomplete=new-password | typecheck + source | `pnpm --filter @familysync/pwa exec tsc --noEmit` + grep new-password | ✅ existing | ⬜ pending | -| 10-04-03 | 04 | 4 | ADMIN-03 | T-10-14 | /admin redirect for non-admin; nav entry hidden for non-admin | e2e (playwright) | `pnpm --filter @familysync/pwa test:e2e -- admin` | ❌ W0 → `apps/pwa/e2e/admin.spec.ts` | ⬜ pending | +| 10-04-02 | 04 | 4 | ADMIN-01 | T-10-15/16 | password never pre-filled; autocomplete=new-password; SetupBanner clears ONLY on success (['me'] invalidation), no dismiss button | typecheck + source | `pnpm --filter @familysync/pwa exec tsc --noEmit` + grep new-password | ✅ existing | ⬜ pending | +| 10-04-03 | 04 | 4 | ADMIN-03 | T-10-14 | /admin redirect for non-admin; nav entry hidden for non-admin; e2e is the gate, playwright-cli supplementary | e2e (playwright) | `pnpm --filter @familysync/pwa test:e2e -- admin` | ❌ W0 → `apps/pwa/e2e/admin.spec.ts` | ⬜ pending | *Status: ⬜ pending · ✅ green · ❌ red · ⚠️ flaky* @@ -65,8 +65,8 @@ Sampling continuity: every task has an `` verify; no 3 consecutive ta ## Wave 0 Requirements - [ ] `apps/api/tests/lib/requireAdmin.test.ts` — NEW unit tests for the guard (403 non-admin / next() admin / no-user / DB-not-client-flag). Created in Plan 02 Task 1 (RED first). -- [ ] `apps/api/tests/routes/admin.test.ts` — NEW integration tests covering ADMIN-01 (credential validate/encrypt/no-echo), ADMIN-02 (exclusive is_shared), ADMIN-03 (403 for non-admin on every admin route — import `app`, never adminRouter directly), and the member self-service credential cases. Created in Plan 03 Tasks 2 & 3 (RED first). -- [ ] `apps/pwa/e2e/admin.spec.ts` — NEW e2e: admin sees nav entry + reaches /admin; non-admin (route-mocked isAdmin:false) sees no entry and is redirected. Created in Plan 04 Task 3 (RED first). +- [ ] `apps/api/tests/routes/admin.test.ts` — NEW integration tests covering ADMIN-01 (credential validate/encrypt/no-echo, all failure modes → one generic 400), ADMIN-02 (exclusive is_shared), ADMIN-03 (403 for non-admin on every admin route — import `app`, never adminRouter directly), and the member self-service credential cases (shared-helper reuse). Created in Plan 03 Tasks 2 & 3 (RED first). +- [ ] `apps/pwa/e2e/admin.spec.ts` — NEW e2e: admin sees nav entry + reaches /admin; non-admin (route-mocked isAdmin:false) sees no entry and is redirected. Created in Plan 04 Task 3 (RED first; this spec is the binding gate, playwright-cli is supplementary). - [ ] Extend `apps/api/tests/auth/user.test.ts` — first-login-wins cases (Plan 02 Task 2). - [ ] Extend `apps/api/tests/routes/me.test.ts` — isAdmin + needsProviderSetup cases (Plan 02 Task 3). @@ -80,7 +80,7 @@ Existing infrastructure (Vitest + Playwright + real-DB harness + global-setup se |----------|-------------|------------|-------------------| | Live CalDAV PROPFIND against the real Fastmail account on a real credential save | ADMIN-01 | CI/e2e mock CalDAV (dev-bypass user 1 has no Fastmail credential — [[dev-data-user1-no-calendars]]); a true end-to-end save against Fastmail needs a real app password | At go-live, an admin enters a real Fastmail app password in the credential sheet; confirm 200 + the member's calendar appears after the initial sync. Optional operator spot-check, not a phase gate. | -All other phase behaviors (route guard, no-echo, encryption-at-rest, exclusive is_shared, nav gating, /admin redirect) have automated coverage (Vitest + Playwright/playwright-cli — the route guard and nav gating are desktop-Chromium-drivable per CLAUDE.md, so no human checkpoint). +All other phase behaviors (route guard, no-echo, encryption-at-rest, exclusive is_shared, nav gating, /admin redirect) have automated coverage (Vitest + Playwright/playwright-cli — the route guard and nav gating are desktop-Chromium-drivable per CLAUDE.md, so no human checkpoint; playwright-cli is a supplementary confirmation while the e2e spec is the binding gate). --- @@ -94,3 +94,4 @@ All other phase behaviors (route guard, no-echo, encryption-at-rest, exclusive i - [x] `nyquist_compliant: true` set in frontmatter **Approval:** approved 2026-06-13 +