89d343d49a
Move completed design/implementation docs from docs/plans/ to docs/archive/ to keep the plans folder focused on active and future work. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
189 lines
11 KiB
Markdown
189 lines
11 KiB
Markdown
# Permissions & RBAC Audit — Design Document
|
|
|
|
> **Date:** 2026-02-05
|
|
> **Status:** Updated after re-audit
|
|
> **Scope:** Full-stack permissions audit across trees, steps, sessions, teams, and admin features
|
|
|
|
---
|
|
|
|
## Background
|
|
|
|
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.
|
|
|
|
Two rounds of audit were performed:
|
|
1. **Initial audit** — three independent reviews (UX, architecture, adversarial)
|
|
2. **Re-audit** — after RBAC implementation commit (`34daa26`) added `is_super_admin`, `permissions.py`, `usePermissions.ts`, and updated endpoint permission checks
|
|
|
|
**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 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
|
|
|
|
- **Session ownership** is properly enforced — users can only see/modify their own sessions
|
|
- **Folder ownership** is properly scoped to the creating user
|
|
- **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
|
|
- **Category/Tag management** is properly gated to super admin and team admin roles
|
|
- **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 Still Broken
|
|
|
|
The permission system still has critical vulnerabilities, a dead-code centralized module, and significant gaps.
|
|
|
|
---
|
|
|
|
## Findings — Combined Status Table
|
|
|
|
### Legend
|
|
- **FIXED** — Issue fully resolved
|
|
- **PARTIAL** — Infrastructure exists but incomplete or not fully wired
|
|
- **OPEN** — No change from original audit
|
|
|
|
| # | 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. |
|
|
|
|
**Summary: 0 FIXED / 4 PARTIAL / 16 OPEN**
|
|
|
|
---
|
|
|
|
## 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
|
|
async def require_engineer_or_admin(current_user):
|
|
if current_user.is_super_admin:
|
|
return current_user
|
|
if current_user.role not in ("engineer",):
|
|
raise HTTPException(403)
|
|
```
|
|
|
|
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.
|
|
|
|
### N3. 49 Endpoints Bypass `get_current_active_user` (HIGH)
|
|
|
|
**Location:** All endpoint files
|
|
|
|
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.
|
|
|
|
### N4. `role` Field Accepts Arbitrary Strings (MEDIUM)
|
|
|
|
**Location:** `backend/app/schemas/user.py:14`, `backend/app/models/user.py:27`
|
|
|
|
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.
|
|
|
|
### N5. TreeEditorPage Checks `canCreateTrees` Not `canEditTree` (MEDIUM)
|
|
|
|
**Location:** `frontend/src/pages/TreeEditorPage.tsx:81-85`
|
|
|
|
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).
|
|
|
|
---
|
|
|
|
## MSP-Specific Concerns
|
|
|
|
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.
|
|
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.
|
|
|
|
---
|
|
|
|
## Challenged Assumptions
|
|
|
|
| Assumption | Reality |
|
|
|-----------|---------|
|
|
| "Invite codes protect registration" | They don't prevent arbitrary role strings — the role field is unvalidated |
|
|
| "Team scoping = tenant isolation" | Super admin transcends all teams with no boundary |
|
|
| "`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 |
|
|
| "`usePermissions` hook covers the frontend" | Only used in 3 of ~10 components that need it |
|
|
|
|
---
|
|
|
|
## Test Coverage Gaps
|
|
|
|
The existing test suite does NOT cover:
|
|
- Cross-user resource access (user A accessing user B's sessions/folders/steps)
|
|
- Non-admin attempting tree deletion
|
|
- Viewer role restrictions
|
|
- Arbitrary role string at registration
|
|
- Team-based access controls (no team fixtures exist)
|
|
- Step library permission checks (no step tests at all)
|
|
- Category/tag/folder cross-user access
|
|
- `start_session` tree access bypass
|
|
- Concurrent session access across users
|
|
- Deactivated user access (blocked by no `is_active` field)
|
|
|
|
---
|
|
|
|
## References
|
|
|
|
- Centralized permissions (dead code): `backend/app/core/permissions.py`
|
|
- Frontend permissions hook: `frontend/src/hooks/usePermissions.ts`
|
|
- 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/`
|
|
- Tests: `backend/tests/`
|