From f51e4bc02f2f214acca04b3804661859db338ec1 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 13:16:14 +0000 Subject: [PATCH 1/2] fix(plugin-security): resolve the org-admin permission set per organization (#11670) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `sys_permission_set` row every auto-provisioned org-admin grant points at was resolved by name alone — no `organization_id` predicate, `limit: 1`, and cached per ObjectQL instance on the name alone. Post-#10103 one name carries a row per organization plus the organization-less platform-bucket row, which is the oldest of them, so a walled deployment could point grants at a row belonging to no organization. The read is now threaded with the granting organization and resolved through `resolveOwnOrganizationRow`, with the cache keyed on `(organization, name)`. `single` is carved out and unchanged. With no own row the resolver refuses loudly rather than falling back to the organization-less row. Revocation is widened in the same change so the narrowing is not a loosening: the superseded, demotion and orphan-sweep legs match every copy of the set name in every posture. The grant target is posture-scoped; the revoke reach is not. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs --- ...scope-org-admin-permission-set-resolver.md | 52 ++ .../src/auto-org-admin-grant.test.ts | 447 ++++++++++++++++-- .../src/auto-org-admin-grant.ts | 388 +++++++++++++-- 3 files changed, 831 insertions(+), 56 deletions(-) create mode 100644 .changeset/scope-org-admin-permission-set-resolver.md diff --git a/.changeset/scope-org-admin-permission-set-resolver.md b/.changeset/scope-org-admin-permission-set-resolver.md new file mode 100644 index 0000000000..ebd04b58da --- /dev/null +++ b/.changeset/scope-org-admin-permission-set-resolver.md @@ -0,0 +1,52 @@ +--- +"@objectstack/plugin-security": patch +--- + +fix(plugin-security): resolve the org-admin permission set per organization, and keep the revoke reach wide (#11670) + +`auto-org-admin-grant.ts` resolved the `sys_permission_set` row that every +auto-provisioned org-admin grant points at by NAME alone: no `organization_id` +predicate, `limit: 1`, and cached per ObjectQL instance on the name alone. Each +property reads as deliberate; together they answer with a row nobody chose. + +Post-#10103 the RBAC catalog is materialized per organization and +`sys_permission_set.name` is unique per organization (ADR-0120 D3), so one name +carries a row per organization PLUS the organization-less platform-bucket row +`bootstrapPlatformAdmin` mints on every boot — and that bucket row is the +OLDEST bearing the name (measured on a fresh walled rig at 1.3 s ahead of the +first `sys_organization`, #11532). An unscoped `limit: 1` read has no reason to +prefer any other, and a per-instance cache keyed on the name made the first +organization reconciled in a process pick the row every later organization got. +The grant target is a foreign key, so on a walled deployment +`sys_user_permission_set` rows granting `organization_admin` could point at a +row belonging to no organization. + +**Walled postures only.** The read is now threaded with the granting +organization — through `SqlDriver.applyTenantScope`, resolved by +`resolveOwnOrganizationRow`, the catalog's own spelling of "which row is this +organization's" — and the cache is keyed on `(organization, name)`. `single` +keeps the unscoped answer and the unscoped `limit: 1` grant-target read +unchanged; no read on a `single` path carries a `tenantId`. + +**When the organization has no own row**, the resolver returns `null` (the +module's existing `skipped` / `permission_set_missing` no-op) and warns loudly, +rather than falling back to the organization-less row: a fallback would keep +minting grants at the platform bucket, and the second one would never be +repaired — once the organization's own row appeared the reconciler would insert +a duplicate beside it. + +**The revoke reach widened in the same change, deliberately.** Narrowing the +grant target without it would be a permission loosening: a demoted admin whose +grant predates this fix names the organization-less row, and the ADR-0105 D4 F2 +close-out (a deployment that drops its wall must not leave the unbounded +`organization_admin` grant standing) converges across copies written under the +other posture. Revocation therefore matches EVERY copy of the set name, in every +posture — the per-pair superseded and demotion legs and the backfill's orphan +sweep alike. The grant target is posture-scoped; the revoke reach never is. + +⛔ No repair of existing rows is claimed or performed. This makes new +resolutions correct; grants already pointing at the organization-less row are +left exactly as they are, including the duplicate that appears beside one when +its holder still qualifies. Accept/reject is unchanged today — `resolve-authz-context` +resolves permission sets by id without tenant scoping, which is why the defect +was invisible — and no published surface changes. diff --git a/packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts b/packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts index 8c14774639..de077e7ba8 100644 --- a/packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts +++ b/packages/plugins/plugin-security/src/auto-org-admin-grant.test.ts @@ -91,6 +91,27 @@ function assertSystemContext(operation: string, object: string, context: any): v } } +/** + * [#11670] `SqlDriver.applyTenantScope`, as much of it as this module can + * observe. + * + * A scoped read returns the caller's rows AND organization-less ones — that + * compatibility arm is exactly why "the first row wins" stopped being right for + * a catalog read, so a double without it would answer the scoped question the + * unscoped way and prove nothing about the repair. It also makes `limit` mean + * what it means in production: a scoped page holds this organization's rows + * plus the platform bucket, not every tenant's copy. + * + * Absent `tenantId` is the unscoped system read (`SYSTEM_CTX`), which sees + * everything — the question every other read in this module asks. + */ +function tenantVisible(row: any, context: any): boolean { + const tenantId = context?.tenantId; + if (typeof tenantId !== 'string' || tenantId === '') return true; + const owner = row?.organization_id ?? null; + return owner === tenantId || owner === null; +} + /** * Tiny in-memory ObjectQL double: just enough surface for the reconciler * (find / insert / delete), with the engine's call shapes ENFORCED. @@ -107,6 +128,12 @@ function makeStub(seed: { }; /** Every delete the module issued, as the engine received it. */ const deleteCalls: Array<{ object: string; options: any }> = []; + /** + * [#11670] Every read the module issued, as the engine received it — the + * `single`-posture invariance pin measures this multiset, and a query is the + * only place a leak of the walled scoping into `single` could show up. + */ + const findCalls: Array<{ object: string; where: any; limit: any; context: any }> = []; const matches = (row: any, where: any) => { for (const [k, v] of Object.entries(where ?? {})) { @@ -124,13 +151,18 @@ function makeStub(seed: { return { tables, deleteCalls, + findCalls, // find(object, query, options) — `where`/`limit` in the query, execution // context in either bag (`options.context` wins, as in the engine). async find(object: string, query?: any, options?: any) { assertOptionBag('find', object, query, FIND_QUERY_KEYS); assertOptionBag('find', object, options, TRAILING_OPTION_KEYS); - assertSystemContext('find', object, options?.context ?? query?.context); - const rows = (tables[object] ?? []).filter((r) => matches(r, query?.where)); + const context = options?.context ?? query?.context; + assertSystemContext('find', object, context); + findCalls.push({ object, where: query?.where, limit: query?.limit, context }); + const rows = (tables[object] ?? []) + .filter((r) => tenantVisible(r, context)) + .filter((r) => matches(r, query?.where)); return typeof query?.limit === 'number' ? rows.slice(0, query.limit) : rows; }, // insert(object, data, options) — context in the TRAILING bag. @@ -168,9 +200,39 @@ function makeStub(seed: { }; } +// --------------------------------------------------------------------------- +// [#11670] What `sys_permission_set` actually holds for these two names. +// +// It is not one row each, and the fixtures used to say it was — which is why +// every walled case in this file described a deployment shape that stopped +// existing at #10103. Post-#10103 the catalog is materialized PER ORGANIZATION +// and the name is unique per organization (ADR-0120 D3), so one name carries: +// +// - the ORGANIZATION-LESS platform-bucket row, minted by +// `bootstrapPlatformAdmin` on every boot and ruled to stay (2026-08-20). +// On a fresh walled rig it is written 1.3 s BEFORE the first +// `sys_organization` exists (#11532), so it is also the OLDEST row bearing +// the name — the one a name-only `limit: 1` read has every reason to +// return; +// - one row per organization, created by the per-organization catalog +// seeding on organization creation and on every boot sweep. +// +// The `single` carve-out keeps the bucket rows FIRST, because `single` reads +// unscoped with `limit: 1` and the first row is the row — that ordering is part +// of what the invariance pin measures. +// --------------------------------------------------------------------------- const ORG_ADMIN_SET = { id: 'ps_org_admin', name: 'organization_admin' }; // [ADR-0105 D4] The wall-less variant a `single`-posture deployment grants instead. const ORG_ADMIN_NO_BYPASS_SET = { id: 'ps_org_admin_nb', name: 'organization_admin_no_bypass' }; +/** The organization-less platform bucket — both names, no `organization_id`. */ +const PLATFORM_BUCKET = [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET]; +/** One organization's own catalog copies of both names. */ +const ownSets = (org: string) => [ + { id: `ps_org_admin_${org}`, name: 'organization_admin', organization_id: org }, + { id: `ps_org_admin_nb_${org}`, name: 'organization_admin_no_bypass', organization_id: org }, +]; +/** The bucket plus each named organization's own copies, bucket first. */ +const catalogFor = (...orgs: string[]) => [...PLATFORM_BUCKET, ...orgs.flatMap(ownSets)]; // Every pre-ADR-0105 case in this file exercised the WALLED behavior (the only // behavior that existed), so they pin `isolated` explicitly. The wall-less @@ -182,7 +244,7 @@ describe('reconcileOrgAdminGrant', () => { beforeEach(() => { stub = makeStub({ - sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_permission_set: catalogFor('o1'), sys_member: [], sys_user_permission_set: [], }); @@ -195,7 +257,9 @@ describe('reconcileOrgAdminGrant', () => { expect(stub.tables.sys_user_permission_set).toHaveLength(1); const row = stub.tables.sys_user_permission_set[0]; expect(row.organization_id).toBe('o1'); - expect(row.permission_set_id).toBe('ps_org_admin'); + // [#11670] o1's OWN copy of the set — not the organization-less bucket row + // (`ps_org_admin`) that an unscoped, name-only, `limit: 1` read returns. + expect(row.permission_set_id).toBe('ps_org_admin_o1'); }); it('grants when membership role is "admin"', async () => { @@ -275,19 +339,22 @@ describe('reconcileOrgAdminGrant', () => { describe('backfillOrgAdminGrants', () => { it('grants for every owner/admin membership and revokes orphans', async () => { const stub = makeStub({ - sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_permission_set: catalogFor('o1', 'o2'), sys_member: [ { id: 'm1', user_id: 'u1', organization_id: 'o1', role: 'owner' }, { id: 'm2', user_id: 'u2', organization_id: 'o1', role: 'admin' }, { id: 'm3', user_id: 'u3', organization_id: 'o1', role: 'member' }, ], sys_user_permission_set: [ - // Orphan grant — no matching membership in o2. + // Orphan grant — no matching membership in o2. [#11670] It points at + // O2'S OWN copy, which is what a post-repair grant looks like; the + // sweep's set ids are resolved installation-wide precisely so a grant + // written for another organization stays reachable. { id: 'ups_orphan', user_id: 'u4', organization_id: 'o2', - permission_set_id: 'ps_org_admin', + permission_set_id: 'ps_org_admin_o2', }, ], }); @@ -316,7 +383,7 @@ describe('backfillOrgAdminGrants', () => { describe('[ADR-0105 D4] posture selects the org-admin variant', () => { const seedBoth = () => makeStub({ - sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_permission_set: catalogFor('o1'), sys_member: [{ id: 'm1', user_id: 'u1', organization_id: 'o1', role: 'owner' }], sys_user_permission_set: [], }); @@ -333,13 +400,13 @@ describe('[ADR-0105 D4] posture selects the org-admin variant', () => { const stub = seedBoth(); const res = await reconcileOrgAdminGrant(stub, 'u1', 'o1', { posture: 'isolated' }); expect(res.action).toBe('granted'); - expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin'); + expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin_o1'); }); it('grants the full set under `group` (the union wall bounds them too)', async () => { const stub = seedBoth(); await reconcileOrgAdminGrant(stub, 'u1', 'o1', { posture: 'group' }); - expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin'); + expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin_o1'); }); it('defaults to the de-VAMA\'d variant when no posture is supplied (fail safe)', async () => { @@ -353,8 +420,14 @@ describe('[ADR-0105 D4] posture selects the org-admin variant', () => { it('revokes the superseded variant when the posture changes', async () => { const stub = seedBoth(); await reconcileOrgAdminGrant(stub, 'u1', 'o1', { posture: 'isolated' }); - expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin'); + expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin_o1'); + // [#11670] The flip crosses COPIES: the standing grant names o1's own row, + // the `single` pass resolves the organization-less one. Convergence + // therefore depends on the revoke matching every copy of the superseded + // name — a revoke narrowed to the posture's own copy converges on nothing + // and leaves the unbounded bits in force, which is the F2 outcome this test + // is the close-out for. await reconcileOrgAdminGrant(stub, 'u1', 'o1', { posture: 'single' }); expect(stub.tables.sys_user_permission_set).toHaveLength(1); expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin_nb'); @@ -362,12 +435,12 @@ describe('[ADR-0105 D4] posture selects the org-admin variant', () => { // ...and back again. await reconcileOrgAdminGrant(stub, 'u1', 'o1', { posture: 'isolated' }); expect(stub.tables.sys_user_permission_set).toHaveLength(1); - expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin'); + expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin_o1'); }); it('backfill converges every pair onto the posture\'s variant', async () => { const stub = makeStub({ - sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_permission_set: catalogFor('o1'), sys_member: [ { id: 'm1', user_id: 'u1', organization_id: 'o1', role: 'owner' }, { id: 'm2', user_id: 'u2', organization_id: 'o1', role: 'admin' }, @@ -400,7 +473,7 @@ describe('[ADR-0105 D4] posture selects the org-admin variant', () => { describe('[#12699] suppressUnboundedOrgAdminGrant', () => { const seedBoth = () => makeStub({ - sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_permission_set: catalogFor('o1'), sys_member: [{ id: 'm1', user_id: 'u1', organization_id: 'o1', role: 'owner' }], sys_user_permission_set: [], }); @@ -413,7 +486,7 @@ describe('[#12699] suppressUnboundedOrgAdminGrant', () => { }); expect(res.action).toBe('granted'); expect(stub.tables.sys_user_permission_set).toHaveLength(1); - expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin_nb'); + expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin_nb_o1'); }); it('suppression ON: `group` grants the de-VAMA\'d variant too', async () => { @@ -422,7 +495,7 @@ describe('[#12699] suppressUnboundedOrgAdminGrant', () => { posture: 'group', suppressUnboundedOrgAdminGrant: true, }); - expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin_nb'); + expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin_nb_o1'); }); it('suppression OFF (explicit false) is byte-identical to today: `isolated` grants the full set', async () => { @@ -431,37 +504,43 @@ describe('[#12699] suppressUnboundedOrgAdminGrant', () => { posture: 'isolated', suppressUnboundedOrgAdminGrant: false, }); - expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin'); + expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin_o1'); }); it('turning suppression on REVOKES a standing unbounded grant (superseded-variant convergence)', async () => { const stub = seedBoth(); await reconcileOrgAdminGrant(stub, 'u1', 'o1', { posture: 'isolated' }); - expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin'); + expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin_o1'); await reconcileOrgAdminGrant(stub, 'u1', 'o1', { posture: 'isolated', suppressUnboundedOrgAdminGrant: true, }); expect(stub.tables.sys_user_permission_set).toHaveLength(1); - expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin_nb'); + expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin_nb_o1'); // ...and a deployment that withdraws the declaration converges back — // the fail-closed default protects any deployment RELYING on the auto-grant. await reconcileOrgAdminGrant(stub, 'u1', 'o1', { posture: 'isolated' }); expect(stub.tables.sys_user_permission_set).toHaveLength(1); - expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin'); + expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin_o1'); }); it('backfill threads the suppression to every pair AND the orphan sweep', async () => { const stub = makeStub({ - sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_permission_set: catalogFor('o1'), sys_member: [ { id: 'm1', user_id: 'u1', organization_id: 'o1', role: 'owner' }, { id: 'm2', user_id: 'u2', organization_id: 'o1', role: 'admin' }, ], // Pre-existing unbounded grants from a pre-suppression walled boot, plus // one orphan (no membership row) that only the sweep can reach. + // + // [#11670] They point at the ORGANIZATION-LESS row, which is what every + // walled grant written before this repair looks like. Converging them is + // a revoke, so it stays reachable: the sweep and the per-pair revoke both + // match every copy of the name. (⛔ Nothing re-points them — a row still + // held by someone who qualifies is left exactly as it is.) sys_user_permission_set: [ { id: 'ups1', user_id: 'u1', organization_id: 'o1', permission_set_id: 'ps_org_admin' }, { id: 'ups2', user_id: 'u2', organization_id: 'o1', permission_set_id: 'ps_org_admin' }, @@ -476,7 +555,7 @@ describe('[#12699] suppressUnboundedOrgAdminGrant', () => { const grants = stub.tables.sys_user_permission_set; expect(grants).toHaveLength(2); - expect(grants.every((g) => g.permission_set_id === 'ps_org_admin_nb')).toBe(true); + expect(grants.every((g) => g.permission_set_id === 'ps_org_admin_nb_o1')).toBe(true); expect(grants.some((g) => g.user_id === 'u9')).toBe(false); }); }); @@ -494,7 +573,7 @@ describe('[#12699] suppressUnboundedOrgAdminGrant', () => { describe('[#4586] the auto-grant records its provenance', () => { const seed = () => makeStub({ - sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_permission_set: catalogFor('o1'), sys_member: [{ id: 'mem_42', user_id: 'u1', organization_id: 'o1', role: 'admin' }], sys_user_permission_set: [], }); @@ -537,7 +616,7 @@ describe('[#4586] the auto-grant records its provenance', () => { // The threaded human is attribution, not authority: a member-grade row does // not become grantable because an admin triggered the write. const stub = makeStub({ - sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_permission_set: catalogFor('o1'), sys_member: [{ id: 'mem_9', user_id: 'u1', organization_id: 'o1', role: 'member' }], sys_user_permission_set: [], }); @@ -567,7 +646,7 @@ describe('[#4586] the auto-grant records its provenance', () => { it('the backfill grants with no human — it is machine-originated by construction', async () => { const stub = makeStub({ - sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_permission_set: catalogFor('o1'), sys_member: [{ id: 'mem_7', user_id: 'u1', organization_id: 'o1', role: 'owner' }], sys_user_permission_set: [], }); @@ -590,7 +669,7 @@ describe('[#4586] the auto-grant records its provenance', () => { describe('[#4640] revoke speaks the engine\'s delete signature', () => { const seedDemoted = () => makeStub({ - sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_permission_set: catalogFor('o1'), sys_member: [{ id: 'm1', user_id: 'u1', organization_id: 'o1', role: 'member' }], sys_user_permission_set: [ { id: 'ups1', user_id: 'u1', organization_id: 'o1', permission_set_id: 'ps_org_admin' }, @@ -647,7 +726,7 @@ describe('[#4640] revoke speaks the engine\'s delete signature', () => { // `noop` and `skipped/delete_failed` are different facts about the // platform's state; collapsing them is how the failure hid. const stub = makeStub({ - sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_permission_set: catalogFor('o1'), sys_member: [{ id: 'm1', user_id: 'u1', organization_id: 'o1', role: 'member' }], sys_user_permission_set: [], }); @@ -659,7 +738,7 @@ describe('[#4640] revoke speaks the engine\'s delete signature', () => { it('membership removal revokes through the same channel', async () => { // The `sys_member` delete path: no membership row at all, grant still there. const stub = makeStub({ - sys_permission_set: [ORG_ADMIN_SET, ORG_ADMIN_NO_BYPASS_SET], + sys_permission_set: catalogFor('o1'), sys_member: [], sys_user_permission_set: [ { id: 'ups1', user_id: 'u1', organization_id: 'o1', permission_set_id: 'ps_org_admin' }, @@ -671,3 +750,315 @@ describe('[#4640] revoke speaks the engine\'s delete signature', () => { expect(stub.deleteCalls[0].options.where).toEqual({ id: 'ups1' }); }); }); + +// --------------------------------------------------------------------------- +// [#11670] The grant target is resolved PER ORGANIZATION; the revoke reach is +// not. +// +// The defect was three properties that each read as deliberate and combine into +// an answer nobody chose: the permission-set read was name-only (no +// `organization_id` predicate, not routed through the governed +// `resolveOwnOrganizationRow`), `limit: 1` (whichever row the driver returned +// first WAS the answer), and cached per ObjectQL instance on the NAME alone (so +// the first organization reconciled in a process picked the row every later one +// got). Post-#10103 one name carries a row per organization PLUS the +// organization-less platform-bucket row, and #11532 measured that the +// organization-less row is the OLDEST of them — so the grant, a foreign key, +// pointed at a row belonging to no organization. +// +// Nothing observable broke, which is why it survived: `resolve-authz-context` +// resolves permission sets BY ID without tenant scoping, so the grant still +// evaluated. The pins below are therefore about WHICH ROW, not about whether +// access works. +// --------------------------------------------------------------------------- +describe('[#11670] the org-admin permission set is resolved per organization', () => { + /** Reads of the catalog table, as the engine received them. */ + const catalogReads = (stub: ReturnType) => + stub.findCalls.filter((c) => c.object === 'sys_permission_set'); + + it('grants against THIS organization\'s own row, never the organization-less one', async () => { + const stub = makeStub({ + sys_permission_set: catalogFor('o1'), + sys_member: [{ id: 'm1', user_id: 'u1', organization_id: 'o1', role: 'owner' }], + sys_user_permission_set: [], + }); + const res = await reconcileOrgAdminGrant(stub, 'u1', 'o1', WALLED); + + expect(res.action).toBe('granted'); + expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin_o1'); + // The bucket row is visible to the scoped read through the driver's + // compatibility arm — it is REACHABLE and still not chosen. + expect(stub.tables.sys_permission_set.some((r) => r.id === 'ps_org_admin')).toBe(true); + }); + + it('routes the catalog read through the tenant scope rather than a local predicate', async () => { + const stub = makeStub({ + sys_permission_set: catalogFor('o1'), + sys_member: [{ id: 'm1', user_id: 'u1', organization_id: 'o1', role: 'owner' }], + sys_user_permission_set: [], + }); + await reconcileOrgAdminGrant(stub, 'u1', 'o1', WALLED); + + // The governed spelling: the organization rides the CONTEXT (so the read + // goes through `SqlDriver.applyTenantScope`), never a hand-rolled + // `organization_id` key in `where`. A local predicate would be a second + // implementation of the wall — the shape this repair exists to retire. + const scoped = catalogReads(stub).filter((c) => c.context?.tenantId === 'o1'); + expect(scoped.length).toBeGreaterThan(0); + expect(scoped.every((c) => Object.keys(c.where).length === 1 && 'name' in c.where)).toBe(true); + // …and `limit: 1` is gone from the scoped read: a scoped page holds this + // organization's row AND the organization-less one, so one row would again + // be whichever the driver ordered first. + expect(scoped.every((c) => c.limit > 1)).toBe(true); + }); + + it('two organizations in ONE process resolve to DIFFERENT ids (the cache key)', async () => { + // The property the `name`-only cache made impossible to hold. A test with + // one organization cannot detect it: the first answer of the process was + // the answer for every organization for the rest of the process, and with + // one organization that is indistinguishable from correct. + const stub = makeStub({ + sys_permission_set: catalogFor('o1', 'o2'), + sys_member: [ + { id: 'm1', user_id: 'u1', organization_id: 'o1', role: 'owner' }, + { id: 'm2', user_id: 'u2', organization_id: 'o2', role: 'owner' }, + ], + sys_user_permission_set: [], + }); + + // Same `ql` instance, so the same WeakMap entry — that is the point. + await reconcileOrgAdminGrant(stub, 'u1', 'o1', WALLED); + await reconcileOrgAdminGrant(stub, 'u2', 'o2', WALLED); + + const byUser = Object.fromEntries( + stub.tables.sys_user_permission_set.map((g) => [g.user_id, g.permission_set_id]), + ); + expect(byUser.u1).toBe('ps_org_admin_o1'); + expect(byUser.u2).toBe('ps_org_admin_o2'); + expect(byUser.u1).not.toBe(byUser.u2); + }); + + it('caching still holds WITHIN one organization — the repair keys it, it does not drop it', async () => { + const stub = makeStub({ + sys_permission_set: catalogFor('o1'), + sys_member: [{ id: 'm1', user_id: 'u1', organization_id: 'o1', role: 'owner' }], + sys_user_permission_set: [], + }); + await reconcileOrgAdminGrant(stub, 'u1', 'o1', WALLED); + const first = catalogReads(stub).filter((c) => c.context?.tenantId === 'o1').length; + await reconcileOrgAdminGrant(stub, 'u1', 'o1', WALLED); + const second = catalogReads(stub).filter((c) => c.context?.tenantId === 'o1').length; + // The grant-target resolution is memoized per (organization, name), so the + // second reconcile adds no scoped read for it. + expect(second).toBe(first); + }); + + describe('no own row — the refusal, and the half it does NOT refuse', () => { + const bucketOnly = () => + makeStub({ + // A walled rig whose per-organization catalog seeding has not run (or + // failed): only the platform bucket exists, and it is visible to o3's + // scoped read through the driver's compatibility arm. + sys_permission_set: [...PLATFORM_BUCKET], + sys_member: [], + sys_user_permission_set: [], + }); + + it('REFUSES to grant, loudly, rather than pointing a new grant at the bucket row', async () => { + const stub = bucketOnly(); + stub.tables.sys_member = [{ id: 'm1', user_id: 'u1', organization_id: 'o3', role: 'owner' }]; + const warnings: string[] = []; + const res = await reconcileOrgAdminGrant(stub, 'u1', 'o3', { + ...WALLED, + logger: { warn: (m: string) => warnings.push(m) }, + }); + + expect(res).toEqual({ action: 'skipped', reason: 'permission_set_missing' }); + expect(stub.tables.sys_user_permission_set).toHaveLength(0); + // The refusal reaches an operator, and says what it did NOT do. Silence + // here is the failure mode: the bucket row is visible, so without this an + // operator sees a plausible row and a missing grant with nothing + // connecting them. + expect(warnings.some((m) => m.includes('no org-admin capability can be GRANTED'))).toBe(true); + }); + + it('still REVOKES in that same state — the refusal is one-directional', async () => { + // The half that keeps the repair from being a permission loosening. A + // narrowed grant target must not narrow the revoke: this pair no longer + // qualifies, and its standing grant points at the organization-less row, + // which is what every walled grant written before this repair looks like. + const stub = bucketOnly(); + stub.tables.sys_user_permission_set = [ + { id: 'ups_old', user_id: 'u1', organization_id: 'o3', permission_set_id: 'ps_org_admin' }, + ]; + const res = await reconcileOrgAdminGrant(stub, 'u1', 'o3', WALLED); + + expect(res.action).toBe('revoked'); + expect(stub.tables.sys_user_permission_set).toHaveLength(0); + }); + + it('warns once per (organization, name), not once per membership pair', async () => { + const stub = bucketOnly(); + stub.tables.sys_member = [ + { id: 'm1', user_id: 'u1', organization_id: 'o3', role: 'owner' }, + { id: 'm2', user_id: 'u2', organization_id: 'o3', role: 'admin' }, + ]; + const warnings: string[] = []; + const logger = { warn: (m: string) => warnings.push(m) }; + await reconcileOrgAdminGrant(stub, 'u1', 'o3', { ...WALLED, logger }); + await reconcileOrgAdminGrant(stub, 'u2', 'o3', { ...WALLED, logger }); + + const refusals = warnings.filter((m) => m.includes('no org-admin capability can be GRANTED')); + expect(refusals).toHaveLength(1); + }); + }); + + it('leaves an EXISTING mis-targeted grant exactly as it is (⛔ no repair claimed)', async () => { + // The card's boundary, pinned so a later reader does not mistake the repair + // for a migration: this makes NEW resolutions correct. A row already + // pointing at the organization-less set, held by someone who still + // qualifies, is neither re-pointed nor deleted — counting and repairing + // those is the reap card's census. The visible consequence is a second row, + // and that is the honest state: two grants, both conferring the same + // capability, one of them the census's to deal with. + const stub = makeStub({ + sys_permission_set: catalogFor('o1'), + sys_member: [{ id: 'm1', user_id: 'u1', organization_id: 'o1', role: 'owner' }], + sys_user_permission_set: [ + { id: 'ups_pre', user_id: 'u1', organization_id: 'o1', permission_set_id: 'ps_org_admin' }, + ], + }); + await reconcileOrgAdminGrant(stub, 'u1', 'o1', WALLED); + + const rows = stub.tables.sys_user_permission_set; + expect(rows.find((r) => r.id === 'ups_pre')).toEqual({ + id: 'ups_pre', + user_id: 'u1', + organization_id: 'o1', + permission_set_id: 'ps_org_admin', + }); + expect(rows.map((r) => r.permission_set_id).sort()).toEqual([ + 'ps_org_admin', + 'ps_org_admin_o1', + ]); + }); + + it('the backfill sweep reaches an orphan grant pointing at ANY organization\'s copy', async () => { + // Post-repair every organization's grants name its own row, so a sweep + // holding one unscoped id would match none of them and an orphaned grant + // would stop being revocable — a capability left standing. + const stub = makeStub({ + sys_permission_set: catalogFor('o1', 'o2'), + sys_member: [{ id: 'm1', user_id: 'u1', organization_id: 'o1', role: 'owner' }], + sys_user_permission_set: [ + { id: 'ups_o2', user_id: 'u9', organization_id: 'o2', permission_set_id: 'ps_org_admin_o2' }, + ], + }); + const summary = await backfillOrgAdminGrants(stub, WALLED); + + expect(summary.revoked).toBe(1); + expect(stub.tables.sys_user_permission_set.map((g) => g.user_id)).toEqual(['u1']); + }); +}); + +// --------------------------------------------------------------------------- +// [#11670] The `single` carve-out, measured. +// +// Under `single` there is no organization for a catalog row to belong to, so +// the organization-less row IS the row and the unscoped answer stays correct. +// This block is the leak detector for the scoping above: if any of it reaches +// the wall-less posture, these go red. +// --------------------------------------------------------------------------- +describe('[#11670] `single` posture is carved out', () => { + const seedSingle = () => + makeStub({ + sys_permission_set: catalogFor('o1'), + sys_member: [{ id: 'm1', user_id: 'u1', organization_id: 'o1', role: 'owner' }], + sys_user_permission_set: [], + }); + + it('grants the organization-less row even where an organization copy exists', async () => { + const stub = seedSingle(); + await reconcileOrgAdminGrant(stub, 'u1', 'o1', { posture: 'single' }); + // The ANSWER, unchanged: the first row of an unscoped read, which is the + // organization-less one. o1's own copy exists in this fixture and is + // deliberately NOT preferred — `single` has no wall for it to belong to. + expect(stub.tables.sys_user_permission_set[0].permission_set_id).toBe('ps_org_admin_nb'); + }); + + it('NO read on a `single` path carries a tenantId', async () => { + // The one-line statement of the carve-out, and the assertion that goes red + // first if the scoping leaks: threading an organization is what routes a + // read through the wall, so its absence is the whole property. + const stub = seedSingle(); + await reconcileOrgAdminGrant(stub, 'u1', 'o1', { posture: 'single' }); + await backfillOrgAdminGrants(stub, { posture: 'single' }); + expect(stub.findCalls.every((c) => c.context?.tenantId === undefined)).toBe(true); + expect(stub.findCalls.length).toBeGreaterThan(0); + }); + + it('the grant-target read is the unscoped `limit: 1` it always was', async () => { + const stub = seedSingle(); + await reconcileOrgAdminGrant(stub, 'u1', 'o1', { posture: 'single' }); + const target = stub.findCalls.find( + (c) => c.object === 'sys_permission_set' && c.where?.name === 'organization_admin_no_bypass', + ); + expect(target).toEqual({ + object: 'sys_permission_set', + where: { name: 'organization_admin_no_bypass' }, + limit: 1, + context: { isSystem: true }, + }); + }); + + it('DECLARED DEVIATION: the revoke reads are wide in `single` too', async () => { + // ⚠️ Not an accident and not the scoping leaking — the one place this diff + // is visible under `single`, recorded here rather than left for a reader to + // discover. + // + // Before this diff the revoke legs matched a scalar id resolved by the same + // unscoped `limit: 1` read as the grant target. That is enough only while + // one row per name exists. The F2 close-out (ADR-0105 D4) is exactly the + // deployment that DROPS its wall: every grant standing at that moment names + // a per-organization copy, which `single`'s own resolution cannot see, so a + // narrow revoke converges on nothing and leaves the unbounded + // `organization_admin` bits in force with nothing left to bound them. + // + // So the revoke reach is posture-independent by design: `{ $in: [every copy + // of the name] }` at `ORG_ADMIN_SET_COPY_SCAN_LIMIT`, in every posture. The + // grant target is unchanged; the reads below are the price, and they are + // still unscoped — no `tenantId`, per the pin above. + const stub = seedSingle(); + await reconcileOrgAdminGrant(stub, 'u1', 'o1', { posture: 'single' }); + const superseded = stub.findCalls.find( + (c) => c.object === 'sys_permission_set' && c.where?.name === 'organization_admin', + ); + expect(superseded?.limit).toBeGreaterThan(1); + expect(superseded?.context).toEqual({ isSystem: true }); + const staleRead = stub.findCalls.find( + (c) => c.object === 'sys_user_permission_set' && c.where?.permission_set_id?.$in, + ); + // Pre-diff this predicate was the scalar `permission_set_id: 'ps_org_admin'` + // — the organization-less row alone. The second id is the whole point: it + // is o1's copy, written while the deployment was walled, and it is the row + // a `single` pass has to be able to revoke. + expect(staleRead?.where.permission_set_id).toEqual({ + $in: ['ps_org_admin', 'ps_org_admin_o1'], + }); + }); + + it('the read ORDER and the objects read are unchanged under `single`', async () => { + // The rest of the multiset: same objects, same order, same predicates — + // only the two reads named in the deviation above differ, and only in + // `limit`/`$in`. + const stub = seedSingle(); + await reconcileOrgAdminGrant(stub, 'u1', 'o1', { posture: 'single' }); + expect(stub.findCalls.map((c) => c.object)).toEqual([ + 'sys_permission_set', // grant-target resolution (limit 1, unchanged) + 'sys_member', // does the pair qualify + 'sys_permission_set', // superseded-variant ids (widened — see above) + 'sys_user_permission_set', // superseded grants for the pair (widened) + 'sys_user_permission_set', // does the grant already exist (scalar, unchanged) + ]); + }); +}); diff --git a/packages/plugins/plugin-security/src/auto-org-admin-grant.ts b/packages/plugins/plugin-security/src/auto-org-admin-grant.ts index 3c5adbab56..62447b9fcc 100644 --- a/packages/plugins/plugin-security/src/auto-org-admin-grant.ts +++ b/packages/plugins/plugin-security/src/auto-org-admin-grant.ts @@ -20,6 +20,11 @@ * permission-set row, schema drift, or a stale row never blocks the * underlying `sys_member` mutation. * + * [#11670] The `organization_admin` row a grant points at is resolved PER + * ORGANIZATION under a walled posture — see {@link resolvePermissionSetId} for + * why an unscoped, name-only, process-cached resolution answered with a row + * nobody chose, and for the no-own-row decision that scoping forces. + * * **Why this isn't done by the better-auth org plugin directly:** * better-auth does not know about ObjectStack permission sets — it * only stores membership roles. Translating "owner/admin role on this @@ -38,6 +43,18 @@ import { ORGANIZATION_ADMIN, ORGANIZATION_ADMIN_NO_BYPASS } from '@objectstack/spec'; import { postureEnforcesWall, type TenancyPosture } from '@objectstack/spec/security'; +// [#11670] The per-organization catalog's own vocabulary — the governed +// spelling of "which row is THIS organization's", the context a scoped +// catalog read runs under, and the posture split that decides whether the +// question applies at all. Same package, so this adds no dependency edge; a +// second local spelling of that question is exactly the shape that produced the +// defect this repair closes. +import { + catalogIsPerOrganization, + resolveOwnOrganizationRow, + seedCtx, + SEED_ORGANIZATION_SCAN_LIMIT, +} from './per-organization-catalog.js'; const SYSTEM_CTX = { isSystem: true } as const; @@ -115,9 +132,23 @@ function genId(prefix: string): string { * That was this module's only revoke channel for its whole life (#4640). */ -async function tryFind(ql: any, object: string, where: any, limit = 50, logger?: MaybeLogger): Promise { +async function tryFind( + ql: any, + object: string, + where: any, + limit = 50, + logger?: MaybeLogger, + /** + * [#11670] The execution context the read runs under. Defaults to + * {@link SYSTEM_CTX} — the installation-wide question every read in this + * module used to ask. A catalog read passes `seedCtx(organizationId)` instead, + * which routes it through `SqlDriver.applyTenantScope` (the governed + * chokepoint) rather than re-implementing a wall predicate here. + */ + context: { isSystem: true; tenantId?: string } = SYSTEM_CTX, +): Promise { try { - const rows = await ql.find(object, { where, limit }, { context: SYSTEM_CTX }); + const rows = await ql.find(object, { where, limit }, { context }); return Array.isArray(rows) ? rows : Array.isArray(rows?.records) ? rows.records : []; } catch (e) { // Reads legitimately fail before the tables exist (boot ordering), so this @@ -220,33 +251,258 @@ export function autoOrgAdminGrantReason( } /** - * Resolve the `sys_permission_set.id` for `organization_admin`. Cached - * across calls per ObjectQL instance via a WeakMap so repeated - * reconciliations do not re-query. + * Per-ObjectQL-instance memo for {@link resolvePermissionSetId}. + * + * [#11670] `ids` is keyed on `(organizationId, name)`, not on `name`. A cache + * keyed on the name alone made the first organization reconciled in a process + * decide the row every later organization got — the property no per-call + * scoping can repair, because the second organization never reaches the read. + * + * `refusalReported` keeps the no-own-row warning to once per key per instance: + * the backfill walks every membership pair, and a per-pair warning would bury + * the line it exists to surface. + */ +interface PermissionSetIdCacheEntry { + ids: Map; + refusalReported: Set; +} + +const permissionSetIdCache = new WeakMap(); + +/** + * `(organizationId, name)` as one string, spelled so no organization id can + * collide with a name boundary (`JSON.stringify` of the pair is injective over + * `[string | null, string]`; a delimiter is not). */ -const permissionSetIdCache = new WeakMap>(); +function permissionSetCacheKey(name: string, organizationId?: string): string { + return JSON.stringify([organizationId ?? null, name]); +} +/** + * Resolve the `sys_permission_set.id` an org-admin grant points at: THIS + * organization's own row under a walled posture, the installation's single row + * under `single`. + * + * ## [#11670] Why the read is threaded with the organization + * + * Post-#10103 the RBAC catalog is materialized PER ORGANIZATION, and + * `sys_permission_set.name` is unique per organization (ADR-0120 D3, + * `COALESCE(organization_id, '__global__')`). One name therefore carries a row + * per organization PLUS the organization-less platform-bucket row + * `bootstrapPlatformAdmin` mints on every boot — measured on a fresh walled rig + * as 8 rows written 1.3 s BEFORE the first `sys_organization` exists (#11532), + * which makes the organization-less row the OLDEST one bearing the name. + * + * This read used to be name-only, `limit: 1`, and cached on the name alone. + * Each looks defensible; together they answer with a row nobody chose. An + * unscoped `limit: 1` read has no reason to prefer any particular row, and the + * grant target is a FOREIGN KEY — so a walled deployment ends up with + * `sys_user_permission_set` rows pointing at a permission set that belongs to + * no organization, or to a different one. It stays invisible because + * `resolve-authz-context` resolves permission sets BY ID without tenant + * scoping, so the grant still evaluates. + * + * The repair is to ask the governed question instead: thread the organization + * (the read routes through `SqlDriver.applyTenantScope`, the chokepoint) and + * let {@link resolveOwnOrganizationRow} — "the one read that distinguishes + * 'this organization has its row' from 'somebody's organization-less row is + * visible here'" — pick the row. The cache key moves with it. + * + * Under `single` nothing is threaded, the read is the unscoped `limit: 1` it + * always was, and `resolveOwnOrganizationRow` returns the first row: the + * carve-out is byte-identical, which is what the posture-invariance pin + * measures. + * + * ## The no-own-row decision — a refusal, not a fallback + * + * Routing through the governed read forces an answer to: what does a walled rig + * do when the granting organization has no own row of that name? This returns + * `null` — the module's existing "not seeded yet" no-op — and never falls back + * to the organization-less row it can see. In order: + * + * 1. a fallback keeps MINTING grants that point at the platform bucket. The + * reap of that bucket is gated on this repair precisely because a live + * producer makes its census unclosable — falling back would leave the + * producer producing; + * 2. a fallback row is never repaired afterwards. Once the organization's own + * row appears, the reconciler looks for a grant carrying THAT id, finds + * none, and inserts a second one — the dedup in the caller only collapses + * duplicates sharing a `permission_set_id`. So a fallback manufactures + * permanent duplicate grants across two set ids; + * 3. the state it declines to act in is one where the organization has no + * catalog AT ALL — its own positions, permission sets and sharing rules are + * equally missing — which the catalog seeding already warns about and + * retries, on organization creation and on every boot sweep. + * + * ⚠️ The refusal is confined to the GRANT direction, and that is what keeps it + * from being a loosening. Revocation does not consult this resolver: it matches + * every copy of the name ({@link resolvePermissionSetIdsForName}), so a pair + * that must lose the capability still loses it in exactly this state. Both + * directions fail closed — nothing new is granted against a row that belongs to + * no organization, and nothing standing escapes a revoke. + * + * ⚠️ And it is LOUD. The organization-less row is visible to the scoped read, + * so an operator would otherwise see a plausible row and a missing grant at the + * same time with nothing connecting them. The caller returns + * `{ action: 'skipped', reason: 'permission_set_missing' }` — the value it + * already returned for the boot-ordering case, so no consumer has to learn a + * new one — and the next `sys_member` write and the `kernel:ready` backfill + * retry it. + * + * ⛔ Grants that ALREADY point at an organization-less row are not repaired + * here and nothing in this module claims they are: this makes new resolutions + * correct. Counting and repairing the existing ones is the reap card's census. + */ async function resolvePermissionSetId( ql: any, name: string, + /** + * The granting organization, under a posture whose catalog is materialized + * per organization. `undefined` is the `single`-posture carve-out — the one + * deployment shape where an organization-less row IS the row — and never a + * fallback for "we could not work out the organization". + */ + organizationId: string | undefined, logger?: MaybeLogger, ): Promise { let perQl = permissionSetIdCache.get(ql); if (!perQl) { - perQl = new Map(); + perQl = { ids: new Map(), refusalReported: new Set() }; permissionSetIdCache.set(ql, perQl); } - const cached = perQl.get(name); + const key = permissionSetCacheKey(name, organizationId); + const cached = perQl.ids.get(key); if (cached) return cached; - const rows = await tryFind(ql, 'sys_permission_set', { name }, 1, logger); - const id = rows[0]?.id; + // Limit 5, not 1, when scoped: a scoped read returns this organization's own + // rows AND organization-less ones through the driver's compatibility arm, so + // one row would be whichever the driver ordered first — the same spelling + // `seed-name-lookup.ts` and `bootstrap-declared-permissions.ts` use (#10103). + const rows = await tryFind( + ql, + 'sys_permission_set', + { name }, + organizationId ? 5 : 1, + logger, + seedCtx(organizationId), + ); + const { own, organizationLessResidue } = resolveOwnOrganizationRow(rows, organizationId); + const id = own?.id; if (typeof id === 'string' && id.length > 0) { - perQl.set(name, id); + // Only an OWN row is memoized. A miss is re-asked on the next reconcile + // because it is a state the catalog seeding is actively repairing, and a + // process-lifetime cache over a transient answer is the third of the three + // properties this repair exists to remove. + perQl.ids.set(key, id); return id; } + if (organizationId && organizationLessResidue && !perQl.refusalReported.has(key)) { + perQl.refusalReported.add(key); + logger?.warn?.( + `[security] no org-admin capability can be GRANTED for this organization: it has no own ` + + `${name} permission set. The organization-less row that IS visible here is deliberately not ` + + `used as a grant target — a grant pointing at it belongs to no organization, and under a ` + + `walled posture that is invalid state rather than a platform-wide default. Revocation is ` + + `unaffected and still matches every copy of the name, so nobody keeps a capability the ` + + `platform decided to take away; standing grants are otherwise left exactly as they are. ` + + `Remedy: seed this organization's RBAC catalog — it is created on organization creation and ` + + `on every boot sweep, so a missing copy means that pass failed or has not run yet, and its ` + + `own warning names why.`, + { + object: 'sys_permission_set', + name, + organization: organizationId, + organizationLessRowId: organizationLessResidue?.id ?? null, + }, + ); + } return null; } +/** + * [#11670] Rows a REVOKE is willing to hold for ONE org-admin set name under a + * walled posture: one per organization the catalog sweep covers, plus the + * organization-less platform-bucket row. + * + * A BUDGET rather than a bound — nothing caps rows-per-name, since the name is + * unique per organization (ADR-0120 D3). Exceeding it is therefore DETECTED and + * reported rather than silently truncated: a grant pointing at a row past the + * budget is a grant no revoke here can reach. + */ +const ORG_ADMIN_SET_COPY_SCAN_LIMIT = SEED_ORGANIZATION_SCAN_LIMIT + 1; + +/** + * EVERY `sys_permission_set.id` bearing `name`, across the installation. + * + * ## [#11670] Resolve NARROW to grant, WIDE to revoke + * + * The scoped resolution above answers "which row may a NEW grant point at", + * and it must be narrow: exactly this organization's own row. This one answers + * a different question — "which rows does a standing grant of that name point + * at" — and it must be WIDE, because the platform does not get to choose which + * copy an already-written foreign key names. + * + * Making the grant target narrower without widening the revoke reach is a + * PERMISSION LOOSENING, which is why the two ship together: + * + * - a demoted admin whose grant was written before this repair points at the + * organization-less row. A demotion matched only against this + * organization's own id would not find it, and the capability the platform + * just decided to take away would stay in force; + * - the ADR-0105 D4 F2 close-out — a deployment that drops its wall must not + * leave the unbounded `organization_admin` grant standing — converges by + * revoking the SUPERSEDED variant for the pair. Across a posture flip the + * standing grant and the newly-resolved id are copies from different + * postures, so a narrow match converges on nothing; + * - the backfill's orphan sweep asks the installation-wide question by + * construction (it scans every membership and every org-admin grant), so a + * single id would match no per-organization grant at all. + * + * ⛔ Wide to REVOKE only. Nothing here re-points, adopts or deletes a + * mis-targeted grant row belonging to someone who still qualifies — that census + * is the reap card's, not this one's. + * + * ## Why this one is NOT posture-keyed, when everything else here is + * + * The `single` carve-out governs the grant TARGET — under `single` the + * organization-less row is the row, and nothing is threaded. It cannot govern + * the revoke reach, because the rows a revoke has to reach were written by the + * OTHER posture: the F2 close-out is precisely the deployment that DROPS its + * wall, and every grant standing at that moment names a per-organization copy + * the wall-less resolution can no longer see. A revoke narrowed to `single`'s + * own row converges on nothing and leaves the unbounded `organization_admin` + * bits in force on a deployment with nothing left to bound them — F2 exactly. + * + * So the rule is uniform and easy to state: the grant target is posture-scoped, + * the revoke reach never is. The cost is measured and small: under `single` the + * revoke legs read `limit: {@link ORG_ADMIN_SET_COPY_SCAN_LIMIT}` with an `$in` + * where they used to read `limit: 1` with a scalar. The unscoped ANSWER — which + * row a `single` deployment grants — is untouched, and no read on any `single` + * path carries a `tenantId`. + */ +async function resolvePermissionSetIdsForName( + ql: any, + name: string, + logger?: MaybeLogger, +): Promise { + const limit = ORG_ADMIN_SET_COPY_SCAN_LIMIT; + const rows = await tryFind(ql, 'sys_permission_set', { name }, limit, logger); + if (rows.length >= limit) { + logger?.warn?.( + '[security] the org-admin permission-set scan hit its bound — a grant pointing at a row past ' + + 'it is NOT reached by this revoke; the next boot sweep asks again', + { object: 'sys_permission_set', name, limit }, + ); + } + const ids: string[] = []; + for (const row of rows) { + const id = row?.id; + if (typeof id === 'string' && id.length > 0 && !ids.includes(id)) ids.push(id); + } + return ids; +} + + + /** * Ensure (or revoke) the org-scoped `organization_admin` grant for * `(userId, orgId)` based on the current `sys_member` rows. @@ -304,10 +560,31 @@ export async function reconcileOrgAdminGrant( const grantSetName = orgAdminSetNameForPosture(posture, suppressUnbounded); const supersededSetName = supersededOrgAdminSetName(posture, suppressUnbounded); - const permSetId = await resolvePermissionSetId(ql, grantSetName, logger); - if (!permSetId) { + // [#11670] WHICH organization's copy of the set this grant points at. The + // posture decides whether the question exists at all, in the catalog's own + // spelling: under `single` there is one organization-less row and threading + // an organization would ask a question that deployment shape cannot answer, + // so the carve-out is `undefined` — the same split `catalogIsPerOrganization` + // makes for the seeders, never a second local one. + const catalogOrganizationId = catalogIsPerOrganization(posture) ? orgId : undefined; + + const permSetId = await resolvePermissionSetId(ql, grantSetName, catalogOrganizationId, logger); + if (!permSetId && !catalogOrganizationId) { // The permission set isn't seeded yet (boot ordering) — caller can retry // later (e.g. via kernel:ready backfill). + // + // [#11670] `single` keeps returning HERE: with one organization-less row per + // name, "no row" also means no standing grant can point at one, so there is + // nothing to revoke either and the early return is the whole answer — + // byte-identical to before this repair. + // + // A walled posture does NOT return here, because the two halves come apart: + // "this organization has no own row to point a NEW grant at" says nothing + // about the grants already standing for this pair. Returning here would + // make a demotion a no-op for exactly the rows this repair is about — a + // capability the platform decided to take away, left in force. So the + // revoke legs below run on their own ids, and the grant leg is the one that + // declines (see `resolvePermissionSetId`). return { action: 'skipped', reason: 'permission_set_missing' }; } @@ -330,12 +607,23 @@ export async function reconcileOrgAdminGrant( // 1b. [ADR-0105 D4] Revoke the OTHER variant for this pair, always. A posture // change (or a downgrade after F2) must converge on exactly one org-admin // grant; leaving the superseded row would keep the old bits in force. - const supersededSetId = await resolvePermissionSetId(ql, supersededSetName, logger); - if (supersededSetId) { + // + // [#11670] Matched against EVERY copy of the superseded name, not just this + // organization's own. The standing grant this leg exists to remove was + // written under the OTHER posture (or before this repair), so it points at + // whichever copy that posture resolved — a narrow match would converge on + // nothing and leave the superseded bits in force, which is the F2 outcome + // this leg is the close-out for. + const supersededSetIds = await resolvePermissionSetIdsForName(ql, supersededSetName, logger); + if (supersededSetIds.length > 0) { const stale = await tryFind( ql, 'sys_user_permission_set', - { user_id: userId, organization_id: orgId, permission_set_id: supersededSetId }, + { + user_id: userId, + organization_id: orgId, + permission_set_id: { $in: supersededSetIds }, + }, 5, logger, ); @@ -352,15 +640,31 @@ export async function reconcileOrgAdminGrant( } // 2. Look at existing grants for this exact pair. - const existingGrants = await tryFind( - ql, - 'sys_user_permission_set', - { user_id: userId, organization_id: orgId, permission_set_id: permSetId }, - 5, - logger, - ); - + // + // [#11670] The two branches ask DIFFERENT questions of the same table, and + // the read moved inside them because of it. Granting asks "does a grant + // already point at the row I would create" — narrow, this organization's own + // id. Revoking asks "does this pair hold ANY grant of that name" — wide, + // every copy. Under `single` both spell the same scalar predicate against the + // same single id, in the same position, so the carve-out issues exactly the + // reads it issued before. if (shouldGrant) { + if (!permSetId) { + // Walled only (the `single` early return above already fired). The + // organization has no own row to point a NEW grant at, and the + // organization-less row that IS visible is deliberately not a grant + // target — `resolvePermissionSetId` has already said so loudly. Nothing + // is granted; the superseded leg above still ran, so nothing that should + // lose the capability keeps it. + return { action: 'skipped', reason: 'permission_set_missing' }; + } + const existingGrants = await tryFind( + ql, + 'sys_user_permission_set', + { user_id: userId, organization_id: orgId, permission_set_id: permSetId }, + 5, + logger, + ); if (existingGrants.length > 0) { // Deduplicate stale duplicates if any slipped through. for (const extra of existingGrants.slice(1)) { @@ -400,6 +704,27 @@ export async function reconcileOrgAdminGrant( } // shouldGrant === false → revoke any pre-existing scoped grant. + // + // [#11670] Every copy of the granted name, not just this organization's own: + // a grant written before this repair points at the organization-less row, and + // a demotion that could not see it would leave the capability in force. ⛔ It + // revokes; it never re-points or adopts a row for someone who still + // qualifies. + const revocableSetIds = await resolvePermissionSetIdsForName(ql, grantSetName, logger); + const existingGrants = + revocableSetIds.length > 0 + ? await tryFind( + ql, + 'sys_user_permission_set', + { + user_id: userId, + organization_id: orgId, + permission_set_id: { $in: revocableSetIds }, + }, + 5, + logger, + ) + : []; if (existingGrants.length === 0) { return { action: 'noop' }; } @@ -454,19 +779,26 @@ export async function backfillOrgAdminGrants( const summary = { scanned: 0, granted: 0, revoked: 0, skipped: 0 }; if (!ql || typeof ql.find !== 'function') return summary; - const permSetId = await resolvePermissionSetId( + // [#11670] The sweep is installation-wide by construction — it scans every + // `sys_member` row and every org-admin grant, across organizations — so its + // set ids are resolved installation-wide too, one per organization holding a + // copy. The PER-PAIR answer is not taken from here: `reconcileOrgAdminGrant` + // resolves its own organization's row below, which is the scoping this repair + // is about. The gate is unchanged: no row anywhere for the GRANTED name means + // the catalog has not been seeded at all yet. + const permSetIds = await resolvePermissionSetIdsForName( ql, orgAdminSetNameForPosture(posture, suppressUnbounded), logger, ); - if (!permSetId) { + if (permSetIds.length === 0) { logger?.debug?.('[security] org-admin backfill skipped — permission set missing'); return summary; } // [ADR-0105 D4] The orphan sweep below must see BOTH variants: a boot that // changed posture leaves grants of the superseded set behind, and those are // exactly the rows whose bits must stop applying. - const supersededId = await resolvePermissionSetId( + const supersededIds = await resolvePermissionSetIdsForName( ql, supersededOrgAdminSetName(posture, suppressUnbounded), logger, @@ -497,7 +829,7 @@ export async function backfillOrgAdminGrants( // Also revoke any organization_admin grant pointing at a (user, org) // pair with NO membership row left (orphaned grants from deletes // that fired before this hook existed). - const grantSetIds = [permSetId, supersededId].filter(Boolean) as string[]; + const grantSetIds = [...permSetIds, ...supersededIds]; const allGrants = await tryFind( ql, 'sys_user_permission_set', From c2f1944c49aa292d8fc99f7e81a4745400462d6c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 15:07:16 +0000 Subject: [PATCH 2/2] docs(permissions): the isSystem declaration census counts 22, not 21 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The repair on this branch adds one DECLARING position for the identifier — the `context: { isSystem: true; tenantId?: string }` parameter type on the org-admin reconciler's read wrapper, which exists to carry `seedCtx(organizationId)`. That moves the enforced `table-declarations` count and nothing else: no elevation read arrives, so the 109 sites, their anchors, and the package and file totals are unmoved. `--fix` does not repair a population change. The count is hand-written, and the paragraph beside it now says what that enforced row counts — the four distinct fields plus the structural type literals that restate the shape inline — so the next arrival is placeable without re-deriving the census. The new site is cited without a line number on purpose: this page anchors elevation reads, and the gate refuses an anchor that is not one. Part of #11670 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs --- content/docs/permissions/system-context.mdx | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index e35abe539a..20d7ce8abe 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -306,7 +306,7 @@ still holds equal to the census on every pull request: | — in tests | 1013 | — | | — in non-test sources | 798 | — | | Appearances of the bare identifier `isSystem` in non-test sources | 813 | — | -| — parsed as a declaration | 21 | ✅ | +| — parsed as a declaration | 22 | ✅ | | — parsed as an object-literal / type key (producers and option objects) | 310 | — | | — parsed as a property **read** | 115 | ✅ | | — parsed in some other syntactic position (a local, a cast, a conditional) | 9 | ✅ | @@ -336,6 +336,24 @@ test files certifies nothing. ⛔ Do not re-add them to `DECLARED_COUNTS` — a self-test case in the gate refuses that by name. Re-measure them with `node scripts/isystem-census.mjs` when you want them current, and move the date. +**What the enforced declarations row counts.** Not the four field declarations +above — those are four *distinct fields* that happen to share a name, and only +the first is elevation. This row counts every position where the parser puts the +identifier in a **declaring** slot: those four, plus the structural type literals +that restate `ExecutionContext.isSystem`'s shape inline rather than importing it +(`{ isSystem: true; tenantId?: string }`, `context?: { isSystem?: boolean }`, and +the `get isSystem()` accessor on the engine's context wrapper). A restatement is +a producer's declaration of the shape it will build, never a read, so a new one +moves this count and moves nothing else on this page — the census's read +population, the anchored rows above, and the packages and files totals all stay +where they are. The most recent arrival is the scoped +seed context threaded into the org-admin permission-set lookup in +`plugins/plugin-security/src/auto-org-admin-grant.ts`, so that read resolves +against the granting organization's own catalog row rather than an +organization-less one (#11670). ⛔ Cited without a line number deliberately: an +anchor here would be refused, and rightly — this page anchors elevation +**reads**, and a declaration is not one. + Counting by hand is what made the previous edition wrong in two independent ways, so both are worth naming. Its headline said "80 distinct sites across 18 packages" while its own tables anchored **77** — the number never matched the