MemberJunction / MemberJunction/MJ
FLS: system-user access guards miss the UPDATE path — re-targeting a rule strips access unchecked
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 323
Description
## What
Field-Level Security has **no runtime exemption for the MJ system user** — by design. What protects it instead is a set of configuration guards that refuse an edit which would strip its access. Two of those guards miss the **UPDATE** path, for one shared reason.
**An update is two rule-set mutations** — a removal from the old target and an addition to the new one. Only the addition is modelled.
## Gap 1 — `MJEntityFieldPermissionEntityServer.server.ts`
`systemUserAccessLossReason()` opens with:
```ts
if (!SystemUserHoldsRole(this.RoleID)) { return null; }
```
On an UPDATE that is the **new** `RoleID`. `resolveTarget()` then matches the **pending** `EntityFieldID`, and `loadCurrentRules` loads siblings of the **new** field. **The file contains zero `OldValue` reads** — verified by grep — so neither original target is ever audited.
Both columns are writable end to end: generated setters, both on `UpdateMJEntityFieldPermissionInput`, and `ISNULL(@col, [col])` in the update proc. `Validate()`'s `SystemUserRoleRejectionReason` does not cover it — it refuses only a *Deny*, and these rows are *Allow*s.
**Role path.** `ComputeFieldPermissionDelta` writes rows only for roles holding entity-level read, so on an entity granted to one system-user role each field carries exactly **one** Allow for it. Create a role `Temp` with no entity permissions, update that row's `RoleID` to `Temp`: `SystemUserHoldsRole('Temp')` is false, the guard returns null, and the system user is now read-denied on that field while keeping entity-level read.
**Field path.** Move `EntityFieldID` to a field that already carries an Allow from another system-user role. The guard evaluates the **destination**, finds it clean, and commits — the field it left is never looked at.
## Gap 2 — `MJUserRoleEntityServer.server.ts`
The class has `Validate()` and `Delete()` but **no `Save()` override** that performs the field-access check. `systemUserRoleRemovalReason()` — the one that calls `FindSystemUserFieldAccessViolations(..., { WithoutRoleID })` — is called only from `Delete()`.
A `Save()` override does now exist, but it arrived from #4305 and guards only `ReplayOnly` saves by non-Owners.
Take the system user's `UI` user-role row and set `UserID` to another user. `SystemUserRejectionReason` exits early on `!UUIDsEqual(systemUser.ID, userID)` before any aggregation runs. If a field was `UI = Allow` / `Integration = No Access` — a state the Save guard legitimately permits — the system user is now read-denied on it while entity-level read survives via `Integration`.
The class comment says the two halves exist because "without both halves an administrator reaches the forbidden state simply by doing the steps in a different order." The update path is a third order.
## Why it matters
Per `systemUserFieldAccess.ts`, the next `BaseEngine` load as the system user seals a record missing that column into the **process-wide** cache that serves every user, until restart.
`ReconcileFieldPermissions` would heal the orphan but fires only on an Entity save, an EntityPermission save, or CodeGen — never on a field-permission save and never on a timer. `SystemUserFieldAccessCheck.Run` detects the state but is startup-only and warns rather than blocks.
## Fix
Both gaps take the same shape, and #4305 already established the idiom in one of the two files (`validatePriorRoleHeld` reads `GetFieldByName('RoleID').OldValue`):
- Field-permission rows: when `RoleID` or `EntityFieldID` is dirty, resolve the **original** pair from `OldValue`, project that rule set with this row removed, and refuse on either side.
- User-role rows: add a `Save()` override that, when `UserID` or `RoleID` is dirty on an existing row, runs the same `FindSystemUserFieldAccessViolations(..., { WithoutRoleID: })` check `Delete()` already performs.
Found reviewing #3367. Not blocking that PR — the flag is set nowhere and #4297 means no UI can set it.
Contributor guide
Assessment
This issue has not been assessed yet.