Ticket: {session.ticket_number}
') -html.append(f'Client: {session.client_name}
') -``` +Most endpoints depend directly on `get_current_user`, bypassing `get_current_active_user`. If `is_active` were implemented, deactivated users could still: list/view sessions, export, update scratchpads, manage folders, list/view categories, tags, steps, rate steps, etc. -If any field contains `` in ticket_number → verify `<script>` in output +- Export session with `Client: {escape(session.client_name or "")}
User: + if current_user.is_super_admin: + return current_user + if current_user.is_team_admin and current_user.team_id is not None: + return current_user + if current_user.role not in ("engineer",): + raise HTTPException(status_code=403, detail="Requires engineer or higher role") + return current_user +``` + +### B4. Add `is_active` Field and Update All Dependencies + +**Files to modify:** +- `backend/app/models/user.py` — Add `is_active = Column(Boolean, default=True, server_default="true")` - `backend/app/api/deps.py` — Implement `get_current_active_user` check +- **All endpoint files** — Change `Depends(get_current_user)` to `Depends(get_current_active_user)` (49 usages) - New migration **Changes:** ```python # deps.py -async def get_current_active_user(current_user = Depends(get_current_user)): +async def get_current_active_user( + current_user: Annotated[User, Depends(get_current_user)] +) -> User: if not current_user.is_active: raise HTTPException(status_code=403, detail="Account has been deactivated") return current_user ``` -**Update all endpoints** to use `get_current_active_user` instead of `get_current_user` as their base dependency. - -**New endpoint:** -- `PUT /api/v1/admin/users/{user_id}/deactivate` — Admin-only - -### B3. Add User Management Endpoints +### B5. Create Admin User Management Endpoints **New file:** `backend/app/api/endpoints/admin.py` **Endpoints:** | Method | Path | Description | |--------|------|-------------| -| GET | `/api/v1/admin/users` | List all users (admin only) | -| GET | `/api/v1/admin/users/{id}` | Get user details (admin only) | -| PUT | `/api/v1/admin/users/{id}/role` | Change user role (admin only) | -| PUT | `/api/v1/admin/users/{id}/team-admin` | Toggle is_team_admin (admin only) | -| PUT | `/api/v1/admin/users/{id}/deactivate` | Deactivate account (admin only) | -| PUT | `/api/v1/admin/users/{id}/activate` | Reactivate account (admin only) | +| GET | `/api/v1/admin/users` | List all users (super admin only) | +| GET | `/api/v1/admin/users/{id}` | Get user details (super admin only) | +| PUT | `/api/v1/admin/users/{id}/role` | Change user role (super admin only) | +| PUT | `/api/v1/admin/users/{id}/team-admin` | Toggle is_team_admin (super admin only) | +| PUT | `/api/v1/admin/users/{id}/deactivate` | Deactivate account (super admin only) | +| PUT | `/api/v1/admin/users/{id}/activate` | Reactivate account (super admin only) | **Files to modify:** - `backend/app/api/router.py` — Register admin router - `backend/app/schemas/user.py` — Add admin response/update schemas -### B4. Add Rate Limiting +### B6. Add Rate Limiting -**New dependency:** `slowapi` or custom middleware +**New dependency:** `slowapi` **Files to modify:** - `backend/requirements.txt` — Add `slowapi` -- `backend/app/main.py` — Add rate limiter +- `backend/app/main.py` — Add rate limiter middleware - `backend/app/api/endpoints/auth.py` — Apply rate limits **Rate limits:** @@ -190,13 +252,13 @@ async def get_current_active_user(current_user = Depends(get_current_user)): | `POST /auth/refresh` | 10/minute per IP | | `GET /invites/validate/{code}` | 5/minute per IP | -### B5. Token Revocation (Short-Term Fix) +### B7. Token Revocation (Short-Term Fix) -**Approach:** Switch to short-lived access tokens (5 min) and store refresh tokens in the database so they can be revoked. +**Approach:** Store refresh tokens in DB so they can be revoked. Reduce access token TTL. **Files to modify:** - `backend/app/models/` — New `RefreshToken` model (token hash, user_id, expires_at, revoked_at) -- `backend/app/api/endpoints/auth.py` — Store refresh tokens on login, validate on refresh, delete on logout +- `backend/app/api/endpoints/auth.py` — Store on login, validate on refresh, revoke on logout - `backend/app/core/config.py` — Reduce `ACCESS_TOKEN_EXPIRE_MINUTES` to 5 - New migration @@ -204,7 +266,6 @@ async def get_current_active_user(current_user = Depends(get_current_user)): ```python @router.post("/logout") async def logout(payload = Depends(get_refresh_token_payload), db = Depends(get_db)): - # Revoke the refresh token in the database await db.execute( update(RefreshToken) .where(RefreshToken.token_hash == hash_token(payload["jti"])) @@ -220,25 +281,14 @@ async def logout(payload = Depends(get_refresh_token_payload), db = Depends(get_ **Branch:** `feat/permissions-ux` **Priority:** Better UX and consistency -### C1. Frontend Role-Based UI Gating +### C1. Frontend: Add 403 Handling and Route Guards **Files to modify:** +- `frontend/src/api/client.ts` — Add 403 interceptor - `frontend/src/components/layout/ProtectedRoute.tsx` — Add `requiredRole` prop - `frontend/src/router.tsx` — Apply role guards to admin routes -- `frontend/src/pages/TreeLibraryPage.tsx` — Conditionally show create/edit buttons -- `frontend/src/api/client.ts` — Add 403 handling in Axios interceptor -**New component:** -```typescript -// ProtectedRoute with role checking -interface ProtectedRouteProps { - requiredRole?: UserRole | UserRole[] - requireTeamAdmin?: boolean - children: React.ReactNode -} -``` - -**403 interceptor pattern:** +**403 interceptor:** ```typescript if (error.response?.status === 403) { toast.error("You don't have permission to perform this action") @@ -246,37 +296,56 @@ if (error.response?.status === 403) { } ``` -### C2. Add Delete UI for Trees and Steps - -**Files to modify:** -- `frontend/src/pages/TreeLibraryPage.tsx` — Add delete button (admin/owner only) -- `frontend/src/components/step-library/StepCard.tsx` — Add delete button (owner/admin only) -- `frontend/src/components/common/ConfirmDialog.tsx` — New reusable confirmation modal - -**Visibility rules:** -- Tree delete button: visible to admin only (matches backend) -- Step delete button: visible to step owner and admin -- Both require confirmation dialog before API call - -### C3. Fix `None == None` Team Visibility Bug - -**Files to modify:** -- `backend/app/api/endpoints/steps.py` - -**Change:** -```python -def can_view_step(user: User, step: StepLibrary) -> bool: - if step.visibility == 'team': - return (step.team_id is not None - and step.team_id == user.team_id) or user.role == 'admin' +**ProtectedRoute enhancement:** +```typescript +interface ProtectedRouteProps { + requiredRole?: UserRole | UserRole[] + requireTeamAdmin?: boolean + children: React.ReactNode +} ``` -### C4. Fix Admin Tree List Inconsistency +### C2. Frontend: Fix TreeEditorPage Permission Check + +**Files to modify:** +- `frontend/src/pages/TreeEditorPage.tsx` + +**Change:** After loading the tree in edit mode, check `canEditTree(tree)` instead of just `canCreateTrees`: +```typescript +useEffect(() => { + if (tree && id && !canEditTree({ author_id: tree.author_id, team_id: tree.team_id })) { + navigate('/trees') + } +}, [tree, id]) +``` + +### C3. Frontend: Add Delete UI for Trees and Steps + +**Files to modify:** +- `frontend/src/pages/TreeLibraryPage.tsx` — Add delete button gated by `canDeleteTree` +- `frontend/src/components/step-library/StepCard.tsx` — Add delete button gated by `canEditStep` +- `frontend/src/components/common/ConfirmDialog.tsx` — New reusable confirmation modal + +### C4. Frontend: Wire `usePermissions` Into All Relevant Components + +**Files to modify:** +- `frontend/src/components/step-library/StepLibraryBrowser.tsx` — Use `canCreateSteps` for create button +- `frontend/src/components/step-library/CustomStepModal.tsx` — Check `canCreateSteps` +- `frontend/src/components/tree-editor/NodeList.tsx` — Add confirmation dialog for node deletion + +### C5. Backend: Fix `None == None` in All Local Helpers + +**Files to modify (if not already fixed by B2):** +- `backend/app/api/endpoints/steps.py` — `can_view_step()` null guard +- `backend/app/api/endpoints/categories.py` — `can_manage_category()` null guard +- `backend/app/api/endpoints/step_categories.py` — `can_manage_step_category()` null guard + +### C6. Backend: Fix Admin Tree List Inconsistency **Files to modify:** - `backend/app/api/endpoints/trees.py` -**Change:** Add admin bypass to `build_tree_access_filter`: +**Change:** Add super admin bypass to `build_tree_access_filter`: ```python def build_tree_access_filter(current_user: User): conditions = [ @@ -286,17 +355,12 @@ def build_tree_access_filter(current_user: User): ] if current_user.team_id: conditions.append(Tree.team_id == current_user.team_id) - if current_user.role == "admin": - conditions.append(True) # Admin sees all + if current_user.is_super_admin: + conditions.append(sqlalchemy.true()) return or_(*conditions) ``` -### C5. Add Node Deletion Confirmation - -**Files to modify:** -- `frontend/src/components/tree-editor/NodeList.tsx` — Add confirmation before `deleteNode()` - -### C6. Add Audit Log Table +### C7. Backend: Add Audit Log Table **New file:** `backend/app/models/audit_log.py` @@ -324,8 +388,8 @@ Integrate into admin endpoints and destructive actions. ### D1. Define and Enforce Viewer Role Permissions Decide what viewers can/cannot do and enforce consistently: -- **Proposed:** Viewers can browse trees, start sessions, and view steps. Cannot create trees, steps, or manage folders/tags/categories. -- Add `require_engineer_or_admin` to step creation, folder creation endpoints. +- **Proposed:** Viewers can browse trees, start sessions, view steps, and rate steps. Cannot create trees, steps, folders, or manage tags/categories. +- Add `require_engineer_or_admin` to folder creation and tag creation endpoints. ### D2. Add Password Complexity Validation @@ -361,25 +425,28 @@ query = select(TreeTag).where(TreeTag.name.ilike(f"%{q_escaped}%")) ``` Phase A (Critical) ──── Single PR, immediate - A1. Remove self-assignable admin role + A1. Lock down registration role field A2. Escape HTML export A3. Fail on default secret key - A4. Add role enum constraint + A4. Add role DB constraint -Phase B (High) ──── 2-3 PRs +Phase B (High) ──── 3-4 PRs B1. Tree access check on start_session - B2. is_active field + account deactivation - B3. User management admin endpoints - B4. Rate limiting - B5. Token revocation + B2. Wire permissions.py into endpoints (fixes dead code + None guards) + B3. Fix require_engineer_or_admin team admin bug + B4. is_active field + update all 49 endpoint dependencies + B5. Admin user management endpoints + B6. Rate limiting + B7. Token revocation Phase C (Medium) ──── 3-4 PRs - C1. Frontend role-based UI gating - C2. Delete UI for trees and steps - C3. Fix None==None team visibility - C4. Fix admin tree list inconsistency - C5. Node deletion confirmation - C6. Audit log table + C1. Frontend 403 handling + route guards + C2. Fix TreeEditorPage canEditTree check + C3. Delete UI for trees and steps + C4. Wire usePermissions into all components + C5. Fix None==None in all local helpers (if not done in B2) + C6. Fix admin tree list inconsistency + C7. Audit log table Phase D (Low) ──── As convenient D1. Viewer role enforcement @@ -396,8 +463,8 @@ Phase D (Low) ──── As convenient | Phase | Backend Changes | Frontend Changes | Migrations | New Tests | |-------|----------------|-----------------|------------|-----------| | A | 4 files modified | 0 | 1 | ~8 tests | -| B | 6 files modified, 2 new | 0 | 2 | ~15 tests | -| C | 3 files modified, 2 new | 5 files modified, 1 new | 1 | ~10 tests | +| B | 10+ files modified, 2 new | 0 | 2 | ~20 tests | +| C | 3 files modified, 2 new | 6 files modified, 1 new | 1 | ~10 tests | | D | 4 files modified | 0 | 0 | ~5 tests | --- @@ -405,25 +472,30 @@ Phase D (Low) ──── As convenient ## Testing Strategy Each phase should include: -1. **Unit tests** for new permission checks +1. **Unit tests** for new/changed permission checks 2. **Integration tests** for cross-user access scenarios 3. **Negative tests** — verify unauthorized access returns 403, not 500 4. Manual verification of frontend behavior per role **Priority test scenarios:** -- Register with `role: admin` → must be engineer +- Register with `role: "admin"` → rejected or ignored +- Register with arbitrary role string → rejected - User A accesses User B's session → 403 - Viewer creates a tree → 403 +- Team admin with viewer role creates a tree → success (B3 fix) - Start session on private tree without access → 403 - Login with deactivated account → 403 - Exported HTML with `