diff --git a/docs/adr/006-grant-realm-permissions-per-realm-and-widen-them-only-with-a-policy.md b/docs/adr/006-grant-realm-permissions-per-realm-and-widen-them-only-with-a-policy.md new file mode 100644 index 00000000000..79dd6cfe3cf --- /dev/null +++ b/docs/adr/006-grant-realm-permissions-per-realm-and-widen-them-only-with-a-policy.md @@ -0,0 +1,160 @@ +--- +status: proposed +date: 2023-04-18 +retroactive: true +recorded: 2026-10-19 +scope: + - packages/postgres/migrations/*permission* + - packages/runtime-common/realm.ts + - packages/runtime-common/realm-permission-checker.ts + - packages/runtime-common/db-queries/realm-permission-queries.ts + - packages/runtime-common/card-operations/gate.ts + - packages/runtime-common/card-operations/policy*.ts + - packages/realm-server/handlers/create-realm.ts + - packages/realm-server/handlers/handle-realm-auth.ts + - packages/realm-server/handlers/handle-create-session.ts + - packages/realm-server/handlers/handle-delegate-session.ts + - packages/realm-server/handlers/handle-upsert-realm-user-permission.ts + - packages/realm-server/handlers/handle-delete-realm.ts + - packages/realm-server/handlers/handle-publish-realm.ts + - packages/realm-server/handlers/handle-unpublish-realm.ts + - packages/realm-server/handlers/handle-unlisted-realm-path.ts + - packages/realm-server/handlers/archive-realm-utils.ts + - packages/realm-server/middleware/multi-realm-authorization.ts +--- + +# 006. Grant realm permissions per realm, and widen them only with a realm policy + +A user's permission applies to a whole realm, never to a folder, file, or card +inside it. There are three levels: `read`, `write`, and `realm-owner`. Access +finer than a whole realm is a grant in the realm's policy card, and a policy +only widens what the permissions allow. To keep content from some of a realm's +members, put it in another realm. The `realm-auth` skill covers tokens and +checks, and the `realm-policy-authoring` skill in boxel-skills covers writing +a policy. + +## 1. Decision + +The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", +"SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this +document are to be interpreted as described in BCP 14 (RFC 2119, RFC 8174) when, +and only when, they appear in all capitals, as shown here. + +### 1.1. Permissions apply to the whole realm + +A realm permission MUST be granted for a whole realm. The permission store +MUST NOT key a permission on a path, folder, file, or card inside a realm. The +permission a request needs MUST depend only on what the request does (its +method, or the realm-level endpoint it calls), never on which file or folder +it names. + +A permission's subject MUST be one user, `users` (any signed-in user), or `*` +(everyone). Roles and groups MUST NOT be added as subjects. + +### 1.2. Three levels + +The levels MUST be exactly `read`, `write`, and `realm-owner`. `read` admits +reading the realm. `write` admits changing its contents. `realm-owner` admits +administering the realm, such as granting and revoking other users' `read` and +`write`, archiving or deleting it, and publishing or unpublishing it. + +The permission check MUST test each level on its own and MUST NOT infer one +level from another. A grant of `write` MUST include `read`, and every endpoint +that grants permissions MUST refuse `write` without `read`. + +`realm-owner` MUST be assigned only when a realm is created or published, and +always with `read`. A migration that seeds a system realm's owner counts as +creating that realm. Every endpoint that grants permissions MUST refuse to +grant `realm-owner` and MUST refuse to change an owner's permissions. + +`assume-user` is not a grantable level. Only the realm's own Matrix user +holds it, and it MUST NOT be stored or granted. + +### 1.3. Finer-grained access is a policy grant + +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 caller its permissions would refuse. A policy +MUST NOT refuse anything the permissions allow. A policy MUST NOT grant +`realm-owner` authority. + +### 1.4. Restriction is a separate realm + +Content that some of a realm's readers or writers must not reach SHOULD go in +a separate realm with its own permissions. + +## 2. Rationale + +Permissions were scoped to the realm from the first plan for them [1], and +each later step kept that scope [2][3][4][5]. Four reasons hold the line +against finer permissions [8]. + +First, the index is per realm. A realm's cards are indexed and prerendered +once, on the realm's own authority, and every reader gets the same search +rows and the same HTML. A permission on part of a realm would make search and +prerendering differ for each viewer. + +Second, the realm is the unit of authorization everywhere else. A session +token carries one permission set for each realm, and the check is one indexed +lookup of the realm's permission rows and an exact match of the token's claim +against them. Publishing and federation also work one realm at a time. + +Third, the model stays simple. Who holds realm-wide access is one list, and a +user who needs to keep something from some members makes another realm. A +policy gives up part of this: who can reach a given card is that list plus the +policy's grants. But the grants only add to the list, so the list still says +the least each user can reach. + +Fourth, the access people actually need to express below the realm is +conditional, such as "a teacher may read the classrooms they teach." A policy +rule over the data says that, and a change to the roster is a change to the +data. Per-user permission rows would need an update each time the roster +changed. The policy layer was added for this purpose [6][7]. + +A policy only widens for two reasons [8]. A caller the permissions allow never +loads the policy or evaluates a predicate, so a writer editing their own realm +pays nothing for it [7]. And the permissions stay truthful: if they say a user +may read a realm, nothing elsewhere takes that away. + +## 3. Considered options + +- **Realm-wide permissions, widened by policy grants (chosen).** +- **Per-folder permissions.** Search and prerendered HTML would differ for + each viewer, a token's claim and the permission lookup would no longer be + one per realm, and who holds realm-wide access would no longer be one list + [8]. +- **Per-file or per-card permissions.** These fail for the same reasons as + per-folder permissions [8]. A policy can already grant access to a single + card by a predicate on its id. +- **Policies that restrict.** Every allowed caller would pay for a policy + load, and the realm permissions would no longer say what a user can reach + [8]. +- **Roles or groups as subjects.** Membership already lives in the data. A + policy predicate over a card's fields, such as + `.teacherIds | any(. == actor())`, expresses it with nothing separate to + keep in sync [8]. + +## 4. Sources + +1. #298 (`ba5a022666`), `docs/realm.md` "Future capabilities": permissions + "will not be card specific but they will apply to all cards in the realm." +2. #975 (`d75de62450`): a `PUT`, `PATCH`, `POST`, or `DELETE` is a write + request, and any other request is a read. +3. #1053 (`633a20cfd4`): the `users` subject. +4. #1306 (`05befcac24`): + `packages/realm-server/migrations/1716284017819_add-realm-user-permissions.js` + (now under `packages/postgres/migrations/`) keys `realm_user_permissions` + on realm and username, with no path column. +5. #1553 (`b2fe4df54b`) adds `realm-owner`, and #1748 (`d0bc07f3a8`) adds the + owner-only `_permissions` endpoint. +6. #6276 (`72f824fa1b`): the policy definitions add access "the coarse + read/write/owner ACL" cannot express. +7. #6300 (`e212a68c4d`) and the header of + `packages/runtime-common/card-operations/gate.ts`: the gate runs only for + a caller the ACL declined, so a policy never narrows access and an allowed + caller pays nothing for it. +8. Luke Melia, 2026-10-19: the reasons in §2 and the rejected options in §3. + +## 5. Remediation + +No known gaps. diff --git a/docs/adr/README.md b/docs/adr/README.md index 7cfb2a89721..1211c3aa3ed 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -12,10 +12,11 @@ or backfill one. ## Index -| ADR | Title | Status | -| ---------------------------------------------------------------------------- | --------------------------------------------------------------------------------- | -------- | -| [001](001-record-architecture-decisions.md) | Record architecture decisions as short, normative ADRs | proposed | -| [003](003-reach-cloud-services-through-adapters.md) | Reach proprietary cloud services only through adapters | proposed | -| [005](005-authorize-key-changes-only-with-a-session-card-code-cant-reach.md) | Authorize changes to a user's keys only with a session that card code can't reach | proposed | -| [007](007-keep-motion-a-reactive-layer-that-state-never-waits-on.md) | Keep motion a reactive layer that application state never waits on | proposed | -| [008](008-build-the-secure-origins-pages-as-their-own-ember-app.md) | Build the secure origin's pages as their own Ember app | proposed | +| ADR | Title | Status | +| --------------------------------------------------------------------------------- | --------------------------------------------------------------------------------- | -------- | +| [001](001-record-architecture-decisions.md) | Record architecture decisions as short, normative ADRs | proposed | +| [003](003-reach-cloud-services-through-adapters.md) | Reach proprietary cloud services only through adapters | proposed | +| [005](005-authorize-key-changes-only-with-a-session-card-code-cant-reach.md) | Authorize changes to a user's keys only with a session that card code can't reach | proposed | +| [006](006-grant-realm-permissions-per-realm-and-widen-them-only-with-a-policy.md) | Grant realm permissions per realm, and widen them only with a realm policy | proposed | +| [007](007-keep-motion-a-reactive-layer-that-state-never-waits-on.md) | Keep motion a reactive layer that application state never waits on | proposed | +| [008](008-build-the-secure-origins-pages-as-their-own-ember-app.md) | Build the secure origin's pages as their own Ember app | proposed | diff --git a/packages/realm-server/handlers/handle-upsert-realm-user-permission.ts b/packages/realm-server/handlers/handle-upsert-realm-user-permission.ts index 30ac03e6085..c5d37178e22 100644 --- a/packages/realm-server/handlers/handle-upsert-realm-user-permission.ts +++ b/packages/realm-server/handlers/handle-upsert-realm-user-permission.ts @@ -1,6 +1,7 @@ import type Koa from 'koa'; import { ensureTrailingSlash, + fetchRealmPermissions, insertPermissions, logger, type RealmAction, @@ -111,6 +112,20 @@ export default function handleUpsertRealmUserPermission({ return; } + // The upsert writes every permission column, so granting read/write to the + // realm owner would clear their `realm_owner` flag (ADR 006 §1.2). + let currentPermissions = await fetchRealmPermissions( + dbAdapter, + new URL(normalizedRealmHref), + ); + if (currentPermissions[user]?.includes('realm-owner')) { + await sendResponseForBadRequest( + ctxt, + `cannot modify permissions of the realm owner ${user}`, + ); + return; + } + let actions: RealmAction[] = []; if (read) { actions.push('read'); diff --git a/packages/realm-server/tests/realm-endpoints/permissions-test.ts b/packages/realm-server/tests/realm-endpoints/permissions-test.ts index bc3b0c53046..3c8d4f7d19b 100644 --- a/packages/realm-server/tests/realm-endpoints/permissions-test.ts +++ b/packages/realm-server/tests/realm-endpoints/permissions-test.ts @@ -521,6 +521,42 @@ module(`realm-endpoints/${basename(import.meta.filename)}`, function () { ); }); + test('cannot grant write without read from PATCH /_permissions', async function (assert) { + let response = await request + .patch('/_permissions') + .set('Accept', 'application/vnd.api+json') + .set( + 'Authorization', + `Bearer ${createJWT(testRealm, 'mary', [ + 'read', + 'write', + 'realm-owner', + ])}`, + ) + .send({ + data: { + id: testRealmHref, + type: 'permissions', + attributes: { + permissions: { + mango: ['write'], + }, + }, + }, + }); + + assert.strictEqual(response.status, 400, 'HTTP 400 status'); + let permissions = await fetchRealmPermissions(dbAdapter, testRealmURL); + assert.deepEqual( + permissions, + { + mary: ['read', 'write', 'realm-owner'], + bob: ['read', 'write'], + }, + 'permissions are correct', + ); + }); + // CS-11126: realm visibility / world-readable used to be memoized // per Realm instance, so a `*: read` grant or revoke on replica A // would be silently ignored by every other replica until restart. diff --git a/packages/realm-server/tests/server-endpoints/maintenance-endpoints-test.ts b/packages/realm-server/tests/server-endpoints/maintenance-endpoints-test.ts index 2fc874d16b3..aba2f3a3f31 100644 --- a/packages/realm-server/tests/server-endpoints/maintenance-endpoints-test.ts +++ b/packages/realm-server/tests/server-endpoints/maintenance-endpoints-test.ts @@ -10,6 +10,7 @@ import { boxelUIChecker } from '../../lib/boxel-ui-change-checker.ts'; import { ensureTrailingSlash, fetchRealmPermissions, + insertPermissions, } from '@cardstack/runtime-common'; import { ensureMatrixAdminUser, @@ -1438,6 +1439,32 @@ module(`server-endpoints/${basename(import.meta.filename)}`, function () { assert.strictEqual(response.status, 400, 'HTTP 400 status'); }); + test("rejects a change to the realm owner's permissions", async function (assert) { + let user = '@op-test-owner:localhost'; + await insertPermissions(context.dbAdapter, testRealmURL, { + [user]: ['read', 'write', 'realm-owner'], + }); + let response = await context.request + .post( + `/_grafana-upsert-realm-user-permission` + + `?realm=${encodeURIComponent(testRealmURL.href)}` + + `&user=${encodeURIComponent(user)}` + + `&read=true&write=true`, + ) + .set('Authorization', `Bearer ${grafanaSecret}`) + .set('Content-Type', 'application/json'); + assert.strictEqual(response.status, 400, 'HTTP 400 status'); + let perms = await fetchRealmPermissions( + context.dbAdapter, + testRealmURL, + ); + assert.deepEqual( + perms[user].sort(), + ['read', 'realm-owner', 'write'], + 'owner keeps realm-owner', + ); + }); + test('rejects read=false write=false (use the delete flow to revoke)', async function (assert) { let user = '@op-test-revoke:localhost'; let response = await context.request diff --git a/packages/runtime-common/realm.ts b/packages/runtime-common/realm.ts index 62f8b1cfc58..3cf82b44a68 100644 --- a/packages/runtime-common/realm.ts +++ b/packages/runtime-common/realm.ts @@ -16351,6 +16351,12 @@ export class Realm { requestContext, }); } + if (permissions?.includes('write') && !permissions.includes('read')) { + return badRequest({ + message: `write permission requires read permission for ${user}`, + requestContext, + }); + } } await insertPermissions(this.#dbAdapter, new URL(this.url), patch);