# Permissions & RBAC Fixes — Implementation Plan

> **Date:** 2026-02-05
> **Status:** Updated after re-audit
> **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 audit and re-audit

---

## Phasing Strategy

Fixes are grouped into four phases by severity and dependency:
- **Phase A:** Critical security fixes (must-do before any multi-user use)
- **Phase B:** High-severity gaps (needed for MSP readiness)
- **Phase C:** Medium-severity improvements (better UX and consistency)
- **Phase D:** Low-severity cleanup (nice-to-haves)

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

**Branch:** `fix/critical-security`
**Priority:** Immediate — blocks all other work

### A1. Lock Down Registration Role Field

**Files to modify:**
- `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

**Changes:**
```python
# schemas/user.py — Option A: Remove role from UserCreate entirely
class UserCreate(UserBase):
    password: str = Field(..., min_length=10)
    # 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)

# auth.py — If using Option A, hardcode:
new_user = User(
    ...
    role="engineer",  # Always engineer; changed only via admin endpoint
    ...
)
```

**Tests to add:**
- Register with `role: "admin"` → should still be engineer (or 422 with Literal)
- Register with `role: "super_admin"` → rejected
- Register with arbitrary string → rejected
- Register with default → engineer

### A2. Escape HTML Export Output

**Files to modify:**
- `backend/app/api/endpoints/sessions.py` — HTML export function

**Changes:**
```python
from html import escape

# 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'<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'<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:**
- Export session with `<script>alert('xss')</script>` in ticket_number → verify `&lt;script&gt;` in output
- Export session with `<img onerror=...>` in notes → verify escaped
- Export session with `&`, `"`, `'` in fields → verify entities

### A3. Fail on Default Secret Key in Production

**Files to modify:**
- `backend/app/core/config.py`

**Changes:**
```python
@validator("SECRET_KEY")
def validate_secret_key(cls, v):
    default = "your-secret-key-change-in-production-use-openssl-rand-hex-32"
    if not v or v == default:
        import os
        if os.getenv("DEBUG", "true").lower() != "true":
            raise ValueError("SECRET_KEY must be set to a secure value in production")
    return v
```

### A4. Add Role Enum Constraint to Database

**Files to modify:**
- `backend/app/models/user.py` — Add check constraint
- New migration

**Changes:**
```python
from sqlalchemy import CheckConstraint

class User(Base):
    __table_args__ = (
        CheckConstraint("role IN ('engineer', 'viewer')", name="ck_user_role"),
    )
```

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"`

---

## Phase B: High-Severity Gaps

**Branch:** `fix/high-security`
**Priority:** Before production MSP use

### B1. Add Tree Access Check to `start_session`

**Files to modify:**
- `backend/app/api/endpoints/sessions.py`

**Changes:** After fetching the tree, add access check matching `get_tree`:
```python
can_access = (
    tree.is_default or tree.is_public or
    tree.author_id == current_user.id or
    (tree.team_id == current_user.team_id and current_user.team_id is not None) or
    current_user.is_super_admin
)
if not can_access:
    raise HTTPException(status_code=403, detail="You don't have access to this tree")
```

**Tests to add:**
- User starts session on own tree → success
- User starts session on public tree → success
- User starts session on private team tree they don't belong to → 403
- Super admin starts session on any tree → success

### 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:**
- `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
- **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: 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
```

### 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 (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

### B6. Add Rate Limiting

**New dependency:** `slowapi`

**Files to modify:**
- `backend/requirements.txt` — Add `slowapi`
- `backend/app/main.py` — Add rate limiter middleware
- `backend/app/api/endpoints/auth.py` — Apply rate limits

**Rate limits:**
| Endpoint | Limit |
|----------|-------|
| `POST /auth/login` | 5/minute per IP |
| `POST /auth/register` | 3/minute per IP |
| `POST /auth/refresh` | 10/minute per IP |
| `GET /invites/validate/{code}` | 5/minute per IP |

### B7. Token Revocation (Short-Term Fix)

**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 on login, validate on refresh, revoke on logout
- `backend/app/core/config.py` — Reduce `ACCESS_TOKEN_EXPIRE_MINUTES` to 5
- New migration

