MemberJunction / MemberJunction/MJ

FLS: system-user access guards miss the UPDATE path — re-targeting a rule strips access unchecked

Open
#4,348 0 comments 0 reactions 1 assignee Claimed by @jordanfanapour View on GitHub
bug priority: high
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.