resolutionflow/docs/plans/2026-02-05-permissions-audit-implementation.md

# Permissions & RBAC Fixes — Implementation Plan

> **Date:** 2026-02-05
> **Status:** Draft
> **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

---

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

---

## Phase A: Critical Security Fixes

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

### A1. Remove Self-Assignable Admin Role

**Files to modify:**
- `backend/app/schemas/user.py` — Remove `role` from `UserCreate`
- `backend/app/api/endpoints/auth.py` — Hardcode `role="engineer"` in registration

**Changes:**
```python
# schemas/user.py — Remove role from UserCreate
class UserCreate(UserBase):
    # role field REMOVED — always defaults to "engineer"
    password: str = Field(..., min_length=10)

# auth.py — Hardcode role
new_user = User(
    ...
    role="engineer",  # Always engineer; admin set via admin endpoint only
    ...
)
```

**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:**
- Attempt to register with `role: "admin"` → should still be engineer
- Verify no field in registration request can escalate privileges

### 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
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>')
# ... escape all interpolated values
```

**Tests to add:**
- Export session with `<script>` in ticket number → verify escaped in output
- Export session with HTML entities in notes → verify escaped

### 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):
    if not v or v == "your-secret-key-change-in-production-use-openssl-rand-hex-32":
        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

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

**Changes:**
```python
from sqlalchemy import Enum as SAEnum

role = Column(SAEnum("admin", "engineer", "viewer", name="user_role"),
              default="engineer", nullable=False)
```

**Migration:** `alembic revision --autogenerate -m "Add role enum 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 the same access check used in `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.role == "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
- Admin starts session on any tree → success

### B2. Add `is_active` Field and Account Deactivation

**Files to modify:**
- `backend/app/models/user.py` — Add `is_active = Column(Boolean, default=True)`
- `backend/app/api/deps.py` — Implement `get_current_active_user` check
- New migration

**Changes:**
```python
# deps.py
async def get_current_active_user(current_user = Depends(get_current_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

**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) |

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

**New dependency:** `slowapi` or custom middleware

**Files to modify:**
- `backend/requirements.txt` — Add `slowapi`
- `backend/app/main.py` — Add rate limiter
- `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 |

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

**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/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)):
    # Revoke the refresh token in the database
    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 Role-Based UI Gating

**Files to modify:**
- `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:**
```typescript
if (error.response?.status === 403) {
  toast.error("You don't have permission to perform this action")
  return Promise.reject(error)
}
```

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

### C4. Fix Admin Tree List Inconsistency

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

**Change:** Add 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.role == "admin":
        conditions.append(True)  # Admin sees all
    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

**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, and view steps. Cannot create trees, steps, or manage folders/tags/categories.
- Add `require_engineer_or_admin` to step creation, folder 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. Remove self-assignable admin role
  A2. Escape HTML export
  A3. Fail on default secret key
  A4. Add role enum constraint

Phase B (High) ──── 2-3 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

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

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 | 6 files modified, 2 new | 0 | 2 | ~15 tests |
| C | 3 files modified, 2 new | 5 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 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
- User A accesses User B's session → 403
- Viewer creates a tree → 403
- 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

---

## Notes

- Phase A changes are backward-compatible — no frontend changes needed
- Phase B introduces new admin endpoints — frontend admin pages can follow in Phase C
- Token revocation (B5) is the most complex change — consider deferring to a dedicated PR
- All new endpoints should follow existing patterns in `deps.py` for consistency