docs(20): create phase plan
This commit is contained in:
Lucas Berger
@@ -0,0 +1,184 @@
|
||||
---
|
||||
phase: 20-admin-member-editor-form-declutter
|
||||
plan: 01
|
||||
type: tdd
|
||||
wave: 1
|
||||
depends_on: []
|
||||
files_modified:
|
||||
- apps/api/src/routes/admin.ts
|
||||
- apps/api/tests/routes/admin.test.ts
|
||||
autonomous: true
|
||||
requirements: []
|
||||
must_haves:
|
||||
truths:
|
||||
- "An admin can update a member's display name and admin flag through one route behind requireAdmin"
|
||||
- "Demoting the only remaining admin is rejected with a 409 and the member stays admin"
|
||||
- "Self-demotion succeeds while another admin exists"
|
||||
- "GET /members returns each member's isAdmin so the editor toggle has correct initial state"
|
||||
artifacts:
|
||||
- path: "apps/api/src/routes/admin.ts"
|
||||
provides: "PATCH /api/admin/members/:id member-profile update route + isAdmin in GET /members select"
|
||||
contains: "members/:id"
|
||||
- path: "apps/api/tests/routes/admin.test.ts"
|
||||
provides: "RED tests for last-admin guard + happy-path profile update + isAdmin in GET /members"
|
||||
contains: "last-admin"
|
||||
key_links:
|
||||
- from: "apps/api/src/routes/admin.ts PATCH /members/:id"
|
||||
to: "apps/api/src/db/schema.ts users.isAdmin"
|
||||
via: "last-admin count query + partial update set()"
|
||||
pattern: "users\\.isAdmin"
|
||||
---
|
||||
|
||||
<objective>
|
||||
Add the one new server route this phase needs: `PATCH /api/admin/members/:id`, accepting `displayName` and/or `isAdmin`, behind the existing `requireAdmin` boundary (D-02), and enforce the D-03 last-admin demotion guard (reject 409 when demoting the only admin). Also surface `isAdmin` from `GET /api/admin/members` so the PWA editor's admin toggle has a correct initial state.
|
||||
|
||||
Purpose: This is the only genuinely new backend logic in Phase 20. The last-admin guard is a lockout-safety invariant with a defined request/response contract — written test-first (RED -> GREEN -> REFACTOR).
|
||||
Output: A tested `PATCH /members/:id` route + `isAdmin` field on `GET /members`, both reachable only by admins.
|
||||
</objective>
|
||||
|
||||
<execution_context>
|
||||
@$HOME/.claude/gsd-core/workflows/execute-plan.md
|
||||
@$HOME/.claude/gsd-core/templates/summary.md
|
||||
</execution_context>
|
||||
|
||||
<context>
|
||||
@.planning/PROJECT.md
|
||||
@.planning/ROADMAP.md
|
||||
@.planning/STATE.md
|
||||
@.planning/phases/20-admin-member-editor-form-declutter/20-CONTEXT.md
|
||||
@.planning/phases/20-admin-member-editor-form-declutter/20-PATTERNS.md
|
||||
</context>
|
||||
|
||||
<artifacts_produced>
|
||||
## Artifacts this phase produces (Plan 20-01)
|
||||
- `PATCH /api/admin/members/:id` route handler in `apps/api/src/routes/admin.ts` (member-profile update: displayName and/or isAdmin)
|
||||
- `updateMemberSchema` Zod schema in `apps/api/src/routes/admin.ts`
|
||||
- `isAdmin` field added to the `GET /api/admin/members` select + mapped member object in `apps/api/src/routes/admin.ts`
|
||||
- New `describe` block for `PATCH /members/:id` in `apps/api/tests/routes/admin.test.ts` (last-admin guard, happy path, auth boundary, validation, 404)
|
||||
</artifacts_produced>
|
||||
|
||||
<tasks>
|
||||
|
||||
<task type="tdd" tdd="true">
|
||||
<name>Task 1: RED — failing tests for the member-profile route + isAdmin read</name>
|
||||
<files>apps/api/tests/routes/admin.test.ts</files>
|
||||
<read_first>
|
||||
- apps/api/tests/routes/admin.test.ts (full — copy the jsonRequest helper, admin/non-admin session setup, member-create flow at lines ~920-990, and the existing password-reset test at line ~956 as the structural analog)
|
||||
- apps/api/src/routes/admin.ts (lines 95-130 GET /members handler; lines 225-269 POST /members/:id/password as the route analog; lines 75-92 noEchoHook + parsePositiveIntParam)
|
||||
- apps/api/src/auth/user.ts (lines 140-156 — the admin-count query to adapt for the guard)
|
||||
- apps/api/src/db/schema.ts (line ~56 users.isAdmin column)
|
||||
- .planning/phases/20-admin-member-editor-form-declutter/20-CONTEXT.md (D-02, D-03)
|
||||
</read_first>
|
||||
<behavior>
|
||||
- Test A (happy path displayName): PATCH /api/admin/members/:id with { displayName: 'New Name' } as an admin -> 200; GET /members reflects the new displayName.
|
||||
- Test B (happy path isAdmin promote): a non-admin member PATCH'd with { isAdmin: true } -> 200; GET /members shows isAdmin true for that member.
|
||||
- Test C (last-admin guard): with exactly ONE admin in the DB, PATCH that admin with { isAdmin: false } -> 409, body has an `error` string; GET /members still shows that member isAdmin true (unchanged).
|
||||
- Test D (self-demotion allowed when another admin exists): seed two admins, PATCH one with { isAdmin: false } -> 200; GET /members shows one admin remaining.
|
||||
- Test E (auth boundary): PATCH /api/admin/members/:id as a non-admin session -> 403 (inherits requireAdmin; no second guard).
|
||||
- Test F (validation): PATCH with { isAdmin: 'yes' } (wrong type) -> 400 { error: 'Invalid request' } via noEchoHook; malformed :id (e.g. '1abc') -> 400.
|
||||
- Test G (not found): PATCH a non-existent member id -> 404.
|
||||
- Test H (GET isAdmin field): GET /api/admin/members as admin -> each member object includes a boolean `isAdmin` field.
|
||||
</behavior>
|
||||
<action>
|
||||
Add a new `describe` block to apps/api/tests/routes/admin.test.ts for `PATCH /api/admin/members/:id` plus one assertion in the existing GET /members test for the `isAdmin` field. Reuse the file's existing `jsonRequest('PATCH', path, body)` helper, admin/non-admin session injection, and the member-create helper used by the password-reset test (~line 956). Seed admins by inserting `users` rows with `isAdmin: true`. Assert response status codes and that `GET /members` reflects (or does NOT reflect, for the guard case) the change. Use the real-DB integration pattern already in this file (DB_HOST=127.0.0.1). Run the suite and confirm these new tests FAIL because neither the route nor the `isAdmin` field exists yet. Do NOT implement the route in this task. Commit: `test(20-01): add failing tests for member-profile update + last-admin guard + isAdmin read`.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd apps/api && DB_HOST=127.0.0.1 pnpm vitest run tests/routes/admin.test.ts 2>&1 | grep -Ei 'fail|members/:id' | head</automated>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- New tests for PATCH /members/:id exist in apps/api/tests/routes/admin.test.ts and reference both `displayName` and `isAdmin`.
|
||||
- Running the suite shows the new tests FAILING (route 404 / no isAdmin field) — RED confirmed.
|
||||
- A test asserts a 409 for last-admin demotion and a separate test asserts 200 self-demotion with a second admin present.
|
||||
- Commit message starts with `test(20-01):`.
|
||||
</acceptance_criteria>
|
||||
<done>The new tests are committed and fail for the right reason (route + field not implemented).</done>
|
||||
</task>
|
||||
|
||||
<task type="tdd" tdd="true">
|
||||
<name>Task 2: GREEN — implement PATCH /members/:id with last-admin guard + isAdmin in GET /members</name>
|
||||
<files>apps/api/src/routes/admin.ts</files>
|
||||
<read_first>
|
||||
- apps/api/src/routes/admin.ts (lines 95-130 GET /members; lines 225-269 POST /members/:id/password analog; line 47 requireAdmin mount; lines 75-92 noEchoHook + parsePositiveIntParam; line 28 eq/sql imports)
|
||||
- apps/api/src/auth/user.ts (lines 140-156 admin-count query)
|
||||
- .planning/phases/20-admin-member-editor-form-declutter/20-PATTERNS.md ("NEW PATCH /members/:id" section — route handler shape + last-admin guard excerpt)
|
||||
</read_first>
|
||||
<action>
|
||||
In apps/api/src/routes/admin.ts:
|
||||
(1) Add `isAdmin: users.isAdmin` to the `GET /members` select and `isAdmin: row.isAdmin` to the mapped member object (no join change — `users.isAdmin` is a base-table column).
|
||||
(2) Add a Zod schema `updateMemberSchema` = object with `displayName` (string min 1 max 256, optional) and `isAdmin` (boolean, optional).
|
||||
(3) Register `adminRouter.patch('/members/:id', zValidator('json', updateMemberSchema, noEchoHook), handler)`. The router-wide `requireAdmin` (line 47) already protects it — add NO second guard (D-02).
|
||||
(4) Handler: parse the id with the existing `parsePositiveIntParam` (400 on null). Verify the target `users` row exists (404 if not). For the last-admin guard (D-03): when `isAdmin === false` is requested AND the target is currently an admin, run the admin-count query (`COUNT(*)` over `users` WHERE `users.isAdmin` is true, adapted from auth/user.ts:151-156) and return 409 `{ error: 'Cannot remove the last admin' }` when count is at most 1. Otherwise build a partial `set({ ... })` from whichever of `displayName`/`isAdmin` is present and `db.update(users)...where(eq(users.id, targetId))`. Mirror the password route's try/catch -> 503 fallback. Return 200 `{ ok: true }`. Apply `noEchoHook` for consistency. Do NOT log the request body.
|
||||
Run the suite; all Task 1 tests must pass. Commit: `feat(20-01): add PATCH /members/:id member-profile update with last-admin guard`.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd apps/api && DB_HOST=127.0.0.1 pnpm vitest run tests/routes/admin.test.ts 2>&1 | tail -20</automated>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- `grep -n "patch('/members/:id'" apps/api/src/routes/admin.ts` returns the new route registration.
|
||||
- `grep -nE "isAdmin: *users\.isAdmin" apps/api/src/routes/admin.ts` confirms isAdmin added to GET /members select.
|
||||
- All new Task 1 tests pass (GREEN); the last-admin demotion test returns 409 and the member stays admin.
|
||||
- No second `requireAdmin` call added in the PATCH handler (boundary inherited per D-02).
|
||||
- Commit message starts with `feat(20-01):`.
|
||||
</acceptance_criteria>
|
||||
<done>PATCH /members/:id and the isAdmin read field both implemented; full admin.test.ts suite green.</done>
|
||||
</task>
|
||||
|
||||
<task type="tdd" tdd="true">
|
||||
<name>Task 3: REFACTOR — tidy + pass CI gates</name>
|
||||
<files>apps/api/src/routes/admin.ts</files>
|
||||
<read_first>
|
||||
- apps/api/src/routes/admin.ts (the new route + GET /members edits from Task 2)
|
||||
- /home/luc/.claude/projects/-home-luc-projects-familysync/memory/MEMORY.md ("CI checks conformance" entry)
|
||||
</read_first>
|
||||
<action>
|
||||
Review the new route for duplication with the password route (the shared id-parse / existence-check shape is fine to keep inline — do not over-extract). Ensure the route's doc-header banner comment matches the style of the sibling routes' headers (the file documents each route in a banner comment). Run the API CI gates locally: typecheck, eslint, prettier. Fix any violations. Commit (only if changes): `refactor(20-01): tidy member-profile route + pass api gates`.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd /home/luc/projects/familysync && pnpm --filter @familysync/api exec tsc --noEmit && pnpm --filter @familysync/api exec eslint src/routes/admin.ts && pnpm exec prettier --check apps/api/src/routes/admin.ts apps/api/tests/routes/admin.test.ts</automated>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- typecheck passes (tsc --noEmit exit 0).
|
||||
- eslint passes on apps/api/src/routes/admin.ts (exit 0).
|
||||
- prettier --check passes on both modified files.
|
||||
- Full admin.test.ts suite still green.
|
||||
</acceptance_criteria>
|
||||
<done>All API CI gates pass locally for the modified files; suite green.</done>
|
||||
</task>
|
||||
|
||||
</tasks>
|
||||
|
||||
<threat_model>
|
||||
## Trust Boundaries
|
||||
|
||||
| Boundary | Description |
|
||||
|----------|-------------|
|
||||
| client -> /api/admin | Untrusted admin-session input crosses here; already guarded by router-wide `requireAdmin` (line 47). No NEW boundary added (D-02). |
|
||||
|
||||
## STRIDE Threat Register
|
||||
|
||||
| Threat ID | Category | Component | Disposition | Mitigation Plan |
|
||||
|-----------|----------|-----------|-------------|-----------------|
|
||||
| T-20-01 | Elevation of Privilege | PATCH /members/:id isAdmin toggle | mitigate | Route inherits router-wide `requireAdmin`; no second/weaker guard added. Test E asserts 403 for non-admin. |
|
||||
| T-20-02 | Denial of Service (self-lockout) | last-admin demotion | mitigate | D-03 guard: count admins, reject 409 when demoting the only admin (Test C). Break-glass CLI (19-D-13) remains true recovery path. |
|
||||
| T-20-03 | Tampering | malformed :id / wrong-type body | mitigate | `parsePositiveIntParam` rejects non-positive-int ids (400); `updateMemberSchema` + `noEchoHook` reject wrong types as 400 `{ error: 'Invalid request' }` (Test F). |
|
||||
| T-20-04 | Information Disclosure | error echo on invalid input | mitigate | `noEchoHook` returns only `{ error: 'Invalid request' }`; request body never logged (preserves T-10-15/16 posture). |
|
||||
</threat_model>
|
||||
|
||||
<verification>
|
||||
- `cd apps/api && DB_HOST=127.0.0.1 pnpm vitest run tests/routes/admin.test.ts` — full admin suite green including new PATCH tests.
|
||||
- `grep -n "patch('/members/:id'" apps/api/src/routes/admin.ts` — route registered.
|
||||
- Last-admin demotion returns 409; member remains admin in a follow-up GET.
|
||||
- API typecheck + eslint + prettier gates pass.
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
- PATCH /api/admin/members/:id updates displayName and/or isAdmin behind requireAdmin.
|
||||
- Demoting the only admin returns 409 and leaves the admin flag set.
|
||||
- Self-demotion with a second admin present returns 200.
|
||||
- GET /api/admin/members returns a boolean `isAdmin` per member.
|
||||
- All API CI gates pass for the modified files.
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
Create `.planning/phases/20-admin-member-editor-form-declutter/20-01-SUMMARY.md` when done.
|
||||
</output>
|
||||
Reference in New Issue
Block a user