# Permission System Refactor — Tri-Agent Analysis

**Date:** 2026-04-03
**Input:** `permission_research.pdf` (SF -> NexusCRM permission migration research)
**Method:** 3 parallel agents (Architect + Security Reviewer + Critic) independently explored codebase to verify report claims

---

## Executive Summary

The research report analyzed only the frontend JS bundle without reviewing backend Go code, leading to systematic misjudgment. At least 4 of 8 claimed problems are based on incorrect premises. The backend already has a mature RBAC system (PermissionLoader + RequirePermission middleware, Role Hierarchy memory cache, 5-layer Record Access checks, FLS, Sharing Rules). The report's claim that "the browser connects directly to TiDB" is factually wrong — a full Go backend (Gin) proxy exists.

However, the agents discovered **3 real security vulnerabilities** the report missed entirely.

---

## Report Claims vs Code Reality

| # | Report Claim | Verdict | Evidence |
|---|-------------|---------|----------|
| 1 | JWT lacks role_id and permissions | **Partially outdated** | JWT already contains `role_id` (`jwt.go:25`); userinfo already returns `permissions` (`oauth_handler.go:279,311`). Only `role_id` missing from userinfo response. |
| 2 | Object permissions not per-Profile | **Impact overstated** | Backend describe API already returns per-user CRUD via `CheckObjectPermissionWithUser` (`sobject_describe.go:28-35`). Only ViewAll/ModifyAll hardcoded in frontend. |
| 3 | Role Hierarchy completely missing | **Inaccurate** | Full implementation exists: `role_hierarchy.go` (memory cache + ancestor traversal), `record_access.go` (5-layer check: ownership, group, role hierarchy, sharing rules, manual share, team member). |
| 4 | ViewAllRecords hardcoded false | **Frontend only** | Backend `_System_ObjectPerms` table stores correctly with Profile+PermSet MAX aggregation. Real gap: `applyRLS` doesn't check this permission. |
| 5 | Report permissions unguarded | **Partially correct** | Missing `RequirePermission(PermRunReports)` on analytics route group, but backend has SQL-level object permission checks via SecurityValidator. |
| 6 | API permissions unchecked | **Correct** | `PermApiEnabled` defined (`perm_system.go:20`) but never checked in route middleware. |
| 7 | Too many Permission Sets migrated | **Correct** | Data governance issue — 139 PSets include many third-party tool integrations. |
| 8 | profile_id uses SF original ID | **Inaccurate** | Backend uses internal profile IDs (`system_admin`, `standard_user`, etc.), not SF 18-char IDs. |

---

## Real Security Vulnerabilities (Not in Report)

### CRITICAL: JOIN Queries Bypass RLS

**Location:** `backend/internal/features/analytics/security_validator.go:170-175`

```go
if stmt.From.TableRefs.Right != nil {
    return nil // Complex query (Join), skipping RLS MVP explicitly
}
```

Any authenticated user can bypass owner_id filtering by using a JOIN query (e.g., `SELECT o.* FROM Opportunity o JOIN Account a ON o.AccountId = a.Id`), accessing all records.

### HIGH: applyRLS Ignores ViewAllRecords Permission

**Location:** `security_validator.go:132-147`

`applyRLS` only checks `IsSuperUser()` to skip RLS. Users granted ViewAllRecords via Permission Set remain restricted by owner_id filtering. The permission is stored correctly but never enforced at query level.

### HIGH: applyRLS Excludes Role Hierarchy Subordinate Records

`applyRLS` filters `owner_id = user.ID` only. Managers cannot see subordinate-owned records via SOQL queries, despite `CheckRecordAccess` correctly checking role hierarchy for single-record GET operations. Inconsistency between query-level and record-level security.

---

## Revised Priority List (Tri-Agent Consensus)

