fix(security): GRANT widens an access rule instead of replacing it

[ako]

Fixes mendixlabs#936 — reported as a silent security regression, and it is one.

The report needs three corrections

Re-granting a role on an entity shrank the rule instead of widening it:

GRANT Viewer ON Customer (READ (Name, Email)) WHERE '[…]';   -- read (Name, Email)
GRANT Viewer ON Customer (READ (Phone))       WHERE '[…]';   -- read (Phone)  ← Name, Email gone

No error, no warning, no diff. Confirmed and persisted to disk. But three things in the report are wrong in ways that change the fix:

1. WHERE is a red herring. The identical sequence without a constraint loses just as much (read (Name, Email) → read (Phone)), and so does READ * → READ (Phone). The report's "GRANT without WHERE behaves additively" does not hold — most likely they only ever re-ran READ *, which looks unchanged. A fix scoped to the constrained path would have passed the reporter's own repro and left most of the bug in place.

2. It is worse than the attribute list. Structural rights go the same way:

(create, delete, read *, write *)  →  re-grant (READ (Name))  →  (read (Name))

CREATE, DELETE and every write right silently revoked. The report only noticed attributes.

3. It is an engine regression, not a longstanding gap. The same script under MXCLI_ENGINE=legacy yields read (Name, Email, Phone) and keeps create, delete, read *, write *. Legacy has had mergeAccessRule since it shipped; the codec engine — the default — never got an equivalent. That is why the documented contract ("GRANT is additive … never removes permissions") held on one engine and not the one everybody uses.

Running the other engine as a control is what supplied the merge semantics rather than my having to invent them: OR create/delete, take the higher default, take the higher rights per member.

Root cause

AddEntityAccessRule (codec engine) found the matching rule, deleted every MemberAccess, and rebuilt from the current statement alone. The executor emits an entry for every member of the entity — unnamed ones at the rule's default, normally None — so each GRANT reset every member it did not mention.

Worth noting the third backend: MCP refuses this outright rather than replacing, which is the ADR-0005 posture. Three implementations, three behaviours; only the default one destroyed data.

A second defect, found while measuring the first

The upsert matched a stored rule on its module-role set alone, ignoring XPathConstraint — in both engines:

GRANT Viewer ON Customer (READ (Name))  WHERE '[Name = 1]';
GRANT Viewer ON Customer (READ (Email)) WHERE '[Name = 2]';
-- → one rule: (read (Email)) where '[Name = 2]'   ← first rule destroyed

Mendix's reference guide is explicit that this is legitimate model content: "Rules are additive — if multiple access rules apply to the same module role, all access rights of those rules are combined." One rule per constraint is the ordinary way to write row-level security, and MDL could not express it at all.

The fix

• A — the codec engine merges instead of rebuilding: reads the matched rule's existing rights, ORs create/delete, takes the higher default, takes the higher rights per member.
• B — XPathConstraint joins the match key on both engines. The empty constraint is a value, not a wildcard, so a constrained and an unconstrained rule coexist, and re-running a script stays idempotent (ADR-0008) because each statement lands on its own rule.

Rights merge on None < ReadOnly < ReadWrite, so a merge only ever widens. Narrowing stays REVOKE's job — that is what keeps the two commands inverses rather than two spellings of "set".

The lattice moved to mdl/types (AccessRightsLevel / HigherAccessRights) and legacy now delegates to it. The engines silently diverging on this is what caused the bug; sharing it means they can't drift again.

One consequence worth reviewing

Fix B breaks anything keyed on roles alone. formatAccessRuleResult was, so once several rules per role existed it echoed a different rule's rights back at the user. It now takes the constraint too (REVOKE passes anyXPath, since it narrows every rule the roles appear in).

Pleasant side effect: that function reads back from storage, so a merging GRANT now prints the merged rule —

GRANT … (READ (Phone));      →  Result: read (Name, Email, Phone)

— which is exactly the visible signal whose absence the report was complaining about.

Verification

• 7 new backend tests, written first and confirmed failing with the reported symptom verbatim: map[Email:None Name:None Phone:ReadOnly]. The idempotence test passed before the fix, so it guards a regression fix B could have introduced rather than restating the bug.
• Reported repro end to end on both engines → read (Name, Email, Phone).
• Structural case → create, delete, read *, write * preserved. Unconstrained case → merged. Two constraints → two rules, both preserved, and describe round-trips them as two statements.
• mx check: 0 errors on the two-rule model. The decisive check for fix B, since it now produces rules where there was one — Mendix accepts it, confirming the refguide.
• make check-mdl green including the new fixture; 76 packages green; gofmt clean; vet clean (remaining warnings are pre-existing, in generated ANTLR code).

Docs

The four places asserting "GRANT is additive" are now true rather than aspirational. Added the one-rule-per-constraint pattern to docs-site and to the synced manage-security skill, so agents writing MDL know a where creates a rule rather than narrowing one. Symptom table and CHANGELOG.md updated; fixture at mdl-examples/bug-tests/936-grant-additive-merge.mdl covers all five cases.

Notes for the reviewer

• Behaviour change beyond the bug: a GRANT with no WHERE following a constrained one used to inherit the stored constraint (legacy's mergeAccessRule kept the existing XPath when the new one was empty), quietly constraining access the user asked to be unconstrained. It now matches — or creates — the unconstrained rule. I believe that is the correct reading, but it is the one place where existing scripts could produce a different model.
• Not addressed: RemoveEntityAccessRule / RevokeEntityMemberAccess still match on roles alone, which is right for REVOKE (it should narrow every rule the role appears in) but means there is currently no way to revoke one constrained rule while leaving its sibling. That is a feature gap, not a data-loss path, so I left it out of scope.

---

Generated by Claude Code