diff --git a/.changeset/delegated-admin-holdings-organization.md b/.changeset/delegated-admin-holdings-organization.md new file mode 100644 index 00000000000..9a617c560bb --- /dev/null +++ b/.changeset/delegated-admin-holdings-organization.md @@ -0,0 +1,5 @@ +--- +'@objectstack/plugin-security': patch +--- + +The delegated-admin gate now counts a `sys_user_position` holding only where the runtime grants it. Self-delegation's "you currently hold this position" check (ADR-0091 D3 rule 4) no longer accepts a holding stamped for a different organization — a user can no longer self-delegate a position in an organization where they hold nothing just because they hold a same-named position elsewhere. Organization-less holdings still count, exactly as the runtime authz resolver grants them in every organization. The binding blast-radius check (ADR-0090 D12) likewise counts only the assignments the bound position row reaches — its own organization's plus organization-less ones — so another organization's same-named assignments no longer refuse a binding as outside the subtree or push it over the assignment cap. An organization-less (`single` posture) caller is unchanged. diff --git a/packages/plugins/plugin-security/src/delegated-admin-gate-holding-organization.test.ts b/packages/plugins/plugin-security/src/delegated-admin-gate-holding-organization.test.ts new file mode 100644 index 00000000000..cd0a550efb3 --- /dev/null +++ b/packages/plugins/plugin-security/src/delegated-admin-gate-holding-organization.test.ts @@ -0,0 +1,348 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The delegated-admin gate counts a `sys_user_position` holding only where the + * runtime would grant it — pinned over a real engine, one witness per read. + * + * ## The defect this exists to keep closed + * + * `sys_user_position.position` is a position NAME, and `sys_position.name` is + * unique per organization only, so in a single-database multi-org posture + * (ADR-0105 D1 `group` / `isolated`) two organizations' holdings of the same + * name are the same string. Two gate reads selected holdings by that name under + * a bare `{ isSystem: true }` context with no organization condition: + * + * - `activeHoldings` — does the delegator CURRENTLY and DIRECTLY hold the + * position (ADR-0091 D3 rule 4)? A holding stamped for ANOTHER + * organization answered yes, so a user self-delegated a position in an + * organization where they held nothing. + * - `assignmentAnchorsOfPosition` — whose capability does re-binding the + * position re-compose (the ADR-0090 D12 blast radius)? Another + * organization's same-named assignments entered it, so a delegate was + * refused over holders their binding never reaches. + * + * ## The rule both reads now follow — the runtime's own + * + * `resolveAuthzContext` (step 4, `@objectstack/core`) keeps a holding when its + * `organization_id` is empty OR equals the active organization, and drops it + * when it names a DIFFERENT one. The gate agrees: a holding stamped for another + * organization never counts; an organization-less holding counts in every + * organization, exactly as it grants in every organization. An + * organization-less caller (`single` posture) drops nothing, as before. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import type { PermissionSet } from '@objectstack/spec/security'; +import { DelegatedAdminGate } from './delegated-admin-gate.js'; + +const ORG_A = 'org_a_acme'; +/** Sorts BEFORE every `org_a` / `*_a_*` id, like the sibling suites. */ +const ORG_B = 'org_0_globex'; + +const text = (name: string) => ({ name, type: 'text' as const }); +const object = (name: string, fields: string[]) => ({ + name, + label: name, + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + organization_id: text('organization_id'), + ...Object.fromEntries(fields.map((f) => [f, text(f)])), + }, +}); + +const OBJECTS = [ + object('sys_business_unit', ['name', 'parent_business_unit_id']), + object('sys_position', ['name', 'delegatable']), + object('sys_permission_set', ['name', 'admin_scope']), + object('sys_position_permission_set', ['position_id', 'permission_set_id']), + object('sys_user_position', [ + 'user_id', 'position', 'business_unit_id', 'granted_by', 'delegated_from', 'valid_until', 'reason', + ]), + object('sys_business_unit_member', ['user_id', 'business_unit_id']), + object('sys_user', ['primary_business_unit_id']), +]; + +/** A binding delegate's scope: the unit NAMED `sales`, may bind `sales_user` only. */ +const BIND_SCOPE = { + businessUnit: 'sales', + includeSubtree: true, + manageAssignments: false, + manageBindings: true, + authorEnvironmentSets: false, + assignablePermissionSets: ['sales_user'], +}; + +const BINDER_SETS = [ + { + name: 'sales_binder', + objects: { sys_position_permission_set: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } }, + adminScope: BIND_SCOPE, + }, +] as unknown as PermissionSet[]; + +const engines: ObjectQL[] = []; +afterEach(async () => { + while (engines.length) { + try { await engines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +const sysCtx = (organizationId?: string) => + organizationId ? { isSystem: true, tenantId: organizationId } : { isSystem: true }; + +async function bootEngine() { + const engine = new ObjectQL(); + engine.registerDriver( + new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }), + true, + ); + await engine.init(); + engine.registerApp({ + id: 'com.objectstack.qa.delegated-admin-holding-organization', + name: 'Delegated admin holding organization scope', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: OBJECTS, + } as any); + await engine.syncSchemas(); + engines.push(engine); + return engine; +} + +/** + * One organization's slice: a unit named `sales` with one child, a delegatable + * `field_lead` position, and a plain `sales_user` permission set. No holdings — + * each witness writes exactly the holdings it is about. + */ +async function seedOrganization(engine: ObjectQL, organizationId: string, idPrefix: string) { + const ctx = sysCtx(organizationId); + const ids = { + sales: `bu_${idPrefix}_sales`, + salesEast: `bu_${idPrefix}_sales_east`, + fieldLead: `pos_${idPrefix}_field_lead`, + salesUser: `ps_${idPrefix}_sales_user`, + }; + await engine.insert('sys_business_unit', [ + { id: ids.sales, name: 'sales', parent_business_unit_id: null }, + { id: ids.salesEast, name: 'sales_east', parent_business_unit_id: ids.sales }, + ], { context: ctx } as any); + await engine.insert('sys_position', [ + { id: ids.fieldLead, name: 'field_lead', delegatable: '1' }, + ], { context: ctx } as any); + await engine.insert('sys_permission_set', [ + { id: ids.salesUser, name: 'sales_user', admin_scope: null }, + ], { context: ctx } as any); + return ids; +} + +/** Write holdings of `field_lead`, stamped for `organizationId` (undefined = organization-less). */ +async function hold( + engine: ObjectQL, + organizationId: string | undefined, + rows: Array<{ id: string; user_id: string; business_unit_id?: string | null; delegated_from?: string }>, +) { + // Chunked: SQLite caps one compound INSERT … SELECT at 500 terms. + for (let i = 0; i < rows.length; i += 100) { + await engine.insert('sys_user_position', rows.slice(i, i + 100).map((r) => ({ + position: 'field_lead', + business_unit_id: null, + ...r, + })), { context: sysCtx(organizationId) } as any); + } +} + +function gateOn(engine: ObjectQL) { + return new DelegatedAdminGate({ + ql: engine, + resolveSets: async (context: any) => (context?.principal === 'binder' ? BINDER_SETS : []), + }); +} + +const DAY = 24 * 60 * 60 * 1000; +const USER = 'usr_u'; +const COLLEAGUE = 'usr_colleague'; + +/** USER self-delegates `field_lead` to a colleague, in `organizationId` (undefined = `single` posture). */ +const selfDelegate = (gate: DelegatedAdminGate, organizationId: string | undefined, businessUnitId?: string) => + gate.assert({ + object: 'sys_user_position', + operation: 'insert', + data: { + user_id: COLLEAGUE, + position: 'field_lead', + delegated_from: USER, + valid_until: new Date(Date.now() + 7 * DAY).toISOString(), + reason: 'annual leave', + ...(businessUnitId ? { business_unit_id: businessUnitId } : {}), + }, + context: organizationId + ? { principal: 'holder', userId: USER, tenantId: organizationId } + : { principal: 'holder', userId: USER }, + }); + +/** A binding delegate in `organizationId` binds `sales_user` to the position row `positionId`. */ +const bind = (gate: DelegatedAdminGate, organizationId: string, positionId: string, permissionSetId: string) => + gate.assert({ + object: 'sys_position_permission_set', + operation: 'insert', + data: { position_id: positionId, permission_set_id: permissionSetId }, + context: { principal: 'binder', userId: 'usr_binder', tenantId: organizationId }, + }); + +/** A gate refusal: the ADR-0112 envelope, then the first sentence that names why. */ +async function expectRefused(p: Promise, why: RegExp) { + const err: any = await p.then(() => null, (e) => e); + expect(err, 'expected a refusal').not.toBeNull(); + expect(err).toMatchObject({ code: 'PERMISSION_DENIED', statusCode: 403 }); + expect(String(err.message)).toMatch(why); +} + +// ── activeHoldings — self-delegation rule 4 ─────────────────────────────── + +describe('DelegatedAdminGate — rule 4 counts only holdings the caller\'s organization grants', () => { + it('ground truth: the organization-less holding is stored with no organization', async () => { + const engine = await bootEngine(); + await seedOrganization(engine, ORG_A, 'a'); + await hold(engine, ORG_B, [{ id: 'up_b', user_id: USER }]); + await hold(engine, undefined, [{ id: 'up_none', user_id: USER }]); + + const driver: any = (engine as any).getDriver('sys_user_position'); + const raw = await driver.knex('sys_user_position').where({ position: 'field_lead' }).orderBy('id', 'asc'); + expect(raw.map((r: any) => [r.id, r.organization_id ?? null])).toEqual([['up_b', ORG_B], ['up_none', null]]); + }); + + it('a user whose ONLY holding is in org B cannot self-delegate org A\'s same-named position', async () => { + const engine = await bootEngine(); + await seedOrganization(engine, ORG_B, '0'); + await seedOrganization(engine, ORG_A, 'a'); + await hold(engine, ORG_B, [{ id: 'up_b', user_id: USER }]); + + await expectRefused(selfDelegate(gateOn(engine), ORG_A), /you do not currently hold 'field_lead'/); + // …and in org B, where the holding lives, it stands. + await expect(selfDelegate(gateOn(engine), ORG_B)).resolves.toBeUndefined(); + }); + + it('a user holding it in org A can self-delegate it in org A', async () => { + const engine = await bootEngine(); + await seedOrganization(engine, ORG_B, '0'); + await seedOrganization(engine, ORG_A, 'a'); + await hold(engine, ORG_A, [{ id: 'up_a', user_id: USER }]); + + await expect(selfDelegate(gateOn(engine), ORG_A)).resolves.toBeUndefined(); + }); + + it('an organization-less holding counts in org A — the runtime grants it in every organization', async () => { + const engine = await bootEngine(); + await seedOrganization(engine, ORG_B, '0'); + await seedOrganization(engine, ORG_A, 'a'); + await hold(engine, undefined, [{ id: 'up_none', user_id: USER }]); + + await expect(selfDelegate(gateOn(engine), ORG_A)).resolves.toBeUndefined(); + }); + + it('a DIRECT holding in org B does not make org A\'s delegated holding re-delegatable', async () => { + const engine = await bootEngine(); + await seedOrganization(engine, ORG_B, '0'); + await seedOrganization(engine, ORG_A, 'a'); + await hold(engine, ORG_B, [{ id: 'up_b', user_id: USER }]); + await hold(engine, ORG_A, [{ id: 'up_a', user_id: USER, delegated_from: 'usr_boss' }]); + + await expectRefused( + selfDelegate(gateOn(engine), ORG_A), + /you hold 'field_lead' only via delegation — a delegated grant is not re-delegatable/, + ); + }); + + it('rule 4b: an org B holding\'s anchor does not widen what an org A delegation may hand out', async () => { + const engine = await bootEngine(); + const b = await seedOrganization(engine, ORG_B, '0'); + const a = await seedOrganization(engine, ORG_A, 'a'); + await hold(engine, ORG_B, [{ id: 'up_b', user_id: USER, business_unit_id: b.sales }]); + await hold(engine, ORG_A, [{ id: 'up_a', user_id: USER, business_unit_id: a.salesEast }]); + + await expectRefused( + selfDelegate(gateOn(engine), ORG_A, b.salesEast), + /business unit anchor 'bu_0_sales_east' is outside your own effective anchor for 'field_lead'/, + ); + await expect(selfDelegate(gateOn(engine), ORG_A, a.salesEast)).resolves.toBeUndefined(); + }); + + it('a `single`-posture caller (no organization in context) keeps counting every holding', async () => { + const engine = await bootEngine(); + await seedOrganization(engine, ORG_B, '0'); + await hold(engine, ORG_B, [{ id: 'up_b', user_id: USER }]); + + await expect(selfDelegate(gateOn(engine), undefined)).resolves.toBeUndefined(); + }); +}); + +// ── assignmentAnchorsOfPosition — the binding blast radius ──────────────── + +describe('DelegatedAdminGate — a binding\'s blast radius is the holders the bound position reaches', () => { + it('org B\'s same-named assignments do not refuse org A\'s binding (outside-subtree anchor)', async () => { + const engine = await bootEngine(); + const b = await seedOrganization(engine, ORG_B, '0'); + const a = await seedOrganization(engine, ORG_A, 'a'); + await hold(engine, ORG_A, [{ id: 'up_a', user_id: 'usr_a1', business_unit_id: a.salesEast }]); + await hold(engine, ORG_B, [{ id: 'up_b', user_id: 'usr_b1', business_unit_id: b.salesEast }]); + + await expect(bind(gateOn(engine), ORG_A, a.fieldLead, a.salesUser)).resolves.toBeUndefined(); + }); + + it('org B\'s same-named assignments do not push org A\'s binding over the blast-radius cap', async () => { + const engine = await bootEngine(); + const b = await seedOrganization(engine, ORG_B, '0'); + const a = await seedOrganization(engine, ORG_A, 'a'); + await hold(engine, ORG_A, [{ id: 'up_a', user_id: 'usr_a1', business_unit_id: a.salesEast }]); + await hold(engine, ORG_B, Array.from({ length: 501 }, (_, i) => ({ + id: `up_b_${String(i).padStart(3, '0')}`, + user_id: `usr_b_${i}`, + business_unit_id: b.salesEast, + }))); + + await expect(bind(gateOn(engine), ORG_A, a.fieldLead, a.salesUser)).resolves.toBeUndefined(); + }); + + it('an organization-less holding IS in org A\'s blast radius — the runtime grants it org A\'s binding', async () => { + const engine = await bootEngine(); + await seedOrganization(engine, ORG_B, '0'); + const a = await seedOrganization(engine, ORG_A, 'a'); + await hold(engine, ORG_A, [{ id: 'up_a', user_id: 'usr_a1', business_unit_id: a.salesEast }]); + await hold(engine, undefined, [{ id: 'up_none', user_id: 'usr_x' }]); + + await expectRefused( + bind(gateOn(engine), ORG_A, a.fieldLead, a.salesUser), + /position 'field_lead' has 1 unanchored assignment\(s\)/, + ); + }); + + it('the radius follows the BOUND row\'s organization — org B\'s holders still guard org B\'s row', async () => { + const engine = await bootEngine(); + const b = await seedOrganization(engine, ORG_B, '0'); + const a = await seedOrganization(engine, ORG_A, 'a'); + await hold(engine, ORG_A, [{ id: 'up_a', user_id: 'usr_a1', business_unit_id: a.salesEast }]); + await hold(engine, ORG_B, [{ id: 'up_b', user_id: 'usr_b1', business_unit_id: b.salesEast }]); + + await expectRefused( + bind(gateOn(engine), ORG_A, b.fieldLead, a.salesUser), + /position 'field_lead' is held in business unit 'bu_0_sales_east', outside the delegated subtree/, + ); + }); + + it('control (dark) — org A\'s own holder outside the subtree still refuses', async () => { + const engine = await bootEngine(); + const a = await seedOrganization(engine, ORG_A, 'a'); + await engine.insert('sys_business_unit', [ + { id: 'bu_a_ops', name: 'ops', parent_business_unit_id: null }, + ], { context: sysCtx(ORG_A) } as any); + await hold(engine, ORG_A, [{ id: 'up_a', user_id: 'usr_a1', business_unit_id: 'bu_a_ops' }]); + + await expectRefused( + bind(gateOn(engine), ORG_A, a.fieldLead, a.salesUser), + /position 'field_lead' is held in business unit 'bu_a_ops', outside the delegated subtree/, + ); + }); +}); diff --git a/packages/plugins/plugin-security/src/delegated-admin-gate.ts b/packages/plugins/plugin-security/src/delegated-admin-gate.ts index dabd229755d..44f891d3293 100644 --- a/packages/plugins/plugin-security/src/delegated-admin-gate.ts +++ b/packages/plugins/plugin-security/src/delegated-admin-gate.ts @@ -95,6 +95,28 @@ function callerOrganizationId(context: any): string | undefined { return typeof id === 'string' && id !== '' ? id : undefined; } +/** + * Does a `sys_user_position` row take effect in `organizationId`? + * + * The runtime resolver's own answer (`resolveAuthzContext` step 4, in + * `@objectstack/core`): a holding stamped with a DIFFERENT organization grants + * nothing there; an organization-less holding grants in every organization; + * `organizationId` undefined (a `single`-posture caller) drops nothing. + * + * Every holding the gate reasons about goes through this, because + * `sys_user_position.position` is a position NAME and `sys_position.name` is + * unique per organization only — so under a walled posture another + * organization's holding of a same-named position is the same string. The + * gate must count exactly the holdings the runtime grants: counting another + * organization's lets a user self-delegate a position they do not hold there; + * dropping organization-less ones refuses holdings the runtime honours. + */ +function holdingTakesEffectIn(row: any, organizationId?: string): boolean { + if (!organizationId) return true; + const org = rowOrganizationId(row); + return !org || org === organizationId; +} + /** Coerce a stored timestamp to epoch ms; undefined = absent, NaN = unparseable. */ function toEpochMs(value: unknown): number | undefined { if (value == null || value === '') return undefined; @@ -566,7 +588,7 @@ export class DelegatedAdminGate { } // 4. Delegator currently holds it, directly (no re-delegation). - const holdings = await this.activeHoldings(String(ctx.userId), positionName, now); + const holdings = await this.activeHoldings(String(ctx.userId), positionName, now, organizationId); const directHolding = holdings.some((h) => h.direct); if (!directHolding) { if (holdings.length > 0) { @@ -629,11 +651,17 @@ export class DelegatedAdminGate { * delegation (only a direct holding is re-delegatable) and carrying each * holding's own `businessUnitId` anchor (null = unanchored). The anchor of a * direct holding bounds what a self-delegation of that position may hand out - * (cloud#830 — the anchor is visibility load-bearing). */ + * (cloud#830 — the anchor is visibility load-bearing). + * + * Only holdings that take effect in the caller's organization count + * ({@link holdingTakesEffectIn}): a holding stamped for another organization + * never answers "you currently hold it" — nor, through its anchor or its + * directness, widens what the caller may delegate here. */ private async activeHoldings( userId: string, positionName: string, now: number, + organizationId?: string, ): Promise> { const ql = this.deps.ql; if (!ql?.find) return []; @@ -648,6 +676,7 @@ export class DelegatedAdminGate { rows = []; } return (Array.isArray(rows) ? rows : []) + .filter((r) => holdingTakesEffectIn(r, organizationId)) .filter((r) => isGrantActive(r, now)) .map((r) => ({ direct: r?.delegated_from == null || r.delegated_from === '', @@ -800,7 +829,8 @@ export class DelegatedAdminGate { const targets = await this.materializeTargets(opCtx, 'sys_position_permission_set'); for (const t of targets) { const row = t.next ?? t.prev ?? {}; - const positionName = await this.positionNameById(row.position_id); + const position = await this.positionById(row.position_id); + const positionName = position.name; if (ANCHOR_POSITIONS.has(positionName)) { throw new PermissionDeniedError( `[Security] Access denied: bindings of the '${positionName}' audience anchor are ` + @@ -810,7 +840,9 @@ export class DelegatedAdminGate { } const setRow = await this.loadSetRowById(row.permission_set_id); const setName = String(setRow?.name ?? row.permission_set_id ?? ''); - const radius = positionName ? await this.assignmentAnchorsOfPosition(positionName) : { anchors: new Set(), overCap: false, unanchored: 0 }; + const radius = positionName + ? await this.assignmentAnchorsOfPosition(positionName, position.organizationId) + : { anchors: new Set(), overCap: false, unanchored: 0 }; const failure = this.firstApprovalFailure(held, (s) => { if (!s.scope.manageBindings) return 'the scope does not grant manageBindings'; @@ -1210,19 +1242,46 @@ export class DelegatedAdminGate { } } - private async positionNameById(id: unknown): Promise { - if (id == null || !this.deps.ql?.find) return ''; + /** A `sys_position` row by id: its name, and the organization that owns it + * (undefined = an organization-less row). */ + private async positionById(id: unknown): Promise<{ name: string; organizationId: string | undefined }> { + const none = { name: '', organizationId: undefined }; + if (id == null || !this.deps.ql?.find) return none; try { const rows = await this.deps.ql.find('sys_position', { where: { id }, limit: 1, context: SYSTEM_CTX }); - return String((Array.isArray(rows) && rows[0] ? (rows[0] as any).name : '') ?? ''); + const row = Array.isArray(rows) && rows[0] ? (rows[0] as any) : null; + if (!row) return none; + return { name: String(row.name ?? ''), organizationId: rowOrganizationId(row) || undefined }; } catch { - return ''; + return none; } } - /** BU anchors of every current assignment of a position (blast radius). */ + /** + * BU anchors of every assignment a re-binding of this position row reaches + * (blast radius). + * + * An assignment names the position by NAME, and a binding hangs off one + * `sys_position` ROW, which the runtime reaches only from the organization + * that owns it. So the holders a binding re-composes are the ones whose + * holding takes effect in the bound row's organization + * ({@link holdingTakesEffectIn}) — that organization's own holdings plus the + * organization-less ones — never another organization's same-named + * assignments, which would otherwise refuse the write as outside the + * subtree or push it over {@link BLAST_RADIUS_CAP}. Keyed on the bound + * row's organization rather than the caller's, so a binding written against + * another organization's row is still judged by THAT organization's holders. + * An organization-less row reaches every organization: no holding is dropped. + * + * The organization rides the read's context, so the driver's tenant scope + * (`organization_id = :org OR organization_id IS NULL`, the governed spelling + * of exactly this rule) applies BEFORE the cap — another organization's rows + * can neither fill the page nor trip {@link BLAST_RADIUS_CAP}. The same rule + * is re-applied to what comes back, for a driver that does not scope. + */ private async assignmentAnchorsOfPosition( positionName: string, + positionOrganizationId?: string, ): Promise<{ anchors: Set; overCap: boolean; unanchored: number }> { const anchors = new Set(); let unanchored = 0; @@ -1233,14 +1292,14 @@ export class DelegatedAdminGate { rows = await ql.find('sys_user_position', { where: { position: positionName }, limit: BLAST_RADIUS_CAP + 1, - context: SYSTEM_CTX, + context: organizationScopedCtx(positionOrganizationId), }); } catch { rows = []; } const list = Array.isArray(rows) ? rows : []; if (list.length > BLAST_RADIUS_CAP) return { anchors, overCap: true, unanchored }; - for (const r of list) { + for (const r of list.filter((row) => holdingTakesEffectIn(row, positionOrganizationId))) { const bu = (r as any)?.business_unit_id; if (bu == null || bu === '') unanchored += 1; else anchors.add(String(bu));