diff --git a/.planning/STATE.md b/.planning/STATE.md index a0cc91a..d0380b5 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -4,13 +4,13 @@ milestone: v1.0 milestone_name: milestone current_phase: 2 status: in_progress -last_updated: "2026-05-22T17:55:55Z" +last_updated: "2026-05-22T18:06:00Z" progress: total_phases: 5 completed_phases: 1 total_plans: 10 - completed_plans: 8 - percent: 30 + completed_plans: 9 + percent: 34 --- # Project State @@ -25,7 +25,7 @@ progress: | Phase | Name | Status | |---|---|---| | 1 | Infrastructure Foundation | ✓ Complete | -| 2 | Users & Authentication | In Progress (3/5 plans complete) | +| 2 | Users & Authentication | In Progress (4/5 plans complete) | | 3 | Document Migration & Multi-User Isolation | Not Started | | 4 | Folders, Sharing, Quotas & Document UX | Not Started | | 5 | Cloud Storage Backends | Not Started | @@ -33,8 +33,8 @@ progress: ## Current Position **Phase:** 02-users-authentication — In Progress -**Plan:** 3/5 complete (Plan 03: TOTP enrollment + password reset + account management UI) -**Progress:** ███░░░░░░░ 30% (1/5 phases + 3/5 Phase 2 plans) +**Plan:** 4/5 complete (Plan 04: Admin backend API) +**Progress:** ████░░░░░░ 34% (1/5 phases + 4/5 Phase 2 plans) ## Performance Metrics @@ -43,7 +43,7 @@ progress: | Phases complete | 1 / 5 | | Requirements mapped | 54 / 54 | | Plans written | 5 (Phase 1) | -| Plans complete | 8 (5 Phase 1 + 3 Phase 2) | +| Plans complete | 9 (5 Phase 1 + 4 Phase 2) | ## Accumulated Context @@ -79,6 +79,9 @@ progress: | Deferred Celery import in /password-reset | send_reset_email.delay called via from tasks.email_tasks import send_reset_email inside handler body — same circular-import fix as document_tasks | | TOTP QR code as otpauth:// link | No QR library installed; plan permits manual secret display for MVP; functional flow complete without rendered QR image | | ConfirmBlock no acknowledgment checkbox | ConfirmBlock handles message + button pair; BackupCodesDisplay owns its separate acknowledgment checkbox — no overlap | +| ADMIN-07 enforced by omission | No impersonation endpoint exists; AST check + test_admin_impersonation_not_found verify absence; violates privacy-first core value | +| _user_to_dict() whitelist for admin responses | Explicit field whitelist prevents accidental password_hash/credentials_enc leakage from admin endpoints | +| Quota warning is 200 not 4xx | Below-usage limit change is applied; warning=True advisory field returned — not a rejection | ### Open Questions @@ -97,6 +100,6 @@ _Updated at each phase transition._ | Field | Value | |---|---| -| Last session | 2026-05-22 — Executed Phase 2 Plan 03 (TOTP enrollment + password reset + account management UI) | -| Next action | Run `/gsd:execute-phase 2` to continue Phase 2 (Plan 04: admin endpoints) | +| Last session | 2026-05-22 — Executed Phase 2 Plan 04 (Admin backend API: user CRUD, quota, AI config) | +| Next action | Run `/gsd:execute-phase 2` to continue Phase 2 (Plan 05: admin panel frontend) | | Pending decisions | See Open Questions above | diff --git a/.planning/phases/02-users-authentication/02-04-SUMMARY.md b/.planning/phases/02-users-authentication/02-04-SUMMARY.md new file mode 100644 index 0000000..59ed67e --- /dev/null +++ b/.planning/phases/02-users-authentication/02-04-SUMMARY.md @@ -0,0 +1,165 @@ +--- +phase: 02-users-authentication +plan: 04 +subsystem: auth +tags: [fastapi, admin, user-management, quota, ai-config, tdd, security, role-based-access] + +# Dependency graph +requires: + - phase: 02-users-authentication + plan: 02 + provides: "backend/api/auth.py (auth router, limiter), deps/auth.py (get_current_admin), services/auth.py (hash_password, revoke_all_refresh_tokens, create_password_reset_token)" + - phase: 02-users-authentication + plan: 03 + provides: "backend/config.py: frontend_url setting for reset link construction, tasks/email_tasks.py: send_reset_email.delay pattern" +provides: + - "backend/api/admin.py: GET /api/admin/users, POST /api/admin/users, PATCH /api/admin/users/{id}/status, POST /api/admin/users/{id}/password-reset, GET /api/admin/users/{id}/quota, PATCH /api/admin/users/{id}/quota, PATCH /api/admin/users/{id}/ai-config" + - "backend/tests/test_admin_api.py: 18 tests covering all admin endpoints (role enforcement, password_must_change, no impersonation, quota warning, AI config)" +affects: [02-05, 03-user-document-isolation, 04-folders-sharing-quotas] + +# Tech tracking +tech-stack: + added: [] # No new packages — all dependencies established in Plans 01/02 + patterns: + - "get_current_admin Depends() on every handler — no handler uses get_current_user alone (SEC-07)" + - "_user_to_dict() whitelist helper — explicitly enumerates safe fields; password_hash/credentials_enc excluded by construction" + - "password_must_change=True on admin-created users — forced password change on first login (ADMIN-01)" + - "Sole-admin deactivation guard — counts active admins before applying deactivation; raises 400 if count would reach 0 (T-02-29)" + - "Celery email dispatch with deferred import — send_reset_email.delay inside handler body to avoid circular imports" + - "Quota warning — 200 OK with warning=True when new limit_bytes < used_bytes; change still applies" + - "field_validator on UserCreate.password and QuotaUpdate.limit_bytes — Pydantic v2 style; raises 422 on invalid input" + +key-files: + created: + - "backend/api/admin.py — 7 admin endpoints with get_current_admin on every handler" + - "backend/tests/test_admin_api.py — 18 tests: list/create/deactivate/reset/quota/AI-config/no-impersonation/no-password_hash" + modified: + - "backend/main.py — added admin_router include after auth_router" + +key-decisions: + - "ADMIN-07 enforced by omission: no impersonation route exists; verified by AST check and test_admin_impersonation_not_found" + - "password_hash only appears in admin.py at line 186 (User constructor for DB write, not response); _user_to_dict() whitelist prevents any leakage" + - "Celery mock in test_password_reset_initiates_email to avoid Redis connection in unit tests — patch targets tasks.email_tasks.send_reset_email.delay" + - "Sole-admin guard uses COUNT query on (role='admin', is_active=True) — prevents lockout scenario (T-02-29)" + +patterns-established: + - "Admin router pattern: APIRouter(prefix='/api/admin') + get_current_admin Depends() on every handler" + - "Safe response helper pattern: _user_to_dict() whitelist — use for any multi-field User response" + - "Quota warning pattern: return 200 with warning=True field rather than error — change is applied, caller informed" + +requirements-completed: [ADMIN-01, ADMIN-02, ADMIN-03, ADMIN-04, ADMIN-05, ADMIN-07, SEC-07] + +# Metrics +duration: ~8min +completed: 2026-05-22 +--- + +# Phase 2 Plan 04: Admin Backend API Summary + +**7-endpoint admin API (user CRUD, quota management, AI provider assignment) with get_current_admin on every handler, _user_to_dict whitelist, password_must_change=True on create, and no impersonation endpoint** + +## Performance + +- **Duration:** ~8 min +- **Started:** 2026-05-22T17:58:00Z +- **Completed:** 2026-05-22T18:06:00Z +- **Tasks:** 2 (both TDD — Task 1: admin.py, Task 2: test file) +- **Files created:** 2, Files modified: 1 + +## Accomplishments + +- All 7 admin endpoints implemented in `backend/api/admin.py` with `get_current_admin` on every handler (SEC-07) +- Admin-created users have `password_must_change=True` — forced password change on first login (ADMIN-01, T-02-32) +- `_user_to_dict()` whitelist helper explicitly enumerates safe response fields; `password_hash` and `credentials_enc` are excluded by construction (T-02-27) +- Sole-admin deactivation guard: raises 400 if deactivating the only active admin (T-02-29) +- Password reset via Celery email dispatch — does not return a token, does not impersonate (T-02-30, ADMIN-07) +- Quota warning: 200 OK + `warning=True` when new `limit_bytes` < `used_bytes`; change is applied (ADMIN-04) +- 18 tests passing: role enforcement, create/deactivate/reset/quota/AI-config, no impersonation route, no password_hash in responses +- No impersonation endpoint — ADMIN-07 verified by AST check and dedicated test + +## Task Commits + +1. **Task 1 RED — test file:** `cbad9ac` (test: RED phase admin tests) +2. **Task 1 GREEN — admin.py + main.py:** `f94e8d8` (feat: admin API endpoints) + +## Files Created/Modified + +- `backend/api/admin.py` — 7 admin endpoints; `_user_to_dict()` helper; `_validate_password_strength()`; no impersonation code +- `backend/tests/test_admin_api.py` — 18 tests: list/create/deactivate/reset/quota/AI-config, sole-admin guard, impersonation 404, password_hash absent from responses +- `backend/main.py` — added `from api.admin import router as admin_router` + `app.include_router(admin_router)` + +## Decisions Made + +- **ADMIN-07 by omission**: No `/impersonate` or `/login-as` route. Verified by AST walk checking for `impersonate`/`login_as` string in dump — returns zero matches. `test_admin_impersonation_not_found` asserts 404/422 for `/api/admin/users/impersonate` +- **`password_hash` appears 3 times in admin.py** — lines 55 and 141 are doc comments; line 186 is `password_hash=hash_password(body.password)` in the User constructor (DB write, not API response). `_user_to_dict()` never includes `password_hash` in its whitelist +- **Celery mock in tests**: `patch("tasks.email_tasks.send_reset_email.delay")` used in `test_password_reset_initiates_email` to avoid Redis connection attempt in unit tests — same mock-target pattern as Plan 03 deviation fix +- **Quota warning is 200 not 4xx**: The plan spec says "still apply the update" and return `warning=True`. This is correct — it's an advisory, not a rejection + +## Deviations from Plan + +### Auto-fixed Issues + +**1. [Rule 1 - Test Fix] Celery task causes Redis connection attempt in unit test** +- **Found during:** Task 1 (GREEN phase — first test run) +- **Issue:** `test_password_reset_initiates_email` called the real `send_reset_email.delay()` which triggered Celery trying to connect to Redis (not available in unit tests), causing a RuntimeError after 20 retry attempts +- **Fix:** Added `unittest.mock.patch("tasks.email_tasks.send_reset_email.delay")` context manager in the test; also added assertions that `mock_delay.call_args[0][0] == target.email` (correct address) and `"token=" in call_args[1]` (reset link contains token) +- **Files modified:** `backend/tests/test_admin_api.py` +- **Verification:** All 18 tests pass in 1.5s without Redis +- **Committed in:** `f94e8d8` (GREEN phase commit) + +--- + +**Total deviations:** 1 auto-fixed (Rule 1 — test Celery mock) +**Impact on plan:** Single test-level fix — identical to the Celery mock fix pattern established in Plan 03. No implementation changes. No scope creep. + +## Issues Encountered + +None beyond the Celery mock deviation above. All implementation proceeded cleanly. + +## Known Stubs + +None — all 7 endpoints are fully implemented and returning correct data. + +## Threat Flags + +All STRIDE mitigations from the plan's threat model are implemented: +- T-02-26: `get_current_admin` Depends() on every handler — grep count = 10 (7 handlers + 3 in helper pattern references) +- T-02-27: `_user_to_dict()` whitelist — never includes `password_hash`, `credentials_enc`, or document content +- T-02-28: No impersonation endpoint — AST check + `test_admin_impersonation_not_found` asserts 404/422 +- T-02-29: Sole-admin guard — COUNT query before deactivation; `test_cannot_deactivate_only_admin` asserts 400 +- T-02-30: Password reset endpoint returns 202 + message, not a token; `send_reset_email.delay` goes to user's inbox +- T-02-31: Quota endpoint accepted as admin-visible operational data — no PII, no document content +- T-02-32: `password_must_change=True` set in `POST /api/admin/users`; `test_create_user_sets_password_must_change` verifies DB state + +No new threat surface beyond what was planned. + +## Next Phase Readiness + +- All 7 admin endpoints operational with correct auth enforcement +- Plan 02-05 (admin panel frontend) can wire to real endpoints +- `password_must_change` flag respected by login flow (Plan 02) — admin-created users are forced to set new password on first login +- All 53 auth + admin tests passing + +--- + +## Self-Check: PASSED + +**Files verified:** + +- `backend/api/admin.py` — FOUND, contains `/api/admin/users` route +- `backend/tests/test_admin_api.py` — FOUND (18 tests passing) +- `backend/main.py` — FOUND, contains `admin_router` + +**Commits verified:** + +- `cbad9ac` (test: RED phase) — verified in git log +- `f94e8d8` (feat: admin API) — verified in git log + +**Test results:** 18 passed (test_admin_api.py), 53 passed (all auth + admin tests combined) +**grep -c get_current_admin backend/api/admin.py:** 10 (at least 7 required) +**grep -c password_must_change backend/api/admin.py:** 4 (at least 1 required) +**grep -c impersonate backend/api/admin.py:** 0 (required) + +--- +*Phase: 02-users-authentication* +*Completed: 2026-05-22*