chihlasm/resolutionflow
1
0
Fork 0
Code Issues 23 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

docs: update permissions audit with re-audit findings

Browse Source 
Re-audited after RBAC commit (34daa26). Key findings:
- permissions.py is dead code (no endpoint imports it)
- require_engineer_or_admin blocks team admins with viewer role
- 49 endpoints bypass get_current_active_user
- 3 critical issues still open (role field, XSS, secret key)
- Updated implementation plan with new Phase B items

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Michael Chihlas
2026-02-05 17:53:25 -05:00
parent 02bd97948e
commit fd8fab97bd
2 changed files with 302 additions and 269 deletions
Download Patch File Download Diff File Expand all files Collapse all files

265
docs/plans/2026-02-05-permissions-audit-design.md
View File

@@ -1,194 +1,151 @@
# Permissions & RBAC Audit — Design Document # Permissions & RBAC Audit — Design Document
> **Date:** 2026-02-05 > **Date:** 2026-02-05
> **Status:** Draft > **Status:** Updated after re-audit
> **Scope:** Full-stack permissions audit across trees, steps, sessions, teams, and admin features > **Scope:** Full-stack permissions audit across trees, steps, sessions, teams, and admin features
--- ---
## Background ## Background
ResolutionFlow has a role-based access control system with three roles (`admin`, `engineer`, `viewer`), a `is_team_admin` boolean, and team-scoped resources. A comprehensive audit was performed across the entire codebase — frontend UX, backend architecture, and adversarial analysis — to assess the current state of permissions enforcement. ResolutionFlow has a role-based access control system with a role hierarchy (`super_admin` > `team_admin` > `engineer` > `viewer`), backed by `is_super_admin` and `is_team_admin` boolean flags on the User model, plus team-scoped resources.
**Audit methodology:** Three independent reviews were conducted: Two rounds of audit were performed:
1. **Frontend UX audit** — every page, component, API client, store, and router 1. **Initial audit** — three independent reviews (UX, architecture, adversarial)
2. **Backend architecture audit** — every endpoint, dependency, model, schema, and test 2. **Re-audit** — after RBAC implementation commit (`34daa26`) added `is_super_admin`, `permissions.py`, `usePermissions.ts`, and updated endpoint permission checks
3. **Devil's advocate critique** — security gaps, edge cases, challenged assumptions
**Audit methodology:** Each round used three specialized reviewers:
- Frontend UX audit — every page, component, API client, store, and router
- Backend architecture audit — every endpoint, dependency, model, schema, and test
- Gap analysis / devil's advocate — security gaps, edge cases, challenged assumptions
--- ---
## Current State Summary ## Current Architecture (Post-RBAC Commit)
### What Was Added in Commit `34daa26`
- `is_super_admin` boolean on User model (migration 010)
- `backend/app/core/permissions.py` — centralized permission functions (role hierarchy, content guards)
- `frontend/src/hooks/usePermissions.ts` — permission hook with role hierarchy
- `require_admin` dependency now checks `is_super_admin` instead of `role == "admin"`
- `require_engineer_or_admin` dependency blocks viewers from tree/step creation
- `usePermissions` hook used in TreeLibraryPage (edit/create gating) and TreeEditorPage (access guard)
- Role badge display in AppLayout header
- `TreeListItem` type includes `team_id` for frontend permission checks
### What Works ### What Works
- **Session ownership** is properly enforced — users can only see/modify their own sessions - **Session ownership** is properly enforced — users can only see/modify their own sessions
- **Folder ownership** is properly scoped to the creating user - **Folder ownership** is properly scoped to the creating user
- **Tree CRUD** has reasonable backend permission checks (engineer+ to create, author/admin/team-admin to edit, admin-only delete) - **Tree CRUD** has backend permission checks (engineer+ to create, author/admin/team-admin to edit, super-admin-only delete)
- **Step Library** enforces visibility levels (private/team/public) and ownership for edit/delete - **Step Library** enforces visibility levels (private/team/public) and ownership for edit/delete
- **Category/Tag management** is properly gated to admin and team admin roles - **Category/Tag management** is properly gated to super admin and team admin roles
- **Invite code management** is admin-only - **Invite code management** is super-admin-only
- **Frontend gating (partial):** Create Tree button hidden from viewers, Edit button checks ownership/team/admin
- **`is_super_admin` flag** prevents the old admin role escalation (flag has `server_default=false`)
### What's Broken ### What's Still Broken
The permission system has critical vulnerabilities, significant gaps, and inconsistencies across resource types. The permission system still has critical vulnerabilities, a dead-code centralized module, and significant gaps.
--- ---
## Findings by Severity ## Findings — Combined Status Table
### CRITICAL ### Legend
- **FIXED** — Issue fully resolved
- **PARTIAL** — Infrastructure exists but incomplete or not fully wired
- **OPEN** — No change from original audit
#### 1. Self-Assignable Admin Role at Registration | # | Severity | Finding | Status | Evidence |
|---|----------|---------|--------|----------|
| 1 | CRITICAL | Self-assignable role at registration | **OPEN** | `UserCreate` still has `role` field (`schemas/user.py:14`). `auth.py:78` passes `user_data.role` to User. No enum validation — arbitrary strings accepted. The `is_super_admin` escalation is blocked (server default false), but `role` field is unvalidated. |
| 2 | CRITICAL | XSS in HTML export | **OPEN** | `sessions.py:349-405` — raw f-string interpolation without `html.escape()`. All user content (tree_name, ticket_number, client_name, question, answer, notes, scratchpad) injected unescaped. |
| 3 | CRITICAL | Default secret key in source code | **OPEN** | `config.py:29` — default string with no production validation. |
| 4 | HIGH | No access control on `start_session` | **OPEN** | `sessions.py:70-109` — checks tree existence and active status only. No tree access check. Any authenticated user can read any tree's full structure via session snapshot. |
| 5 | HIGH | No token revocation | **OPEN** | Logout (`auth.py:188-193`) is still a no-op. JWTs valid until expiry. |
| 6 | HIGH | No account deactivation (`is_active`) | **OPEN** | No `is_active` field. `get_current_active_user` (`deps.py:66-72`) is a no-op. |
| 7 | HIGH | No rate limiting | **OPEN** | No rate limiting on any endpoint. |
| 8 | HIGH | Admin flags cannot be set via API | **PARTIAL** | `is_super_admin` field added with migration 010. But no API endpoint exists to set `is_super_admin` or `is_team_admin`. Must be set via direct DB access. |
| 9 | MEDIUM | Frontend role awareness | **PARTIAL** | `usePermissions` hook exists, used in 3 files (TreeLibraryPage, TreeEditorPage, AppLayout). But: `ProtectedRoute` still auth-only, no route-level role guards, no 403 handling in Axios, no admin pages. |
| 10 | MEDIUM | `None == None` team visibility bug | **PARTIAL** | Fixed in `trees.py:33` (`if current_user.team_id else False`). Still present in local helpers: `steps.py:36`, `categories.py:31`, `step_categories.py:28`. |
| 11 | MEDIUM | Admin list vs get tree inconsistency | **OPEN** | `build_tree_access_filter` has no admin bypass. `get_tree` checks `is_super_admin`. |
| 12 | MEDIUM | No audit logging | **OPEN** | No change. |
| 13 | MEDIUM | No delete UI for trees or steps | **OPEN** | API methods exist, no buttons wired. `canDeleteTree` exists in hook but unused. |
| 14 | MEDIUM | Node deletion without confirmation | **OPEN** | No change. |
| 15 | LOW | Viewer role barely enforced | **PARTIAL** | `require_engineer_or_admin` blocks tree/step creation. Viewers can still create folders, start sessions, etc. |
| 16 | LOW | No password complexity | **OPEN** | No change. |
| 17 | LOW | Soft delete cascade cleanup | **OPEN** | No change. |
| 18 | LOW | Debug endpoint in production | **OPEN** | No change. |
| 19 | LOW | Tag search wildcard escaping | **OPEN** | No change. |
| 20 | LOW | No team existence validation | **OPEN** | No change. |
**Location:** `backend/app/schemas/user.py:14`, `backend/app/api/endpoints/auth.py:74-78` **Summary: 0 FIXED / 4 PARTIAL / 16 OPEN**
The `UserCreate` schema accepts a `role` field from client input. The registration endpoint passes it directly to the User model. Any user with a valid invite code can register with `role: "admin"` and gain full admin privileges. ---
## New Issues Found in Re-Audit
### N1. `permissions.py` is Dead Code (HIGH)
**Location:** `backend/app/core/permissions.py`
The centralized permissions module defines `can_edit_tree`, `can_delete_tree`, `can_edit_step`, `can_manage_category`, `can_manage_tree_tags`, `has_minimum_role`, `get_effective_role`, and `can_create_content`.
**None of these functions are imported or called by any endpoint.** Zero imports across the entire backend.
Each endpoint file (`trees.py`, `steps.py`, `categories.py`, `tags.py`, `step_categories.py`, `folders.py`) defines its own inline permission helpers that are similar but not identical. Key differences:
| Function in `permissions.py` | Local equivalent | Difference |
|------------------------------|-----------------|------------|
| `can_edit_tree` | `trees.py:403-407` inline | Local version doesn't call `can_create_content` |
| `can_manage_category` | `categories.py:25-33` inline | Local version lacks `team_id is not None` guard |
| `can_manage_step_category` | `step_categories.py:22-41` inline | Local version lacks `team_id is not None` guard |
| `has_minimum_role` / `get_effective_role` | Not used anywhere | Entirely dead code |
**Risk:** False confidence that permissions are centralized. Maintaining two diverging copies.
### N2. `require_engineer_or_admin` Blocks Team Admins with Viewer Role (BUG)
**Location:** `backend/app/api/deps.py:87-98`
```python ```python
# Current — VULNERABLE async def require_engineer_or_admin(current_user):
class UserCreate(UserBase): if current_user.is_super_admin:
role: str = Field(default="engineer", ...) return current_user
if current_user.role not in ("engineer",):
# Registration blindly trusts client input raise HTTPException(403)
new_user = User(role=user_data.role, ...)
``` ```
**Impact:** Complete authorization bypass. A single request escalates to admin. Checks `is_super_admin` but NOT `is_team_admin`. A user with `role="viewer"` + `is_team_admin=True` would be blocked from creating trees — contradicting the role hierarchy where team_admin > engineer.
#### 2. XSS in HTML Export ### N3. 49 Endpoints Bypass `get_current_active_user` (HIGH)
**Location:** `backend/app/api/endpoints/sessions.py:349-405` **Location:** All endpoint files
The HTML export function directly interpolates user-provided strings without escaping: 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.
```python
html.append(f'<h1>{tree_name}</h1>')
html.append(f'<p><strong>Ticket:</strong> {session.ticket_number}</p>')
html.append(f'<p><strong>Client:</strong> {session.client_name}</p>')
```
If any field contains `<script>` tags, the exported HTML becomes an XSS payload. Since exports are likely pasted into ticketing systems or emailed, this is a stored XSS vector. ### N4. `role` Field Accepts Arbitrary Strings (MEDIUM)
**Impact:** Arbitrary JavaScript execution in any system that renders the exported HTML. **Location:** `backend/app/schemas/user.py:14`, `backend/app/models/user.py:27`
#### 3. Default Secret Key in Source Code The `role` field is `String(50)` with no DB constraint or schema validation. Users could register with `role="superadmin"`, `role="root"`, etc. While these don't grant permissions (only `is_super_admin` flag does), it creates inconsistent data. The `UserCreate` schema describes "engineer or viewer" but doesn't enforce it.
**Location:** `backend/app/core/config.py:29` ### N5. TreeEditorPage Checks `canCreateTrees` Not `canEditTree` (MEDIUM)
If `.env` is missing or `SECRET_KEY` isn't set, JWTs are signed with a publicly known default string. Anyone can forge admin tokens. **Location:** `frontend/src/pages/TreeEditorPage.tsx:81-85`
**Impact:** Complete authentication bypass if deployed without proper `.env` configuration. The editor page uses `canCreateTrees` (a blanket role check) instead of `canEditTree(tree)` (ownership/team check). A non-author engineer can navigate to `/trees/{someone-else's-id}/edit` and the frontend won't redirect them — the backend will block the save, but the UX is poor (loads editor, then fails on save).
---
### HIGH
#### 4. No Access Control on `start_session`
**Location:** `backend/app/api/endpoints/sessions.py:69-109`
The endpoint checks tree existence and active status but does NOT check if the user has permission to access the tree. Any authenticated user can start a session on any active tree (including private/team-only) if they know the UUID, receiving the full `tree_structure` in the session's `tree_snapshot`.
**Impact:** Information disclosure — bypasses all tree visibility rules.
#### 5. No Token Revocation
**Location:** `backend/app/api/endpoints/auth.py:188-193`
Logout is a client-side no-op. JWTs remain valid until expiration (15 min access, 7 day refresh). Combined with no account deactivation, a terminated employee retains 7 days of access.
**Impact:** Cannot immediately revoke access to client troubleshooting data.
#### 6. No Account Deactivation (`is_active`)
**Location:** `backend/app/api/deps.py:66-72`
The `get_current_active_user` dependency is a no-op — no `is_active` field exists on the User model. The only way to block a user is to delete their database row, which cascades all their data.
**Impact:** No graceful way to disable accounts.
#### 7. No Rate Limiting on Auth Endpoints
Login, registration, and invite code validation have no rate limiting. The unauthenticated `/invites/validate/{code}` endpoint allows unlimited invite code enumeration.
**Impact:** Brute force attacks and invite code enumeration.
#### 8. `is_team_admin` Cannot Be Set
**Location:** `backend/app/models/user.py:28`
The field exists and permission checks reference it, but no API endpoint can set it. The entire "team admin" permission tier is dead code in production.
**Impact:** Category management, step category management, and team-scoped tag management are admin-only in practice.
---
### MEDIUM
#### 9. Frontend Ignores All Roles
**Location:** `frontend/src/components/layout/ProtectedRoute.tsx`, `frontend/src/router.tsx`
`ProtectedRoute` only checks `isAuthenticated`. No role-based route guards, no conditional UI rendering, no admin pages. All users see the same interface regardless of role.
Specific issues:
- Edit button appears on ALL tree cards for ALL users (`TreeLibraryPage.tsx:309-318`)
- "Create Tree" button visible to viewers who will get 403 (`TreeLibraryPage.tsx:146-155`)
- No 403 error handling in the Axios interceptor
- No admin dashboard despite backend admin APIs existing
#### 10. `None == None` Team Visibility Bug
**Location:** `backend/app/api/endpoints/steps.py:29-37`
```python
if step.visibility == 'team':
return step.team_id == user.team_id or user.role == 'admin'
```
When both `step.team_id` and `user.team_id` are `None`, this evaluates to `True`. Users with no team can see "team" steps that also have no team.
#### 11. Admin List vs. Get Tree Inconsistency
`build_tree_access_filter` (used in `list_trees`) does NOT include an admin bypass, but `get_tree` does. Admins can access individual trees by ID but cannot discover them via the list endpoint.
#### 12. No Audit Logging
No record of administrative actions: tree deletion, invite code creation/revocation, category changes, permission modifications, or failed login attempts.
#### 13. No Delete UI for Trees or Steps
`treesApi.delete()` and `stepsApi.delete()` exist in the frontend API client but are never wired to any button or UI component. Trees and steps cannot be deleted from the frontend.
#### 14. Tree Node Deletion Without Confirmation
**Location:** `frontend/src/components/tree-editor/NodeList.tsx:288-298`
Clicking the trash icon in the tree editor immediately deletes the node with no confirmation dialog.
---
### LOW
| # | Issue | Location |
|---|-------|----------|
| 15 | `viewer` role accepted but barely enforced | `schemas/user.py`, all endpoints |
| 16 | No password complexity beyond 10-char minimum | `schemas/user.py:13` |
| 17 | Soft delete doesn't cascade-clean tags, folder memberships | `trees.py` delete endpoint |
| 18 | Debug CORS endpoint exposed in production | `main.py:86-94` |
| 19 | Tag search `ilike` doesn't escape `%` and `_` wildcards | `tags.py:97` |
| 20 | No team existence validation for admin step creation | `steps.py` create endpoint |
--- ---
## MSP-Specific Concerns ## MSP-Specific Concerns
1. **Weak multi-tenancy:** Admin role transcends all teams. Team A's admin could access Team B's client troubleshooting data (ticket numbers, client names, IP addresses in scratchpads). 1. **Weak multi-tenancy:** Super admin transcends all teams. Team A's super admin could access Team B's client troubleshooting data.
2. **No data retention/purge:** Session scratchpads and notes contain sensitive client information with no cleanup mechanism.
2. **No data retention/purge:** Session scratchpads and decision notes contain sensitive client information with no cleanup mechanism. 3. **No team-level session visibility:** Supervisors can't review their team's sessions.
4. **Step Library IP leakage:** Public steps expose team-specific troubleshooting procedures to all teams.
3. **No team-level session visibility:** Supervisors can't review their team's sessions — only the session owner can view them.
4. **Step Library IP leakage:** Public steps expose team-specific troubleshooting procedures (commands, scripts, procedures) to all teams with no approval workflow.
--- ---
@@ -196,11 +153,11 @@ Clicking the trash icon in the tree editor immediately deletes the node with no
| Assumption | Reality | | Assumption | Reality |
|-----------|---------| |-----------|---------|
| "Invite codes protect registration" | They don't prevent role escalation — the role field is independent | | "Invite codes protect registration" | They don't prevent arbitrary role strings — the role field is unvalidated |
| "Team scoping = tenant isolation" | Admin role transcends all teams with no boundary | | "Team scoping = tenant isolation" | Super admin transcends all teams with no boundary |
| "Soft delete is sufficient" | Leaves data artifacts (sessions with tree_snapshot, folder memberships, tag assignments) | | "`permissions.py` centralizes permissions" | It's dead code — no endpoint imports it |
| "JWT statelessness is acceptable" | For an MSP tool with client data, inability to immediately revoke access is a liability | | "JWT statelessness is acceptable" | For an MSP tool with client data, inability to immediately revoke access is a liability |
| "Frontend doesn't need role awareness" | Without it, every user sees affordances they can't use — confusing UX and attacker map | | "`usePermissions` hook covers the frontend" | Only used in 3 of ~10 components that need it |
--- ---
@@ -210,18 +167,22 @@ The existing test suite does NOT cover:
- Cross-user resource access (user A accessing user B's sessions/folders/steps) - Cross-user resource access (user A accessing user B's sessions/folders/steps)
- Non-admin attempting tree deletion - Non-admin attempting tree deletion
- Viewer role restrictions - Viewer role restrictions
- Admin role escalation at registration - Arbitrary role string at registration
- Team-based access controls (no team fixtures exist) - Team-based access controls (no team fixtures exist)
- Step library permission checks (no step tests at all) - Step library permission checks (no step tests at all)
- Category/tag/folder cross-user access - Category/tag/folder cross-user access
- `start_session` tree access bypass
- Concurrent session access across users - Concurrent session access across users
- Deactivated user access (blocked by no `is_active` field)
--- ---
## References ## References
- Frontend source: `frontend/src/` - Centralized permissions (dead code): `backend/app/core/permissions.py`
- Backend endpoints: `backend/app/api/endpoints/` - Frontend permissions hook: `frontend/src/hooks/usePermissions.ts`
- Auth dependencies: `backend/app/api/deps.py` - Auth dependencies: `backend/app/api/deps.py`
- RBAC questions doc: `docs/RBAC-IMPLEMENTATION-QUESTIONS.md`
- Backend endpoints: `backend/app/api/endpoints/`
- Models: `backend/app/models/` - Models: `backend/app/models/`
- Tests: `backend/tests/` - Tests: `backend/tests/`

