From 5e6b3368715cbc689c837ddd5324a0167ff761df Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 08:03:25 +0000 Subject: [PATCH] fix(plugin-security): every delegable-scope branch leaves out the built-in identity positions describeDelegableScope's tenant-level and delegated branches offered the ADR-0068 D2 identity names (platform_admin, org_owner, org_admin, org_member), which sys_user_position.position refuses from every writer. The shared position listing every branch reads now applies the isReservedIdentityName predicate once, and the organization-level branch's own copy of the skip is folded into it. Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK Co-authored-by: Claude --- ...7-delegable-scope-no-identity-positions.md | 11 + ...cope-no-identity-positions.posture.test.ts | 218 ++++++++++++++++++ .../src/delegated-admin-gate.ts | 30 ++- 3 files changed, 251 insertions(+), 8 deletions(-) create mode 100644 .changeset/24737-delegable-scope-no-identity-positions.md create mode 100644 packages/plugins/plugin-security/src/delegable-scope-no-identity-positions.posture.test.ts diff --git a/.changeset/24737-delegable-scope-no-identity-positions.md b/.changeset/24737-delegable-scope-no-identity-positions.md new file mode 100644 index 00000000000..2ad0c917e66 --- /dev/null +++ b/.changeset/24737-delegable-scope-no-identity-positions.md @@ -0,0 +1,11 @@ +--- +'@objectstack/plugin-security': patch +--- + +fix(plugin-security): the delegable-scope report no longer offers the built-in identity positions to a platform administrator or a delegate + +Clause-②: no + +`GET /security/my-delegable-scope` (`describeDelegableScope`) listed `platform_admin`, `org_owner`, `org_admin` and `org_member` in `assignablePositions` for a tenant-level administrator and for a delegate holding a placing `adminScope`. `sys_user_position.position` refuses those four names from every writer with `400 VALIDATION_FAILED`, so the invite dialog and Setup's position picker offered choices that could only fail. + +Every branch of the report (tenant-level, organization-level, delegated) now reads one position listing that leaves out the ADR-0068 D2 identity names through the same `isReservedIdentityName` predicate the write's validation is built from, beside the audience anchors it already left out. No other position leaves any branch, and placeable business units are unchanged. The write path's refusal and the reserved list are unchanged. No spec key, endpoint or configuration changes. diff --git a/packages/plugins/plugin-security/src/delegable-scope-no-identity-positions.posture.test.ts b/packages/plugins/plugin-security/src/delegable-scope-no-identity-positions.posture.test.ts new file mode 100644 index 00000000000..ea4d7765c7f --- /dev/null +++ b/packages/plugins/plugin-security/src/delegable-scope-no-identity-positions.posture.test.ts @@ -0,0 +1,218 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [ADR-0090 D12 / ADR-0105 D8 × ADR-0068 D2] The delegable-scope report offers + * only positions the write admits, in EVERY branch of `describeDelegableScope`: + * the tenant-level administrator, the organization-level administrator and the + * delegate holding a placing `adminScope`. The ADR-0068 D2 built-in identity + * names are declared positions the security catalog lists, carry no sets, and + * are refused by `sys_user_position.position` from every writer — so no branch + * offers one. + * + * Measured on a REAL `ObjectQL` engine over a real SQL driver with the REAL + * `SecurityPlugin` registered on it (its declared built-in positions, the + * delegated-administration gate, the CRUD check, the object validations and + * the grant-holder membership hooks in their production order), in the three + * postures the sibling posture suites cover. Each branch is held three ways: + * none of the four identity names is listed; a non-identity control position + * still is; and every listed position is admitted by the real write, read + * back from storage. The ground truth beside it — the catalog this stack + * serves holds the four names, and the write refuses each one from the same + * caller with the ADR-0112 envelope — is what keeps the first assertion from + * passing over a catalog that never held them. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { resolveThrownHttpError } from '@objectstack/types'; +import { securityCatalogReaderOf } from '@objectstack/core'; +import { SysUser, SysMember, SysBusinessUnit, SysBusinessUnitMember } from '@objectstack/platform-objects/identity'; + +import { SecurityPlugin } from './security-plugin.js'; +import { SysUserPosition } from './objects/sys-user-position.object.js'; +import { SysUserPermissionSet } from './objects/sys-user-permission-set.object.js'; +import { RESERVED_IDENTITY_NAMES } from './objects/reserved-identity-names.js'; +import { defaultPermissionSets } from './objects/index.js'; + +const SYS = { isSystem: true } as const; +const ORG_A = 'org_a'; +const CONTROL = 'field_rep'; + +/** + * The catalog. `field_user` is inside the organization boundary (no superuser + * bit, no adminScope, no capability) and is the one set the delegate's scope + * allowlists; `field_rep` distributes it alone, so every branch may assign it. + */ +const PERMISSIONS = [ + ...defaultPermissionSets, + { name: 'field_user', label: 'Field user', objects: { sys_user: { allowRead: true } } }, + { + name: 'east_admin', + label: 'East delegate', + objects: { sys_user_position: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } }, + adminScope: { businessUnit: 'east', includeSubtree: true, manageAssignments: true, assignablePermissionSets: ['field_user'] }, + }, +]; +const POSITIONS = [{ name: CONTROL, label: 'Field rep', permissionSets: ['field_user'] }]; + +type Posture = 'single' | 'group' | 'isolated'; +const POSTURES: Posture[] = ['single', 'group', 'isolated']; + +/** One caller per branch of the report, and how the report must name the branch it took. */ +const BRANCHES = { + 'tenant-level': { + context: { userId: 'u_platform', positions: [], permissions: ['admin_full_access'], posture: 'PLATFORM_ADMIN', tenantId: ORG_A, accessible_org_ids: [ORG_A] }, + isTenantAdmin: true, + scopes: [] as string[], + }, + 'organization-level': { + context: { userId: 'u_org_admin', positions: [], permissions: ['organization_admin'], tenantId: ORG_A, accessible_org_ids: [ORG_A] }, + isTenantAdmin: false, + scopes: [] as string[], + }, + delegated: { + context: { userId: 'u_delegate', positions: [], permissions: ['east_admin'], tenantId: ORG_A, accessible_org_ids: [ORG_A] }, + isTenantAdmin: false, + scopes: ['east_admin'], + }, +} as const; +type Branch = keyof typeof BRANCHES; + +const MEMBERSHIPS = [ + { id: 'm_platform', user_id: 'u_platform', role: 'member' }, + { id: 'm_org_admin', user_id: 'u_org_admin', role: 'admin' }, + { id: 'm_delegate', user_id: 'u_delegate', role: 'member' }, + { id: 'm_target', user_id: 'u_target', role: 'member' }, +]; + +const engines: ObjectQL[] = []; +afterEach(async () => { + vi.restoreAllMocks(); + while (engines.length) { + try { await engines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +async function boot(posture: Posture) { + 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.delegable-scope-no-identity-positions', + name: 'The delegable-scope report offers no built-in identity position', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: [SysUserPosition, SysUserPermissionSet, SysMember, SysBusinessUnit, SysBusinessUnitMember], + } as any); + await engine.syncSchemas(); + // `sys_user` registered and unprovisioned, as in the sibling posture suites. + if (!engine.registry.getObject(SysUser.name)) engine.registry.registerObject(SysUser as never, 'qa.delegable-scope-identity'); + engines.push(engine); + + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + // [ADR-0131 D3/D4] The stack-declared half of the catalog; the built-in + // positions are the plugin's own declarations, in the engine registry. + metadata: { + get: async (type: string, name: string) => + (type === 'position' ? POSITIONS.find((p) => p.name === name) + : type === 'permission' ? PERMISSIONS.find((p) => p.name === name) + : type === 'capability' ? undefined + : engine.getSchema(name)) ?? null, + list: async (type: string) => (type === 'position' ? POSITIONS : type === 'permission' ? PERMISSIONS : []), + }, + ...(posture === 'single' ? {} : { 'org-scoping': { name: 'com.objectstack.org-scoping' }, tenancy: { posture } }), + }; + const ctx: any = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + hook: vi.fn(), + registerService: vi.fn(), + getService: (name: string) => { + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + await plugin.init(ctx); + await plugin.start(ctx); + vi.spyOn((engine as any).logger, 'warn').mockImplementation(() => undefined); + + const sys = { context: { isSystem: true, tenantId: ORG_A } } as any; + await engine.insert('sys_business_unit', [ + { id: 'bu_hq', name: 'hq', parent_business_unit_id: null, organization_id: ORG_A }, + { id: 'bu_east', name: 'east', parent_business_unit_id: 'bu_hq', organization_id: ORG_A }, + { id: 'bu_east_field', name: 'east_field', parent_business_unit_id: 'bu_east', organization_id: ORG_A }, + ], sys); + for (const m of MEMBERSHIPS) await engine.insert('sys_member', { ...m, organization_id: ORG_A }, sys); + await engine.insert('sys_business_unit_member', + { id: 'bum_target', user_id: 'u_target', business_unit_id: 'bu_east_field', organization_id: ORG_A }, sys); + return { engine, plugin }; +} +type Harness = Awaited>; + +async function outcome(run: () => Promise): Promise<{ status: number; code?: string; message?: string }> { + try { + await run(); + return { status: 200 }; + } catch (e) { + const r = resolveThrownHttpError(e); + return { status: r.status, code: r.code, message: (e as Error).message }; + } +} + +/** Assign `position` to the target, in a unit every branch's caller may place into. */ +const assign = (h: Harness, context: any, position: string) => + outcome(() => h.engine.insert('sys_user_position', + { user_id: 'u_target', position, business_unit_id: 'bu_east_field' }, { context } as any)); + +async function storedRows(h: Harness, position: string): Promise { + const rows = await h.engine.find('sys_user_position', { where: { user_id: 'u_target', position }, context: SYS }); + return Array.isArray(rows) ? rows : []; +} + +describe('[ADR-0090 D12 / ADR-0105 D8 × ADR-0068 D2] every branch of the delegable-scope report offers only positions the write admits — three postures', () => { + for (const posture of POSTURES) { + describe(`posture '${posture}'`, () => { + for (const branch of Object.keys(BRANCHES) as Branch[]) { + it(`${branch}: no built-in identity name is offered, the control still is, and every offered position is admitted`, async () => { + const h = await boot(posture); + const security = h.plugin as any; + const { context, isTenantAdmin, scopes } = BRANCHES[branch]; + + // Ground truth: the catalog this stack serves lists the four names, + // and the write refuses each one from this very caller. + const catalogNames = (await securityCatalogReaderOf(h.engine)!.list('position')).map((e) => e.name); + expect(catalogNames).toEqual(expect.arrayContaining([...RESERVED_IDENTITY_NAMES, CONTROL])); + for (const name of RESERVED_IDENTITY_NAMES) { + const r = await assign(h, context, name); + expect([r.status, r.code], `${name}: ${r.message}`).toEqual([400, 'VALIDATION_FAILED']); + expect(await storedRows(h, name), `${name} stored nothing`).toHaveLength(0); + } + + const sets = await security.resolvePermissionSetsForContext(context); + const report = await security.delegatedAdminGate.describeDelegableScope(sets, context); + // The caller reached the branch this case names. + expect(report.isTenantAdmin).toBe(isTenantAdmin); + expect(report.scopes.map((s: any) => s.setName)).toEqual(scopes); + + const offered = report.assignablePositions as string[]; + for (const name of RESERVED_IDENTITY_NAMES) expect(offered, `${name} offered`).not.toContain(name); + expect(offered).toContain(CONTROL); + for (const position of offered) { + const r = await assign(h, context, position); + expect(r, `${position}: ${r.message ?? 'admitted'}`).toEqual({ status: 200 }); + const rows = await storedRows(h, position); + expect(rows, `${position} stored`).toHaveLength(1); + await h.engine.delete('sys_user_position', { where: { id: rows[0].id }, context: SYS } as any); + } + }); + } + }); + } +}); diff --git a/packages/plugins/plugin-security/src/delegated-admin-gate.ts b/packages/plugins/plugin-security/src/delegated-admin-gate.ts index 2a202d1153e..08da6da24c9 100644 --- a/packages/plugins/plugin-security/src/delegated-admin-gate.ts +++ b/packages/plugins/plugin-security/src/delegated-admin-gate.ts @@ -224,7 +224,11 @@ export interface DelegatedAdminGateDeps { * {@link DelegatedAdminGate.describeDelegableScope}). */ export interface DelegableScopeReport { - /** ADR-0066 superuser wildcard: unconstrained. The lists below then enumerate everything. */ + /** + * ADR-0066 superuser wildcard: unconstrained. The lists below then enumerate + * everything the write path admits — never an audience anchor or a built-in + * identity name, which no writer may assign. + */ isTenantAdmin: boolean; /** Every `adminScope` the caller holds, with its subtree resolved to ids. */ scopes: Array<{ @@ -553,8 +557,10 @@ export class DelegatedAdminGate { * organization-less surface, correct for a `single` posture. * * A tenant-level admin (ADR-0066 superuser wildcard) is unconstrained; - * the report says so AND enumerates everything, so a consumer can render - * one uniform picker instead of special-casing. An organization + * the report says so AND enumerates everything the write path admits, so a + * consumer can render one uniform picker instead of special-casing. Every + * branch reads one position listing, which never offers a name no writer + * may assign (an audience anchor or a built-in identity). An organization * administrator (D12 as amended 2026-10-17) is not: `isTenantAdmin` is * false, every unit of its organization is placeable and the positions * inside the organization boundary are assignable — on the same report @@ -570,13 +576,22 @@ export class DelegatedAdminGate { // per-organization `sys_position` rows: a position declared only in the // registry is assignable, and a row with no definition grants nothing // (the resolver reads the catalog), so it is not offered. + // + // Two kinds of catalog name are never offered, by ANY branch below, + // because no writer may assign them: the ADR-0090 D9 audience anchors + // (refused by `authorize`'s unconditional invariant) and the ADR-0068 D2 + // built-in identity names (`sys_user_position.position` refuses them from + // every writer, tenant-level administrators included — the shared + // `isReservedIdentityName` predicate that validation is generated from). + // They carry no sets, so no branch's own test would settle them: the + // listing every branch reads leaves them out, once. const allPositions = async (): Promise => { const catalog = this.catalog(); if (!catalog) return []; try { return (await catalog.list('position')) .map((entry) => entry.name) - .filter((n) => n && !ANCHOR_POSITIONS.has(n)); + .filter((n) => n && !ANCHOR_POSITIONS.has(n) && !isReservedIdentityName(n)); } catch { return []; // a picker over an unreadable catalog offers nothing (fail closed) } @@ -636,15 +651,14 @@ export class DelegatedAdminGate { // into every unit of its organization and assigns every position inside // the organization boundary — `organizationAdminFailure`'s own test, so an // option offered here is one the write path admits. The ADR-0068 D2 - // identity names carry no sets, so the boundary does not settle them, but - // `sys_user_position.position` refuses them from every writer: they are - // not offered either. + // identity names carry no sets, so the boundary does not settle them; the + // shared listing has already left them out (see `allPositions`). if (holdsOrganizationAdmin(sets) && organizationId) { for (const id of await this.organizationBusinessUnitIds(organizationId)) { if (!placeableBusinessUnitIds.includes(id)) placeableBusinessUnitIds.push(id); } for (const positionName of await allPositions()) { - if (assignablePositions.includes(positionName) || isReservedIdentityName(positionName)) continue; + if (assignablePositions.includes(positionName)) continue; let failure: unknown; try { failure = await this.positionBoundaryFailure(positionName);