Repository navigation
Add ADR 006: grant realm permissions per realm, widen them only with a realm policy - #6821
Conversation
…a policy Records that a realm permission applies to the whole realm, that the grantable levels are read, write and realm-owner, and that access finer than a realm is a realm-policy grant that only widens. Lists the rejected designs: per-folder and per-file permissions, restrictive policies, and roles or groups as subjects. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fzra93HkDDtkekyW4ukjCr
lukemelia
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review checked the ADR against ADR 001 (structure, sourcing, scope) and each Decision requirement against the code it describes. It does not weigh whether the decision itself is right.
No blocking issues. Two wording fixes inline, and one scope decision.
- §1.2's list of owner powers misses archiving and the unlisted path, and the owner-gated handlers sit outside
scope. See the thread on §1.2. - One rejected-option reason in §3 says more than its source does. See the thread on per-folder permissions.
- Decide whether
packages/runtime-common/realm.tsstays inscope. §1.1's request-to-permission mapping andcheckPermissionlive there, so dropping it leaves §1.1 unsurfaced for the main enforcement site; keeping it makesadr-forlist ADR 006 for nearly every realm change. I'd keep it.
| `realm-owner` admits administering the realm: changing its permissions, | ||
| deleting it, and publishing or unpublishing it. Each level is its own grant, |
There was a problem hiding this comment.
[Claude Code 🤖] This list reads as exhaustive but misses two owner-only operations: archiving a realm (archive-realm-utils.ts refuses a requester who isn't a realm-owner) and setting its unlisted path (handle-unlisted-realm-path.ts). As written, the next owner-only endpoint would look like it breaks the Decision. Making the list illustrative fixes both:
| `realm-owner` admits administering the realm: changing its permissions, | |
| deleting it, and publishing or unpublishing it. Each level is its own grant, | |
| `realm-owner` admits administering the realm, such as changing its | |
| permissions, archiving or deleting it, and publishing or unpublishing it. Each | |
| level is its own grant, |
Related: those two handlers, and handle-delete-realm.ts, handle-publish-realm.ts and handle-unpublish-realm.ts, are where realm-owner is enforced, and none is in scope, so adr-for won't surface §1.2 for a change to who may run them. Worth adding each as its own scope line.
Regression, non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 3a8833997a. §1.2 now gives the owner powers as examples, including archiving, and the delete, publish, unpublish, unlisted-path and archive handlers are each in scope. adr-for lists ADR 006 for all five.
| - **Per-folder permissions.** Search and prerendered HTML would differ for | ||
| each viewer, the session token could no longer settle a request, and who can | ||
| reach a realm would no longer be one list [8]. |
There was a problem hiding this comment.
[Claude Code 🤖] "the session token could no longer settle a request" says more than source [8] does. [8] is that one claim per realm keeps the check a cheap in-memory match, as §2 puts it; a token could still carry path-scoped claims, so this reason as worded has no source (ADR 001 §1.7). Reusing §2's wording keeps it cited:
| - **Per-folder permissions.** Search and prerendered HTML would differ for | |
| each viewer, the session token could no longer settle a request, and who can | |
| reach a realm would no longer be one list [8]. | |
| - **Per-folder permissions.** Search and prerendered HTML would differ for | |
| each viewer, the permission check would no longer be one in-memory match | |
| against the token's claim for the realm, and who can reach a realm would no | |
| longer be one list [8]. |
Regression, non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 3a8833997a. The per-folder option now says the check would no longer be one in-memory match against the token's claim for the realm, which matches §2 and source [8].
Lists archiving among realm-owner's powers as examples rather than an exhaustive list, adds the owner-gated realm-server handlers to scope, and rewords the per-folder option's token reason to match its source. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fzra93HkDDtkekyW4ukjCr
|
[Claude Code 🤖] On the scope question: |
|
|
||
| ### 1.2. Three levels | ||
|
|
||
| The grantable levels MUST be exactly `read`, `write`, and `realm-owner`. |
There was a problem hiding this comment.
§1.2 says realm-owner is grantable, but the code doesn't let anyone grant it. Realm#patchRealmPermissions in realm.ts refuses any patch that includes realm-owner ("cannot create new realm owner") and any patch to an existing owner's row ("cannot modify permissions of the realm owner"). handle-upsert-realm-user-permission.ts accepts only read and write. realm-owner is assigned only when a realm is created or published (handle-publish-realm.ts writes ['read', 'realm-owner']). So an owner can't change another owner's permissions either, which the "changing its permissions" wording implies they can. Once this ADR is accepted, §1.9 makes this a MUST, and a reviewer could read it as permission to add an owner-granting path. Suggest saying that realm-owner is assigned at creation or publish and can't be granted or changed through the permissions endpoints.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7c42b89e39. §1.2 now says realm-owner is assigned only when a realm is created or published, always with read, and that the permissions endpoints MUST refuse to grant it or to change an owner's permissions. The owner powers now read "granting and revoking other users' read and write".
|
|
||
| Second, the realm is the unit of authorization everywhere else. A session | ||
| token carries one permission set for each realm, and the check is a cheap | ||
| in-memory match against that claim. Publishing and federation also work one |
There was a problem hiding this comment.
This reason doesn't match how the check works. createRequestContext reads realm_user_permissions from the database on every request, with no memoization, so a PATCH on one replica takes effect on the others. checkPermission then compares the token's claim against that fresh DB result, and effectiveRealmPermissions calls Matrix getProfile whenever the realm has a users row. The realm-auth skill describes the same thing: "claim comparisons against live database state". The same claim is repeated at L107 as the reason to reject per-folder permissions. This is an ADR, so a wrong rationale here is permanent and misleading. Suggest rewording to describe the check as it is (one indexed lookup per realm plus an exact claim match), or moving the efficiency argument to Open questions.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7c42b89e39. §2 now describes the check as one indexed lookup of the realm's permission rows plus an exact match of the token's claim against them. The per-folder option in §3 now says a token's claim and the permission lookup would no longer be one per realm, not that the in-memory match would be lost.
| `read` admits reading the realm. `write` admits changing its contents. | ||
| `realm-owner` admits administering the realm, such as changing its | ||
| permissions, archiving or deleting it, and publishing or unpublishing it. Each | ||
| level is its own grant, and holding one MUST NOT imply another. |
There was a problem hiding this comment.
"Holding one MUST NOT imply another" leaves out a coupling the server enforces. handle-upsert-realm-user-permission.ts rejects write without read ("write permission requires read permission"), and owner rows are always written together with read. Strictly, the check doesn't imply one level from another, but the levels can't be granted independently. If the ADR is accepted unchanged, that validation looks like it conflicts with §1.2. Suggest stating the write-requires-read rule so a later change doesn't remove it as "non-compliant".
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7c42b89e39. §1.2 now says the check MUST test each level on its own, and that a grant of write MUST include read. One gap: handle-upsert-realm-user-permission.ts enforces write-requires-read, but Realm#patchRealmPermissions (the _permissions PATCH) doesn't check it, because assertRealmPermissions only validates level names. §1.2 names that gap rather than hiding it. Closing it is a small follow-up.
There was a problem hiding this comment.
[Claude Code 🤖] The gap is closed in 5cae3c4534. Realm#patchRealmPermissions now refuses write without read with a 400, the same as handle-upsert-realm-user-permission.ts, and §1.2 now says every endpoint that grants permissions MUST refuse it. A new test in realm-endpoints/permissions-test.ts covers it and fails with the check reverted.
| date: 2023-04-18 | ||
| retroactive: true | ||
| recorded: 2026-10-19 | ||
| scope: |
There was a problem hiding this comment.
The scope misses where the §1.1 invariant would be broken. "The permission store MUST NOT key a permission on a path" would be broken by a migration that adds a path column, but packages/postgres/migrations/*permission* isn't in scope. That's the file Source 4 cites. The token-minting paths that §2's "one permission set for each realm" relies on (packages/realm-server/handlers/handle-realm-auth.ts, handle-create-session.ts, handle-delegate-session.ts) are also missing. scripts/adr-for.mts won't show this ADR for changes to any of these files. Suggest adding those globs.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7c42b89e39. scope now includes packages/postgres/migrations/*permission*, handle-realm-auth.ts, handle-create-session.ts, handle-delegate-session.ts, and create-realm.ts (where owner rows are written at creation). adr-for lists ADR 006 for the cited migration, the token handlers, and create-realm.ts.
| in-memory match against that claim. Publishing and federation also work one | ||
| realm at a time. | ||
|
|
||
| Third, the model stays simple. Who can reach a realm is one list, and a user |
There was a problem hiding this comment.
This reason also applies against the chosen option. Once a realm has a RealmPolicy, who can reach its content is no longer one list: the permissions list plus every policy grant and predicate. L108–109 reject per-folder permissions because "who can reach a realm would no longer be one list". The chosen design already gives that up. Suggest narrowing it to something like "who holds realm-wide authority is one list", or naming the trade-off.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7c42b89e39. Both places now say "who holds realm-wide access is one list". The third reason in §2 names the trade-off: with a policy, who can reach a given card is that list plus the policy's grants. Because grants only add to the list, the list still gives the least each user can reach.
|
|
||
| Access finer than a whole realm MUST be a grant in the `RealmPolicy` card that | ||
| the realm's `realm.json` names. A policy MUST only widen access. The realm | ||
| MUST consult its policy only for a request its permissions refused. A policy |
There was a problem hiding this comment.
"MUST consult its policy only for a request its permissions refused" is stricter than the code. checkPermission lets every HEAD pass the ACL (request.method === 'HEAD' && !probe), and the HEAD routes then call #readProbe / #admitsDespiteCoarseRefusal, which consult the policy for a request the ACL nominally allowed. _capabilities is also mapped to read and settles its write lane against the policy inside the handler. Suggest phrasing the MUST in terms of "a caller the permissions would refuse", not "a request its permissions refused", so the HEAD path isn't a violation once the ADR is accepted.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7c42b89e39. §1.3 now reads "MUST consult its policy only for a caller its permissions would refuse". That covers the HEAD path through #readProbe / #admitsDespiteCoarseRefusal and the write lane of _capabilities.
| request, and any other request is a read. | ||
| 3. #1053 (`633a20cfd4`): the `users` subject. | ||
| 4. #1306 (`05befcac24`): | ||
| `packages/postgres/migrations/1716284017819_add-realm-user-permissions.js` |
There was a problem hiding this comment.
Wrong path for the cited commit. At 05befcac24 (#1306), this migration was at packages/realm-server/migrations/1716284017819_add-realm-user-permissions.js. It later moved to packages/postgres/migrations/. A reader who follows the citation at that commit won't find the file. Suggest giving the path as of that commit, or saying "now at".
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 7c42b89e39. Source 4 now gives packages/realm-server/migrations/1716284017819_add-realm-user-permissions.js, the path at 05befcac24, and notes that the file now lives under packages/postgres/migrations/.
- realm-owner is assigned at create or publish, never through the permissions endpoints; owners administer other users' read and write - write requires read; the _permissions PATCH does not check it - the policy is consulted for a caller the ACL would refuse, which covers the HEAD probe path - the check is an indexed DB lookup plus an exact claim match, not an in-memory match - "one list" narrowed to realm-wide access, naming what a policy gives up - scope adds the permission migrations, create-realm, and the token-minting handlers; source 4 gives the migration path as of its commit Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fzra93HkDDtkekyW4ukjCr
The user-permission endpoint already refuses a write grant without read; the realm's own _permissions PATCH accepted one. Both now refuse it, and ADR 006 §1.2 states the rule for every granting endpoint. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fzra93HkDDtkekyW4ukjCr
The Grafana upsert-realm-user-permission endpoint writes every permission column, so granting read/write to a realm owner silently cleared their realm_owner flag. It now returns 400 for a user who holds realm-owner, matching the owner guard on the _permissions PATCH. ADR 006 §1.2 now says that a migration seeding a system realm's owner counts as creating that realm, and the ADR gains a Remediation section with no known gaps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fzra93HkDDtkekyW4ukjCr
Summary
Adds ADR 006, a retroactive record of how realm permissions are shaped:
users, or*.read,writeandrealm-owner, and the check tests each on its own. Awritegrant must includeread.realm-owneris assigned only at creation or publish, and the permissions endpoints can't grant it or change an owner's row.assume-useris not grantable.RealmPolicycard. A policy only widens: it is consulted only for a caller the permissions would refuse, and never grants owner authority.It also records the rejected designs: per-folder and per-file permissions, policies that restrict, and roles or groups as subjects.
Code change
PATCH /_permissions(Realm#patchRealmPermissions) now refuses a grant ofwritewithoutreadwith a 400, matching the user-permission endpoint, so §1.2 holds on every endpoint that grants. Its clients are the host'sRealmService.setPermissionsand the load harness'ssetup-realm.ts, and both sendreadwithwrite.Test plan
TEST_FILES=realm-endpoints/permissions-test pnpm testinpackages/realm-server: 15/15 pass. The newcannot grant write without readtest fails (200 instead of 400) with therealm.tscheck reverted.Sourcing
The decision dates to #298 (2023-04-18), and #975, #1053, #1306, #1553, #1748, #6276 and #6300 shaped it. No PR or commit states why permissions stay realm-wide, so the reasons and rejected options are cited to Luke Melia (2026-10-19), per ADR 001 §1.7. The Decision was checked against the code as it is today:
realm.tsrequest→permission mapping andassertRealmPermissions,realm-permission-checker.ts, therealm_user_permissionsmigrations, and thecard-operations/gate.tsheader.Numbered 006 because #6758, #6794 and #6795 claim 002–004.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Fzra93HkDDtkekyW4ukjCr