306
docs/plans/2026-02-05-permissions-audit-implementation.md
View File

@@ -1,9 +1,9 @@
# Permissions & RBAC Fixes — Implementation Plan # Permissions & RBAC Fixes — Implementation Plan
> **Date:** 2026-02-05 > **Date:** 2026-02-05
> **Status:** Draft > **Status:** Updated after re-audit
> **Depends on:** [2026-02-05-permissions-audit-design.md](2026-02-05-permissions-audit-design.md) > **Depends on:** [2026-02-05-permissions-audit-design.md](2026-02-05-permissions-audit-design.md)
> **Scope:** Phased fix of all permission issues identified in the audit > **Scope:** Phased fix of all permission issues identified in audit and re-audit
--- ---
@@ -17,6 +17,17 @@ Fixes are grouped into four phases by severity and dependency:
Each phase is independently deployable. Phase A should be done as a single PR. Each phase is independently deployable. Phase A should be done as a single PR.
### What Changed Since Initial Plan
The RBAC commit (`34daa26`) added infrastructure (`permissions.py`, `usePermissions.ts`, `is_super_admin`), but the re-audit revealed:
- `permissions.py` is dead code — no endpoint imports it
- `require_engineer_or_admin` has a bug blocking team admins with viewer role
- 49 endpoints bypass `get_current_active_user`
- The `role` field still accepts arbitrary strings
- TreeEditorPage uses wrong permission check for edit mode
These new findings are integrated into the phases below.
--- ---
## Phase A: Critical Security Fixes ## Phase A: Critical Security Fixes
@@ -24,33 +35,38 @@ Each phase is independently deployable. Phase A should be done as a single PR.
**Branch:** `fix/critical-security` **Branch:** `fix/critical-security`
**Priority:** Immediate — blocks all other work **Priority:** Immediate — blocks all other work
### A1. Remove Self-Assignable Admin Role ### A1. Lock Down Registration Role Field
**Files to modify:** **Files to modify:**
- `backend/app/schemas/user.py` — Remove `role` from `UserCreate` - `backend/app/schemas/user.py` — Constrain `role` to `Literal["engineer", "viewer"]` or remove entirely
- `backend/app/api/endpoints/auth.py` — Hardcode `role="engineer"` in registration - `backend/app/api/endpoints/auth.py` — Hardcode `role="engineer"` in registration
**Changes:** **Changes:**
```python ```python
# schemas/user.py — Remove role from UserCreate # schemas/user.py — Option A: Remove role from UserCreate entirely
class UserCreate(UserBase): class UserCreate(UserBase):
password: str = Field(..., min_length=10)
# role field REMOVED — always defaults to "engineer" # role field REMOVED — always defaults to "engineer"
# schemas/user.py — Option B: Constrain with Literal (if viewer self-registration is desired)
from typing import Literal
class UserCreate(UserBase):
role: Literal["engineer", "viewer"] = "engineer"
password: str = Field(..., min_length=10) password: str = Field(..., min_length=10)
# auth.py — Hardcode role # auth.py — If using Option A, hardcode:
new_user = User( new_user = User(
... ...
role="engineer", # Always engineer; admin set via admin endpoint only role="engineer", # Always engineer; changed only via admin endpoint
... ...
) )
``` ```
**New endpoint (optional, can defer to Phase B):**
- `PUT /api/v1/admin/users/{user_id}/role` — Admin-only endpoint to change user roles
**Tests to add:** **Tests to add:**
- Attempt to register with `role: "admin"` → should still be engineer - Register with `role: "admin"` → should still be engineer (or 422 with Literal)
- Verify no field in registration request can escalate privileges - Register with `role: "super_admin"` → rejected
- Register with arbitrary string → rejected
- Register with default → engineer
### A2. Escape HTML Export Output ### A2. Escape HTML Export Output
@@ -61,16 +77,21 @@ new_user = User(
```python ```python
from html import escape from html import escape
# Escape all user-provided content # Escape ALL user-provided content in _generate_html_export()
html.append(f'<title>{escape(tree_name)}</title>')
html.append(f'<h1>{escape(tree_name)}</h1>') html.append(f'<h1>{escape(tree_name)}</h1>')
html.append(f'<p><strong>Ticket:</strong> {escape(session.ticket_number or "")}</p>') html.append(f'<p><strong>Ticket:</strong> {escape(session.ticket_number or "")}</p>')
html.append(f'<p><strong>Client:</strong> {escape(session.client_name or "")}</p>') html.append(f'<p><strong>Client:</strong> {escape(session.client_name or "")}</p>')
# ... escape all interpolated values html.append(f'<div class="scratchpad">{escape(scratchpad)}</div>')
html.append(f'<h3>Step {i}: {escape(question)}</h3>')
html.append(f'<p class="answer">Answer: {escape(answer)}</p>')
html.append(f'<p class="notes">Notes: {escape(notes)}</p>')
``` ```
**Tests to add:** **Tests to add:**
- Export session with `<script>` in ticket number → verify escaped in output - Export session with `<script>alert('xss')</script>` in ticket_number → verify `&lt;script&gt;` in output
- Export session with HTML entities in notes → verify escaped - Export session with `<img onerror=...>` in notes → verify escaped
- Export session with `&`, `"`, `'` in fields → verify entities
### A3. Fail on Default Secret Key in Production ### A3. Fail on Default Secret Key in Production
@@ -81,28 +102,33 @@ html.append(f'<p><strong>Client:</strong> {escape(session.client_name or "")}</p
```python ```python
@validator("SECRET_KEY") @validator("SECRET_KEY")
def validate_secret_key(cls, v): def validate_secret_key(cls, v):
if not v or v == "your-secret-key-change-in-production-use-openssl-rand-hex-32": default = "your-secret-key-change-in-production-use-openssl-rand-hex-32"
if not v or v == default:
import os import os
if os.getenv("DEBUG", "true").lower() != "true": if os.getenv("DEBUG", "true").lower() != "true":
raise ValueError("SECRET_KEY must be set to a secure value in production") raise ValueError("SECRET_KEY must be set to a secure value in production")
return v return v
``` ```
### A4. Add Role Enum Constraint ### A4. Add Role Enum Constraint to Database
**Files to modify:** **Files to modify:**
- `backend/app/models/user.py` — Add check constraint or use SQLAlchemy Enum - `backend/app/models/user.py` — Add check constraint
- New migration - New migration
**Changes:** **Changes:**
```python ```python
from sqlalchemy import Enum as SAEnum from sqlalchemy import CheckConstraint
role = Column(SAEnum("admin", "engineer", "viewer", name="user_role"), class User(Base):
default="engineer", nullable=False) __table_args__ = (
CheckConstraint("role IN ('engineer', 'viewer')", name="ck_user_role"),
)
``` ```
**Migration:** `alembic revision --autogenerate -m "Add role enum constraint"` Note: Role is now only `engineer` or `viewer`. The old `admin` role is fully replaced by `is_super_admin` flag (migration 010 already converted existing admin users).
**Migration:** `alembic revision --autogenerate -m "Add role check constraint"`
--- ---
@@ -116,13 +142,13 @@ role = Column(SAEnum("admin", "engineer", "viewer", name="user_role"),
**Files to modify:** **Files to modify:**
- `backend/app/api/endpoints/sessions.py` - `backend/app/api/endpoints/sessions.py`
**Changes:** After fetching the tree, add the same access check used in `get_tree`: **Changes:** After fetching the tree, add access check matching `get_tree`:
```python ```python
can_access = ( can_access = (
tree.is_default or tree.is_public or tree.is_default or tree.is_public or
tree.author_id == current_user.id or tree.author_id == current_user.id or
(tree.team_id == current_user.team_id and current_user.team_id is not None) or (tree.team_id == current_user.team_id and current_user.team_id is not None) or
current_user.role == "admin" current_user.is_super_admin
) )
if not can_access: if not can_access:
raise HTTPException(status_code=403, detail="You don't have access to this tree") raise HTTPException(status_code=403, detail="You don't have access to this tree")
@@ -132,54 +158,90 @@ if not can_access:
- User starts session on own tree → success - User starts session on own tree → success
- User starts session on public tree → success - User starts session on public tree → success
- User starts session on private team tree they don't belong to → 403 - User starts session on private team tree they don't belong to → 403
- Admin starts session on any tree → success - Super admin starts session on any tree → success
### B2. Add `is_active` Field and Account Deactivation ### B2. Wire `permissions.py` Into Endpoints (or Delete It)
**Decision needed:** Wire the centralized module into all endpoints, or delete it and keep local helpers.
**Recommended: Wire it in.** Replace all 6 local permission helpers with imports from `permissions.py`.
**Files to modify:** **Files to modify:**
- `backend/app/models/user.py` — Add `is_active = Column(Boolean, default=True)` - `backend/app/api/endpoints/trees.py` — Import and use `can_edit_tree`, `can_delete_tree`
- `backend/app/api/endpoints/steps.py` — Import and use `can_edit_step`, `can_view_step`
- `backend/app/api/endpoints/tags.py` — Import and use `can_manage_tree_tags`, `can_create_tag`
- `backend/app/api/endpoints/categories.py` — Import and use `can_manage_category`, `can_create_category`
- `backend/app/api/endpoints/step_categories.py` — Import and use `can_manage_step_category`, `can_create_step_category`
- `backend/app/api/endpoints/folders.py` — Import and use `can_access_tree`
- `backend/app/core/permissions.py` — Add any missing functions (`can_view_step`, `can_create_tag`, `can_access_tree`)
**This also fixes:**
- N1 (dead code)
- Finding #10 partial (`None` guards from centralized module applied everywhere)
### B3. Fix `require_engineer_or_admin` Team Admin Bug
**Files to modify:**
- `backend/app/api/deps.py`
**Changes:**
```python
async def require_engineer_or_admin(
current_user: Annotated[User, Depends(get_current_active_user)]
) -> 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 - `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 - New migration
**Changes:** **Changes:**
```python ```python
# deps.py # 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: if not current_user.is_active:
raise HTTPException(status_code=403, detail="Account has been deactivated") raise HTTPException(status_code=403, detail="Account has been deactivated")
return current_user return current_user
``` ```
**Update all endpoints** to use `get_current_active_user` instead of `get_current_user` as their base dependency. ### B5. Create Admin User Management Endpoints
**New endpoint:**
- `PUT /api/v1/admin/users/{user_id}/deactivate` — Admin-only
### B3. Add User Management Endpoints
**New file:** `backend/app/api/endpoints/admin.py` **New file:** `backend/app/api/endpoints/admin.py`
**Endpoints:** **Endpoints:**
| Method | Path | Description | | Method | Path | Description |
|--------|------|-------------| |--------|------|-------------|
| GET | `/api/v1/admin/users` | List all users (admin only) | | GET | `/api/v1/admin/users` | List all users (super admin only) |
| GET | `/api/v1/admin/users/{id}` | Get user details (admin only) | | GET | `/api/v1/admin/users/{id}` | Get user details (super admin only) |
| PUT | `/api/v1/admin/users/{id}/role` | Change user role (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 (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 (admin only) | | PUT | `/api/v1/admin/users/{id}/deactivate` | Deactivate account (super admin only) |
| PUT | `/api/v1/admin/users/{id}/activate` | Reactivate account (admin only) | | PUT | `/api/v1/admin/users/{id}/activate` | Reactivate account (super admin only) |
**Files to modify:** **Files to modify:**
- `backend/app/api/router.py` — Register admin router - `backend/app/api/router.py` — Register admin router
- `backend/app/schemas/user.py` — Add admin response/update schemas - `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:** **Files to modify:**
- `backend/requirements.txt` — Add `slowapi` - `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 - `backend/app/api/endpoints/auth.py` — Apply rate limits
**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 | | `POST /auth/refresh` | 10/minute per IP |
| `GET /invites/validate/{code}` | 5/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:** **Files to modify:**
- `backend/app/models/` — New `RefreshToken` model (token hash, user_id, expires_at, revoked_at) - `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 - `backend/app/core/config.py` — Reduce `ACCESS_TOKEN_EXPIRE_MINUTES` to 5
- New migration - New migration
@@ -204,7 +266,6 @@ async def get_current_active_user(current_user = Depends(get_current_user)):
```python ```python
@router.post("/logout") @router.post("/logout")
async def logout(payload = Depends(get_refresh_token_payload), db = Depends(get_db)): async def logout(payload = Depends(get_refresh_token_payload), db = Depends(get_db)):
# Revoke the refresh token in the database
await db.execute( await db.execute(
update(RefreshToken) update(RefreshToken)
.where(RefreshToken.token_hash == hash_token(payload["jti"])) .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` **Branch:** `feat/permissions-ux`
**Priority:** Better UX and consistency **Priority:** Better UX and consistency
### C1. Frontend Role-Based UI Gating ### C1. Frontend: Add 403 Handling and Route Guards
**Files to modify:** **Files to modify:**
- `frontend/src/api/client.ts` — Add 403 interceptor
- `frontend/src/components/layout/ProtectedRoute.tsx` — Add `requiredRole` prop - `frontend/src/components/layout/ProtectedRoute.tsx` — Add `requiredRole` prop
- `frontend/src/router.tsx` — Apply role guards to admin routes - `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:** **403 interceptor:**
```typescript
// ProtectedRoute with role checking
interface ProtectedRouteProps {
requiredRole?: UserRole | UserRole[]
requireTeamAdmin?: boolean
children: React.ReactNode
}
```
**403 interceptor pattern:**
```typescript ```typescript
if (error.response?.status === 403) { if (error.response?.status === 403) {
toast.error("You don't have permission to perform this action") 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 **ProtectedRoute enhancement:**
```typescript
**Files to modify:** interface ProtectedRouteProps {
- `frontend/src/pages/TreeLibraryPage.tsx` — Add delete button (admin/owner only) requiredRole?: UserRole | UserRole[]
- `frontend/src/components/step-library/StepCard.tsx` — Add delete button (owner/admin only) requireTeamAdmin?: boolean
- `frontend/src/components/common/ConfirmDialog.tsx` — New reusable confirmation modal children: React.ReactNode
}
**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'
``` ```
### 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:** **Files to modify:**
- `backend/app/api/endpoints/trees.py` - `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 ```python
def build_tree_access_filter(current_user: User): def build_tree_access_filter(current_user: User):
conditions = [ conditions = [
@@ -286,17 +355,12 @@ def build_tree_access_filter(current_user: User):
] ]
if current_user.team_id: if current_user.team_id:
conditions.append(Tree.team_id == current_user.team_id) conditions.append(Tree.team_id == current_user.team_id)
if current_user.role == "admin": if current_user.is_super_admin:
conditions.append(True) # Admin sees all conditions.append(sqlalchemy.true())
return or_(*conditions) return or_(*conditions)
``` ```
### C5. Add Node Deletion Confirmation ### C7. Backend: Add Audit Log Table
**Files to modify:**
- `frontend/src/components/tree-editor/NodeList.tsx` — Add confirmation before `deleteNode()`
### C6. Add Audit Log Table
**New file:** `backend/app/models/audit_log.py` **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 ### D1. Define and Enforce Viewer Role Permissions
Decide what viewers can/cannot do and enforce consistently: 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. - **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 step creation, folder creation endpoints. - Add `require_engineer_or_admin` to folder creation and tag creation endpoints.
### D2. Add Password Complexity Validation ### 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 Phase A (Critical) ──── Single PR, immediate
A1. Remove self-assignable admin role A1. Lock down registration role field
A2. Escape HTML export A2. Escape HTML export
A3. Fail on default secret key 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 B1. Tree access check on start_session
B2. is_active field + account deactivation B2. Wire permissions.py into endpoints (fixes dead code + None guards)
B3. User management admin endpoints B3. Fix require_engineer_or_admin team admin bug
B4. Rate limiting B4. is_active field + update all 49 endpoint dependencies
B5. Token revocation B5. Admin user management endpoints
B6. Rate limiting
B7. Token revocation
Phase C (Medium) ──── 3-4 PRs Phase C (Medium) ──── 3-4 PRs
C1. Frontend role-based UI gating C1. Frontend 403 handling + route guards
C2. Delete UI for trees and steps C2. Fix TreeEditorPage canEditTree check
C3. Fix None==None team visibility C3. Delete UI for trees and steps
C4. Fix admin tree list inconsistency C4. Wire usePermissions into all components
C5. Node deletion confirmation C5. Fix None==None in all local helpers (if not done in B2)
C6. Audit log table C6. Fix admin tree list inconsistency
C7. Audit log table
Phase D (Low) ──── As convenient Phase D (Low) ──── As convenient
D1. Viewer role enforcement D1. Viewer role enforcement
@@ -396,8 +463,8 @@ Phase D (Low) ──── As convenient
| Phase | Backend Changes | Frontend Changes | Migrations | New Tests | | Phase | Backend Changes | Frontend Changes | Migrations | New Tests |
|-------|----------------|-----------------|------------|-----------| |-------|----------------|-----------------|------------|-----------|
| A | 4 files modified | 0 | 1 | ~8 tests | | A | 4 files modified | 0 | 1 | ~8 tests |
| B | 6 files modified, 2 new | 0 | 2 | ~15 tests | | B | 10+ files modified, 2 new | 0 | 2 | ~20 tests |
| C | 3 files modified, 2 new | 5 files modified, 1 new | 1 | ~10 tests | | C | 3 files modified, 2 new | 6 files modified, 1 new | 1 | ~10 tests |
| D | 4 files modified | 0 | 0 | ~5 tests | | D | 4 files modified | 0 | 0 | ~5 tests |
--- ---
@@ -405,25 +472,30 @@ Phase D (Low) ──── As convenient
## Testing Strategy ## Testing Strategy
Each phase should include: 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 2. **Integration tests** for cross-user access scenarios
3. **Negative tests** — verify unauthorized access returns 403, not 500 3. **Negative tests** — verify unauthorized access returns 403, not 500
4. Manual verification of frontend behavior per role 4. Manual verification of frontend behavior per role
**Priority test scenarios:** **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 - User A accesses User B's session → 403
- Viewer creates a tree → 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 - Start session on private tree without access → 403
- Login with deactivated account → 403 - Login with deactivated account → 403
- Exported HTML with `<script>` tag → escaped - Exported HTML with `<script>` tag → escaped
- Brute force login → rate limited after 5 attempts - Brute force login → rate limited after 5 attempts
- `permissions.py` functions match endpoint behavior (after B2)
--- ---
## Notes ## Notes
- Phase A changes are backward-compatible — no frontend changes needed - Phase A changes are backward-compatible — no frontend changes needed
- Phase B introduces new admin endpoints — frontend admin pages can follow in Phase C - Phase B is larger than originally planned due to dead code wiring (B2) and dependency chain fix (B4)
- Token revocation (B5) is the most complex change — consider deferring to a dedicated PR - B2 (wiring permissions.py) and B4 (is_active dependency chain) touch many files — plan for careful review
- All new endpoints should follow existing patterns in `deps.py` for consistency - Token revocation (B7) is the most complex change — consider deferring to a dedicated PR
- Phase C partially started: `usePermissions` hook exists, TreeLibraryPage and TreeEditorPage use it
- All `require_admin` checks should use `is_super_admin` (never `role == "admin"`)