**Logout becomes meaningful:**
```python
@router.post("/logout")
async def logout(payload = Depends(get_refresh_token_payload), db = Depends(get_db)):
    await db.execute(
        update(RefreshToken)
        .where(RefreshToken.token_hash == hash_token(payload["jti"]))
        .values(revoked_at=datetime.now(timezone.utc))
    )
    return {"message": "Successfully logged out"}
```

---

## Phase C: Medium-Severity Improvements

**Branch:** `feat/permissions-ux`
**Priority:** Better UX and consistency

### 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

**403 interceptor:**
```typescript
if (error.response?.status === 403) {
  toast.error("You don't have permission to perform this action")
  return Promise.reject(error)
}
```

**ProtectedRoute enhancement:**
```typescript
interface ProtectedRouteProps {
  requiredRole?: UserRole | UserRole[]
  requireTeamAdmin?: boolean
  children: React.ReactNode
}
```

### 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 super admin bypass to `build_tree_access_filter`:
```python
def build_tree_access_filter(current_user: User):
    conditions = [
        Tree.is_default == True,
        Tree.is_public == True,
        Tree.author_id == current_user.id,
    ]
    if current_user.team_id:
        conditions.append(Tree.team_id == current_user.team_id)
    if current_user.is_super_admin:
        conditions.append(sqlalchemy.true())
    return or_(*conditions)
```

### C7. Backend: Add Audit Log Table

**New file:** `backend/app/models/audit_log.py`

```python
class AuditLog(Base):
    __tablename__ = "audit_logs"
    id = Column(UUID, primary_key=True, default=uuid4)
    user_id = Column(UUID, ForeignKey("users.id"))
    action = Column(String(50))       # "tree.delete", "user.role_change", etc.
    resource_type = Column(String(50)) # "tree", "step", "user", etc.
    resource_id = Column(UUID)
    details = Column(JSONB)            # Before/after values
    created_at = Column(DateTime(timezone=True), default=lambda: datetime.now(timezone.utc))
```

Integrate into admin endpoints and destructive actions.

---

## Phase D: Low-Severity Cleanup

**Branch:** `chore/permissions-cleanup`
**Priority:** When convenient

### D1. Define and Enforce Viewer Role Permissions

Decide what viewers can/cannot do and enforce consistently:
- **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

**Files to modify:** `backend/app/schemas/user.py`

Add regex validator: at least one uppercase, one lowercase, one digit, 10+ characters.

### D3. Soft Delete Cascade Cleanup

When soft-deleting a tree, also:
- Remove from all folder assignments
- Remove tag assignments
- Keep sessions intact (they have the snapshot)

### D4. Remove Debug Endpoint in Production

**Files to modify:** `backend/app/main.py`

Conditionally register `/debug/cors` only when `DEBUG=true`.

### D5. Escape Wildcards in Tag Search

**Files to modify:** `backend/app/api/endpoints/tags.py`

```python
q_escaped = q.replace("%", "\\%").replace("_", "\\_")
query = select(TreeTag).where(TreeTag.name.ilike(f"%{q_escaped}%"))
```

---

## Execution Order Summary

```
Phase A (Critical) ──── Single PR, immediate
  A1. Lock down registration role field
  A2. Escape HTML export
  A3. Fail on default secret key
  A4. Add role DB constraint

Phase B (High) ──── 3-4 PRs
  B1. Tree access check on start_session
  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 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
  D2. Password complexity
  D3. Soft delete cascade cleanup
  D4. Remove debug endpoint in prod
  D5. Escape wildcards in tag search
```

---

## Estimated Scope

| Phase | Backend Changes | Frontend Changes | Migrations | New Tests |
|-------|----------------|-----------------|------------|-----------|
| A | 4 files modified | 0 | 1 | ~8 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 |

---

## Testing Strategy

Each phase should include:
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"` → 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 `<script>` tag → escaped
- Brute force login → rate limited after 5 attempts
- `permissions.py` functions match endpoint behavior (after B2)

---

## Notes

- Phase A changes are backward-compatible — no frontend changes needed
- Phase B is larger than originally planned due to dead code wiring (B2) and dependency chain fix (B4)
- B2 (wiring permissions.py) and B4 (is_active dependency chain) touch many files — plan for careful review
- 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"`)