From 9ce6261742c16218f7f38632d67eafc94de995d3 Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Fri, 16 Oct 2026 18:10:02 -0400 Subject: [PATCH 1/6] Share a key into a realm, suspend on loss of write, swap, and delete-suspends Adds `key_shares` and `key_share_notices`, with triggers on `realm_user_permissions` and `user_keys` so a share is suspended when its owner stops being a direct writer (resuming when they write again) and when its key is deleted, however those rows change. Each suspension records a notice for the realm's direct writers. The `/_key-shares` routes let a key's owner share it into a realm they directly write (with a `share` key write ticket), unshare it, and let a realm's direct writers list its shares and notices, suspend and resume them, and swap one of their own keys into a suspended share (with a `swap` ticket). Adds the `key-shares` operator skill and updates the `user-keys` skill. Co-Authored-By: Claude Opus 5.5 --- .claude/skills/key-shares/SKILL.md | 134 +++ .claude/skills/user-keys/SKILL.md | 41 +- ...74_schema.sql => 1792187770930_schema.sql} | 0 .../1792187770930_add-key-shares.js | 285 +++++++ packages/postgres/scripts/schema-dump.sh | 2 + .../handlers/handle-key-shares.ts | 166 ++++ .../realm-server/handlers/handle-user-keys.ts | 29 +- packages/realm-server/lib/key-shares.ts | 456 +++++++++++ .../realm-server/lib/request-telemetry.ts | 9 +- packages/realm-server/lib/user-keys.ts | 123 ++- packages/realm-server/routes.ts | 46 ++ .../realm-server/tests/key-shares-test.ts | 763 ++++++++++++++++++ packages/realm-server/tests/user-keys-test.ts | 4 + .../db-queries/realm-permission-queries.ts | 22 + 14 files changed, 2020 insertions(+), 60 deletions(-) create mode 100644 .claude/skills/key-shares/SKILL.md rename packages/host/config/schema/{1792107467874_schema.sql => 1792187770930_schema.sql} (100%) create mode 100644 packages/postgres/migrations/1792187770930_add-key-shares.js create mode 100644 packages/realm-server/handlers/handle-key-shares.ts create mode 100644 packages/realm-server/lib/key-shares.ts create mode 100644 packages/realm-server/tests/key-shares-test.ts diff --git a/.claude/skills/key-shares/SKILL.md b/.claude/skills/key-shares/SKILL.md new file mode 100644 index 00000000000..1203bc92c03 --- /dev/null +++ b/.claude/skills/key-shares/SKILL.md @@ -0,0 +1,134 @@ +--- +name: key-shares +description: Operate and diagnose shares — keys their owners have put into a realm for other people there to consume — the `key_shares` and `key_share_notices` tables and the `/_key-shares` and `/_key-share-notices` routes. Covers share states (`active`, `suspended`) and every suspension reason (`owner-lost-write`, `suspended-by-writer`, `key-deleted`), which change makes each, how a share resumes, the triggers on `realm_user_permissions` and `user_keys` that suspend shares however those rows change, who may share, unshare, suspend, resume and swap, who sees what, and every refusal the API answers with. Use when a user reports a share that stopped working or that they can't share, swap, suspend or see; when reading or querying `key_shares` / `key_share_notices` as an operator; or when changing `lib/key-shares.ts`, `handlers/handle-key-shares.ts` or the triggers. +--- + +# Key shares (operators) + +A **share** puts a key into a realm so that other people there can consume it +(a weather subscription, say). It holds no secret, only a reference to the +key, so one key can be shared into several realms while its value is stored +once, in `user_keys` (see the `user-keys` skill). Using your own key in a +realm is a different thing and needs no share. + +The code is `packages/realm-server/lib/key-shares.ts` (the rules) and +`packages/realm-server/handlers/handle-key-shares.ts` (the routes). The +triggers are in the migration that adds `key_shares`. + +## States and suspension reasons + +A share is `active` or `suspended`. A suspended share serves nothing and +always carries one reason: + +| `suspension_reason` | Made by | `key_id` | Ends when | +| --------------------- | --------------------------------------------------------------------------- | -------- | ----------------------------------------------------------------------------------------- | +| `owner-lost-write` | The owner's own permission row stops granting write, or is deleted (leaves) | kept | The owner's row grants write again (resumes by itself), or a direct writer swaps a key in | +| `suspended-by-writer` | A direct writer of the realm (`suspended_by`), for abuse or any reason | kept | A direct writer resumes it, or swaps a key in | +| `key-deleted` | The owner deleted the key the share holds | null | A direct writer swaps a key in | + +`key_shares_state_check` enforces these combinations. Only an active share is +suspended: a share suspended for one reason keeps that reason when another +applies too, so a share a writer suspended stays suspended when its owner +loses write and regains it. Resuming a `suspended-by-writer` share whose +owner is no longer a direct writer leaves it suspended as `owner-lost-write`, +and it resumes when they write again. + +A suspension keeps the share's place, label and history: nothing but its +owner's unshare deletes a share, so cards that name it by label keep +working once it is brought back. + +## Triggers + +Suspensions that follow from another change are made by triggers, so they +hold however the change is made (the realm's `_permissions` endpoint, the +admin permission upsert, publishing, deleting a realm, a database writer): + +- `key_shares_follow_permissions` on `realm_user_permissions`, after any + insert, update or delete of a row: suspends that person's active shares in + that realm when the row no longer grants write, and resumes their + `owner-lost-write` shares there when it does again. A share's owner is + always a person (only a direct writer can share or swap in), so their own + row is the whole answer. +- `key_shares_follow_key_delete` on `user_keys`, before a delete: suspends + every share holding the key as `key-deleted` and clears `key_id`. +- `key_shares_notice_suspension` on `key_shares`: each share going from active + to suspended records a row in `key_share_notices`. + +Sharing and swapping lock the person's permission row (`isDirectWriterLocked`) +until they commit, so a concurrent removal of their write waits and then +suspends what they made. + +## Who may do what + +| Action | Who | Ticket | +| ------- | -------------------------------------------------------------------------------------------------------------------------- | ---------------------------- | +| Share | The key's owner, into a realm where they are a direct writer | `share` for that key + realm | +| Unshare | The share's owner, whether or not they still write the realm | none | +| Suspend | Any direct writer of the realm, on an active share | none | +| Resume | Any direct writer of the realm, on a `suspended-by-writer` share | none | +| Swap | Any direct writer of the realm, putting one of their own keys into a suspended share; they become its owner and it resumes | `swap` for that key + share | + +A direct writer is a person whose own permission row grants write +(`isDirectWriter`): a `*` or `users` row, a bot account or a policy grant +makes no one a direct writer. Nobody swaps in another user's key: a ticket to +do so is refused at minting with 404, as for a key that doesn't exist. + +Changing a share's `limits` is for its owner only; the column is reserved and +no route sets it. + +## Visibility + +- A share's owner sees every share they own, in every realm, with its key + (`GET /_key-shares`); `?key=` narrows that to the shares holding one + key — the ones deleting it would suspend. +- A realm's direct writers see its shares with owner and state, without the + key of a share they don't own (`GET /_key-shares?realm=`), and its + notices, newest first, at most 100 (`GET /_key-share-notices?realm=`). + An owner who loses write stops seeing the realm's shares and notices, and + still sees their own. +- Anyone else, readers included, sees nothing of a realm's shares: the + listings answer 403 and a share by id answers 404. + +## The tables + +```sql +-- A realm's shares. +SELECT id, label, owner, key_id, state, suspension_reason, suspended_by, suspended_at +FROM key_shares WHERE realm_url = 'https://app.boxel.ai/someone/realm/' ORDER BY label; + +-- Why a share is suspended, and since when. +SELECT created_at, reason, owner, suspended_by +FROM key_share_notices WHERE share_id = '' ORDER BY created_at DESC; +``` + +Both tables hold no secret, but they are withheld from `readonly_role` and its +members like `user_keys`, and excluded from the SQLite schema dump. +`realm_url` is the realm's canonical URL, with a trailing slash, as +`realm_user_permissions` stores it. + +## The API + +All routes take the user's realm-server session and refuse a delegated or a +realm-authority session, as the keys API does. Bodies are JSON:API with type +`key-share`; a ticket goes in `meta.ticket`, minted from +`POST /_key-write-tickets` by a secure session (see the `user-keys` skill) +with `attributes.{action: 'share', keyId, realm}` or +`attributes.{action: 'swap', keyId, shareId}`. The request log keeps only the +method and path of these routes. + +| Route | Does | +| ------------------------------------- | ------------------------------------------------------------------------------ | +| `GET /_key-shares` | The user's own shares; `?key=` for one key's; `?realm=` for a realm's | +| `POST /_key-shares` | Share: `attributes.{realm, keyId, label}`, `meta.{ticket}` | +| `DELETE /_key-shares/:id` | Unshare | +| `POST /_key-shares/:id/suspend` | Suspend an active share | +| `POST /_key-shares/:id/resume` | Lift a writer's suspension | +| `POST /_key-shares/:id/swap` | Swap a key in: `attributes.{keyId}`, `meta.{ticket}` | +| `GET /_key-share-notices?realm=` | A realm's suspension notices | + +| Status | Means | +| ------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| 400 | Malformed body, a missing realm or key, a missing, over-long or control-character label, an attribute that can't be set | +| 403 | No valid ticket; sharing into a realm the owner isn't a direct writer of; a writer action by someone who isn't one; unsharing someone else's share; listing a realm's shares or notices without being a direct writer | +| 404 | No such share, or one the user can't see; no such key of the user's | +| 409 | A label the realm already has; suspending a suspended share; resuming one a writer didn't suspend; swapping into an active one | diff --git a/.claude/skills/user-keys/SKILL.md b/.claude/skills/user-keys/SKILL.md index 90c8ae77304..16ad80d20fa 100644 --- a/.claude/skills/user-keys/SKILL.md +++ b/.claude/skills/user-keys/SKILL.md @@ -34,9 +34,11 @@ has lost track of a value replaces it. | `created_at`, `updated_at` | `timestamptz` | `key_write_tickets` — one row per unused ticket: `ticket_sha256`, `owner`, -`action` (`add`, `replace` or `delete`), `key_id` (the key a replace or -delete changes; null for an add, enforced by -`key_write_tickets_action_key_id_check`) and `expires_at`. Only the SHA-256 +`action` (`add`, `replace`, `delete`, `share` or `swap`), `key_id` (the key +a replace, delete, share or swap uses; null for an add), `realm_url` (the +realm a share puts the key into; null otherwise), `share_id` (the share a swap +puts the key into; null otherwise) and `expires_at`. +`key_write_tickets_action_key_id_check` enforces which action names what. Only the SHA-256 is stored; the ticket itself exists only in the response that minted it. Both are excluded from the SQLite schema dump and withheld from @@ -63,14 +65,17 @@ A value moved to another row or owner by a database writer does not open: ## Key write tickets -A key is added, replaced or deleted only with a ticket: minted for the -signed-in user and for one change, valid for 10 minutes, single use. A ticket -names its change when it is minted: `add`, or `replace` or `delete` of one of -the user's keys. It spends only on that change, so a ticket that leaks can't +A key is added, replaced, deleted, shared into a realm or swapped into a +share only with a ticket: minted for the signed-in user and for one change, +valid for 10 minutes, single use. A ticket names its change when it is +minted: `add`; `replace` or `delete` of one of the user's keys; `share` of one +of them into one realm; or `swap` of one of them into one share (see the +`key-shares` skill). It spends only on that change, so a ticket that leaks can't be put to a different one: a ticket to add a key replaces or deletes nothing, and a ticket to replace or delete one key does nothing to another, nor the -other action to the same key. A ticket for a key the user doesn't have is -refused with 404 at minting, as for no key. +other action to the same key, and a ticket to share a key into one realm or +swap it into one share does nothing elsewhere. A ticket for a key the user +doesn't have is refused with 404 at minting, as for no key. The change consumes the ticket in the same transaction, so a change refused for another reason (a duplicate name, an unknown key, the wrong ticket for @@ -98,20 +103,20 @@ the ticket goes in `meta`. A body is at most exceed that. A query string on these routes is ignored, and left out of the request log line and Sentry breadcrumbs. -| Route | Does | -| -------------------------- | ------------------------------------------------------------------------------------------------------------------------------ | -| `POST /_key-write-tickets` | Mint a ticket for one change: type `key-write-ticket`, `attributes.{action, keyId?}`; answers `attributes.{ticket, expiresAt}` | -| `GET /_user-keys` | List the user's keys' metadata, by name | -| `GET /_user-keys/:id` | One key's metadata | -| `POST /_user-keys` | Add: `attributes.{name, kind?, value}`, `meta.{ticket}` (a ticket to `add`) | -| `PATCH /_user-keys/:id` | Replace the value: `attributes.{value}`, `meta.{ticket}` (a ticket to `replace` this key) | -| `DELETE /_user-keys/:id` | Delete: `meta.{ticket}` (a ticket to `delete` this key) | +| Route | Does | +| -------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------ | +| `POST /_key-write-tickets` | Mint a ticket for one change: type `key-write-ticket`, `attributes.{action, keyId?, realm?, shareId?}`; answers `attributes.{ticket, expiresAt}` | +| `GET /_user-keys` | List the user's keys' metadata, by name | +| `GET /_user-keys/:id` | One key's metadata | +| `POST /_user-keys` | Add: `attributes.{name, kind?, value}`, `meta.{ticket}` (a ticket to `add`) | +| `PATCH /_user-keys/:id` | Replace the value: `attributes.{value}`, `meta.{ticket}` (a ticket to `replace` this key) | +| `DELETE /_user-keys/:id` | Delete: `meta.{ticket}` (a ticket to `delete` this key); suspends every share holding it (`GET /_key-shares?key=` lists them first) | An attribute other than these, such as a `destination`, is refused with 400. | Status | Means | | ------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| 400 | Malformed body, a missing or over-long name or value, an attribute that can't be set, a ticket request with no valid action or with the wrong `keyId` for it | +| 400 | Malformed body, a missing or over-long name or value, an attribute that can't be set, a ticket request with no valid action or with the wrong `keyId`, `realm` or `shareId` for it | | 401 | No session, an invalid one, or a delegated one | | 403 | No valid ticket (missing, used, expired, another user's, or minted for a different change); a ticket asked for without a secure session, or by the app page; a realm-authority session | | 404 | No such key — also what another user's key answers, including when a ticket is asked for to change it | diff --git a/packages/host/config/schema/1792107467874_schema.sql b/packages/host/config/schema/1792187770930_schema.sql similarity index 100% rename from packages/host/config/schema/1792107467874_schema.sql rename to packages/host/config/schema/1792187770930_schema.sql diff --git a/packages/postgres/migrations/1792187770930_add-key-shares.js b/packages/postgres/migrations/1792187770930_add-key-shares.js new file mode 100644 index 00000000000..60c229e9f27 --- /dev/null +++ b/packages/postgres/migrations/1792187770930_add-key-shares.js @@ -0,0 +1,285 @@ +// Shares: a key its owner has put into a realm for other people there to +// consume. A share holds no secret, only the key it refers to; one key may be +// shared into several realms and its value is stored once, in `user_keys`. +// +// `key_shares` holds one row per share. `owner` is the owner of the key the +// share holds: whoever shared it, or the writer who last swapped one of their +// own keys in. `label` names the share for people and is unique within its +// realm. `state` is `active` or `suspended`; a suspended share carries the +// reason: +// +// - `owner-lost-write`: the owner is no longer a direct writer of the realm. +// The share resumes by itself if they become one again. +// - `suspended-by-writer`: a direct writer of the realm suspended it +// (`suspended_by`). +// - `key-deleted`: the owner deleted the key; `key_id` is then null. +// +// A suspended share serves nothing. A direct writer swaps one of their own +// keys into it to bring it back. `limits` is reserved for the limits a share's +// owner sets on its use. +// +// Triggers keep the state right whatever writes the rows they watch: +// +// - On `realm_user_permissions`, any change to a person's row suspends that +// person's active shares in the realm when the row no longer grants write, +// and resumes the ones it suspended for that reason when it does again. +// Only a direct writer (see `isDirectWriter`) can share or swap a key in, +// and a wildcard or bot account owns no key, so a share's owner is always a +// person and their own row is the whole answer. +// - On `user_keys`, deleting a key suspends every share holding it. +// - On `key_shares`, a share going from active to suspended records a row in +// `key_share_notices` for the realm's direct writers. +// +// Both tables are withheld from the read-only role used by dashboards and +// read-only database access, and from every role that is a member of it, as +// `user_keys` is. Every statement is idempotent, so running `up` again changes +// nothing. + +exports.up = (pgm) => { + pgm.sql(` + CREATE TABLE IF NOT EXISTS key_shares ( + id uuid PRIMARY KEY, + realm_url varchar NOT NULL, + label varchar NOT NULL, + -- The Matrix user id of the owner of the key the share holds. + owner varchar NOT NULL, + key_id uuid REFERENCES user_keys (id) ON DELETE SET NULL, + state varchar NOT NULL, + suspension_reason varchar, + suspended_by varchar, + suspended_at timestamptz, + limits jsonb NOT NULL DEFAULT '{}'::jsonb, + created_at timestamptz NOT NULL DEFAULT NOW(), + updated_at timestamptz NOT NULL DEFAULT NOW(), + CONSTRAINT key_shares_realm_url_label_key UNIQUE (realm_url, label), + CONSTRAINT key_shares_state_check CHECK ( + ( + state = 'active' + AND key_id IS NOT NULL + AND suspension_reason IS NULL + AND suspended_by IS NULL + AND suspended_at IS NULL + ) + OR ( + state = 'suspended' + AND suspended_at IS NOT NULL + AND ( + (suspension_reason = 'owner-lost-write' AND key_id IS NOT NULL AND suspended_by IS NULL) + OR (suspension_reason = 'suspended-by-writer' AND key_id IS NOT NULL AND suspended_by IS NOT NULL) + OR (suspension_reason = 'key-deleted' AND key_id IS NULL AND suspended_by IS NULL) + ) + ) + ) + ) + `); + pgm.sql(` + CREATE INDEX IF NOT EXISTS key_shares_realm_url_owner_idx + ON key_shares (realm_url, owner) + `); + pgm.sql(` + CREATE INDEX IF NOT EXISTS key_shares_owner_idx ON key_shares (owner) + `); + pgm.sql(` + CREATE INDEX IF NOT EXISTS key_shares_key_id_idx ON key_shares (key_id) + `); + pgm.sql(` + CREATE TABLE IF NOT EXISTS key_share_notices ( + id bigserial PRIMARY KEY, + share_id uuid NOT NULL REFERENCES key_shares (id) ON DELETE CASCADE, + realm_url varchar NOT NULL, + label varchar NOT NULL, + owner varchar NOT NULL, + reason varchar NOT NULL, + suspended_by varchar, + created_at timestamptz NOT NULL DEFAULT NOW() + ) + `); + pgm.sql(` + CREATE INDEX IF NOT EXISTS key_share_notices_realm_url_idx + ON key_share_notices (realm_url, created_at) + `); + + pgm.sql(` + CREATE OR REPLACE FUNCTION key_shares_follow_owner_write( + share_realm_url varchar, + share_owner varchar + ) RETURNS void LANGUAGE plpgsql AS $$ + BEGIN + IF EXISTS ( + SELECT 1 FROM realm_user_permissions + WHERE realm_url = share_realm_url + AND username = share_owner + AND write + ) THEN + UPDATE key_shares + SET state = 'active', + suspension_reason = NULL, + suspended_at = NULL, + updated_at = NOW() + WHERE realm_url = share_realm_url + AND owner = share_owner + AND state = 'suspended' + AND suspension_reason = 'owner-lost-write'; + ELSE + UPDATE key_shares + SET state = 'suspended', + suspension_reason = 'owner-lost-write', + suspended_at = NOW(), + updated_at = NOW() + WHERE realm_url = share_realm_url + AND owner = share_owner + AND state = 'active'; + END IF; + END + $$ + `); + pgm.sql(` + CREATE OR REPLACE FUNCTION key_shares_on_permission_change() + RETURNS trigger LANGUAGE plpgsql AS $$ + BEGIN + IF TG_OP IN ('UPDATE', 'DELETE') THEN + PERFORM key_shares_follow_owner_write(OLD.realm_url, OLD.username); + END IF; + IF TG_OP IN ('INSERT', 'UPDATE') THEN + PERFORM key_shares_follow_owner_write(NEW.realm_url, NEW.username); + END IF; + RETURN NULL; + END + $$ + `); + pgm.sql(` + DROP TRIGGER IF EXISTS key_shares_follow_permissions ON realm_user_permissions + `); + pgm.sql(` + CREATE TRIGGER key_shares_follow_permissions + AFTER INSERT OR UPDATE OR DELETE ON realm_user_permissions + FOR EACH ROW EXECUTE FUNCTION key_shares_on_permission_change() + `); + + pgm.sql(` + CREATE OR REPLACE FUNCTION key_shares_on_key_delete() + RETURNS trigger LANGUAGE plpgsql AS $$ + BEGIN + UPDATE key_shares + SET state = 'suspended', + suspension_reason = 'key-deleted', + suspended_by = NULL, + suspended_at = COALESCE(suspended_at, NOW()), + key_id = NULL, + updated_at = NOW() + WHERE key_id = OLD.id; + RETURN OLD; + END + $$ + `); + pgm.sql(`DROP TRIGGER IF EXISTS key_shares_follow_key_delete ON user_keys`); + pgm.sql(` + CREATE TRIGGER key_shares_follow_key_delete + BEFORE DELETE ON user_keys + FOR EACH ROW EXECUTE FUNCTION key_shares_on_key_delete() + `); + + pgm.sql(` + CREATE OR REPLACE FUNCTION key_shares_record_suspension() + RETURNS trigger LANGUAGE plpgsql AS $$ + BEGIN + INSERT INTO key_share_notices + (share_id, realm_url, label, owner, reason, suspended_by) + VALUES + (NEW.id, NEW.realm_url, NEW.label, NEW.owner, NEW.suspension_reason, NEW.suspended_by); + RETURN NULL; + END + $$ + `); + pgm.sql(`DROP TRIGGER IF EXISTS key_shares_notice_suspension ON key_shares`); + pgm.sql(` + CREATE TRIGGER key_shares_notice_suspension + AFTER UPDATE OF state ON key_shares + FOR EACH ROW + WHEN (OLD.state = 'active' AND NEW.state = 'suspended') + EXECUTE FUNCTION key_shares_record_suspension() + `); + + // A ticket to share a key names the key and the realm it goes into; a + // ticket to swap a key in names the key and the share it goes into. + pgm.sql(` + ALTER TABLE key_write_tickets ADD COLUMN IF NOT EXISTS realm_url varchar + `); + pgm.sql(` + ALTER TABLE key_write_tickets ADD COLUMN IF NOT EXISTS share_id uuid + `); + pgm.sql(` + ALTER TABLE key_write_tickets + DROP CONSTRAINT IF EXISTS key_write_tickets_action_key_id_check + `); + pgm.sql(` + ALTER TABLE key_write_tickets + ADD CONSTRAINT key_write_tickets_action_key_id_check CHECK ( + (action = 'add' AND key_id IS NULL AND realm_url IS NULL AND share_id IS NULL) + OR (action IN ('replace', 'delete') AND key_id IS NOT NULL AND realm_url IS NULL AND share_id IS NULL) + OR (action = 'share' AND key_id IS NOT NULL AND realm_url IS NOT NULL AND share_id IS NULL) + OR (action = 'swap' AND key_id IS NOT NULL AND realm_url IS NULL AND share_id IS NOT NULL) + ) + `); + + pgm.sql(` + DO $$ + DECLARE + member_name name; + table_name text; + BEGIN + IF EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'readonly_role') THEN + FOREACH table_name IN ARRAY ARRAY['key_shares', 'key_share_notices'] + LOOP + EXECUTE format('REVOKE SELECT ON TABLE %I FROM readonly_role', table_name); + FOR member_name IN + SELECT member.rolname + FROM pg_auth_members membership + JOIN pg_roles grp ON grp.oid = membership.roleid + JOIN pg_roles member ON member.oid = membership.member + WHERE grp.rolname = 'readonly_role' + LOOP + EXECUTE format( + 'REVOKE SELECT ON TABLE %I FROM %I', + table_name, + member_name + ); + END LOOP; + END LOOP; + END IF; + END + $$ + `); +}; + +exports.down = (pgm) => { + pgm.sql(` + DELETE FROM key_write_tickets WHERE action IN ('share', 'swap') + `); + pgm.sql(` + ALTER TABLE key_write_tickets + DROP CONSTRAINT IF EXISTS key_write_tickets_action_key_id_check + `); + pgm.sql(` + ALTER TABLE key_write_tickets + ADD CONSTRAINT key_write_tickets_action_key_id_check CHECK ( + (action = 'add' AND key_id IS NULL) + OR (action IN ('replace', 'delete') AND key_id IS NOT NULL) + ) + `); + pgm.sql(`ALTER TABLE key_write_tickets DROP COLUMN IF EXISTS share_id`); + pgm.sql(`ALTER TABLE key_write_tickets DROP COLUMN IF EXISTS realm_url`); + pgm.sql(`DROP TRIGGER IF EXISTS key_shares_notice_suspension ON key_shares`); + pgm.sql(`DROP TRIGGER IF EXISTS key_shares_follow_key_delete ON user_keys`); + pgm.sql(` + DROP TRIGGER IF EXISTS key_shares_follow_permissions ON realm_user_permissions + `); + pgm.sql(`DROP FUNCTION IF EXISTS key_shares_record_suspension()`); + pgm.sql(`DROP FUNCTION IF EXISTS key_shares_on_key_delete()`); + pgm.sql(`DROP FUNCTION IF EXISTS key_shares_on_permission_change()`); + pgm.sql( + `DROP FUNCTION IF EXISTS key_shares_follow_owner_write(varchar, varchar)`, + ); + pgm.sql(`DROP TABLE IF EXISTS key_share_notices`); + pgm.sql(`DROP TABLE IF EXISTS key_shares`); +}; diff --git a/packages/postgres/scripts/schema-dump.sh b/packages/postgres/scripts/schema-dump.sh index 9b6e1c311ff..83e77530011 100755 --- a/packages/postgres/scripts/schema-dump.sh +++ b/packages/postgres/scripts/schema-dump.sh @@ -34,6 +34,8 @@ docker exec boxel-pg pg_dump \ --exclude-table-and-children=proxy_endpoints \ --exclude-table-and-children=user_keys \ --exclude-table-and-children=key_write_tickets \ + --exclude-table-and-children=key_shares \ + --exclude-table-and-children=key_share_notices \ --exclude-table-and-children=secure_sessions \ --exclude-table-and-children=proxy_endpoint_history \ --exclude-table-and-children=billable_call_reservations \ diff --git a/packages/realm-server/handlers/handle-key-shares.ts b/packages/realm-server/handlers/handle-key-shares.ts new file mode 100644 index 00000000000..4f6115ba5ad --- /dev/null +++ b/packages/realm-server/handlers/handle-key-shares.ts @@ -0,0 +1,166 @@ +import type Koa from 'koa'; +import { setContextResponse } from '../middleware/index.ts'; +import type { CreateRoutesArgs } from '../routes.ts'; +import { + listOwnShares, + listRealmShareNotices, + listRealmShares, + resumeShare, + shareKey, + suspendShare, + swapKeyIntoShare, + unshareKey, + type KeyShare, + type KeyShareNotice, +} from '../lib/key-shares.ts'; +import { UserKeyError } from '../lib/user-keys.ts'; +import { + keysHandler, + ownerOf, + readJSON, + sendJSON, + ticketOf, +} from './handle-user-keys.ts'; + +// The shares API: sharing the signed-in user's keys into realms, and what a +// realm's direct writers do with its shares. Who may do what is set out in +// `lib/key-shares.ts`. Sharing and swapping each take a key write ticket in +// `meta`, so, as for the keys API, nothing here logs a body and the request +// log keeps only the method and path of these routes. + +type Handler = (ctxt: Koa.Context, next: Koa.Next) => Promise; + +function serialize(share: KeyShare) { + let { id, ...attributes } = share; + return { type: 'key-share', id, attributes }; +} + +function serializeNotice(notice: KeyShareNotice) { + let { id, ...attributes } = notice; + return { type: 'key-share-notice', id, attributes }; +} + +async function readShareBody( + ctxt: Koa.Context, + allowed: Set, +): Promise<{ attributes: Record; ticket: unknown }> { + let body = await readJSON(ctxt); + let data = body.data as + | { type?: unknown; attributes?: Record } + | undefined; + if (!data || typeof data !== 'object' || data.type !== 'key-share') { + throw new UserKeyError(400, `the resource type is 'key-share'`); + } + let attributes = data.attributes ?? {}; + if (typeof attributes !== 'object' || Array.isArray(attributes)) { + throw new UserKeyError(400, 'attributes must be an object'); + } + for (let name of Object.keys(attributes)) { + if (!allowed.has(name)) { + throw new UserKeyError(400, `'${name}' can't be set here`); + } + } + return { attributes, ticket: ticketOf(body) }; +} + +const SHARE_ATTRIBUTES = new Set(['realm', 'keyId', 'label']); +const SWAP_ATTRIBUTES = new Set(['keyId']); + +// `GET /_key-shares` lists the user's own shares, in every realm; with +// `?key=`, only those holding that key, which deleting it would suspend. +// `GET /_key-shares?realm=` lists a realm's shares, for one of its +// direct writers. +export function handleListKeyShares({ dbAdapter }: CreateRoutesArgs): Handler { + return keysHandler(async (ctxt) => { + let user = ownerOf(ctxt); + let realm = ctxt.URL.searchParams.get('realm'); + let keyId = ctxt.URL.searchParams.get('key'); + if (realm !== null && keyId !== null) { + throw new UserKeyError(400, 'list by realm or by key, not both'); + } + let shares = + realm !== null + ? await listRealmShares(dbAdapter, user, realm) + : await listOwnShares(dbAdapter, user, { + keyId: keyId ?? undefined, + }); + await sendJSON(ctxt, 200, { data: shares.map(serialize) }); + }); +} + +export function handleListKeyShareNotices({ + dbAdapter, +}: CreateRoutesArgs): Handler { + return keysHandler(async (ctxt) => { + let user = ownerOf(ctxt); + let notices = await listRealmShareNotices( + dbAdapter, + user, + ctxt.URL.searchParams.get('realm'), + ); + await sendJSON(ctxt, 200, { data: notices.map(serializeNotice) }); + }); +} + +export function handleShareKey({ dbAdapter }: CreateRoutesArgs): Handler { + return keysHandler(async (ctxt) => { + let owner = ownerOf(ctxt); + let { attributes, ticket } = await readShareBody(ctxt, SHARE_ATTRIBUTES); + let share = await shareKey(dbAdapter, { + owner, + realm: attributes.realm, + keyId: attributes.keyId, + label: attributes.label, + ticket, + }); + await sendJSON(ctxt, 201, { data: serialize(share) }); + }); +} + +export function handleUnshareKey({ dbAdapter }: CreateRoutesArgs): Handler { + return keysHandler(async (ctxt) => { + let user = ownerOf(ctxt); + await unshareKey(dbAdapter, { user, shareId: ctxt.params.shareId }); + await setContextResponse(ctxt, new Response(null, { status: 204 })); + }); +} + +export function handleSuspendKeyShare({ + dbAdapter, +}: CreateRoutesArgs): Handler { + return keysHandler(async (ctxt) => { + let user = ownerOf(ctxt); + let share = await suspendShare(dbAdapter, { + user, + shareId: ctxt.params.shareId, + }); + await sendJSON(ctxt, 200, { data: serialize(share) }); + }); +} + +export function handleResumeKeyShare({ dbAdapter }: CreateRoutesArgs): Handler { + return keysHandler(async (ctxt) => { + let user = ownerOf(ctxt); + let share = await resumeShare(dbAdapter, { + user, + shareId: ctxt.params.shareId, + }); + await sendJSON(ctxt, 200, { data: serialize(share) }); + }); +} + +export function handleSwapKeyIntoShare({ + dbAdapter, +}: CreateRoutesArgs): Handler { + return keysHandler(async (ctxt) => { + let user = ownerOf(ctxt); + let { attributes, ticket } = await readShareBody(ctxt, SWAP_ATTRIBUTES); + let share = await swapKeyIntoShare(dbAdapter, { + user, + shareId: ctxt.params.shareId, + keyId: attributes.keyId, + ticket, + }); + await sendJSON(ctxt, 200, { data: serialize(share) }); + }); +} diff --git a/packages/realm-server/handlers/handle-user-keys.ts b/packages/realm-server/handlers/handle-user-keys.ts index e09023928d7..b428dadcb29 100644 --- a/packages/realm-server/handlers/handle-user-keys.ts +++ b/packages/realm-server/handlers/handle-user-keys.ts @@ -20,7 +20,8 @@ import { type UserKeyMetadata, } from '../lib/user-keys.ts'; -// The management API for the signed-in user's own keys. Every route answers +// The management API for the signed-in user's own keys. The shares API +// (`handle-key-shares.ts`) answers with the same helpers. Every route answers // for the token's user alone, and no route returns a key's value. Adding, // replacing and deleting a key each take a key write ticket minted for that // change. A request body @@ -51,7 +52,11 @@ function serialize(key: UserKeyMetadata) { return { type: 'user-key', id, attributes }; } -async function sendJSON(ctxt: Koa.Context, status: number, body: unknown) { +export async function sendJSON( + ctxt: Koa.Context, + status: number, + body: unknown, +) { await setContextResponse( ctxt, new Response(JSON.stringify(body), { @@ -65,7 +70,7 @@ async function sendJSON(ctxt: Koa.Context, status: number, body: unknown) { // session check ahead of these routes already refuses a delegated session; a // realm-authority session is a realm acting on its own authority, not the // user, so it is refused here. -function ownerOf(ctxt: Koa.Context): string { +export function ownerOf(ctxt: Koa.Context): string { let token = ctxt.state.token as RealmServerTokenClaim | undefined; if (!token?.user) { throw new UserKeyError(403, 'keys are managed by a signed-in user'); @@ -100,7 +105,9 @@ async function readRawBody(ctxt: Koa.Context): Promise { return Buffer.concat(chunks).toString('utf8'); } -async function readJSON(ctxt: Koa.Context): Promise> { +export async function readJSON( + ctxt: Koa.Context, +): Promise> { let raw = await readRawBody(ctxt); let json: unknown; try { @@ -114,7 +121,7 @@ async function readJSON(ctxt: Koa.Context): Promise> { return json as Record; } -function ticketOf(body: { meta?: unknown }): unknown { +export function ticketOf(body: { meta?: unknown }): unknown { let meta = body.meta; return meta && typeof meta === 'object' ? (meta as Record).ticket @@ -158,7 +165,7 @@ function attributesOf( // Runs a handler, answering a `UserKeyError` with its status and message and // anything else with a bare 500. Only an error's message is logged, never the // request. -function keysHandler(fn: (ctxt: Koa.Context) => Promise): Handler { +export function keysHandler(fn: (ctxt: Koa.Context) => Promise): Handler { return async function (ctxt: Koa.Context, _next: Koa.Next) { try { await fn(ctxt); @@ -246,10 +253,12 @@ export function handleMintKeyWriteTicket({ if (!data || typeof data !== 'object' || data.type !== 'key-write-ticket') { throw new UserKeyError(400, `the resource type is 'key-write-ticket'`); } - let target = validateKeyWriteTarget( - data.attributes?.action, - data.attributes?.keyId, - ); + let target = validateKeyWriteTarget({ + action: data.attributes?.action, + keyId: data.attributes?.keyId, + realm: data.attributes?.realm, + shareId: data.attributes?.shareId, + }); let { ticket, expiresAt } = await mintKeyWriteTicket( dbAdapter, owner, diff --git a/packages/realm-server/lib/key-shares.ts b/packages/realm-server/lib/key-shares.ts new file mode 100644 index 00000000000..d5bc8d284ea --- /dev/null +++ b/packages/realm-server/lib/key-shares.ts @@ -0,0 +1,456 @@ +import { randomUUID } from 'node:crypto'; +import { + isDirectWriter, + isDirectWriterLocked, + param, + query, + type DBAdapter, +} from '@cardstack/runtime-common'; +import type { Querier } from '@cardstack/runtime-common/expression'; +import { normalizeRealmURL } from '../utils/realm-url.ts'; +import { consumeKeyWriteTicket, UserKeyError } from './user-keys.ts'; + +// Shares: a key its owner has put into a realm for other people there to +// consume. A share holds no secret, only the key it refers to, so one key may +// be shared into several realms and its value is stored once. +// +// Who may do what: +// +// - Only a key's owner shares it, and only into a realm where they are a +// direct writer (`isDirectWriter`). Sharing takes a key write ticket for +// that key and that realm. +// - Only a share's owner unshares it, whether or not they still write the +// realm: withdrawing one's own key needs no write. +// - Any direct writer of the realm suspends an active share, and lifts a +// suspension a direct writer made. +// - Any direct writer of the realm swaps one of their own keys into a +// suspended share, which makes them its owner and resumes it. Swapping takes +// a key write ticket for that key and that share. Nobody swaps in another +// user's key. +// - A share's owner sees all of their shares, in every realm. A realm's direct +// writers see its shares, with owner and state, and the notices recorded +// when one is suspended. Nobody else sees anything of a realm's shares. +// +// Suspensions that follow from other changes are made by triggers on the +// tables those changes write (see the migration that adds `key_shares`), so +// they hold however the change is made: a share is suspended when its owner +// stops being a direct writer of the realm, and resumes when they are one +// again; and it is suspended, keeping its place, when its key is deleted. +// Every share going from active to suspended records a notice. +// +// Every check that a person is a direct writer runs inside the change's +// transaction with their permission row locked (`isDirectWriterLocked`), so a +// concurrent removal of their write waits for the change to commit and then +// suspends what it made. + +export type KeyShareState = 'active' | 'suspended'; + +export type KeyShareSuspensionReason = + | 'owner-lost-write' + | 'suspended-by-writer' + | 'key-deleted'; + +export interface KeyShare { + id: string; + realm: string; + label: string; + owner: string; + // The key the share holds; given only to the share's owner, and null once + // the key is deleted. + keyId?: string | null; + state: KeyShareState; + suspensionReason: KeyShareSuspensionReason | null; + suspendedBy: string | null; + suspendedAt: string | null; + createdAt: string; + updatedAt: string; +} + +export interface KeyShareNotice { + id: string; + shareId: string; + realm: string; + label: string; + owner: string; + reason: KeyShareSuspensionReason; + suspendedBy: string | null; + createdAt: string; +} + +// The most notices a realm's listing returns, newest first. +export const MAX_KEY_SHARE_NOTICES = 100; + +const SHARE_COLUMNS = `id, realm_url, label, owner, key_id, state, suspension_reason, suspended_by, suspended_at, created_at, updated_at`; + +function iso(value: unknown): string | null { + return value == null ? null : new Date(value as string).toISOString(); +} + +function toShare( + row: Record, + { withKey }: { withKey: boolean }, +): KeyShare { + return { + id: row.id as string, + realm: row.realm_url as string, + label: row.label as string, + owner: row.owner as string, + ...(withKey ? { keyId: (row.key_id as string | null) ?? null } : {}), + state: row.state as KeyShareState, + suspensionReason: + (row.suspension_reason as KeyShareSuspensionReason | null) ?? null, + suspendedBy: (row.suspended_by as string | null) ?? null, + suspendedAt: iso(row.suspended_at), + createdAt: iso(row.created_at)!, + updatedAt: iso(row.updated_at)!, + }; +} + +const UUID = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; + +function noSuchShare() { + return new UserKeyError(404, 'no such share'); +} + +function noSuchKey() { + return new UserKeyError(404, 'no such key'); +} + +function notADirectWriter() { + return new UserKeyError( + 403, + 'only a direct writer of the realm can do this to its shares', + ); +} + +export function validateRealm(realm: unknown): URL { + let realmURL = typeof realm === 'string' ? normalizeRealmURL(realm) : null; + if (!realmURL) { + throw new UserKeyError(400, 'a share names the realm it is in'); + } + return realmURL; +} + +export const MAX_KEY_SHARE_LABEL_LENGTH = 100; + +export function validateShareLabel(label: unknown): string { + let trimmed = typeof label === 'string' ? label.trim() : ''; + if (trimmed.length === 0) { + throw new UserKeyError(400, 'a share needs a label'); + } + if (trimmed.length > MAX_KEY_SHARE_LABEL_LENGTH) { + throw new UserKeyError( + 400, + `a share's label is at most ${MAX_KEY_SHARE_LABEL_LENGTH} characters`, + ); + } + // eslint-disable-next-line no-control-regex + if (/[\u0000-\u001f\u007f]/.test(trimmed)) { + throw new UserKeyError( + 400, + `a share's label can't contain control characters`, + ); + } + return trimmed; +} + +// The user's own key, locked against a concurrent delete until the caller's +// transaction ends. Another user's key answers as one that doesn't exist. +async function lockOwnKey( + txQuery: Querier, + owner: string, + keyId: unknown, +): Promise { + if (typeof keyId !== 'string' || !UUID.test(keyId)) { + throw noSuchKey(); + } + let rows = await txQuery([ + `SELECT id FROM user_keys WHERE id = `, + param(keyId), + ` AND owner = `, + param(owner), + ` FOR SHARE`, + ]); + if (rows.length === 0) { + throw noSuchKey(); + } + return keyId; +} + +// The share, locked for the caller's change, if `user` may see it: as its +// owner, or as a direct writer of its realm. Any other share answers as one +// that doesn't exist. +async function lockVisibleShare( + txQuery: Querier, + user: string, + shareId: string, +): Promise<{ row: Record; isWriter: boolean }> { + if (!UUID.test(shareId)) { + throw noSuchShare(); + } + let [row] = await txQuery([ + `SELECT ${SHARE_COLUMNS} FROM key_shares WHERE id = `, + param(shareId), + ` FOR UPDATE`, + ]); + if (!row) { + throw noSuchShare(); + } + let isWriter = await isDirectWriterLocked( + txQuery, + new URL(row.realm_url as string), + user, + ); + if (!isWriter && row.owner !== user) { + throw noSuchShare(); + } + return { row, isWriter }; +} + +export async function shareKey( + dbAdapter: DBAdapter, + { + owner, + realm, + keyId, + label, + ticket, + }: { + owner: string; + realm: unknown; + keyId: unknown; + label: unknown; + ticket: unknown; + }, +): Promise { + let realmURL = validateRealm(realm); + let validLabel = validateShareLabel(label); + if (typeof keyId !== 'string') { + throw new UserKeyError(400, 'a share names the key it holds'); + } + let id = randomUUID(); + return await dbAdapter.withTransaction(async (txQuery) => { + await consumeKeyWriteTicket(txQuery, owner, ticket, { + action: 'share', + keyId, + realmURL: realmURL.href, + shareId: null, + }); + if (!(await isDirectWriterLocked(txQuery, realmURL, owner))) { + throw new UserKeyError( + 403, + 'a key is shared only into a realm its owner is a direct writer of', + ); + } + await lockOwnKey(txQuery, owner, keyId); + let [row] = await txQuery([ + `INSERT INTO key_shares (id, realm_url, label, owner, key_id, state) VALUES (`, + param(id), + `, `, + param(realmURL.href), + `, `, + param(validLabel), + `, `, + param(owner), + `, `, + param(keyId), + `, 'active') ON CONFLICT (realm_url, label) DO NOTHING RETURNING ${SHARE_COLUMNS}`, + ]); + if (!row) { + throw new UserKeyError( + 409, + `the realm already has a share labelled ${validLabel}`, + ); + } + return toShare(row, { withKey: true }); + }); +} + +export async function unshareKey( + dbAdapter: DBAdapter, + { user, shareId }: { user: string; shareId: string }, +): Promise { + await dbAdapter.withTransaction(async (txQuery) => { + let { row } = await lockVisibleShare(txQuery, user, shareId); + if (row.owner !== user) { + throw new UserKeyError(403, 'only its owner unshares a share'); + } + await txQuery([`DELETE FROM key_shares WHERE id = `, param(shareId)]); + }); +} + +// A direct writer suspends an active share, for abuse or any other reason of +// theirs. The realm's direct writers get a notice of it. +export async function suspendShare( + dbAdapter: DBAdapter, + { user, shareId }: { user: string; shareId: string }, +): Promise { + return await dbAdapter.withTransaction(async (txQuery) => { + let { row, isWriter } = await lockVisibleShare(txQuery, user, shareId); + if (!isWriter) { + throw notADirectWriter(); + } + if (row.state !== 'active') { + throw new UserKeyError(409, 'the share is already suspended'); + } + let [updated] = await txQuery([ + `UPDATE key_shares SET state = 'suspended', suspension_reason = 'suspended-by-writer', suspended_by = `, + param(user), + `, suspended_at = NOW(), updated_at = NOW() WHERE id = `, + param(shareId), + ` RETURNING ${SHARE_COLUMNS}`, + ]); + return toShare(updated, { withKey: updated.owner === user }); + }); +} + +// A direct writer lifts a suspension a direct writer made. If the share's +// owner is no longer a direct writer, the share stays suspended for that +// reason instead, and resumes when they are one again. +export async function resumeShare( + dbAdapter: DBAdapter, + { user, shareId }: { user: string; shareId: string }, +): Promise { + return await dbAdapter.withTransaction(async (txQuery) => { + let { row, isWriter } = await lockVisibleShare(txQuery, user, shareId); + if (!isWriter) { + throw notADirectWriter(); + } + if (row.suspension_reason !== 'suspended-by-writer') { + throw new UserKeyError( + 409, + row.state === 'active' + ? 'the share is not suspended' + : 'only a suspension a writer made is lifted; swap a key in instead', + ); + } + let ownerWrites = await isDirectWriterLocked( + txQuery, + new URL(row.realm_url as string), + row.owner as string, + ); + let [updated] = await txQuery([ + ownerWrites + ? `UPDATE key_shares SET state = 'active', suspension_reason = NULL, suspended_at = NULL` + : `UPDATE key_shares SET suspension_reason = 'owner-lost-write'`, + `, suspended_by = NULL, updated_at = NOW() WHERE id = `, + param(shareId), + ` RETURNING ${SHARE_COLUMNS}`, + ]); + return toShare(updated, { withKey: updated.owner === user }); + }); +} + +// A direct writer puts one of their own keys into a suspended share. They +// become its owner and it resumes; its label, and so every card that names +// it, is unchanged. +export async function swapKeyIntoShare( + dbAdapter: DBAdapter, + { + user, + shareId, + keyId, + ticket, + }: { user: string; shareId: string; keyId: unknown; ticket: unknown }, +): Promise { + if (typeof keyId !== 'string') { + throw new UserKeyError(400, 'a swap names the key it puts in'); + } + return await dbAdapter.withTransaction(async (txQuery) => { + await consumeKeyWriteTicket(txQuery, user, ticket, { + action: 'swap', + keyId, + realmURL: null, + shareId, + }); + let { row, isWriter } = await lockVisibleShare(txQuery, user, shareId); + if (!isWriter) { + throw notADirectWriter(); + } + if (row.state !== 'suspended') { + throw new UserKeyError(409, 'only a suspended share takes another key'); + } + await lockOwnKey(txQuery, user, keyId); + let [updated] = await txQuery([ + `UPDATE key_shares SET owner = `, + param(user), + `, key_id = `, + param(keyId), + `, state = 'active', suspension_reason = NULL, suspended_by = NULL, suspended_at = NULL, updated_at = NOW() WHERE id = `, + param(shareId), + ` RETURNING ${SHARE_COLUMNS}`, + ]); + return toShare(updated, { withKey: true }); + }); +} + +// The user's own shares, in every realm, by realm and label. With `keyId`, +// only the shares holding that key: the ones deleting it would suspend. +export async function listOwnShares( + dbAdapter: DBAdapter, + owner: string, + { keyId }: { keyId?: string } = {}, +): Promise { + if (keyId !== undefined && !UUID.test(keyId)) { + return []; + } + let rows = await query(dbAdapter, [ + `SELECT ${SHARE_COLUMNS} FROM key_shares WHERE owner = `, + param(owner), + ...(keyId !== undefined ? [` AND key_id = `, param(keyId)] : []), + ` ORDER BY realm_url, label`, + ]); + return rows.map((row) => toShare(row, { withKey: true })); +} + +async function assertDirectWriter( + dbAdapter: DBAdapter, + realmURL: URL, + user: string, +): Promise { + if (!(await isDirectWriter(dbAdapter, realmURL, user))) { + throw new UserKeyError(403, `only a realm's direct writers see its shares`); + } +} + +// A realm's shares, by label, for one of its direct writers. +export async function listRealmShares( + dbAdapter: DBAdapter, + user: string, + realm: unknown, +): Promise { + let realmURL = validateRealm(realm); + await assertDirectWriter(dbAdapter, realmURL, user); + let rows = await query(dbAdapter, [ + `SELECT ${SHARE_COLUMNS} FROM key_shares WHERE realm_url = `, + param(realmURL.href), + ` ORDER BY label`, + ]); + return rows.map((row) => toShare(row, { withKey: row.owner === user })); +} + +// The notices recorded when one of a realm's shares was suspended, newest +// first, for one of its direct writers. +export async function listRealmShareNotices( + dbAdapter: DBAdapter, + user: string, + realm: unknown, +): Promise { + let realmURL = validateRealm(realm); + await assertDirectWriter(dbAdapter, realmURL, user); + let rows = await query(dbAdapter, [ + `SELECT id, share_id, realm_url, label, owner, reason, suspended_by, created_at FROM key_share_notices WHERE realm_url = `, + param(realmURL.href), + ` ORDER BY created_at DESC, id DESC LIMIT ${MAX_KEY_SHARE_NOTICES}`, + ]); + return rows.map((row) => ({ + id: String(row.id), + shareId: row.share_id as string, + realm: row.realm_url as string, + label: row.label as string, + owner: row.owner as string, + reason: row.reason as KeyShareSuspensionReason, + suspendedBy: (row.suspended_by as string | null) ?? null, + createdAt: iso(row.created_at)!, + })); +} diff --git a/packages/realm-server/lib/request-telemetry.ts b/packages/realm-server/lib/request-telemetry.ts index dd00649cae0..ec305650df6 100644 --- a/packages/realm-server/lib/request-telemetry.ts +++ b/packages/realm-server/lib/request-telemetry.ts @@ -46,15 +46,20 @@ const proxyRoutePaths = new Set([ ]); // The routes that manage a user's own keys: `/_user-keys`, each key beneath -// it, and the key write tickets. With them, the secure-origin sign-in routes, -// whose bodies carry a password or the session they sign in. +// it, the key write tickets, and the shares of keys into realms with their +// notices. With them, the secure-origin sign-in routes, whose bodies carry a +// password or the session they sign in. export const USER_KEYS_PATH = '/_user-keys'; export const KEY_WRITE_TICKETS_PATH = '/_key-write-tickets'; +export const KEY_SHARES_PATH = '/_key-shares'; +export const KEY_SHARE_NOTICES_PATH = '/_key-share-notices'; export const SECURE_SESSION_PATH = '/_secure-session'; const keyRoutePaths = [ USER_KEYS_PATH, KEY_WRITE_TICKETS_PATH, + KEY_SHARES_PATH, + KEY_SHARE_NOTICES_PATH, SECURE_SESSION_PATH, ]; diff --git a/packages/realm-server/lib/user-keys.ts b/packages/realm-server/lib/user-keys.ts index 376246887a4..d6c349549e2 100644 --- a/packages/realm-server/lib/user-keys.ts +++ b/packages/realm-server/lib/user-keys.ts @@ -1,6 +1,7 @@ import { createHash, randomBytes, randomUUID } from 'node:crypto'; import { param, query, type DBAdapter } from '@cardstack/runtime-common'; import type { Expression, Querier } from '@cardstack/runtime-common/expression'; +import { normalizeRealmURL } from '../utils/realm-url.ts'; import type { SealedSecret, SecretBinding, @@ -15,10 +16,11 @@ import type { // A key is only its secret. It records no destination: nothing here says // where a key may be sent, and the value is sealed to the key alone. // -// A key is added, replaced or deleted only with a key write ticket: -// short-lived, single-use, minted for the signed-in user and for one change -// (an add, or a replace or delete of one key), and consumed in the same -// transaction as that change. The routes mint tickets only for requests from +// A key is added, replaced, deleted, shared into a realm or swapped into a +// share only with a key write ticket: short-lived, single-use, minted for the +// signed-in user and for one change (an add; a replace or delete of one key; a +// share of one key into one realm; or a swap of one key into one share), and +// consumed in the same transaction as that change. The routes mint tickets only for requests from // the secure origin and for clients that send no `Origin`, as the CLI does, so // card code in the app page can't change a key from inside the browser. @@ -29,7 +31,13 @@ export type UserKeyKind = (typeof USER_KEY_KINDS)[number]; export type UserKeyState = 'active'; -export const KEY_WRITE_ACTIONS = ['add', 'replace', 'delete'] as const; +export const KEY_WRITE_ACTIONS = [ + 'add', + 'replace', + 'delete', + 'share', + 'swap', +] as const; export type KeyWriteAction = (typeof KEY_WRITE_ACTIONS)[number]; export const KEY_WRITE_TICKET_TTL_MS = 10 * 60 * 1000; @@ -65,45 +73,82 @@ function sha256(value: string): string { return createHash('sha256').update(value, 'utf8').digest('hex'); } -// The change a key write ticket is minted for: an add, or a replace or delete -// of one key. +// The change a key write ticket is minted for: an add; a replace or delete of +// one key; a share of one key into one realm; or a swap of one key into one +// share. export interface KeyWriteTarget { action: KeyWriteAction; - // The key a replace or delete changes; an add names none. + // The key a replace, delete, share or swap uses; an add names none. keyId: string | null; + // The realm a share puts the key into; only a share names one. + realmURL: string | null; + // The share a swap puts the key into; only a swap names one. + shareId: string | null; } -export function validateKeyWriteTarget( - action: unknown, - keyId: unknown, -): KeyWriteTarget { +export function validateKeyWriteTarget({ + action, + keyId, + realm, + shareId, +}: { + action: unknown; + keyId?: unknown; + realm?: unknown; + shareId?: unknown; +}): KeyWriteTarget { if (!KEY_WRITE_ACTIONS.includes(action as KeyWriteAction)) { throw new UserKeyError( 400, `a ticket's action is one of: ${KEY_WRITE_ACTIONS.join(', ')}`, ); } + let named = (value: unknown) => value !== undefined && value !== null; + if (named(realm) && action !== 'share') { + throw new UserKeyError(400, `a ticket to ${action} names no realm`); + } + if (named(shareId) && action !== 'swap') { + throw new UserKeyError(400, `a ticket to ${action} names no share`); + } if (action === 'add') { - if (keyId !== undefined && keyId !== null) { + if (named(keyId)) { throw new UserKeyError(400, 'a ticket to add a key names no key'); } - return { action, keyId: null }; + return { action, keyId: null, realmURL: null, shareId: null }; } if (typeof keyId !== 'string') { throw new UserKeyError(400, `a ticket to ${action} a key names the key`); } - return { action: action as KeyWriteAction, keyId }; + if (action === 'share') { + let realmURL = typeof realm === 'string' ? normalizeRealmURL(realm) : null; + if (!realmURL) { + throw new UserKeyError(400, 'a ticket to share a key names the realm'); + } + return { action, keyId, realmURL: realmURL.href, shareId: null }; + } + if (action === 'swap') { + if (typeof shareId !== 'string') { + throw new UserKeyError(400, 'a ticket to swap a key names the share'); + } + return { action, keyId, realmURL: null, shareId }; + } + return { + action: action as KeyWriteAction, + keyId, + realmURL: null, + shareId: null, + }; } -// Mints a ticket for `owner` and one change. A ticket to replace or delete a -// key is minted only for a key the owner has; another user's key answers as -// one that doesn't exist. Only the ticket's SHA-256 is stored, so the ticket +// Mints a ticket for `owner` and one change. A ticket that names a key is +// minted only for a key the owner has; another user's key answers as one that +// doesn't exist. Only the ticket's SHA-256 is stored, so the ticket // itself exists only in the response that carries it to the caller. Expired // tickets are swept on the way. export async function mintKeyWriteTicket( dbAdapter: DBAdapter, owner: string, - { action, keyId }: KeyWriteTarget, + { action, keyId, realmURL, shareId }: KeyWriteTarget, ): Promise<{ ticket: string; expiresAt: string }> { if (keyId !== null) { await getUserKey(dbAdapter, owner, keyId); @@ -114,7 +159,7 @@ export async function mintKeyWriteTicket( `DELETE FROM key_write_tickets WHERE expires_at <= NOW()`, ]); await query(dbAdapter, [ - `INSERT INTO key_write_tickets (ticket_sha256, owner, action, key_id, expires_at) VALUES (`, + `INSERT INTO key_write_tickets (ticket_sha256, owner, action, key_id, realm_url, share_id, expires_at) VALUES (`, param(sha256(ticket)), `, `, param(owner), @@ -123,6 +168,10 @@ export async function mintKeyWriteTicket( `, `, param(keyId), `, `, + param(realmURL), + `, `, + param(shareId), + `, `, param(expiresAt.toISOString()), `)`, ]); @@ -131,14 +180,14 @@ export async function mintKeyWriteTicket( // Consumes `ticket` inside the caller's transaction, so a write that fails // afterwards leaves the ticket unused. A ticket spends only on the change it -// was minted for: its owner, its action and, for a replace or delete, its key. -// Deleting the row is what makes a ticket single-use: a concurrent second use -// waits on the row and then finds it gone. -async function consumeKeyWriteTicket( +// was minted for: its owner, its action and whatever that action names (its +// key, realm or share). Deleting the row is what makes a ticket single-use: a +// concurrent second use waits on the row and then finds it gone. +export async function consumeKeyWriteTicket( txQuery: Querier, owner: string, ticket: unknown, - { action, keyId }: KeyWriteTarget, + { action, keyId, realmURL, shareId }: KeyWriteTarget, ): Promise { if (typeof ticket !== 'string' || ticket.length === 0) { throw new UserKeyError(403, INVALID_TICKET_MESSAGE); @@ -150,9 +199,9 @@ async function consumeKeyWriteTicket( param(owner), ` AND action = `, param(action), - ...(keyId === null - ? [` AND key_id IS NULL`] - : [` AND key_id = `, param(keyId)]), + ...matches('key_id', keyId), + ...matches('realm_url', realmURL), + ...matches('share_id', shareId), ` AND expires_at > NOW() RETURNING ticket_sha256`, ]); if (rows.length === 0) { @@ -160,6 +209,12 @@ async function consumeKeyWriteTicket( } } +function matches(column: string, value: string | null): Expression { + return value === null + ? [` AND ${column} IS NULL`] + : [` AND ${column} = `, param(value)]; +} + export function validateUserKeyName(name: unknown): string { if (typeof name !== 'string') { throw new UserKeyError(400, 'a key needs a name'); @@ -309,6 +364,8 @@ export async function addUserKey( await consumeKeyWriteTicket(txQuery, owner, ticket, { action: 'add', keyId: null, + realmURL: null, + shareId: null, }); let sealed = keyring.seal(validValue, binding(owner, id)); let rows = await txQuery([ @@ -348,6 +405,8 @@ export async function replaceUserKeyValue( await consumeKeyWriteTicket(txQuery, owner, ticket, { action: 'replace', keyId: id, + realmURL: null, + shareId: null, }); let sealed = keyring.seal(validValue, binding(owner, id)); let [updated] = await txQuery([ @@ -366,8 +425,10 @@ export async function replaceUserKeyValue( }); } -// Deletes a key. Nothing else refers to a key: no table records a key shared -// into a realm or approved for an operation. +// Deletes a key. Every share holding it is suspended in the same statement, +// by a trigger on `user_keys`, and keeps its place in its realm for a direct +// writer to swap another key into; `listKeyShares` with the key's id lists +// the shares a delete would suspend. export async function deleteUserKey( dbAdapter: DBAdapter, { owner, id, ticket }: { owner: string; id: string; ticket: unknown }, @@ -377,6 +438,8 @@ export async function deleteUserKey( await consumeKeyWriteTicket(txQuery, owner, ticket, { action: 'delete', keyId: id, + realmURL: null, + shareId: null, }); let rows = await txQuery([ `DELETE FROM user_keys WHERE id = `, diff --git a/packages/realm-server/routes.ts b/packages/realm-server/routes.ts index ad0f7f1172c..65c6679a30a 100644 --- a/packages/realm-server/routes.ts +++ b/packages/realm-server/routes.ts @@ -103,6 +103,8 @@ import handleCapture from './handlers/handle-capture.ts'; import { buildCreatePrerenderAuth } from './prerender/auth.ts'; import type { RealmRegistryReconciler } from './lib/realm-registry-reconciler.ts'; import { + KEY_SHARE_NOTICES_PATH, + KEY_SHARES_PATH, KEY_WRITE_TICKETS_PATH, OPENROUTER_PASSTHROUGH_PATH, REQUEST_FORWARD_PATH, @@ -125,6 +127,15 @@ import { handleReplaceUserKey, handleShowUserKey, } from './handlers/handle-user-keys.ts'; +import { + handleListKeyShareNotices, + handleListKeyShares, + handleResumeKeyShare, + handleShareKey, + handleSuspendKeyShare, + handleSwapKeyIntoShare, + handleUnshareKey, +} from './handlers/handle-key-shares.ts'; export type CreateRoutesArgs = { serverURL: string; @@ -310,6 +321,41 @@ export function createRoutes(args: CreateRoutesArgs) { jwtMiddleware(args.realmSecretSeed, args.dbAdapter), handleDeleteUserKey(args), ); + router.get( + KEY_SHARES_PATH, + jwtMiddleware(args.realmSecretSeed, args.dbAdapter), + handleListKeyShares(args), + ); + router.post( + KEY_SHARES_PATH, + jwtMiddleware(args.realmSecretSeed, args.dbAdapter), + handleShareKey(args), + ); + router.delete( + `${KEY_SHARES_PATH}/:shareId`, + jwtMiddleware(args.realmSecretSeed, args.dbAdapter), + handleUnshareKey(args), + ); + router.post( + `${KEY_SHARES_PATH}/:shareId/suspend`, + jwtMiddleware(args.realmSecretSeed, args.dbAdapter), + handleSuspendKeyShare(args), + ); + router.post( + `${KEY_SHARES_PATH}/:shareId/resume`, + jwtMiddleware(args.realmSecretSeed, args.dbAdapter), + handleResumeKeyShare(args), + ); + router.post( + `${KEY_SHARES_PATH}/:shareId/swap`, + jwtMiddleware(args.realmSecretSeed, args.dbAdapter), + handleSwapKeyIntoShare(args), + ); + router.get( + KEY_SHARE_NOTICES_PATH, + jwtMiddleware(args.realmSecretSeed, args.dbAdapter), + handleListKeyShareNotices(args), + ); router.post( REQUEST_FORWARD_PATH, jwtMiddleware(args.realmSecretSeed, args.dbAdapter), diff --git a/packages/realm-server/tests/key-shares-test.ts b/packages/realm-server/tests/key-shares-test.ts new file mode 100644 index 00000000000..d5d7abf394b --- /dev/null +++ b/packages/realm-server/tests/key-shares-test.ts @@ -0,0 +1,763 @@ +import QUnit from 'qunit'; +const { module, test } = QUnit; +import { basename, dirname, join } from 'path'; +import { createRequire } from 'module'; +import type { SuperTest, Test } from 'supertest'; +import supertest from 'supertest'; +import type { PgAdapter } from '@cardstack/postgres'; +import { insertPermissions } from '@cardstack/runtime-common'; +import { + setupDB, + setupSimpleRealmServerTemplate, + startSimpleRealmServer, + closeServer, + realmSecretSeed, +} from './helpers/index.ts'; +import { secureSessionAuth } from './helpers/secure-session.ts'; +import { createJWT as createRealmServerJWT } from '../utils/jwt.ts'; +import type { RealmHttpServer as Server } from '../server.ts'; +import { + installSecretsKeyring, + LocalKeyProvider, + resetSecretsAtRest, + type SecretsKeyring, +} from '../lib/secrets-at-rest/index.ts'; +import { addUserKey, mintKeyWriteTicket } from '../lib/user-keys.ts'; + +const require = createRequire(import.meta.url); +const KEY_SHARES_MIGRATION = join( + dirname(require.resolve('@cardstack/postgres/package.json')), + 'migrations', + '1792187770930_add-key-shares.js', +); + +const OWNER = '@share-owner:localhost'; +const WRITER = '@other-writer:localhost'; +const READER = '@realm-reader:localhost'; +const STRANGER = '@stranger:localhost'; +const REALM = 'http://127.0.0.1:4444/shared-realm/'; +const OTHER_REALM = 'http://127.0.0.1:4444/other-realm/'; + +module(basename(import.meta.filename), function () { + module('key shares API', function (hooks) { + let testRealmServer: Server; + let request: SuperTest; + let dbAdapter: PgAdapter; + let keyring: SecretsKeyring; + let secureOriginURL = new URL('https://secure.keys.test/secure'); + + let templateDatabase = setupSimpleRealmServerTemplate(hooks); + + setupDB(hooks, { + templateDatabase, + beforeEach: async (_dbAdapter, publisher, runner) => { + dbAdapter = _dbAdapter; + keyring = installSecretsKeyring( + await new LocalKeyProvider().loadDataKeys(), + ); + testRealmServer = await startSimpleRealmServer({ + dbAdapter, + publisher, + runner, + secureOriginURL, + }); + request = supertest(testRealmServer); + await insertPermissions(dbAdapter, new URL(REALM), { + [OWNER]: ['read', 'write'], + [WRITER]: ['read', 'write'], + [READER]: ['read'], + }); + await insertPermissions(dbAdapter, new URL(OTHER_REALM), { + [OWNER]: ['read', 'write'], + }); + }, + afterEach: async () => { + await closeServer(testRealmServer); + resetSecretsAtRest(); + }, + }); + + function auth(user: string): string { + return `Bearer ${createRealmServerJWT( + { user, sessionRoom: 'test-session' }, + realmSecretSeed, + )}`; + } + + async function keyFor(user: string, name = 'weather'): Promise { + let { ticket } = await mintKeyWriteTicket(dbAdapter, user, { + action: 'add', + keyId: null, + realmURL: null, + shareId: null, + }); + let key = await addUserKey(dbAdapter, keyring, { + owner: user, + name, + value: `sk-${name}-value`, + ticket, + }); + return key.id; + } + + async function mint(user: string, attributes: Record) { + return await request + .post('/_key-write-tickets') + .set('Authorization', await secureSessionAuth(dbAdapter, user)) + .set('Content-Type', 'application/json') + .send({ data: { type: 'key-write-ticket', attributes } }); + } + + async function ticketFor( + user: string, + attributes: Record, + ): Promise { + let response = await mint(user, attributes); + if (response.status !== 201) { + throw new Error( + `could not mint a ticket: ${response.status} ${response.text}`, + ); + } + return response.body.data.attributes.ticket; + } + + async function share( + user: string, + attributes: { realm: string; keyId: string; label: string }, + ticket?: string, + ) { + ticket ??= await ticketFor(user, { + action: 'share', + keyId: attributes.keyId, + realm: attributes.realm, + }); + return await request + .post('/_key-shares') + .set('Authorization', auth(user)) + .set('Content-Type', 'application/json') + .send({ data: { type: 'key-share', attributes }, meta: { ticket } }); + } + + async function sharedId( + user = OWNER, + label = 'Weather', + realm = REALM, + ): Promise { + let keyId = await keyFor(user, `${label}-key`); + let response = await share(user, { realm, keyId, label }); + if (response.status !== 201) { + throw new Error( + `could not share a key: ${response.status} ${response.text}`, + ); + } + return response.body.data.id; + } + + async function swap( + user: string, + shareId: string, + keyId: string, + ticket?: string, + ) { + ticket ??= await ticketFor(user, { action: 'swap', keyId, shareId }); + return await request + .post(`/_key-shares/${shareId}/swap`) + .set('Authorization', auth(user)) + .set('Content-Type', 'application/json') + .send({ + data: { type: 'key-share', attributes: { keyId } }, + meta: { ticket }, + }); + } + + async function act(user: string, shareId: string, action: string) { + return await request + .post(`/_key-shares/${shareId}/${action}`) + .set('Authorization', auth(user)); + } + + async function realmShares(user: string, realm = REALM) { + return await request + .get(`/_key-shares?realm=${encodeURIComponent(realm)}`) + .set('Authorization', auth(user)); + } + + async function notices(user: string, realm = REALM) { + return await request + .get(`/_key-share-notices?realm=${encodeURIComponent(realm)}`) + .set('Authorization', auth(user)); + } + + async function shareState(shareId: string) { + let [row] = await dbAdapter.execute( + `SELECT owner, key_id, state, suspension_reason FROM key_shares WHERE id = $1`, + { bind: [shareId] }, + ); + return row + ? { + owner: row.owner, + keyId: row.key_id, + state: row.state, + reason: row.suspension_reason, + } + : undefined; + } + + test('a direct writer shares their own key into the realm', async function (assert) { + let keyId = await keyFor(OWNER); + let response = await share(OWNER, { + realm: REALM, + keyId, + label: 'Weather', + }); + assert.strictEqual(response.status, 201, response.text); + let { id, attributes } = response.body.data; + assert.strictEqual(response.body.data.type, 'key-share'); + assert.deepEqual( + { + realm: attributes.realm, + label: attributes.label, + owner: attributes.owner, + keyId: attributes.keyId, + state: attributes.state, + suspensionReason: attributes.suspensionReason, + }, + { + realm: REALM, + label: 'Weather', + owner: OWNER, + keyId, + state: 'active', + suspensionReason: null, + }, + ); + assert.notOk( + JSON.stringify(response.body).includes('sk-'), + 'no value is returned', + ); + + let own = await request + .get('/_key-shares') + .set('Authorization', auth(OWNER)); + assert.deepEqual( + own.body.data.map((s: { id: string }) => s.id), + [id], + 'the owner sees the share', + ); + + let listed = await realmShares(WRITER); + assert.strictEqual(listed.status, 200, listed.text); + assert.deepEqual( + listed.body.data.map( + (s: { attributes: Record }) => s.attributes, + ), + [ + { + realm: REALM, + label: 'Weather', + owner: OWNER, + state: 'active', + suspensionReason: null, + suspendedBy: null, + suspendedAt: null, + createdAt: attributes.createdAt, + updatedAt: attributes.updatedAt, + }, + ], + `another writer sees the share's owner and state, not its key`, + ); + }); + + test('one key is shared into several realms, its label unique within each', async function (assert) { + let keyId = await keyFor(OWNER); + let first = await share(OWNER, { realm: REALM, keyId, label: 'Weather' }); + assert.strictEqual(first.status, 201, first.text); + let other = await share(OWNER, { + realm: OTHER_REALM, + keyId, + label: 'Weather', + }); + assert.strictEqual(other.status, 201, 'the same label in another realm'); + + let anotherKey = await keyFor(OWNER, 'second'); + let clash = await share(OWNER, { + realm: REALM, + keyId: anotherKey, + label: 'Weather', + }); + assert.strictEqual(clash.status, 409, 'a label is unique in its realm'); + }); + + test('a user who is not a direct writer of the realm cannot share into it', async function (assert) { + let readerKey = await keyFor(READER); + let byReader = await share(READER, { + realm: REALM, + keyId: readerKey, + label: 'Weather', + }); + assert.strictEqual(byReader.status, 403, 'a reader is refused'); + + let strangerKey = await keyFor(STRANGER); + let byStranger = await share(STRANGER, { + realm: REALM, + keyId: strangerKey, + label: 'Weather', + }); + assert.strictEqual(byStranger.status, 403, 'a stranger is refused'); + + await insertPermissions(dbAdapter, new URL(REALM), { + '*': ['read', 'write'], + }); + let byWildcard = await share(STRANGER, { + realm: REALM, + keyId: strangerKey, + label: 'Weather', + }); + assert.strictEqual( + byWildcard.status, + 403, + 'write through a `*` row does not make a direct writer', + ); + let [{ count }] = await dbAdapter.execute( + `SELECT COUNT(*)::int AS count FROM key_shares`, + ); + assert.strictEqual(count, 0, 'nothing was shared'); + }); + + test('only the owner shares a key, with a ticket for that key and realm', async function (assert) { + let ownerKey = await keyFor(OWNER); + let minted = await mint(WRITER, { + action: 'share', + keyId: ownerKey, + realm: REALM, + }); + assert.strictEqual( + minted.status, + 404, + `no ticket is minted to share another user's key`, + ); + + let writerTicket = await ticketFor(WRITER, { + action: 'share', + keyId: await keyFor(WRITER), + realm: REALM, + }); + let withOwnTicket = await share( + WRITER, + { realm: REALM, keyId: ownerKey, label: 'Weather' }, + writerTicket, + ); + assert.strictEqual( + withOwnTicket.status, + 403, + `a ticket for one key doesn't share another`, + ); + + let otherRealmTicket = await ticketFor(OWNER, { + action: 'share', + keyId: ownerKey, + realm: OTHER_REALM, + }); + let wrongRealm = await share( + OWNER, + { realm: REALM, keyId: ownerKey, label: 'Weather' }, + otherRealmTicket, + ); + assert.strictEqual( + wrongRealm.status, + 403, + `a ticket for one realm doesn't share into another`, + ); + + let noTicket = await request + .post('/_key-shares') + .set('Authorization', auth(OWNER)) + .set('Content-Type', 'application/json') + .send({ + data: { + type: 'key-share', + attributes: { realm: REALM, keyId: ownerKey, label: 'Weather' }, + }, + }); + assert.strictEqual(noTicket.status, 403, 'a share needs a ticket'); + }); + + test(`removing the owner's write suspends their shares, and restoring it resumes them`, async function (assert) { + let shareId = await sharedId(); + + await insertPermissions(dbAdapter, new URL(REALM), { + [OWNER]: ['read'], + }); + let afterLoss = await shareState(shareId); + assert.deepEqual( + [afterLoss?.owner, afterLoss?.state, afterLoss?.reason], + [OWNER, 'suspended', 'owner-lost-write'], + 'losing write suspends the share, which is kept', + ); + + await insertPermissions(dbAdapter, new URL(REALM), { + [OWNER]: ['read', 'write'], + }); + assert.strictEqual( + (await shareState(shareId))!.state, + 'active', + 'regaining write resumes it', + ); + + await insertPermissions(dbAdapter, new URL(REALM), { [OWNER]: [] }); + let afterLeaving = await shareState(shareId); + assert.deepEqual( + [afterLeaving?.state, afterLeaving?.reason], + ['suspended', 'owner-lost-write'], + 'leaving the realm suspends it', + ); + + let listed = await notices(WRITER); + assert.strictEqual(listed.status, 200, listed.text); + assert.deepEqual( + listed.body.data.map((n: { attributes: Record }) => [ + n.attributes.shareId, + n.attributes.label, + n.attributes.owner, + n.attributes.reason, + ]), + [ + [shareId, 'Weather', OWNER, 'owner-lost-write'], + [shareId, 'Weather', OWNER, 'owner-lost-write'], + ], + `the realm's direct writers get a notice of each suspension`, + ); + + let formerWriter = await realmShares(OWNER); + assert.strictEqual( + formerWriter.status, + 403, + `the former writer no longer sees the realm's shares`, + ); + let own = await request + .get('/_key-shares') + .set('Authorization', auth(OWNER)); + assert.deepEqual( + own.body.data.map( + (s: { id: string; attributes: { state: string } }) => [ + s.id, + s.attributes.state, + ], + ), + [[shareId, 'suspended']], + 'the owner still sees where their key is shared', + ); + }); + + test('a writer suspended share stays suspended when the owner regains write', async function (assert) { + let shareId = await sharedId(); + let suspended = await act(WRITER, shareId, 'suspend'); + assert.strictEqual(suspended.status, 200, suspended.text); + await insertPermissions(dbAdapter, new URL(REALM), { + [OWNER]: ['read'], + }); + await insertPermissions(dbAdapter, new URL(REALM), { + [OWNER]: ['read', 'write'], + }); + let state = await shareState(shareId); + assert.deepEqual( + [state?.state, state?.reason], + ['suspended', 'suspended-by-writer'], + ); + }); + + test('any direct writer suspends a share, and lifts that suspension', async function (assert) { + let shareId = await sharedId(); + + let byReader = await act(READER, shareId, 'suspend'); + assert.strictEqual(byReader.status, 404, `a reader can't see the share`); + + let suspended = await act(WRITER, shareId, 'suspend'); + assert.strictEqual(suspended.status, 200, suspended.text); + assert.deepEqual( + [ + suspended.body.data.attributes.state, + suspended.body.data.attributes.suspensionReason, + suspended.body.data.attributes.suspendedBy, + ], + ['suspended', 'suspended-by-writer', WRITER], + ); + assert.strictEqual( + (await act(WRITER, shareId, 'suspend')).status, + 409, + 'a suspended share is not suspended again', + ); + let listed = await notices(OWNER); + assert.deepEqual( + listed.body.data.map((n: { attributes: Record }) => [ + n.attributes.reason, + n.attributes.suspendedBy, + ]), + [['suspended-by-writer', WRITER]], + 'the suspension is noticed', + ); + + let resumed = await act(OWNER, shareId, 'resume'); + assert.strictEqual(resumed.status, 200, resumed.text); + assert.strictEqual(resumed.body.data.attributes.state, 'active'); + }); + + test('a share suspended by a writer, whose owner has lost write, stays suspended for that when lifted', async function (assert) { + let shareId = await sharedId(); + await act(WRITER, shareId, 'suspend'); + await insertPermissions(dbAdapter, new URL(REALM), { + [OWNER]: ['read'], + }); + let resumed = await act(WRITER, shareId, 'resume'); + assert.strictEqual(resumed.status, 200, resumed.text); + assert.deepEqual( + [ + resumed.body.data.attributes.state, + resumed.body.data.attributes.suspensionReason, + ], + ['suspended', 'owner-lost-write'], + ); + await insertPermissions(dbAdapter, new URL(REALM), { + [OWNER]: ['read', 'write'], + }); + assert.strictEqual((await shareState(shareId))!.state, 'active'); + }); + + test('a remaining direct writer swaps their own key into a suspended share', async function (assert) { + let shareId = await sharedId(); + let writerKey = await keyFor(WRITER); + + let whileActive = await swap(WRITER, shareId, writerKey); + assert.strictEqual( + whileActive.status, + 409, + 'an active share takes no other key', + ); + + await insertPermissions(dbAdapter, new URL(REALM), { [OWNER]: [] }); + + let swapped = await swap(WRITER, shareId, writerKey); + assert.strictEqual(swapped.status, 200, swapped.text); + let { attributes } = swapped.body.data; + assert.deepEqual( + [ + attributes.label, + attributes.owner, + attributes.keyId, + attributes.state, + attributes.suspensionReason, + ], + ['Weather', WRITER, writerKey, 'active', null], + 'the share keeps its label and takes the writer as owner', + ); + + await insertPermissions(dbAdapter, new URL(REALM), { + [WRITER]: ['read'], + }); + assert.strictEqual( + (await shareState(shareId))!.reason, + 'owner-lost-write', + 'it follows its new owner', + ); + }); + + test(`a non-writer can't swap, and nobody swaps in another user's key`, async function (assert) { + let shareId = await sharedId(); + await insertPermissions(dbAdapter, new URL(REALM), { [OWNER]: [] }); + + let readerKey = await keyFor(READER); + let byReader = await swap(READER, shareId, readerKey); + assert.strictEqual(byReader.status, 404, `a reader can't see the share`); + + await insertPermissions(dbAdapter, new URL(REALM), { + [STRANGER]: ['read'], + }); + let strangerKey = await keyFor(STRANGER); + let byStranger = await swap(STRANGER, shareId, strangerKey); + assert.strictEqual(byStranger.status, 404); + + let readerKeyTicket = await mint(WRITER, { + action: 'swap', + keyId: readerKey, + shareId, + }); + assert.strictEqual( + readerKeyTicket.status, + 404, + `no ticket is minted to swap in another user's key`, + ); + let writerTicket = await ticketFor(WRITER, { + action: 'swap', + keyId: await keyFor(WRITER), + shareId, + }); + let withOwnTicket = await swap(WRITER, shareId, readerKey, writerTicket); + assert.strictEqual( + withOwnTicket.status, + 403, + `a ticket for the writer's key doesn't swap in another`, + ); + + let state = await shareState(shareId); + assert.deepEqual( + [state!.owner, state!.state], + [OWNER, 'suspended'], + 'the share is unchanged', + ); + }); + + test('deleting a key lists the shares it would affect, and suspends them', async function (assert) { + let keyId = await keyFor(OWNER); + let inRealm = await share(OWNER, { realm: REALM, keyId, label: 'A' }); + let inOther = await share(OWNER, { + realm: OTHER_REALM, + keyId, + label: 'B', + }); + await sharedId(OWNER, 'Unrelated'); + + let affected = await request + .get(`/_key-shares?key=${keyId}`) + .set('Authorization', auth(OWNER)); + assert.strictEqual(affected.status, 200, affected.text); + assert.deepEqual( + affected.body.data.map((s: { id: string }) => s.id).sort(), + [inRealm.body.data.id, inOther.body.data.id].sort(), + 'the shares holding the key are listed', + ); + + let ticket = await ticketFor(OWNER, { action: 'delete', keyId }); + let deleted = await request + .delete(`/_user-keys/${keyId}`) + .set('Authorization', auth(OWNER)) + .set('Content-Type', 'application/json') + .send({ meta: { ticket } }); + assert.strictEqual(deleted.status, 204, deleted.text); + + for (let id of [inRealm.body.data.id, inOther.body.data.id]) { + assert.deepEqual( + await shareState(id), + { + owner: OWNER, + keyId: null, + state: 'suspended', + reason: 'key-deleted', + }, + 'the share is suspended and keeps its place', + ); + } + let listed = await notices(WRITER); + assert.deepEqual( + listed.body.data.map( + (n: { attributes: Record }) => n.attributes.reason, + ), + ['key-deleted'], + ); + + let writerKey = await keyFor(WRITER); + let swapped = await swap(WRITER, inRealm.body.data.id, writerKey); + assert.strictEqual(swapped.status, 200, 'a writer swaps a key into it'); + }); + + test(`a reader sees nothing of a realm's shares`, async function (assert) { + let shareId = await sharedId(); + for (let user of [READER, STRANGER]) { + assert.strictEqual((await realmShares(user)).status, 403); + assert.strictEqual((await notices(user)).status, 403); + let own = await request + .get('/_key-shares') + .set('Authorization', auth(user)); + assert.deepEqual(own.body.data, [], `${user} owns no shares`); + assert.strictEqual((await act(user, shareId, 'resume')).status, 404); + let unshare = await request + .delete(`/_key-shares/${shareId}`) + .set('Authorization', auth(user)); + assert.strictEqual(unshare.status, 404); + } + }); + + test('only its owner unshares a share, with or without write', async function (assert) { + let shareId = await sharedId(); + let byWriter = await request + .delete(`/_key-shares/${shareId}`) + .set('Authorization', auth(WRITER)); + assert.strictEqual(byWriter.status, 403, 'another writer is refused'); + + await insertPermissions(dbAdapter, new URL(REALM), { [OWNER]: [] }); + let byOwner = await request + .delete(`/_key-shares/${shareId}`) + .set('Authorization', auth(OWNER)); + assert.strictEqual(byOwner.status, 204, byOwner.text); + assert.strictEqual(await shareState(shareId), undefined); + }); + }); + + module('key shares tables', function (hooks) { + let dbAdapter: PgAdapter; + + setupDB(hooks, { + beforeEach: async (_dbAdapter) => { + dbAdapter = _dbAdapter; + }, + }); + + test("the read-only role can't select either table", async function (assert) { + // The role and its blanket grant as staging and production have them, + // then the migration that withholds these tables. + await dbAdapter.execute( + `DO $$ + BEGIN + CREATE ROLE readonly_role; + EXCEPTION WHEN duplicate_object OR unique_violation THEN NULL; + END + $$`, + ); + await dbAdapter.execute( + `GRANT SELECT ON ALL TABLES IN SCHEMA public TO readonly_role`, + ); + // A user that inherits the role and was also granted SELECT on the + // tables directly. + await dbAdapter.execute( + `DO $$ + BEGIN + CREATE ROLE readonly_member_for_key_shares_test; + EXCEPTION WHEN duplicate_object OR unique_violation THEN NULL; + END + $$`, + ); + await dbAdapter.execute( + `GRANT readonly_role TO readonly_member_for_key_shares_test`, + ); + await dbAdapter.execute( + `GRANT SELECT ON TABLE key_shares, key_share_notices TO readonly_member_for_key_shares_test`, + ); + let statements: string[] = []; + require(KEY_SHARES_MIGRATION).up({ + sql: (statement: string) => statements.push(statement), + }); + for (let statement of statements) { + await dbAdapter.execute(statement); + } + + async function selectAs(role: string, sql: string) { + return await dbAdapter.withTransaction(async (query: any) => { + await query([`SET LOCAL ROLE ${role}`]); + return await query([sql]); + }); + } + + for (let role of [ + 'readonly_role', + 'readonly_member_for_key_shares_test', + ]) { + for (let table of ['key_shares', 'key_share_notices']) { + await assert.rejects( + selectAs(role, `SELECT * FROM ${table}`), + new RegExp(`permission denied for table ${table}`), + `${role} can't select ${table}`, + ); + } + } + }); + }); +}); diff --git a/packages/realm-server/tests/user-keys-test.ts b/packages/realm-server/tests/user-keys-test.ts index 185cd442ce5..8eced79f00f 100644 --- a/packages/realm-server/tests/user-keys-test.ts +++ b/packages/realm-server/tests/user-keys-test.ts @@ -719,6 +719,8 @@ module(basename(import.meta.filename), function () { let { ticket } = await mintKeyWriteTicket(dbAdapter, OWNER, { action: 'add', keyId: null, + realmURL: null, + shareId: null, }); return await addUserKey(dbAdapter, sealWith, { owner: OWNER, @@ -841,6 +843,8 @@ module(basename(import.meta.filename), function () { await mintKeyWriteTicket(dbAdapter, OWNER, { action: 'add', keyId: null, + realmURL: null, + shareId: null, }); async function selectAs(role: string, sql: string) { diff --git a/packages/runtime-common/db-queries/realm-permission-queries.ts b/packages/runtime-common/db-queries/realm-permission-queries.ts index 82e6d81c875..8f1f5c6758a 100644 --- a/packages/runtime-common/db-queries/realm-permission-queries.ts +++ b/packages/runtime-common/db-queries/realm-permission-queries.ts @@ -215,6 +215,28 @@ export async function isDirectWriter( return !!row && isDirectWriterRow(userId, row.write); } +/** + * `isDirectWriter`, asked inside the caller's transaction with the person's + * permission row locked until it ends. Anything that takes something on in a + * person's name on the strength of the answer asks it this way, so a change + * to that row made meanwhile waits for the caller's work to commit and then + * sees it. + */ +export async function isDirectWriterLocked( + txQuerier: Querier, + realmURL: URL, + userId: string, +): Promise { + let [row] = (await txQuerier([ + `SELECT write FROM realm_user_permissions WHERE realm_url =`, + param(realmURL.href), + `AND username =`, + param(userId), + `FOR SHARE`, + ])) as { write: boolean }[]; + return !!row && isDirectWriterRow(userId, row.write); +} + /** * The realm's direct writers, as `isDirectWriter` answers it, in username * order. From 3ad4d94a52f13ed6d49e56ec309686269c3a77c7 Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Fri, 16 Oct 2026 18:27:12 -0400 Subject: [PATCH 2/6] Address review: lock order, realm deletion, malformed ids, resume tickets - Take locks in the order the triggers do (permission rows, then the key, then the share), so an owner suspending or unsharing while their write is removed no longer deadlocks the revoke. - Delete a realm's shares, with their notices, when the realm is deleted, so a realm later made at the same URL gets none of them back. - Answer a share or key id that isn't a UUID with 404 instead of a 500, at minting and at every route. - Require a `resume` key write ticket to lift a writer's suspension, so card code in a writer's app page can't put a suspended key back into use. - Record a notice when deleting a key changes why a suspended share is suspended, and say so in the skill. - Read a realm's shares and notices with the writer's permission row locked. - Test a swap or resume ticket spent on another share, the malformed ids, realm deletion, and the key-deleted notice; fix two inaccurate comments. Co-Authored-By: Claude Opus 5.5 --- .claude/skills/key-shares/SKILL.md | 24 +- .claude/skills/user-keys/SKILL.md | 17 +- .../1792187770930_add-key-shares.js | 34 ++- .../handlers/handle-key-shares.ts | 6 +- .../handlers/realm-destruction-utils.ts | 4 + packages/realm-server/lib/key-shares.ts | 227 ++++++++++++------ packages/realm-server/lib/user-keys.ts | 57 +++-- .../realm-server/tests/key-shares-test.ts | 146 ++++++++++- 8 files changed, 398 insertions(+), 117 deletions(-) diff --git a/.claude/skills/key-shares/SKILL.md b/.claude/skills/key-shares/SKILL.md index 1203bc92c03..7ac5ee1a4b0 100644 --- a/.claude/skills/key-shares/SKILL.md +++ b/.claude/skills/key-shares/SKILL.md @@ -29,13 +29,17 @@ always carries one reason: `key_shares_state_check` enforces these combinations. Only an active share is suspended: a share suspended for one reason keeps that reason when another applies too, so a share a writer suspended stays suspended when its owner -loses write and regains it. Resuming a `suspended-by-writer` share whose +loses write and regains it. The exception is deleting the key: a share then +holds no key, so whatever it was suspended for, it becomes `key-deleted` +(clearing `suspended_by`), and the realm's writers get a notice of it. Resuming a `suspended-by-writer` share whose owner is no longer a direct writer leaves it suspended as `owner-lost-write`, and it resumes when they write again. -A suspension keeps the share's place, label and history: nothing but its -owner's unshare deletes a share, so cards that name it by label keep -working once it is brought back. +A suspension keeps the share's place, label and history: only its owner's +unshare, or deleting its realm, deletes a share, so cards that name it by +label keep working once it is brought back. Deleting a realm deletes its +shares and their notices, so a realm later made at the same URL gets none of +them back. ## Triggers @@ -52,7 +56,8 @@ admin permission upsert, publishing, deleting a realm, a database writer): - `key_shares_follow_key_delete` on `user_keys`, before a delete: suspends every share holding the key as `key-deleted` and clears `key_id`. - `key_shares_notice_suspension` on `key_shares`: each share going from active - to suspended records a row in `key_share_notices`. + to suspended, and each suspended share whose key is deleted, records a row + in `key_share_notices`. Sharing and swapping lock the person's permission row (`isDirectWriterLocked`) until they commit, so a concurrent removal of their write waits and then @@ -65,7 +70,7 @@ suspends what they made. | Share | The key's owner, into a realm where they are a direct writer | `share` for that key + realm | | Unshare | The share's owner, whether or not they still write the realm | none | | Suspend | Any direct writer of the realm, on an active share | none | -| Resume | Any direct writer of the realm, on a `suspended-by-writer` share | none | +| Resume | Any direct writer of the realm, on a `suspended-by-writer` share | `resume` for that share | | Swap | Any direct writer of the realm, putting one of their own keys into a suspended share; they become its owner and it resumes | `swap` for that key + share | A direct writer is a person whose own permission row grants write @@ -112,8 +117,9 @@ All routes take the user's realm-server session and refuse a delegated or a realm-authority session, as the keys API does. Bodies are JSON:API with type `key-share`; a ticket goes in `meta.ticket`, minted from `POST /_key-write-tickets` by a secure session (see the `user-keys` skill) -with `attributes.{action: 'share', keyId, realm}` or -`attributes.{action: 'swap', keyId, shareId}`. The request log keeps only the +with `attributes.{action: 'share', keyId, realm}`, +`attributes.{action: 'swap', keyId, shareId}` or +`attributes.{action: 'resume', shareId}`. The request log keeps only the method and path of these routes. | Route | Does | @@ -122,7 +128,7 @@ method and path of these routes. | `POST /_key-shares` | Share: `attributes.{realm, keyId, label}`, `meta.{ticket}` | | `DELETE /_key-shares/:id` | Unshare | | `POST /_key-shares/:id/suspend` | Suspend an active share | -| `POST /_key-shares/:id/resume` | Lift a writer's suspension | +| `POST /_key-shares/:id/resume` | Lift a writer's suspension: `meta.{ticket}` | | `POST /_key-shares/:id/swap` | Swap a key in: `attributes.{keyId}`, `meta.{ticket}` | | `GET /_key-share-notices?realm=` | A realm's suspension notices | diff --git a/.claude/skills/user-keys/SKILL.md b/.claude/skills/user-keys/SKILL.md index 16ad80d20fa..3bd16546361 100644 --- a/.claude/skills/user-keys/SKILL.md +++ b/.claude/skills/user-keys/SKILL.md @@ -34,10 +34,11 @@ has lost track of a value replaces it. | `created_at`, `updated_at` | `timestamptz` | `key_write_tickets` — one row per unused ticket: `ticket_sha256`, `owner`, -`action` (`add`, `replace`, `delete`, `share` or `swap`), `key_id` (the key -a replace, delete, share or swap uses; null for an add), `realm_url` (the -realm a share puts the key into; null otherwise), `share_id` (the share a swap -puts the key into; null otherwise) and `expires_at`. +`action` (`add`, `replace`, `delete`, `share`, `swap` or `resume`), `key_id` +(the key a replace, delete, share or swap uses; null for an add or a resume), +`realm_url` (the realm a share puts the key into; null otherwise), `share_id` +(the share a swap puts the key into, or a resume resumes; null otherwise) and +`expires_at`. `key_write_tickets_action_key_id_check` enforces which action names what. Only the SHA-256 is stored; the ticket itself exists only in the response that minted it. @@ -69,12 +70,12 @@ A key is added, replaced, deleted, shared into a realm or swapped into a share only with a ticket: minted for the signed-in user and for one change, valid for 10 minutes, single use. A ticket names its change when it is minted: `add`; `replace` or `delete` of one of the user's keys; `share` of one -of them into one realm; or `swap` of one of them into one share (see the -`key-shares` skill). It spends only on that change, so a ticket that leaks can't +of them into one realm; `swap` of one of them into one share; or `resume` of +one share a writer suspended (see the `key-shares` skill). It spends only on that change, so a ticket that leaks can't be put to a different one: a ticket to add a key replaces or deletes nothing, and a ticket to replace or delete one key does nothing to another, nor the -other action to the same key, and a ticket to share a key into one realm or -swap it into one share does nothing elsewhere. A ticket for a key the user +other action to the same key, and a ticket to share a key into one realm, +swap it into one share or resume one share does nothing elsewhere. A ticket for a key the user doesn't have is refused with 404 at minting, as for no key. The change consumes the ticket in the same transaction, so a change refused diff --git a/packages/postgres/migrations/1792187770930_add-key-shares.js b/packages/postgres/migrations/1792187770930_add-key-shares.js index 60c229e9f27..bd772426600 100644 --- a/packages/postgres/migrations/1792187770930_add-key-shares.js +++ b/packages/postgres/migrations/1792187770930_add-key-shares.js @@ -23,12 +23,15 @@ // - On `realm_user_permissions`, any change to a person's row suspends that // person's active shares in the realm when the row no longer grants write, // and resumes the ones it suspended for that reason when it does again. -// Only a direct writer (see `isDirectWriter`) can share or swap a key in, -// and a wildcard or bot account owns no key, so a share's owner is always a -// person and their own row is the whole answer. -// - On `user_keys`, deleting a key suspends every share holding it. -// - On `key_shares`, a share going from active to suspended records a row in -// `key_share_notices` for the realm's direct writers. +// A share's owner is always a person whose own row granted write when they +// shared or swapped a key in, since only a direct writer (see +// `isDirectWriter`, which refuses wildcard rows and bot accounts) can do +// either; so their own row is the whole answer. +// - On `user_keys`, deleting a key suspends every share holding it, whatever +// it was suspended for before, since the share then holds no key. +// - On `key_shares`, a share going from active to suspended, or a suspended +// share whose key is deleted, records a row in `key_share_notices` for the +// realm's direct writers. // // Both tables are withheld from the read-only role used by dashboards and // read-only database access, and from every role that is a member of it, as @@ -194,14 +197,24 @@ exports.up = (pgm) => { pgm.sql(`DROP TRIGGER IF EXISTS key_shares_notice_suspension ON key_shares`); pgm.sql(` CREATE TRIGGER key_shares_notice_suspension - AFTER UPDATE OF state ON key_shares + AFTER UPDATE OF state, suspension_reason ON key_shares FOR EACH ROW - WHEN (OLD.state = 'active' AND NEW.state = 'suspended') + WHEN ( + NEW.state = 'suspended' + AND ( + OLD.state = 'active' + OR ( + NEW.suspension_reason = 'key-deleted' + AND OLD.suspension_reason IS DISTINCT FROM 'key-deleted' + ) + ) + ) EXECUTE FUNCTION key_shares_record_suspension() `); // A ticket to share a key names the key and the realm it goes into; a - // ticket to swap a key in names the key and the share it goes into. + // ticket to swap a key in names the key and the share it goes into; a + // ticket to resume a share names the share. pgm.sql(` ALTER TABLE key_write_tickets ADD COLUMN IF NOT EXISTS realm_url varchar `); @@ -219,6 +232,7 @@ exports.up = (pgm) => { OR (action IN ('replace', 'delete') AND key_id IS NOT NULL AND realm_url IS NULL AND share_id IS NULL) OR (action = 'share' AND key_id IS NOT NULL AND realm_url IS NOT NULL AND share_id IS NULL) OR (action = 'swap' AND key_id IS NOT NULL AND realm_url IS NULL AND share_id IS NOT NULL) + OR (action = 'resume' AND key_id IS NULL AND realm_url IS NULL AND share_id IS NOT NULL) ) `); @@ -254,7 +268,7 @@ exports.up = (pgm) => { exports.down = (pgm) => { pgm.sql(` - DELETE FROM key_write_tickets WHERE action IN ('share', 'swap') + DELETE FROM key_write_tickets WHERE action IN ('share', 'swap', 'resume') `); pgm.sql(` ALTER TABLE key_write_tickets diff --git a/packages/realm-server/handlers/handle-key-shares.ts b/packages/realm-server/handlers/handle-key-shares.ts index 4f6115ba5ad..f977da61e10 100644 --- a/packages/realm-server/handlers/handle-key-shares.ts +++ b/packages/realm-server/handlers/handle-key-shares.ts @@ -24,8 +24,8 @@ import { // The shares API: sharing the signed-in user's keys into realms, and what a // realm's direct writers do with its shares. Who may do what is set out in -// `lib/key-shares.ts`. Sharing and swapping each take a key write ticket in -// `meta`, so, as for the keys API, nothing here logs a body and the request +// `lib/key-shares.ts`. Sharing, swapping and resuming each take a key write +// ticket in `meta`, so, as for the keys API, nothing here logs a body and the request // log keeps only the method and path of these routes. type Handler = (ctxt: Koa.Context, next: Koa.Next) => Promise; @@ -141,9 +141,11 @@ export function handleSuspendKeyShare({ export function handleResumeKeyShare({ dbAdapter }: CreateRoutesArgs): Handler { return keysHandler(async (ctxt) => { let user = ownerOf(ctxt); + let body = await readJSON(ctxt); let share = await resumeShare(dbAdapter, { user, shareId: ctxt.params.shareId, + ticket: ticketOf(body), }); await sendJSON(ctxt, 200, { data: serialize(share) }); }); diff --git a/packages/realm-server/handlers/realm-destruction-utils.ts b/packages/realm-server/handlers/realm-destruction-utils.ts index 76ef956b22e..868ef171fae 100644 --- a/packages/realm-server/handlers/realm-destruction-utils.ts +++ b/packages/realm-server/handlers/realm-destruction-utils.ts @@ -136,4 +136,8 @@ export async function removeRealmDatabaseArtifacts(args: { ]); await q([`DELETE FROM realm_file_meta WHERE realm_url =`, param(realmURL)]); await q([`DELETE FROM realm_metadata WHERE url =`, param(realmURL)]); + // A deleted realm's shares serve no one, and kept, they would resume for a + // realm later made at the same URL that grants their owner write. Their + // notices go with them. + await q([`DELETE FROM key_shares WHERE realm_url =`, param(realmURL)]); } diff --git a/packages/realm-server/lib/key-shares.ts b/packages/realm-server/lib/key-shares.ts index d5bc8d284ea..200429e6a37 100644 --- a/packages/realm-server/lib/key-shares.ts +++ b/packages/realm-server/lib/key-shares.ts @@ -1,6 +1,5 @@ import { randomUUID } from 'node:crypto'; import { - isDirectWriter, isDirectWriterLocked, param, query, @@ -21,8 +20,9 @@ import { consumeKeyWriteTicket, UserKeyError } from './user-keys.ts'; // that key and that realm. // - Only a share's owner unshares it, whether or not they still write the // realm: withdrawing one's own key needs no write. -// - Any direct writer of the realm suspends an active share, and lifts a -// suspension a direct writer made. +// - Any direct writer of the realm suspends an active share. Any direct writer +// lifts a suspension a direct writer made, with a key write ticket for that +// share: lifting one puts a key back into use. // - Any direct writer of the realm swaps one of their own keys into a // suspended share, which makes them its owner and resumes it. Swapping takes // a key write ticket for that key and that share. Nobody swaps in another @@ -36,7 +36,8 @@ import { consumeKeyWriteTicket, UserKeyError } from './user-keys.ts'; // they hold however the change is made: a share is suspended when its owner // stops being a direct writer of the realm, and resumes when they are one // again; and it is suspended, keeping its place, when its key is deleted. -// Every share going from active to suspended records a notice. +// Every share going from active to suspended records a notice, as does a +// suspended share whose key is deleted. Deleting a realm deletes its shares. // // Every check that a person is a direct writer runs inside the change's // transaction with their permission row locked (`isDirectWriterLocked`), so a @@ -108,6 +109,10 @@ function toShare( const UUID = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; +export function isUUID(value: unknown): value is string { + return typeof value === 'string' && UUID.test(value); +} + function noSuchShare() { return new UserKeyError(404, 'no such share'); } @@ -154,16 +159,34 @@ export function validateShareLabel(label: unknown): string { return trimmed; } +// Locks are taken in the order the triggers take them: permission rows, then +// the key, then the share. A permission change locks its row and then, +// through its trigger, the shares it suspends or resumes; deleting a key locks +// the key and then the shares holding it. Taking a share before a row either +// of them holds would deadlock against them. + +// Whether each of `users` is a direct writer of the realm, with their +// permission rows locked in username order, so two changes locking the same +// rows can't wait on each other. +async function lockDirectWriters( + txQuery: Querier, + realmURL: URL, + users: string[], +): Promise> { + let writers = new Map(); + for (let user of [...new Set(users)].sort()) { + writers.set(user, await isDirectWriterLocked(txQuery, realmURL, user)); + } + return writers; +} + // The user's own key, locked against a concurrent delete until the caller's // transaction ends. Another user's key answers as one that doesn't exist. async function lockOwnKey( txQuery: Querier, owner: string, - keyId: unknown, -): Promise { - if (typeof keyId !== 'string' || !UUID.test(keyId)) { - throw noSuchKey(); - } + keyId: string, +): Promise { let rows = await txQuery([ `SELECT id FROM user_keys WHERE id = `, param(keyId), @@ -174,37 +197,64 @@ async function lockOwnKey( if (rows.length === 0) { throw noSuchKey(); } - return keyId; } // The share, locked for the caller's change, if `user` may see it: as its // owner, or as a direct writer of its realm. Any other share answers as one -// that doesn't exist. +// that doesn't exist. The user's permission row is locked first, and with +// `withOwner` the share's owner's too; with `ownKeyId`, the user's key is +// locked next. A swap made between reading the share's owner and locking the +// share changes the owner, so the locks are taken again for the new one. async function lockVisibleShare( txQuery: Querier, user: string, shareId: string, -): Promise<{ row: Record; isWriter: boolean }> { - if (!UUID.test(shareId)) { - throw noSuchShare(); - } - let [row] = await txQuery([ - `SELECT ${SHARE_COLUMNS} FROM key_shares WHERE id = `, - param(shareId), - ` FOR UPDATE`, - ]); - if (!row) { + { + withOwner = false, + ownKeyId, + }: { withOwner?: boolean; ownKeyId?: string } = {}, +): Promise<{ + row: Record; + isWriter: boolean; + ownerIsWriter: boolean; +}> { + if (!isUUID(shareId)) { throw noSuchShare(); } - let isWriter = await isDirectWriterLocked( - txQuery, - new URL(row.realm_url as string), - user, - ); - if (!isWriter && row.owner !== user) { - throw noSuchShare(); + for (;;) { + let [current] = await txQuery([ + `SELECT realm_url, owner FROM key_shares WHERE id = `, + param(shareId), + ]); + if (!current) { + throw noSuchShare(); + } + let owner = current.owner as string; + let writers = await lockDirectWriters( + txQuery, + new URL(current.realm_url as string), + withOwner ? [user, owner] : [user], + ); + if (ownKeyId !== undefined) { + await lockOwnKey(txQuery, user, ownKeyId); + } + let [row] = await txQuery([ + `SELECT ${SHARE_COLUMNS} FROM key_shares WHERE id = `, + param(shareId), + ` FOR UPDATE`, + ]); + if (!row) { + throw noSuchShare(); + } + if (withOwner && row.owner !== owner) { + continue; + } + let isWriter = writers.get(user)!; + if (!isWriter && row.owner !== user) { + throw noSuchShare(); + } + return { row, isWriter, ownerIsWriter: writers.get(owner) ?? false }; } - return { row, isWriter }; } export async function shareKey( @@ -225,9 +275,12 @@ export async function shareKey( ): Promise { let realmURL = validateRealm(realm); let validLabel = validateShareLabel(label); - if (typeof keyId !== 'string') { + if (keyId === undefined || keyId === null) { throw new UserKeyError(400, 'a share names the key it holds'); } + if (!isUUID(keyId)) { + throw noSuchKey(); + } let id = randomUUID(); return await dbAdapter.withTransaction(async (txQuery) => { await consumeKeyWriteTicket(txQuery, owner, ticket, { @@ -280,7 +333,8 @@ export async function unshareKey( } // A direct writer suspends an active share, for abuse or any other reason of -// theirs. The realm's direct writers get a notice of it. +// theirs. The realm's direct writers get a notice of it. Suspending takes no +// ticket: it only stops a key being used. export async function suspendShare( dbAdapter: DBAdapter, { user, shareId }: { user: string; shareId: string }, @@ -304,15 +358,31 @@ export async function suspendShare( }); } -// A direct writer lifts a suspension a direct writer made. If the share's -// owner is no longer a direct writer, the share stays suspended for that -// reason instead, and resumes when they are one again. +// A direct writer lifts a suspension a direct writer made, with a key write +// ticket for that share: lifting one puts a key back into use, which card +// code in the writer's app page must not be able to do. If the share's owner +// is no longer a direct writer, the share stays suspended for that reason +// instead, and resumes when they are one again. export async function resumeShare( dbAdapter: DBAdapter, - { user, shareId }: { user: string; shareId: string }, + { user, shareId, ticket }: { user: string; shareId: string; ticket: unknown }, ): Promise { + if (!isUUID(shareId)) { + throw noSuchShare(); + } return await dbAdapter.withTransaction(async (txQuery) => { - let { row, isWriter } = await lockVisibleShare(txQuery, user, shareId); + await consumeKeyWriteTicket(txQuery, user, ticket, { + action: 'resume', + keyId: null, + realmURL: null, + shareId, + }); + let { row, isWriter, ownerIsWriter } = await lockVisibleShare( + txQuery, + user, + shareId, + { withOwner: true }, + ); if (!isWriter) { throw notADirectWriter(); } @@ -324,13 +394,8 @@ export async function resumeShare( : 'only a suspension a writer made is lifted; swap a key in instead', ); } - let ownerWrites = await isDirectWriterLocked( - txQuery, - new URL(row.realm_url as string), - row.owner as string, - ); let [updated] = await txQuery([ - ownerWrites + ownerIsWriter ? `UPDATE key_shares SET state = 'active', suspension_reason = NULL, suspended_at = NULL` : `UPDATE key_shares SET suspension_reason = 'owner-lost-write'`, `, suspended_by = NULL, updated_at = NOW() WHERE id = `, @@ -353,9 +418,15 @@ export async function swapKeyIntoShare( ticket, }: { user: string; shareId: string; keyId: unknown; ticket: unknown }, ): Promise { - if (typeof keyId !== 'string') { + if (keyId === undefined || keyId === null) { throw new UserKeyError(400, 'a swap names the key it puts in'); } + if (!isUUID(keyId)) { + throw noSuchKey(); + } + if (!isUUID(shareId)) { + throw noSuchShare(); + } return await dbAdapter.withTransaction(async (txQuery) => { await consumeKeyWriteTicket(txQuery, user, ticket, { action: 'swap', @@ -363,14 +434,15 @@ export async function swapKeyIntoShare( realmURL: null, shareId, }); - let { row, isWriter } = await lockVisibleShare(txQuery, user, shareId); + let { row, isWriter } = await lockVisibleShare(txQuery, user, shareId, { + ownKeyId: keyId, + }); if (!isWriter) { throw notADirectWriter(); } if (row.state !== 'suspended') { throw new UserKeyError(409, 'only a suspended share takes another key'); } - await lockOwnKey(txQuery, user, keyId); let [updated] = await txQuery([ `UPDATE key_shares SET owner = `, param(user), @@ -391,7 +463,7 @@ export async function listOwnShares( owner: string, { keyId }: { keyId?: string } = {}, ): Promise { - if (keyId !== undefined && !UUID.test(keyId)) { + if (keyId !== undefined && !isUUID(keyId)) { return []; } let rows = await query(dbAdapter, [ @@ -403,14 +475,24 @@ export async function listOwnShares( return rows.map((row) => toShare(row, { withKey: true })); } -async function assertDirectWriter( +// Reads `realm`'s rows for one of its direct writers, with their permission +// row locked for the read, so write removed meanwhile can't let them read on. +async function readAsDirectWriter( dbAdapter: DBAdapter, - realmURL: URL, user: string, -): Promise { - if (!(await isDirectWriter(dbAdapter, realmURL, user))) { - throw new UserKeyError(403, `only a realm's direct writers see its shares`); - } + realm: unknown, + read: (txQuery: Querier, realmURL: URL) => Promise[]>, +): Promise[]> { + let realmURL = validateRealm(realm); + return await dbAdapter.withTransaction(async (txQuery) => { + if (!(await isDirectWriterLocked(txQuery, realmURL, user))) { + throw new UserKeyError( + 403, + `only a realm's direct writers see its shares`, + ); + } + return await read(txQuery, realmURL); + }); } // A realm's shares, by label, for one of its direct writers. @@ -419,30 +501,39 @@ export async function listRealmShares( user: string, realm: unknown, ): Promise { - let realmURL = validateRealm(realm); - await assertDirectWriter(dbAdapter, realmURL, user); - let rows = await query(dbAdapter, [ - `SELECT ${SHARE_COLUMNS} FROM key_shares WHERE realm_url = `, - param(realmURL.href), - ` ORDER BY label`, - ]); + let rows = await readAsDirectWriter( + dbAdapter, + user, + realm, + async (txQuery, realmURL) => + await txQuery([ + `SELECT ${SHARE_COLUMNS} FROM key_shares WHERE realm_url = `, + param(realmURL.href), + ` ORDER BY label`, + ]), + ); return rows.map((row) => toShare(row, { withKey: row.owner === user })); } -// The notices recorded when one of a realm's shares was suspended, newest -// first, for one of its direct writers. +// The notices recorded when one of a realm's shares was suspended, or when +// deleting its key changed why it is suspended, newest first, for one of its +// direct writers. export async function listRealmShareNotices( dbAdapter: DBAdapter, user: string, realm: unknown, ): Promise { - let realmURL = validateRealm(realm); - await assertDirectWriter(dbAdapter, realmURL, user); - let rows = await query(dbAdapter, [ - `SELECT id, share_id, realm_url, label, owner, reason, suspended_by, created_at FROM key_share_notices WHERE realm_url = `, - param(realmURL.href), - ` ORDER BY created_at DESC, id DESC LIMIT ${MAX_KEY_SHARE_NOTICES}`, - ]); + let rows = await readAsDirectWriter( + dbAdapter, + user, + realm, + async (txQuery, realmURL) => + await txQuery([ + `SELECT id, share_id, realm_url, label, owner, reason, suspended_by, created_at FROM key_share_notices WHERE realm_url = `, + param(realmURL.href), + ` ORDER BY created_at DESC, id DESC LIMIT ${MAX_KEY_SHARE_NOTICES}`, + ]), + ); return rows.map((row) => ({ id: String(row.id), shareId: row.share_id as string, diff --git a/packages/realm-server/lib/user-keys.ts b/packages/realm-server/lib/user-keys.ts index d6c349549e2..e0476fbb606 100644 --- a/packages/realm-server/lib/user-keys.ts +++ b/packages/realm-server/lib/user-keys.ts @@ -17,9 +17,10 @@ import type { // where a key may be sent, and the value is sealed to the key alone. // // A key is added, replaced, deleted, shared into a realm or swapped into a -// share only with a key write ticket: short-lived, single-use, minted for the -// signed-in user and for one change (an add; a replace or delete of one key; a -// share of one key into one realm; or a swap of one key into one share), and +// share, and a share suspended by a writer is resumed, only with a key write +// ticket: short-lived, single-use, minted for the signed-in user and for one +// change (an add; a replace or delete of one key; a share of one key into one +// realm; a swap of one key into one share; or a resume of one share), and // consumed in the same transaction as that change. The routes mint tickets only for requests from // the secure origin and for clients that send no `Origin`, as the CLI does, so // card code in the app page can't change a key from inside the browser. @@ -37,6 +38,7 @@ export const KEY_WRITE_ACTIONS = [ 'delete', 'share', 'swap', + 'resume', ] as const; export type KeyWriteAction = (typeof KEY_WRITE_ACTIONS)[number]; @@ -74,15 +76,17 @@ function sha256(value: string): string { } // The change a key write ticket is minted for: an add; a replace or delete of -// one key; a share of one key into one realm; or a swap of one key into one -// share. +// one key; a share of one key into one realm; a swap of one key into one +// share; or a resume of one share. export interface KeyWriteTarget { action: KeyWriteAction; - // The key a replace, delete, share or swap uses; an add names none. + // The key a replace, delete, share or swap uses; an add or a resume names + // none. keyId: string | null; // The realm a share puts the key into; only a share names one. realmURL: string | null; - // The share a swap puts the key into; only a swap names one. + // The share a swap puts the key into, or a resume resumes; only those name + // one. shareId: string | null; } @@ -107,14 +111,22 @@ export function validateKeyWriteTarget({ if (named(realm) && action !== 'share') { throw new UserKeyError(400, `a ticket to ${action} names no realm`); } - if (named(shareId) && action !== 'swap') { + if (named(shareId) && action !== 'swap' && action !== 'resume') { throw new UserKeyError(400, `a ticket to ${action} names no share`); } - if (action === 'add') { + if (action === 'add' || action === 'resume') { if (named(keyId)) { - throw new UserKeyError(400, 'a ticket to add a key names no key'); + throw new UserKeyError(400, `a ticket to ${action} names no key`); } - return { action, keyId: null, realmURL: null, shareId: null }; + if (action === 'add') { + return { action, keyId: null, realmURL: null, shareId: null }; + } + return { + action, + keyId: null, + realmURL: null, + shareId: validShareId(shareId, action), + }; } if (typeof keyId !== 'string') { throw new UserKeyError(400, `a ticket to ${action} a key names the key`); @@ -127,10 +139,12 @@ export function validateKeyWriteTarget({ return { action, keyId, realmURL: realmURL.href, shareId: null }; } if (action === 'swap') { - if (typeof shareId !== 'string') { - throw new UserKeyError(400, 'a ticket to swap a key names the share'); - } - return { action, keyId, realmURL: null, shareId }; + return { + action, + keyId, + realmURL: null, + shareId: validShareId(shareId, action), + }; } return { action: action as KeyWriteAction, @@ -209,6 +223,17 @@ export async function consumeKeyWriteTicket( } } +// A share's id is a UUID; anything else names no share. +function validShareId(shareId: unknown, action: KeyWriteAction): string { + if (typeof shareId !== 'string') { + throw new UserKeyError(400, `a ticket to ${action} names the share`); + } + if (!UUID.test(shareId)) { + throw new UserKeyError(404, 'no such share'); + } + return shareId; +} + function matches(column: string, value: string | null): Expression { return value === null ? [` AND ${column} IS NULL`] @@ -427,7 +452,7 @@ export async function replaceUserKeyValue( // Deletes a key. Every share holding it is suspended in the same statement, // by a trigger on `user_keys`, and keeps its place in its realm for a direct -// writer to swap another key into; `listKeyShares` with the key's id lists +// writer to swap another key into; `listOwnShares` with the key's id lists // the shares a delete would suspend. export async function deleteUserKey( dbAdapter: DBAdapter, diff --git a/packages/realm-server/tests/key-shares-test.ts b/packages/realm-server/tests/key-shares-test.ts index d5d7abf394b..2eaf2032812 100644 --- a/packages/realm-server/tests/key-shares-test.ts +++ b/packages/realm-server/tests/key-shares-test.ts @@ -5,7 +5,11 @@ import { createRequire } from 'module'; import type { SuperTest, Test } from 'supertest'; import supertest from 'supertest'; import type { PgAdapter } from '@cardstack/postgres'; -import { insertPermissions } from '@cardstack/runtime-common'; +import { + insertPermissions, + removeRealmPermissions, +} from '@cardstack/runtime-common'; +import { removeRealmDatabaseArtifacts } from '../handlers/realm-destruction-utils.ts'; import { setupDB, setupSimpleRealmServerTemplate, @@ -176,6 +180,15 @@ module(basename(import.meta.filename), function () { .set('Authorization', auth(user)); } + async function resume(user: string, shareId: string, ticket?: string) { + ticket ??= await ticketFor(user, { action: 'resume', shareId }); + return await request + .post(`/_key-shares/${shareId}/resume`) + .set('Authorization', auth(user)) + .set('Content-Type', 'application/json') + .send({ meta: { ticket } }); + } + async function realmShares(user: string, realm = REALM) { return await request .get(`/_key-shares?realm=${encodeURIComponent(realm)}`) @@ -497,7 +510,24 @@ module(basename(import.meta.filename), function () { 'the suspension is noticed', ); - let resumed = await act(OWNER, shareId, 'resume'); + let noTicket = await request + .post(`/_key-shares/${shareId}/resume`) + .set('Authorization', auth(OWNER)) + .set('Content-Type', 'application/json') + .send({}); + assert.strictEqual(noTicket.status, 403, 'lifting one needs a ticket'); + let otherShare = await sharedId(OWNER, 'Other'); + let otherTicket = await ticketFor(OWNER, { + action: 'resume', + shareId: otherShare, + }); + assert.strictEqual( + (await resume(OWNER, shareId, otherTicket)).status, + 403, + `a ticket to resume one share doesn't resume another`, + ); + + let resumed = await resume(OWNER, shareId); assert.strictEqual(resumed.status, 200, resumed.text); assert.strictEqual(resumed.body.data.attributes.state, 'active'); }); @@ -508,7 +538,7 @@ module(basename(import.meta.filename), function () { await insertPermissions(dbAdapter, new URL(REALM), { [OWNER]: ['read'], }); - let resumed = await act(WRITER, shareId, 'resume'); + let resumed = await resume(WRITER, shareId); assert.strictEqual(resumed.status, 200, resumed.text); assert.deepEqual( [ @@ -598,6 +628,21 @@ module(basename(import.meta.filename), function () { `a ticket for the writer's key doesn't swap in another`, ); + let otherShare = await sharedId(WRITER, 'Other'); + await act(WRITER, otherShare, 'suspend'); + let writerKey = await keyFor(WRITER, 'third'); + let otherShareTicket = await ticketFor(WRITER, { + action: 'swap', + keyId: writerKey, + shareId: otherShare, + }); + let wrongShare = await swap(WRITER, shareId, writerKey, otherShareTicket); + assert.strictEqual( + wrongShare.status, + 403, + `a ticket to swap into one share doesn't swap into another`, + ); + let state = await shareState(shareId); assert.deepEqual( [state!.owner, state!.state], @@ -659,6 +704,99 @@ module(basename(import.meta.filename), function () { assert.strictEqual(swapped.status, 200, 'a writer swaps a key into it'); }); + test(`deleting the key of a share a writer suspended records a notice`, async function (assert) { + let keyId = await keyFor(OWNER); + let shared = await share(OWNER, { realm: REALM, keyId, label: 'A' }); + let shareId = shared.body.data.id; + await act(WRITER, shareId, 'suspend'); + + let ticket = await ticketFor(OWNER, { action: 'delete', keyId }); + await request + .delete(`/_user-keys/${keyId}`) + .set('Authorization', auth(OWNER)) + .set('Content-Type', 'application/json') + .send({ meta: { ticket } }); + + let state = await shareState(shareId); + assert.deepEqual( + [state?.state, state?.reason, state?.keyId], + ['suspended', 'key-deleted', null], + ); + let listed = await notices(WRITER); + assert.deepEqual( + listed.body.data.map( + (n: { attributes: Record }) => n.attributes.reason, + ), + ['key-deleted', 'suspended-by-writer'], + 'the writers learn the key is gone', + ); + }); + + test('deleting a realm deletes its shares', async function (assert) { + let shareId = await sharedId(); + let elsewhere = await sharedId(OWNER, 'Elsewhere', OTHER_REALM); + await removeRealmPermissions(dbAdapter, new URL(REALM)); + await removeRealmDatabaseArtifacts({ dbAdapter, realmURL: REALM }); + assert.strictEqual(await shareState(shareId), undefined); + let [{ count }] = await dbAdapter.execute( + `SELECT COUNT(*)::int AS count FROM key_share_notices WHERE share_id = $1`, + { bind: [shareId] }, + ); + assert.strictEqual(count, 0, 'its notices go with it'); + + await insertPermissions(dbAdapter, new URL(REALM), { + [OWNER]: ['read', 'write'], + }); + assert.deepEqual( + ( + await request.get('/_key-shares').set('Authorization', auth(OWNER)) + ).body.data.map((s: { id: string }) => s.id), + [elsewhere], + 'a realm later made at the same URL gets none of them back', + ); + }); + + test('ids that are not UUIDs name nothing', async function (assert) { + let shareId = await sharedId(); + let keyId = await keyFor(WRITER); + assert.strictEqual( + (await mint(WRITER, { action: 'swap', keyId, shareId: 'nope' })).status, + 404, + 'no swap ticket is minted for a malformed share id', + ); + assert.strictEqual( + (await mint(WRITER, { action: 'resume', shareId: 'nope' })).status, + 404, + 'no resume ticket is minted for a malformed share id', + ); + let ticket = await ticketFor(WRITER, { action: 'swap', keyId, shareId }); + assert.strictEqual( + (await swap(WRITER, 'nope', keyId, ticket)).status, + 404, + ); + assert.strictEqual( + (await swap(WRITER, shareId, 'nope', ticket)).status, + 404, + ); + assert.strictEqual((await act(WRITER, 'nope', 'suspend')).status, 404); + assert.strictEqual((await resume(WRITER, 'nope', ticket)).status, 404); + assert.strictEqual( + ( + await share( + WRITER, + { realm: REALM, keyId: 'nope', label: 'X' }, + ticket, + ) + ).status, + 404, + ); + assert.strictEqual( + (await swap(WRITER, shareId, keyId, ticket)).status, + 409, + 'the refusals left the ticket unspent', + ); + }); + test(`a reader sees nothing of a realm's shares`, async function (assert) { let shareId = await sharedId(); for (let user of [READER, STRANGER]) { @@ -668,7 +806,7 @@ module(basename(import.meta.filename), function () { .get('/_key-shares') .set('Authorization', auth(user)); assert.deepEqual(own.body.data, [], `${user} owns no shares`); - assert.strictEqual((await act(user, shareId, 'resume')).status, 404); + assert.strictEqual((await resume(user, shareId)).status, 404); let unshare = await request .delete(`/_key-shares/${shareId}`) .set('Authorization', auth(user)); From 988fec01c4e50bfbba358f84189473bb2c214bd8 Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Fri, 16 Oct 2026 19:28:28 -0400 Subject: [PATCH 3/6] Withhold the share tables from readonly_role without touching their owner The revoke loop skips a member that holds only the ADMIN option on readonly_role (the role that creates it holds that membership on Postgres 16) and the table's owner, so it no longer revokes the owner's own SELECT. After it, `up` stops if readonly_role, or any role reading through it, can still read a column of either table, as the check for the sealed tables does. Tests cover a PUBLIC grant, a grant through another role, an ADMIN-only member and an owning member, and remove the memberships they make. Co-Authored-By: Claude Opus 5.5 --- .claude/skills/key-shares/SKILL.md | 8 +- .../1792187770930_add-key-shares.js | 65 ++++++- .../realm-server/tests/key-shares-test.ts | 183 ++++++++++++++---- 3 files changed, 213 insertions(+), 43 deletions(-) diff --git a/.claude/skills/key-shares/SKILL.md b/.claude/skills/key-shares/SKILL.md index 7ac5ee1a4b0..86ddbf9059d 100644 --- a/.claude/skills/key-shares/SKILL.md +++ b/.claude/skills/key-shares/SKILL.md @@ -106,8 +106,12 @@ SELECT created_at, reason, owner, suspended_by FROM key_share_notices WHERE share_id = '' ORDER BY created_at DESC; ``` -Both tables hold no secret, but they are withheld from `readonly_role` and its -members like `user_keys`, and excluded from the SQLite schema dump. +Both tables hold no secret, but they are withheld from `readonly_role`, like +`user_keys`: the migration revokes SELECT from the role and from each member +that inherits it or can SET ROLE to it (not a member holding only the ADMIN +option, nor a table's owner), then stops, naming the role and the column, if +any role reading through `readonly_role` can still read a column (through a +PUBLIC grant, say). They are also excluded from the SQLite schema dump. `realm_url` is the realm's canonical URL, with a trailing slash, as `realm_user_permissions` stores it. diff --git a/packages/postgres/migrations/1792187770930_add-key-shares.js b/packages/postgres/migrations/1792187770930_add-key-shares.js index bd772426600..c385464c4f3 100644 --- a/packages/postgres/migrations/1792187770930_add-key-shares.js +++ b/packages/postgres/migrations/1792187770930_add-key-shares.js @@ -34,9 +34,20 @@ // realm's direct writers. // // Both tables are withheld from the read-only role used by dashboards and -// read-only database access, and from every role that is a member of it, as -// `user_keys` is. Every statement is idempotent, so running `up` again changes -// nothing. +// read-only database access, as `user_keys` is. `up` revokes SELECT from the +// role and from each direct member that reads through it: one that inherits +// its privileges or can SET ROLE to it. A member holding only the ADMIN option +// reads nothing through the role (on Postgres 16 and later the role that +// creates `readonly_role` holds that membership), and a table's owner reads it +// by owning it, so neither loses SELECT. SELECT granted to PUBLIC, or to +// another role a member also belongs to, still lets them read, and revoking it +// isn't a migration's call, so `up` then asks `has_column_privilege` about +// every column of both tables, for `readonly_role` and every role that reads +// through it directly or through other roles, and stops, naming the role and +// the column, if any of them can still read one. `readonly_role` exists only +// where it was provisioned (staging and production), so both steps are +// skipped wherever it doesn't. Every statement is idempotent, so running `up` +// again changes nothing. exports.up = (pgm) => { pgm.sql(` @@ -251,7 +262,10 @@ exports.up = (pgm) => { FROM pg_auth_members membership JOIN pg_roles grp ON grp.oid = membership.roleid JOIN pg_roles member ON member.oid = membership.member + JOIN pg_class tbl ON tbl.oid = to_regclass(table_name) WHERE grp.rolname = 'readonly_role' + AND (membership.inherit_option OR membership.set_option) + AND member.oid <> tbl.relowner LOOP EXECUTE format( 'REVOKE SELECT ON TABLE %I FROM %I', @@ -264,6 +278,51 @@ exports.up = (pgm) => { END $$ `); + pgm.sql(` + DO $$ + DECLARE + readable_by name; + readable_table name; + readable_column name; + BEGIN + IF EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'readonly_role') THEN + WITH RECURSIVE reader (oid) AS ( + SELECT oid FROM pg_roles WHERE rolname = 'readonly_role' + UNION + SELECT membership.member + FROM pg_auth_members membership + JOIN reader ON reader.oid = membership.roleid + WHERE membership.inherit_option OR membership.set_option + ) + SELECT reader_role.rolname, tbl.relname, col.attname + INTO readable_by, readable_table, readable_column + FROM reader + JOIN pg_roles reader_role ON reader_role.oid = reader.oid + CROSS JOIN pg_class tbl + JOIN pg_attribute col ON col.attrelid = tbl.oid + WHERE tbl.oid IN ( + to_regclass('key_shares'), + to_regclass('key_share_notices') + ) + AND reader.oid <> tbl.relowner + AND col.attnum > 0 + AND NOT col.attisdropped + AND has_column_privilege(reader.oid, tbl.oid, col.attnum, 'SELECT') + ORDER BY tbl.relname, reader_role.rolname, col.attnum + LIMIT 1; + IF FOUND THEN + RAISE EXCEPTION + '% can read %.%, which is withheld from readonly_role and the roles that read through it', + readable_by, readable_table, readable_column + USING HINT = format( + 'Revoke the grant or role membership that lets %I read %I.%I, then run the migration again.', + readable_by, readable_table, readable_column + ); + END IF; + END IF; + END + $$ + `); }; exports.down = (pgm) => { diff --git a/packages/realm-server/tests/key-shares-test.ts b/packages/realm-server/tests/key-shares-test.ts index 2eaf2032812..3447a4372a9 100644 --- a/packages/realm-server/tests/key-shares-test.ts +++ b/packages/realm-server/tests/key-shares-test.ts @@ -833,42 +833,37 @@ module(basename(import.meta.filename), function () { module('key shares tables', function (hooks) { let dbAdapter: PgAdapter; + // Roles and memberships are shared by every database on the server, so + // each test removes the memberships it makes, whatever happens. + const MEMBER = 'readonly_member_for_key_shares_test'; + const ADMIN_MEMBER = 'readonly_admin_member_for_key_shares_test'; + const OWNER = 'readonly_owner_for_key_shares_test'; + const OTHER_ROLE = 'other_role_for_key_shares_test'; + const TABLES = ['key_shares', 'key_share_notices']; + setupDB(hooks, { beforeEach: async (_dbAdapter) => { dbAdapter = _dbAdapter; + // The role and its blanket grant as staging and production have them. + await createRole('readonly_role'); + await dbAdapter.execute( + `GRANT SELECT ON ALL TABLES IN SCHEMA public TO readonly_role`, + ); }, }); - test("the read-only role can't select either table", async function (assert) { - // The role and its blanket grant as staging and production have them, - // then the migration that withholds these tables. - await dbAdapter.execute( - `DO $$ - BEGIN - CREATE ROLE readonly_role; - EXCEPTION WHEN duplicate_object OR unique_violation THEN NULL; - END - $$`, - ); - await dbAdapter.execute( - `GRANT SELECT ON ALL TABLES IN SCHEMA public TO readonly_role`, - ); - // A user that inherits the role and was also granted SELECT on the - // tables directly. + async function createRole(name: string) { await dbAdapter.execute( `DO $$ BEGIN - CREATE ROLE readonly_member_for_key_shares_test; + CREATE ROLE ${name}; EXCEPTION WHEN duplicate_object OR unique_violation THEN NULL; END $$`, ); - await dbAdapter.execute( - `GRANT readonly_role TO readonly_member_for_key_shares_test`, - ); - await dbAdapter.execute( - `GRANT SELECT ON TABLE key_shares, key_share_notices TO readonly_member_for_key_shares_test`, - ); + } + + async function runUp() { let statements: string[] = []; require(KEY_SHARES_MIGRATION).up({ sql: (statement: string) => statements.push(statement), @@ -876,26 +871,138 @@ module(basename(import.meta.filename), function () { for (let statement of statements) { await dbAdapter.execute(statement); } + } - async function selectAs(role: string, sql: string) { - return await dbAdapter.withTransaction(async (query: any) => { - await query([`SET LOCAL ROLE ${role}`]); - return await query([sql]); - }); + async function withMemberships( + memberships: [role: string, member: string, options?: string][], + fn: () => Promise, + ) { + for (let [role, member] of memberships) { + await createRole(role); + await createRole(member); } - - for (let role of [ - 'readonly_role', - 'readonly_member_for_key_shares_test', - ]) { - for (let table of ['key_shares', 'key_share_notices']) { - await assert.rejects( - selectAs(role, `SELECT * FROM ${table}`), - new RegExp(`permission denied for table ${table}`), - `${role} can't select ${table}`, + try { + for (let [role, member, options] of memberships) { + await dbAdapter.execute( + `GRANT ${role} TO ${member}${options ? ` WITH ${options}` : ''}`, ); } + await fn(); + } finally { + for (let [role, member] of [...memberships].reverse()) { + await dbAdapter.execute(`REVOKE ${role} FROM ${member}`); + } } + } + + async function selectAs(role: string, sql: string) { + return await dbAdapter.withTransaction(async (query: any) => { + await query([`SET LOCAL ROLE ${role}`]); + return await query([sql]); + }); + } + + async function canSelect(role: string, table: string): Promise { + let [{ readable }] = await dbAdapter.execute( + `SELECT has_table_privilege('${role}', '${table}', 'SELECT') AS readable`, + ); + return readable as boolean; + } + + test("the read-only role and its members can't select either table", async function (assert) { + // A member that inherits the role and was also granted SELECT on the + // tables directly. + await withMemberships([['readonly_role', MEMBER]], async () => { + await dbAdapter.execute( + `GRANT SELECT ON TABLE ${TABLES.join(', ')} TO ${MEMBER}`, + ); + + await runUp(); + + for (let role of ['readonly_role', MEMBER]) { + for (let table of TABLES) { + await assert.rejects( + selectAs(role, `SELECT * FROM ${table}`), + new RegExp(`permission denied for table ${table}`), + `${role} can't select ${table}`, + ); + } + } + }); + }); + + test('stops when PUBLIC can read a column', async function (assert) { + await dbAdapter.execute( + `GRANT SELECT (owner) ON TABLE key_shares TO PUBLIC`, + ); + + await assert.rejects( + runUp(), + /can read key_shares\.owner, which is withheld from readonly_role/, + ); + }); + + test('stops when a member reads a table through another role', async function (assert) { + await withMemberships( + [ + ['readonly_role', MEMBER], + [OTHER_ROLE, MEMBER], + ], + async () => { + await dbAdapter.execute( + `GRANT SELECT ON TABLE key_share_notices TO ${OTHER_ROLE}`, + ); + + await assert.rejects( + runUp(), + new RegExp( + `${MEMBER} can read key_share_notices\\.\\w+, which is withheld`, + ), + ); + }, + ); + }); + + test('leaves SELECT to a member that holds only the ADMIN option on the role', async function (assert) { + // The role that creates the read-only role holds this membership on + // Postgres 16 and later, and reads nothing through it. + await withMemberships( + [ + [ + 'readonly_role', + ADMIN_MEMBER, + 'ADMIN TRUE, INHERIT FALSE, SET FALSE', + ], + ], + async () => { + await dbAdapter.execute( + `GRANT SELECT ON TABLE ${TABLES.join(', ')} TO ${ADMIN_MEMBER}`, + ); + + await runUp(); + + for (let table of TABLES) { + assert.true( + await canSelect(ADMIN_MEMBER, table), + `it still reads ${table}`, + ); + } + }, + ); + }); + + test('leaves SELECT to a member that owns a table', async function (assert) { + await createRole(OWNER); + await dbAdapter.execute(`ALTER TABLE key_shares OWNER TO ${OWNER}`); + await withMemberships([['readonly_role', OWNER]], async () => { + await runUp(); + + assert.true(await canSelect(OWNER, 'key_shares'), 'the owner reads it'); + assert.false( + await canSelect('readonly_role', 'key_shares'), + 'the role itself still does not', + ); + }); }); }); }); From 6f4f16eb957f605161a6d4e810a5dfa7730faaa8 Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Fri, 16 Oct 2026 19:42:04 -0400 Subject: [PATCH 4/6] Revoke the share tables' read-only grants as each grantor, with CASCADE A REVOKE removes only the grants its grantor made, so a SELECT another role granted to readonly_role or one of its readers would survive the revoke loop (and the read check would then stop the migration). The loop now finds every SELECT grant held by readonly_role or a role reading through it, recursively, and revokes it acting as its grantor with CASCADE, so grants passed on from it go too, as the sealing migrations' loop does. A test passes a grant-option grant from an outside role to a member and on to readonly_role. Co-Authored-By: Claude Opus 5.5 --- .claude/skills/key-shares/SKILL.md | 13 +- .../1792187770930_add-key-shares.js | 130 ++++++++++++++---- .../realm-server/tests/key-shares-test.ts | 35 +++++ 3 files changed, 146 insertions(+), 32 deletions(-) diff --git a/.claude/skills/key-shares/SKILL.md b/.claude/skills/key-shares/SKILL.md index 86ddbf9059d..38713d18a76 100644 --- a/.claude/skills/key-shares/SKILL.md +++ b/.claude/skills/key-shares/SKILL.md @@ -107,11 +107,14 @@ FROM key_share_notices WHERE share_id = '' ORDER BY created_at DESC; ``` Both tables hold no secret, but they are withheld from `readonly_role`, like -`user_keys`: the migration revokes SELECT from the role and from each member -that inherits it or can SET ROLE to it (not a member holding only the ADMIN -option, nor a table's owner), then stops, naming the role and the column, if -any role reading through `readonly_role` can still read a column (through a -PUBLIC grant, say). They are also excluded from the SQLite schema dump. +`user_keys`, and from every role that reads with its privileges (granted it +with INHERIT or SET, directly or through other roles). The migration revokes +each SELECT grant those roles hold, acting as the role that made it and with +CASCADE, never touching a table's owner or a member that only administers the +role. It then stops, naming the role and the column, if any of them can still +read a column (through a PUBLIC grant, say). Where the migrating role can't act +as a grant's grantor, it stops and names the grant. They are also excluded from +the SQLite schema dump. `realm_url` is the realm's canonical URL, with a trailing slash, as `realm_user_permissions` stores it. diff --git a/packages/postgres/migrations/1792187770930_add-key-shares.js b/packages/postgres/migrations/1792187770930_add-key-shares.js index c385464c4f3..4da314cfd6d 100644 --- a/packages/postgres/migrations/1792187770930_add-key-shares.js +++ b/packages/postgres/migrations/1792187770930_add-key-shares.js @@ -34,20 +34,27 @@ // realm's direct writers. // // Both tables are withheld from the read-only role used by dashboards and -// read-only database access, as `user_keys` is. `up` revokes SELECT from the -// role and from each direct member that reads through it: one that inherits -// its privileges or can SET ROLE to it. A member holding only the ADMIN option -// reads nothing through the role (on Postgres 16 and later the role that -// creates `readonly_role` holds that membership), and a table's owner reads it -// by owning it, so neither loses SELECT. SELECT granted to PUBLIC, or to -// another role a member also belongs to, still lets them read, and revoking it -// isn't a migration's call, so `up` then asks `has_column_privilege` about -// every column of both tables, for `readonly_role` and every role that reads -// through it directly or through other roles, and stops, naming the role and -// the column, if any of them can still read one. `readonly_role` exists only -// where it was provisioned (staging and production), so both steps are -// skipped wherever it doesn't. Every statement is idempotent, so running `up` -// again changes nothing. +// read-only database access, as `user_keys` is, and from every role that reads +// with its privileges: one granted it with INHERIT or SET, directly or through +// other roles granted that way. A role that only administers the membership +// isn't one (on Postgres 16 the role that creates `readonly_role` holds that +// membership, and it is often the tables' owner), and a table's owner is never +// treated as one, so its own privileges are left alone. +// +// A REVOKE removes only the grants its grantor made, so `up` revokes each +// SELECT grant those roles hold on a table or its columns acting as the role +// that made it, with CASCADE, which also removes every grant passed on from +// it. Acting as another role needs the migrating role to be a superuser or able +// to SET ROLE to it; where it isn't, `up` stops and names the grant rather than +// leave the table readable. SELECT granted to PUBLIC, or to a role outside the +// read-only role that one of its readers also belongs to, still lets them read, +// and revoking it isn't a migration's call, so `up` then asks +// `has_column_privilege` about every column of both tables for each reader and +// stops, naming the role and the column, if any of them can still read one. +// +// `readonly_role` exists only where it was provisioned (staging and +// production), so both steps are skipped wherever it doesn't. Every statement +// is idempotent, so running `up` again changes nothing. exports.up = (pgm) => { pgm.sql(` @@ -250,28 +257,97 @@ exports.up = (pgm) => { pgm.sql(` DO $$ DECLARE - member_name name; + migrating_role name := current_user; table_name text; + grantor_name name; + grantee_name name; + revoked_grantor oid; + revoked_grantee oid; BEGIN IF EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'readonly_role') THEN FOREACH table_name IN ARRAY ARRAY['key_shares', 'key_share_notices'] LOOP - EXECUTE format('REVOKE SELECT ON TABLE %I FROM readonly_role', table_name); - FOR member_name IN - SELECT member.rolname - FROM pg_auth_members membership - JOIN pg_roles grp ON grp.oid = membership.roleid - JOIN pg_roles member ON member.oid = membership.member - JOIN pg_class tbl ON tbl.oid = to_regclass(table_name) - WHERE grp.rolname = 'readonly_role' - AND (membership.inherit_option OR membership.set_option) - AND member.oid <> tbl.relowner LOOP + -- One SELECT grant the read-only role or a member of it still + -- holds on the table or one of its columns. A grant the table's + -- owner made has no grantor name. + SELECT + CASE WHEN held.grantor <> held.relowner + THEN grantor_role.rolname END, + grantee_role.rolname, + held.grantor, + held.grantee + INTO grantor_name, grantee_name, revoked_grantor, revoked_grantee + FROM ( + SELECT tbl.relowner, acl.* + FROM pg_class tbl + CROSS JOIN LATERAL aclexplode(tbl.relacl) acl + WHERE tbl.oid = table_name::regclass + UNION ALL + SELECT tbl.relowner, acl.* + FROM pg_class tbl + JOIN pg_attribute col ON col.attrelid = tbl.oid + CROSS JOIN LATERAL aclexplode(col.attacl) acl + WHERE tbl.oid = table_name::regclass + AND col.attnum > 0 + AND NOT col.attisdropped + ) held + JOIN pg_roles grantor_role ON grantor_role.oid = held.grantor + JOIN pg_roles grantee_role ON grantee_role.oid = held.grantee + WHERE held.privilege_type = 'SELECT' + AND grantee_role.oid <> held.relowner + AND grantee_role.oid IN ( + WITH RECURSIVE reader(oid) AS ( + SELECT oid FROM pg_roles WHERE rolname = 'readonly_role' + UNION + SELECT membership.member + FROM pg_auth_members membership + JOIN reader ON membership.roleid = reader.oid + WHERE membership.inherit_option OR membership.set_option + ) + SELECT oid FROM reader + ) + LIMIT 1; + EXIT WHEN NOT FOUND; + IF grantor_name IS NOT NULL THEN + BEGIN + EXECUTE format('SET ROLE %I', grantor_name); + EXCEPTION WHEN insufficient_privilege THEN + RAISE EXCEPTION + 'cannot revoke the SELECT on % that % granted to %', + table_name, grantor_name, grantee_name + USING HINT = format( + 'Run the migration as a superuser or as a role that can SET ROLE %I.', + grantor_name + ); + END; + END IF; EXECUTE format( - 'REVOKE SELECT ON TABLE %I FROM %I', + 'REVOKE SELECT ON TABLE %I FROM %I CASCADE', table_name, - member_name + grantee_name ); + EXECUTE format('SET ROLE %I', migrating_role); + IF EXISTS ( + SELECT 1 + FROM pg_class tbl + CROSS JOIN LATERAL aclexplode(tbl.relacl) acl + WHERE tbl.oid = table_name::regclass + AND acl.privilege_type = 'SELECT' + AND acl.grantor = revoked_grantor + AND acl.grantee = revoked_grantee + UNION ALL + SELECT 1 + FROM pg_attribute col + CROSS JOIN LATERAL aclexplode(col.attacl) acl + WHERE col.attrelid = table_name::regclass + AND acl.privilege_type = 'SELECT' + AND acl.grantor = revoked_grantor + AND acl.grantee = revoked_grantee + ) THEN + RAISE EXCEPTION 'the SELECT on % that % granted to % survived its REVOKE', + table_name, COALESCE(grantor_name, 'the owner'), grantee_name; + END IF; END LOOP; END LOOP; END IF; diff --git a/packages/realm-server/tests/key-shares-test.ts b/packages/realm-server/tests/key-shares-test.ts index 3447a4372a9..687e65da2eb 100644 --- a/packages/realm-server/tests/key-shares-test.ts +++ b/packages/realm-server/tests/key-shares-test.ts @@ -931,6 +931,41 @@ module(basename(import.meta.filename), function () { }); }); + test('revokes a grant another role made, and every grant passed on from it', async function (assert) { + // A role outside the read-only role holds SELECT with the grant option + // and passes it to a member, which passes it to the role itself. + await createRole(OTHER_ROLE); + await dbAdapter.execute( + `GRANT SELECT ON TABLE key_shares TO ${OTHER_ROLE} WITH GRANT OPTION`, + ); + await withMemberships([['readonly_role', MEMBER]], async () => { + await dbAdapter.withTransaction(async (query: any) => { + await query([`SET LOCAL ROLE ${OTHER_ROLE}`]); + await query([ + `GRANT SELECT ON TABLE key_shares TO ${MEMBER} WITH GRANT OPTION`, + ]); + }); + await dbAdapter.withTransaction(async (query: any) => { + await query([`SET LOCAL ROLE ${MEMBER}`]); + await query([ + `GRANT SELECT (owner) ON TABLE key_shares TO readonly_role`, + ]); + }); + + await runUp(); + + assert.false(await canSelect(MEMBER, 'key_shares')); + let [{ readable }] = await dbAdapter.execute( + `SELECT has_column_privilege('readonly_role', 'key_shares', 'owner', 'SELECT') AS readable`, + ); + assert.false(readable as boolean, 'the grant passed on is gone too'); + assert.true( + await canSelect(OTHER_ROLE, 'key_shares'), + 'the role outside the read-only role keeps its own', + ); + }); + }); + test('stops when PUBLIC can read a column', async function (assert) { await dbAdapter.execute( `GRANT SELECT (owner) ON TABLE key_shares TO PUBLIC`, From b94a5778b500884cdc38beee18aeced4bc72d7a5 Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Mon, 19 Oct 2026 00:02:57 -0400 Subject: [PATCH 5/6] Lock a person's shares before the permission trigger decides A resume that finds no permission row for the share's owner has nothing to lock there, so a concurrent grant of the owner's write could skip the share as still writer-suspended and leave it as owner-lost-write while the owner writes. The trigger now locks the person's shares in the realm first, so it waits on the resume and its UPDATE sees the committed reason. Co-Authored-By: Claude Opus 5.5 --- .../postgres/migrations/1792187770930_add-key-shares.js | 8 ++++++++ packages/realm-server/lib/key-shares.ts | 9 ++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/packages/postgres/migrations/1792187770930_add-key-shares.js b/packages/postgres/migrations/1792187770930_add-key-shares.js index 4da314cfd6d..84a68e486bb 100644 --- a/packages/postgres/migrations/1792187770930_add-key-shares.js +++ b/packages/postgres/migrations/1792187770930_add-key-shares.js @@ -126,6 +126,14 @@ exports.up = (pgm) => { share_owner varchar ) RETURNS void LANGUAGE plpgsql AS $$ BEGIN + -- Lock the person's shares before deciding. A resume that read no + -- permission row (there was none to lock) may be about to set a share + -- to 'owner-lost-write'. Waiting on its lock means the UPDATE below, + -- a new statement with a new snapshot, sees that committed reason and + -- resumes the share, rather than skipping it as 'suspended-by-writer'. + PERFORM 1 FROM key_shares + WHERE realm_url = share_realm_url AND owner = share_owner + FOR UPDATE; IF EXISTS ( SELECT 1 FROM realm_user_permissions WHERE realm_url = share_realm_url diff --git a/packages/realm-server/lib/key-shares.ts b/packages/realm-server/lib/key-shares.ts index 200429e6a37..3686c6c27ca 100644 --- a/packages/realm-server/lib/key-shares.ts +++ b/packages/realm-server/lib/key-shares.ts @@ -161,9 +161,12 @@ export function validateShareLabel(label: unknown): string { // Locks are taken in the order the triggers take them: permission rows, then // the key, then the share. A permission change locks its row and then, -// through its trigger, the shares it suspends or resumes; deleting a key locks -// the key and then the shares holding it. Taking a share before a row either -// of them holds would deadlock against them. +// through its trigger, every share the person holds in that realm, waiting +// out any change to one before deciding what to suspend or resume. A row +// that doesn't exist can't be locked, so that wait is what keeps a resume +// and the person's write being granted from missing each other. Deleting a +// key locks the key and then the shares holding it. Taking a share before a +// row either of them holds would deadlock against them. // Whether each of `users` is a direct writer of the realm, with their // permission rows locked in username order, so two changes locking the same From 3bc98c6c5b7f58bb05cd0d122536132b3e18b7fc Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Mon, 19 Oct 2026 01:12:31 -0400 Subject: [PATCH 6/6] Revoke column grants first, then owner-made grants, in the read-only loop Grants a table's owner made go first, so CASCADE takes what a reader passed on from one without acting as that reader. Column grants go before table grants, because CASCADE on a table grant leaves the column grants passed on from it, and once their grantor has lost the table grant nobody can revoke them. Co-Authored-By: Claude Opus 5.5 --- .../1792187770930_add-key-shares.js | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/packages/postgres/migrations/1792187770930_add-key-shares.js b/packages/postgres/migrations/1792187770930_add-key-shares.js index 84a68e486bb..6b2953e3049 100644 --- a/packages/postgres/migrations/1792187770930_add-key-shares.js +++ b/packages/postgres/migrations/1792187770930_add-key-shares.js @@ -43,12 +43,16 @@ // // A REVOKE removes only the grants its grantor made, so `up` revokes each // SELECT grant those roles hold on a table or its columns acting as the role -// that made it, with CASCADE, which also removes every grant passed on from -// it. Acting as another role needs the migrating role to be a superuser or able -// to SET ROLE to it; where it isn't, `up` stops and names the grant rather than -// leave the table readable. SELECT granted to PUBLIC, or to a role outside the -// read-only role that one of its readers also belongs to, still lets them read, -// and revoking it isn't a migration's call, so `up` then asks +// that made it, with CASCADE, which also removes the grants passed on from it +// at the same level. CASCADE on a table grant leaves the column grants passed +// on from it, which nobody could then revoke, so column grants go first. +// Within each level, grants a table's owner made go first, so a grant one +// reader passed on to another goes with the grant it came from. Acting as +// another role needs the migrating role to be a superuser or able to SET ROLE +// to it; where it isn't, `up` stops and names the grant rather than leave the +// table readable. SELECT granted to PUBLIC, or to a role outside the read-only +// role that one of its readers also belongs to, still lets them read, and +// revoking it isn't a migration's call, so `up` then asks // `has_column_privilege` about every column of both tables for each reader and // stops, naming the role and the column, if any of them can still read one. // @@ -287,12 +291,12 @@ exports.up = (pgm) => { held.grantee INTO grantor_name, grantee_name, revoked_grantor, revoked_grantee FROM ( - SELECT tbl.relowner, acl.* + SELECT tbl.relowner, true AS on_table, acl.* FROM pg_class tbl CROSS JOIN LATERAL aclexplode(tbl.relacl) acl WHERE tbl.oid = table_name::regclass UNION ALL - SELECT tbl.relowner, acl.* + SELECT tbl.relowner, false AS on_table, acl.* FROM pg_class tbl JOIN pg_attribute col ON col.attrelid = tbl.oid CROSS JOIN LATERAL aclexplode(col.attacl) acl @@ -315,6 +319,13 @@ exports.up = (pgm) => { ) SELECT oid FROM reader ) + -- Column grants come before table grants: CASCADE on a table + -- grant leaves the column grants passed on from it, and once + -- their grantor has lost the table grant, nobody can revoke + -- them. Within each, grants the owner made come first, so + -- CASCADE takes what a reader passed on from one without acting + -- as that reader. + ORDER BY held.on_table, held.grantor <> held.relowner LIMIT 1; EXIT WHEN NOT FOUND; IF grantor_name IS NOT NULL THEN