Skip to content

Commit 81bd9fa

Browse files
fix(plugin-security)!: a position assignment or permission-set grant scoped to an organization must name a member of it (#22275)
Fixes #22226 Clause-②: no (narrowing) Executes the maintainer's ruling A on objectstack-ai/cloud#1765 (`6053780796`) as #22226 states it: a non-system insert or update of `sys_user_position` whose `user_id` has no `sys_member` row in the row's `organization_id` is refused, for every caller, platform administrators included, with the row's existing `400 VALIDATION_FAILED` envelope (`fields[].code: reference_not_found`), no new error code; system writes stand down. The sibling junction `sys_user_permission_set` measured as the same class and takes the same predicate. Not taken: B (narrowing the picker) and C (accepting the row). ## What changes - **`packages/plugins/plugin-security/src/grant-holder-membership-refusal.ts`** (new, beside `position-catalog-refusal.ts`): `registerGrantHolderMembershipRefusal` binds a `beforeInsert` and a `beforeUpdate` engine hook on `sys_user_position` and `sys_user_permission_set`. - **Insert** (one row or a batch): judges the organization the row is STORED with. That is the row's own non-empty `organization_id`, or else the caller's active organization, which the driver writes into the empty slot after the hooks run. `organizationTheInsertStores` mirrors that stamp: `buildDriverOptions` in `@objectstack/objectql` and `injectTenantOnInsert` in `@objectstack/driver-sql`. - **Update** (by id and by predicate): the engine hands each matched row's stored image to the hook as `previous`. The post-image pair (`user_id`, `organization_id`) is judged only when it differs from the stored pair. An edit that leaves both alone, such as end-dating the row of a holder who has since left, is not judged. - **Membership read:** `sys_member` by (`user_id`, `organization_id`) under `{ isSystem: true, tenantId }`, with `tenantId` set to the row's own organization. It joins the writer's transaction. A placeholder-shaped identifier is compared literally. - **Stand-downs:** every `isSystem` write; a row stored with no organization (a global grant names no organization to be a member of); a `user_id` that is not an id (the engine answers `required` itself); and a composition that registers no `sys_member` object. - **Failure:** a membership read that throws propagates and the write is refused. A security refusal that cannot read its input does not admit the write. - **Order:** priority 40. That is after the authority/standing guards (10, 20) and the grant-name derivation (30). `sys_user_position.position` is judged by the catalog refusal middleware, ahead of every hook. So on both tables the value a row names is judged before its holder, and a caller the CRUD check or the delegated-admin gate refuses never sees a membership verdict. - **`security-plugin.ts`**: one import plus one registration line, placed beside the catalog refusal's wiring. No other region is touched. - **`.changeset/22226-grant-holder-must-be-organization-member.md`**: `minor`, declared breaking, ADR-0087 `not-required (no-migration-prescription)`. - **`content/docs/permissions/system-context.mdx`**: row 23d plus regenerated counts. The hooks read `isSystem` to stand down, and `check:system-context-census` requires a row for each such read site. - **Sibling suites** (`position-catalog-refusal.test.ts`, `grant-permission-set-name.test.ts`): their harnesses now provision `sys_member`, and the holders of their organization-scoped grants become members. ## Why hooks rather than a branch inside `createPositionCatalogRefusal` (H1) H1 is confirmed on `73a0a6bf`: `createPositionCatalogRefusal` is the middleware on `sys_user_position`, wired once at `security-plugin.ts:4386`. It stands down on `isSystem` and answers the `400` envelope. A membership branch there would see only the payload. A predicate update needs every matched row's stored organization, and the engine hands exactly that to a `beforeUpdate` hook. The sibling table needs the same rule too, so the refusal is a module of its own with one wiring line. ## Before / after, per write case Measured on a real `ObjectQL` engine over SQLite with the real `SecurityPlugin`. The posture is `isolated` unless marked `single`. The holder belongs to another organization only. | Write | Before (`73a0a6bf`, pins red) | After | |:--|:--|:--| | platform administrator inserts `sys_user_position` (stamped organization) | stored | 400 `VALIDATION_FAILED`, `reference_not_found` at `user_id`; row absent | | same insert, holder is a member (positive control) | stored | stored, `organization_id` = the writer's organization | | insert naming the writer's organization explicitly | stored | refused, same envelope | | batch insert, one row a non-member | stored | refused whole; nothing stored | | update by id moving `user_id` to a non-member | landed | refused; stored row unchanged | | predicate update moving `user_id` to a non-member | landed | refused; no matched row changes | | update leaving holder and organization alone, on a non-member's stored row | landed | lands (not judged) | | system write of a non-member | stored | stored (stands down) | | caller the CRUD check refuses | 403 | 403, member or not | | platform administrator grants `sys_user_permission_set` (stamped organization) | stored | refused, same envelope; row absent | | update by id of that grant to a non-member | landed | refused | | `single`, writer with no active organization (row stored with no organization) | stored | stored (not judged) | | `single`, writer's active organization holds the user | stored | stored | | `single`, organization-scoped row naming a non-member | stored | refused | **`single` posture (H2).** Under the default `auto` membership policy, plugin-auth's reconciler binds every created user to the default organization (`reconcile-membership.ts`, ADR-0093 D7), and the one-time backfill binds users who predate it. The platform administrator is bound as owner by `ensureDefaultOrganization`. So on a stock single-organization deployment, every organization-scoped row names a member. Users created under `invite-only` and not yet invited are refused, which is the predicate as ruled. ## Tests At `74f8180344` (merge of `origin/main` `3513ac7781`). Code is byte-identical at head `cfaac2bf9c`, whose last commit touches only the census page. - `pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2`: 180 files passed, 3796 tests passed, 45 skipped. - `src/grant-holder-membership-refusal.test.ts`: 21 tests. 8 were red on the base before the fix (every negative pin: the write landed). All 21 are green after it. - `pnpm --filter @objectstack/plugin-security typecheck` at `cfaac2bf9c`: exit 0, including `check:test-typecheck: OK`. - Downstream consumers, measured at `2fbc0ba585`: - `@objectstack/plugin-auth`: 128 files, 2637 passed. - `@objectstack/organizations`: 11 files, 147 passed. Both suites alias `plugin-security` to source. - `@objectstack/dogfood`: the 39 test files that touch these two tables, 369 passed against a rebuilt `plugin-security` dist. ## Ablation Each leg is a mutation of the committed module through `scripts/ablation-replace.mjs`. The anchor hits 1 and then 0, the marker shows 1 on disk, and the blob changes. Each leg is followed by a restore to the HEAD blob `37dea505`, with `git diff HEAD` empty. The suite imports the module from source, so no build sits in the path. - **Leg 1: the refusal's throw disabled.** Exactly the 8 negative pins turn red; 13 stay green. - **Leg 2: the insert-stamp mirror disabled** (only a row's own organization is judged). The 5 pins that rely on the stamped organization turn red: platform-admin insert, batch, permission-set insert, `single` predicate, and fail-closed read. The explicit-organization pin stays green, as predicted. ## Gates `node scripts/pm/dispatch-gates.mjs` derived 96 families at `cfaac2bf9c`. All 96 were run, and the reconciliation via `--ran` reports `96 run, 0 NOT-MEASURED (a DERIVED zero)`. - `check-changeset-no-major` and `check-adr-0087-registration` are both green. - `check:dual-build-cjs-loads` is green after building the packages whose `dist/` was missing. - `check:changeset-gate-self-tests` failed once on the container's commit-signing service (a 503 inside its throwaway repositories), then passed on rerun. Lint was a proven narrowing, not a repo-wide run. `eslint --no-inline-config --format json` over the 5 touched `.ts` files reports 5 files, 0 errors, 0 warnings. All 5 are in the population of `eslint.config.mjs` (`files: **/*.{ts,…}`, and `--print-config` resolves). The config enables no type-aware linting (no `parserOptions.project`), so this diff cannot move the verdict on any untouched file. ## Acceptance notes - `content/docs/permissions/system-context.mdx` is outside the claim's enumerated file surface. `check:system-context-census` requires it, because the stand-down for system writes is two new `isSystem` read sites. - The hooks are not unbound in `SecurityPlugin.destroy()`. Doing so would be a second edit region in `security-plugin.ts`. Instead, registration first unbinds its own `packageId`, so a re-run `start()` replaces the binding rather than doubling it. That registration lifecycle matches the engine middlewares this plugin registers. - Membership is read fail-closed. The sibling catalog refusal fails open; the sibling grant-name hook fails closed. This refusal follows the grant-name hook because it is a security refusal. - A row stored with no organization is outside this ruling's predicate. An adjacent class, re-scoping an existing organization-scoped grant, is reported to the seat for triage and is not changed here. --- _Generated by [Claude Code](https://claude.ai/code/session_01WkL6Eijt432S1Y7ekb6ovQ)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent fbcbcf1 commit 81bd9fa

7 files changed

Lines changed: 881 additions & 11 deletions

File tree

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
---
2+
'@objectstack/plugin-security': minor
3+
---
4+
5+
fix(plugin-security)!: a position assignment or permission-set grant scoped to an organization must name a member of that organization
6+
7+
Clause-②: no (narrowing)
8+
9+
<!-- adr-0087: not-required (no-migration-prescription) No metadata moves: no spec key, authorable spelling, export or stored shape is removed, renamed or re-shaped, and no stored row is read, rewritten, converted or dropped, so there is nothing for `objectstack migrate meta` to rewrite. What narrows is a runtime write door: a non-system insert or update of a sys_user_position or sys_user_permission_set row whose user holds no sys_member row in the row's organization is refused with the existing 400 VALIDATION_FAILED envelope. The other categories are closed on facts: the one bumped package publishes (not unpublished); no ADR-0087 id covers these paths and this diff adds none (not registered / already-registered); and nothing exported is removed or narrowed, so it is neither runtime-interface-only nor type-surface-only. -->
10+
11+
**BREAKING** (an accept-set narrowing), shipped as `minor` under the launch-window convention for breaking changes.
12+
13+
**What stops being accepted.** A `sys_user_position` or `sys_user_permission_set` row whose `organization_id` is set may name only a user who holds a `sys_member` row in that organization. A non-system insert or update that would store any other user is refused with `400 VALIDATION_FAILED`, one `fields[]` entry at `user_id` with `code: reference_not_found` — the envelope a `user_id` naming no user already receives. No new error code. It applies to every non-system caller, platform administrators included, and to every engine write: `POST` / `PATCH /api/v1/data/...`, batch inserts, predicate updates, and scripts and flows that run as a user or a service principal. The organization judged is the one the row is stored with: its own `organization_id`, or, when it names none, the caller's active organization stamped at write time.
14+
15+
**What stays accepted.**
16+
17+
- System-context writes: seed replay, invitation acceptance, the organization-admin reconcile and the platform bootstraps.
18+
- A row stored with no organization: a global grant names no organization to be a member of.
19+
- An update that changes neither `user_id` nor `organization_id`, so a stored row whose holder has since left the organization stays editable and can be end-dated.
20+
- A stock `single`-posture deployment: under the default `auto` membership policy every user is a member of the default organization, so every assignment there names a member.
21+
22+
**What changes for you.** Add the user to the organization first, then assign the position or grant the permission set. A script that assigned a grant before the user joined now receives the `400` above; reorder it.

‎content/docs/permissions/system-context.mdx‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ the seed loader replaying package fixtures, a plugin's boot reconciler, a
1010
service self-write, a migration.
1111

1212
This page is **the authority** for what that flag actually does. It exists
13-
because the flag is not one concept: it is a single boolean read at **115
13+
because the flag is not one concept: it is a single boolean read at **117
1414
distinct sites across 19 packages**, and knowing three of those behaviours gives
1515
no hint that the other hundred-and-four exist. Every documented app-side bug
1616
traced to `isSystem` had the same shape — the metadata was complete and correct,
@@ -128,6 +128,7 @@ that silently does not happen.
128128
| 23 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `packages/objectql/src/engine.ts#assertReferencesResolve` |
129129
| 23b | **Position-catalog check skipped** — a `sys_user_position` write whose `position` names no `sys_position` row is not refused | plugin-security | Get: a system writer can store an assignment naming no catalog row — the seed loader writes `stack.data` from `AppPlugin.start()`, before `kernel:ready` seeds the declared position catalog, so a refusal there would fail every authored assignment seed on a fresh boot. Lose: such a row grants nothing and nothing says so. A non-system insert, or a non-system update that changes `position`, is refused `400 VALIDATION_FAILED` / `reference_not_found` instead when no catalog row the writer's context reaches carries the name: its organization's rows plus the organization-less ones, read as `{ ...context, isSystem: true }`, never a bare `{ isSystem: true }`, so a name only another organization carries is refused like one nobody carries. It is the same stand-down, and the same tenant scope, as the engine's referential-integrity check for a lookup column | `packages/plugins/plugin-security/src/position-catalog-refusal.ts#assertPositionNamesCatalogRow` |
130130
| 23c | **Grant-name check relaxed for an unresolvable id** — a `sys_user_permission_set` write that supplies `permission_set` beside a `permission_set_id` naming no set the writer's context reaches keeps the supplied name | plugin-security | Get: a system writer can store a grant naming a set whose row does not exist yet — seed replay and boot provisioning may write the grant before the set row, the ordering the engine's referential-integrity check also stands down for. Lose: such a row's name is unchecked until a backfill names it. Everything else is judged for every caller, system included: the name is derived from the id on every write that carries one, and a supplied name that names any other set is refused `400 VALIDATION_FAILED` / `invalid_value`; a non-system caller's name beside an unresolvable id is refused the same way, read as `{ ...context, isSystem: true }` through the hook's `ctx.api.sudo()`, never a bare `{ isSystem: true }` (ADR-0131 D4) | `packages/plugins/plugin-security/src/grant-permission-set-name.ts#settle` |
131+
| 23d | **Grant-holder membership check skipped** — a `sys_user_position` or `sys_user_permission_set` write naming a user with no `sys_member` row in the row's organization is not refused | plugin-security | Get: a system writer can store such a grant — invitation acceptance writes the membership and its placement together, and seed replay, the organization-admin reconcile and the platform bootstraps write grants as the system. Lose: such a row takes effect once its user is admitted to the organization. A non-system insert, or a non-system update that moves `user_id` or `organization_id`, is refused `400 VALIDATION_FAILED` / `reference_not_found` at `user_id` instead, for every caller, a platform administrator included; the membership is read as `{ isSystem: true, tenantId }` with `tenantId` the row's own organization, never another one. A row stored with no organization is a global grant and is not judged | `packages/plugins/plugin-security/src/grant-holder-membership-refusal.ts#onInsert`, `#onUpdate` |
131132
| 24 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `packages/objectql/src/engine.ts#buildDriverOptions` |
132133
| 25 | Engine-owned / append-only write guard bypassed | plugin-security | Get: generic writes to `managedBy` engine-owned objects | `packages/plugins/plugin-security/src/system-write-guard.ts#isUserContextWrite`, `#assertEngineOwnedWriteAllowed` |
133134
| 26 | Identity write guard bypassed (ADR-0092) | plugin-auth | Get: direct writes to identity tables through the generic data path | `packages/plugins/plugin-auth/src/identity-write-guard.ts#isUserContextWrite` |
@@ -139,7 +140,7 @@ that silently does not happen.
139140

140141
### 3. Sharing (`plugin-sharing`)
141142

142-
The largest single consumer — **17 of the 115 sites**.
143+
The largest single consumer — **17 of the 117 sites**.
143144

144145
| # | Behaviour when `isSystem` | What you get / what you lose | Anchor |
145146
|:--|:---|:---|:---|
@@ -282,7 +283,7 @@ Ownership injection, `readonly` bypass and sharing materialisation are
282283
independent decisions, and a seed loader plausibly wants the first two but not
283284
the third. The concept is nevertheless **staying as one boolean**:
284285

285-
- **Shipped semantics.** `isSystem` is a published contract with 115 read sites
286+
- **Shipped semantics.** `isSystem` is a published contract with 117 read sites
286287
in 19 packages. Splitting it is a breaking contract change across all of them.
287288
(The ruling was taken when the census read 80 sites in 18 packages; the count
288289
has grown, which strengthens rather than weakens the argument.)
@@ -356,16 +357,16 @@ still holds equal to the census on every pull request:
356357
| Appearances of the bare identifier `isSystem` in non-test sources | 813 | — |
357358
| — parsed as a declaration | 27 | ✅ |
358359
| — parsed as an object-literal / type key (producers and option objects) | 310 | — |
359-
| — parsed as a property **read** | 121 | ✅ |
360+
| — parsed as a property **read** | 123 | ✅ |
360361
| — parsed in some other syntactic position (a local, a cast, a conditional) | 9 | ✅ |
361362
| — the remainder: text inside comments and string literals | 358 | — |
362363
| Of those reads: reads of one of the unrelated metadata fields | 6 | ✅ |
363-
| Of those reads: reads of `ExecutionContext.isSystem` | **115** | ✅ |
364-
| — behaviour-bearing (rows 1–61 above) | 112 | ✅ |
364+
| Of those reads: reads of `ExecutionContext.isSystem` | **117** | ✅ |
365+
| — behaviour-bearing (rows 1–61 above) | 114 | ✅ |
365366
| — carry the flag onward only (rows 62–64 above) | 3 | ✅ |
366367
| Packages containing at least one elevation read | **19** | ✅ |
367-
| Files containing at least one elevation read | 53 | ✅ |
368-
| — the distinct symbols those reads live in — what this page anchors | 98 | ✅ |
368+
| Files containing at least one elevation read | 54 | ✅ |
369+
| — the distinct symbols those reads live in — what this page anchors | 100 | ✅ |
369370
| — of those files, the ones holding more than one read in one symbol | 8 | ✅ |
370371

371372
The six rows marked — are a **dated decomposition, not a live claim**: they were
@@ -429,7 +430,7 @@ same resolver, and the same registration shape, that holds `docs/adr/**`.
429430
Renaming a symbol is now a loud red instead of a silent misdirection.
430431

431432
⚠️ **The precision that costs, priced here rather than buried.** A symbol anchor
432-
cannot say WHICH read inside a function it means, and **8** of the **53**
433+
cannot say WHICH read inside a function it means, and **8** of the **54**
433434
anchored files hold more than one read inside a single symbol. So the population
434435
check runs per file at symbol granularity: every file the census finds a read in
435436
must be anchored, and the set of symbols this page cites into that file must

0 commit comments

Comments
 (0)