From 8b880a16becab5250dc173e2662fdda0e5a17629 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Mon, 24 Aug 2026 16:31:42 +0800 Subject: [PATCH] fix(security): an organization-less permission-set row grants again (#11121 residue) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-organization loader read resolveOwnOrganizationRow().own alone, which is never a residue once an organization is supplied — so every walled deployment carrying pre-#11121 rows silently lost those grants on upgrade, while the SAME row's system/tab permissions kept applying through the unscoped by-id read. The helper is written for seeders, where not seeing a residue as 'already seeded' is the point; enforcement needs the opposite reading. This implements the fallback the loader's own comment already promised. Own still beats residue, so #11121's cross-tenant fix is intact. Co-Authored-By: Claude Fable 5 --- .changeset/permission-set-residue-fallback.md | 38 +++++++++ ...ve-permission-sets-for-context.pin.test.ts | 83 +++++++++++++++++++ .../plugin-security/src/security-plugin.ts | 25 +++++- 3 files changed, 143 insertions(+), 3 deletions(-) create mode 100644 .changeset/permission-set-residue-fallback.md diff --git a/.changeset/permission-set-residue-fallback.md b/.changeset/permission-set-residue-fallback.md new file mode 100644 index 0000000000..c493669844 --- /dev/null +++ b/.changeset/permission-set-residue-fallback.md @@ -0,0 +1,38 @@ +--- +'@objectstack/plugin-security': patch +--- + +An organization-less `sys_permission_set` row grants again — #11121 revoked standing access silently + +#11121 made the request-time permission-set loader tenant-scoped so two +organizations holding a row for the same name stop answering each other's +requests. It shipped the second half as a COMMENT — "an organization-less +leftover only where it does not [have its own]" — and the code read `.own` +alone, which by `resolveOwnOrganizationRow`'s own documented contract is never +a residue once an organization is supplied. + +That helper is written for SEEDERS, where refusing to read a residue as +"already seeded" is the entire point. Enforcement wants the opposite reading: an +organization-less row is still a row the principal was granted, and dropping it +revokes standing access with no signal at the moment of loss — the failure this +catalog's own header, and `resolve-authz-context`'s `sys_position` read, both +name as the thing not to do. + +The asymmetry was observable on a single row: its `system_permissions` and +`tab_permissions` kept applying, because that read is unscoped and by id, while +its `object_permissions` and `admin_scope` stopped. One row, two enforcement +planes, opposite verdicts. Every walled deployment carrying pre-#11121 rows — +or any row authored without a tenant, which includes admin-UI-authored sets — +lost those grants on upgrade, reported only as a boot WARN about "leftovers" +that states the catalog is complete. + +Found by cloud's `apps/ee-group-showcase` dogfood suites, which had been failing +four ADR-0111 / ADR-0105 assertions on cloud main while turbo replayed them from +cache. + +Preference order is unchanged, so the cross-tenant bleed #11121 closed stays +closed: this organization's own row still WINS wherever it exists, and a +leftover is consulted only in its absence. #11121's suite covers seeding and the +`sys_position` sweep; the three cases added here cover the loader path it did +not — residue resolves, own beats residue, and the single-posture carve-out is +untouched. Reverting the one-line fix reddens exactly the first of them. diff --git a/packages/plugins/plugin-security/src/resolve-permission-sets-for-context.pin.test.ts b/packages/plugins/plugin-security/src/resolve-permission-sets-for-context.pin.test.ts index f4113af4bb..8c5705e1ec 100644 --- a/packages/plugins/plugin-security/src/resolve-permission-sets-for-context.pin.test.ts +++ b/packages/plugins/plugin-security/src/resolve-permission-sets-for-context.pin.test.ts @@ -186,3 +186,86 @@ describe('[#7616] resolvePermissionSetsForContext is reachable through the servi expect(sets).toEqual([]); }); }); + +/** + * [#11121 residue] An ORGANIZATION-LESS `sys_permission_set` row still grants. + * + * #11121 made this loader tenant-scoped so two organizations holding a row for + * the same name stop answering each other's requests. Its comment promised the + * other half — "an organization-less leftover only where it does not [have its + * own]" — and the code read `.own` alone, which by + * `resolveOwnOrganizationRow`'s documented contract is NEVER a residue once an + * organization is supplied. That helper is written for SEEDERS, where refusing + * to see a residue as "already seeded" is the entire point; enforcement wants + * the opposite reading. + * + * The consequence was a silent revocation on upgrade: every walled deployment + * carrying pre-#11121 rows (or any row authored without a tenant — the admin + * UI, a hand-run seed) kept its `system_permissions` and `tab_permissions`, + * because that read is unscoped and by id, while its `object_permissions` and + * `admin_scope` stopped applying. One row, two enforcement planes, opposite + * verdicts, no signal at the moment of loss. + */ +const RESIDUE_ROW = { + name: 'legacy_auditor', + label: 'Legacy Auditor', + organization_id: null, + object_permissions: JSON.stringify({ deal: { allowRead: true, allowDelete: true } }), + field_permissions: JSON.stringify({}), + system_permissions: JSON.stringify([]), + tab_permissions: JSON.stringify({}), +}; + +/** The SAME name, stamped with the caller's organization — this one must win. */ +const OWN_ROW = { + name: 'legacy_auditor', + label: 'Legacy Auditor (this organization)', + organization_id: 'org_a', + object_permissions: JSON.stringify({ deal: { allowRead: true, allowDelete: false } }), + field_permissions: JSON.stringify({}), + system_permissions: JSON.stringify([]), + tab_permissions: JSON.stringify({}), +}; + +describe('[#11121] the per-organization loader does not silently revoke organization-less grants', () => { + it('resolves an organization-LESS row for a caller who has an organization', async () => { + const svc = await locateSecurityService([RESIDUE_ROW]); + + const sets = await svc.resolvePermissionSetsForContext?.({ + userId: 'u1', + organizationId: 'org_a', + permissions: ['legacy_auditor'], + } as any); + + const found = (sets ?? []).find((s) => s.name === 'legacy_auditor'); + expect(found, 'the grant vanished for a caller in an organization — this is the silent revocation').toBeTruthy(); + // Whole, not merely present: the columns that went missing are the point. + expect((found as any)?.objects?.deal?.allowDelete).toBe(true); + }); + + it('still prefers THIS organization\'s own row when both exist — the cross-tenant bleed stays closed', async () => { + const svc = await locateSecurityService([RESIDUE_ROW, OWN_ROW]); + + const sets = await svc.resolvePermissionSetsForContext?.({ + userId: 'u1', + organizationId: 'org_a', + permissions: ['legacy_auditor'], + } as any); + + const found = (sets ?? []).find((s) => s.name === 'legacy_auditor'); + expect(found?.label).toBe('Legacy Auditor (this organization)'); + // The residue's broader grant must NOT leak in behind the own row. + expect((found as any)?.objects?.deal?.allowDelete).toBe(false); + }); + + it('resolves the same row for a caller with NO organization (single posture, unchanged)', async () => { + const svc = await locateSecurityService([RESIDUE_ROW]); + + const sets = await svc.resolvePermissionSetsForContext?.({ + userId: 'u1', + permissions: ['legacy_auditor'], + } as any); + + expect((sets ?? []).some((s) => s.name === 'legacy_auditor')).toBe(true); + }); +}); diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index da29792526..b06e331906 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -1088,13 +1088,32 @@ export class SecurityPlugin implements Plugin { const fetched = Array.isArray(rows) ? rows : rows?.records ?? []; // One row per NAME: this organization's own where it has one, an // organization-less leftover only where it does not. + // + // The residue arm is load-bearing and was missing (#11121 shipped the + // comment without it, and its suite covers seeding rather than this + // loader). `resolveOwnOrganizationRow` is written for SEEDERS, where + // refusing to see a residue as "already seeded" is the whole point — + // so it never returns one as `own`. ENFORCEMENT wants the opposite + // reading: an organization-less row is still a row this principal was + // granted, and dropping it revokes standing access with no signal at + // the moment of loss — the failure mode this catalog's own header, + // and `resolve-authz-context`'s `sys_position` read, both call out as + // the thing not to do. The asymmetry was observable on a single row: + // its `system_permissions` and `tab_permissions` kept applying (that + // read is unscoped by id) while its `object_permissions` and + // `admin_scope` silently stopped. + // + // Preference order is unchanged and still closes the cross-tenant + // bleed #11121 fixed: this organization's own row WINS wherever it + // exists, and a leftover is consulted only in its absence. const byName = new Map(); for (const name of new Set(names)) { - const own = resolveOwnOrganizationRow( + const { own, organizationLessResidue } = resolveOwnOrganizationRow( fetched.filter((r: any) => r?.name === name), organizationId, - ).own; - if (own) byName.set(name, own); + ); + const row = own ?? organizationLessResidue; + if (row) byName.set(name, row); } const all = Array.from(byName.values()); // [ADR-0049] A DEACTIVATED set grants nothing. Not defence in depth