Total: **7-9 days** (vs report's 14-23 days)

### P0 — Security Fixes (Immediate, 2 days)

| Task | Effort | Detail |
|------|--------|--------|
| Fix applyRLS JOIN bypass | 1 day | For JOIN queries: either apply RLS on primary table, or reject without ViewAllRecords permission |
| applyRLS check ViewAllRecords | 0.5 day | Query user's ViewAllRecords for target object; if true, skip owner_id filter |
| Analytics route guard | 0.5 day | `analyticsGroup.Use(middleware.RequirePermission(constants.PermRunReports))` + frontend `RequireSystemPermission` on `/analytics/report-builder` |

### P1 — Functional Completeness (1-2 weeks, 4-5 days)

| Task | Effort | Detail |
|------|--------|--------|
| applyRLS add Role Hierarchy subordinates | 2-3 days | Use `roleHierarchyCache` to compute subordinate role IDs -> collect user IDs -> `owner_id IN (...)` instead of `owner_id = ?` |
| userinfo add role_id | 0.5 day | Add `"role_id"` and `"role_name"` to `oauth_handler.go:291-312` response |
| Frontend ViewAll/ModifyAll data source | 1 day | New REST endpoint exposing `ListEffectiveObjectPermissionsForUser`; update `permissionQueries.ts:35-36` |
| API route PermApiEnabled check | 0.5 day | Add middleware to `api` group in `routes.go` |

### P2 — Data Cleanup (Planned)

| Task | Effort | Detail |
|------|--------|--------|
| PermissionSet cleanup | 1-2 days | Remove third-party tool PSets (CRUSH, PROLIFIQ, Chorus, Zendesk, Zoom, etc.), keep ~15 core business PSets |

---

## Architecture Decisions (Do NOT Do)

| Don't | Why | Instead |
|-------|-----|---------|
| Put permissions in JWT | Current PermissionLoader + memory cache (TTL 5min) is more secure; permission changes take effect without re-login | Keep slim JWT (identity only) + server-side per-request permission loading |
| Add JSON column to profiles table | `_System_ObjectPerms` relational table already exists and works correctly | Use existing table; add REST endpoint to expose it |
| Use path LIKE queries for Role Hierarchy | Current in-memory cache (43 roles, O(depth) traversal) is more performant | Keep memory cache; if scaling needed, consider Closure Table |
| Rebuild QB() data source from scratch | Backend describe API already returns per-user CRUD | Only supplement ViewAll/ModifyAll data |

---

## Existing Permission Infrastructure (Already Implemented)

The report missed these existing backend systems:

| Layer | Implementation | Location |
|-------|---------------|----------|
| Auth middleware | `RequireAuth` — JWT validation | `middleware/auth.go:15-55` |
| Permission loading | `PermissionLoader` — per-request from DB/cache | `middleware/permission_loader.go:324-398` |
| System permission guard | `RequirePermission(perm)` | `middleware/permission_loader.go:402-442` |
| Composite guards | `RequireAnyPermission` / `RequireAllPermissions` | `middleware/permission_loader.go:488-580` |
| Object-level CRUD | `CheckObjectPermissionWithUser` | `permissions/service.go:152-195` |
| Field-level security | `CheckFieldVisibilityWithUser` / `CheckFieldEditabilityWithUser` | `permissions/service.go:228-256` |
| Record-level access | 5-layer check (owner, group, role hierarchy, sharing rules, manual share, team member) | `permissions/record_access.go:15-104` |
| Role Hierarchy | In-memory parent-pointer tree with ancestor traversal | `permissions/role_hierarchy.go` |
| Sharing Rules | Owner-based + criteria-based (with formula engine) | `permissions/sharing_rules.go` |
| SQL RLS injection | AST-based owner_id filter injection (parameterized, no SQL injection risk) | `analytics/security_validator.go:163-233` |
| Permission cache | Singleflight + TTL-based cache (5min permissions, 60s profile) | `middleware/permission_loader.go:51-73` |

---

## Open Questions

1. **Bootstrap data quality** — Report's observation that "Finance users see Edit/Delete buttons" may be a real UX issue caused by incorrect ObjectPermissions bootstrap data, not code defects
2. **OWD (Organization-Wide Defaults)** — Neither report nor current code has explicit OWD configuration; applyRLS implicitly assumes all objects are Private
3. **Permission Set Group** — SF has 3 PSGs; backend mentions Muting PSets but implementation status unclear
4. **PermApiEnabled enforcement path** — OAuth token endpoint (`/services/oauth2/token`) doesn't require auth (standard OAuth), but API data endpoints should check this permission

---

## Verification Criteria

After implementing P0 fixes:

1. **JOIN RLS** — `SELECT o.* FROM Opportunity o JOIN Account a ON ...` as non-admin user should only return owned records (or records visible via role hierarchy)
2. **ViewAllRecords** — User with ViewAll PermSet on Account should see all Account records via SOQL
3. **Analytics guard** — Tech Support user (RunReports: false) accessing `/analytics/report-builder` should get 403
4. **Role Hierarchy in SOQL** — Sales manager (APAC Team A) querying Opportunities should see own + subordinate-owned records
5. **API guard** — Finance user (ApiEnabled: false) calling API data endpoints should get 403