diff --git a/.changeset/positions-name-is-not-platform-authority.md b/.changeset/positions-name-is-not-platform-authority.md new file mode 100644 index 0000000000..76c63b3a8f --- /dev/null +++ b/.changeset/positions-name-is-not-platform-authority.md @@ -0,0 +1,21 @@ +--- +"@objectstack/plugin-sharing": patch +"@objectstack/plugin-approvals": patch +"@objectstack/plugin-security": patch +"@objectstack/runtime": patch +--- + +Four server-side authorization sites stop deriving platform-operator authority from a NAME in `ExecutionContext.positions`, and read the ADR-0095 posture rung instead. + +`positions[]` is the security axis, so it carries ADR-0057 D4 `sys_user_position` names alongside the built-ins. `sys_user_position` is `apiEnabled` and its `position` values are unconstrained, so a tenant could mint a row spelling `platform_admin` for one of their own users: `resolveUserAuthzGrants` pushed that name straight onto `grants.positions`, while `grants.posture` — derived from the unscoped `admin_full_access` grant and nothing else — correctly stayed `MEMBER`. Every reader of the name therefore answered `true` for a principal enforcement treats as an ordinary member. `resolve-authz-context.ts` states the rule at `hasPlatformAdminStanding` ("read the RUNG — never `positions.includes(...)`"), but a comment is not a gate and these four had not followed it. + +Each site now tests `posture === 'PLATFORM_ADMIN'`, byte-for-byte what `hasPlatformAdminStanding` returns: + +- **`plugin-sharing`** — `hasPlatformAuthority`. The minted row satisfied `assertResolvableAdminScope`, so an org-less caller holding only the ORG-scoped `manage_sharing` capability was answered with **every tenant's** sharing rules, and could delete platform-global rules. The `manage_platform_settings` capability spelling is unchanged. +- **`plugin-approvals`** — `isOverrideActor`. This predicate already read the rung and then ORed the name onto it, which is no protection: an OR is only as strong as its weakest arm. Because the platform arm deliberately crosses the tenant wall, the minted row let a member of one organization approve, reject or recall a **different organization's** pending request while holding no slot in its slate. The `ADMIN_FULL_ACCESS` capability arm and both TENANT_ADMIN arms are unchanged. +- **`runtime`** — the ADR-0126 §5 activation gate. Under a `group` or `isolated` posture this gate is the only thing between a tenant org admin and the **install-wide** `sys_metadata_activation` row, so the minted row reopened #10243 with a durable row behind it. +- **`plugin-security`** — `derivePosture` in the explain engine. Narrower than the other three, and stated precisely rather than overclaimed: the name-read sat behind an early `ctx.posture` return that `buildContextForUser` always populates, so the shipping path was already gated and a D4 row never moved it. What the read did reach was a posture-less hand-built context, where it made the panel **report** `PLATFORM_ADMIN` for a principal enforcement treats as a MEMBER — a misreport rather than an admission, but in the one tool an administrator opens to check exactly this. + +No behaviour changes for a genuine platform operator: their resolved context carries the rung, and the built-in position is still projected onto `positions[]` for display and predicate use. What changes is that the name alone no longer answers the authorization question. + +Graded `patch` on the surface it moves: no exported type, signature or contract changes, and no authorable metadata is added, removed or renamed. The only observable difference is that a principal who never held the capability grant stops being admitted — which is the defect, not a feature anyone could have depended on. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index ac5d133bfe..1a78541625 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -137,7 +137,7 @@ The largest single consumer — **17 of the 105 sites**. | 36 | `sys_record_share` reads are **not** self-scoped | Get: tenant-wide share listing without `manage_sharing` | `sharing-plugin.ts:1189` | | 37 | Share-link policy `enabled` check bypassed; system callers re-enter under a system context | Get: link **creation** while the policy is off — resolution is **not** bypassed since #14033 (`publicSharing.enabled` is a standing policy held at every redemption): a link minted this way does not resolve until the block is enabled | `plugin-sharing/src/share-link-service.ts:459`, `:513`, `:517`, `:590`, `:620` | | 38 | Sharing-rule provenance stamp skipped | Lose: the row is not marked as an admin customization — seeder / `defineRule` / boot reconcilers are "the package door" | `sharing-rule-provenance.ts:66` | -| 39 | Sharing-rule service write + delete paths return early | Lose: the manage-rules gate on the service surface, and the platform-global-rule delete guard | `sharing-rule-service.ts:278`, `:503` | +| 39 | Sharing-rule service write + delete paths return early | Lose: the manage-rules gate on the service surface, and the platform-global-rule delete guard | `sharing-rule-service.ts:279`, `:518` | ### 4. Approvals, reports, attachments, comments, knowledge @@ -145,7 +145,7 @@ The largest single consumer — **17 of the 105 sites**. |:--|:---|:---|:---|:---| | 40 | **Approval record lock released** — a locked record is writable | plugin-approvals | Get: engine self-writes (the status mirror) pass. Lose: the lock that stops edits while an approval is live. Note there is deliberately **no admin exemption** here — only `isSystem` | `lifecycle-hooks.ts:347` | | 41 | Delegation write guard bypassed | plugin-approvals | Get: service / seed / import may write delegation rows naming another delegator | `lifecycle-hooks.ts:570` | -| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:1093`, `:1202`, `:3458`, `:3606`, `:3774`, `:3845`, `:4034`, `:4074` | +| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:1096`, `:1219`, `:3475`, `:3623`, `:3791`, `:3862`, `:4051`, `:4091` | | 43 | Saved-report ownership is **assignable**, and an update may reassign it | plugin-reports | Get: `ownerId` from input is honoured. A non-system caller always owns what it creates and can never reassign | `plugin-reports/src/report-service.ts:404`, `:425` | | 44 | Saved-report access / export / mutation gates bypassed | plugin-reports | Get: read, bulk-export and overwrite any report | `plugin-reports/src/report-service.ts:343`, `:372`, `:447`, `:684` | | 45 | Attachment access hooks return early (insert + update + delete, and the read AST) | service-storage | Lose: attachment visibility scoping | `attachment-access-hooks.ts:300`, `:349`, `:448`, `:524` | @@ -164,7 +164,7 @@ The largest single consumer — **17 of the 105 sites**. | 53 | MCP principal check satisfied | runtime | Get: MCP surface reachable with no user | `domains/mcp.ts:61` | | 54 | Package REST route capability gate bypassed | rest | Get: package read/write over REST without `manage_metadata` / `studio.access` / `setup.access` | `package-routes.ts:102` | | 55 | Package domain capability gates bypassed | runtime | Get: package management and package-inventory reads without the capability | `domains/packages.ts:241`, `:274` | -| 56 | Activation write / authoring refusals do not fire | runtime | Get: activation artifacts writable and authorable without the activation-authoring capability | `activation-gate.ts:138`, `:189` | +| 56 | Activation write / authoring refusals do not fire | runtime | Get: activation artifacts writable and authorable without the activation-authoring capability | `activation-gate.ts:157`, `:211` | | 57 | Automation run-state read, flow-authoring write and unrelated-screen read all pass | runtime | Get: run state, flow writes and screen reads with no grant | `domains/automation.ts:254`, `:545`, `:635` | | 58 | Audience-binding suggestion recording skipped | plugin-security | Lose: install-time suggestions are not recorded for system callers | `suggested-audience-bindings.ts:703` | | 59 | Email-template / webhook provenance stamps skipped | plugin-email, plugin-webhooks | Lose: the row is not marked as an admin customization | `email-template-provenance.ts:77`, `webhook-provenance.ts:68` | diff --git a/packages/plugins/plugin-approvals/src/approval-positions-name-authority.test.ts b/packages/plugins/plugin-approvals/src/approval-positions-name-authority.test.ts new file mode 100644 index 0000000000..b75bdbf35b --- /dev/null +++ b/packages/plugins/plugin-approvals/src/approval-positions-name-authority.test.ts @@ -0,0 +1,404 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#15981] A `sys_user_position` row SPELLING `platform_admin` confers no + * override authority on the approvals surface. + * + * ## The defect this pins + * + * `isOverrideActor` read the capability rung FIRST and then ORed a NAME onto + * it: + * + * const isPlatformAdmin = posture === 'PLATFORM_ADMIN' + * || perms.includes(ADMIN_FULL_ACCESS) + * || positions.includes(BUILTIN_IDENTITY_PLATFORM_ADMIN); // ← the hole + * + * Reading the rung first does not close the hole; an OR is only as strong as + * its weakest arm. `sys_user_position` is `apiEnabled` and its `position` + * values are unconstrained, so a tenant can mint an ADR-0057 D4 row spelling + * that built-in name, `resolveUserAuthzGrants` §4 pushes it into + * `grants.positions`, and the third arm answered `true` while `grants.posture` + * — derived from the unscoped `admin_full_access` grant and nothing else — + * stayed `MEMBER`. + * + * ⭐ WHAT THE ESCALATION BUYS, driven rather than argued: the platform arm of + * `isOverrideActor` deliberately CROSSES THE TENANT WALL (the tenant-admin arm + * below it is confined to the actor's own org). So a member of one tenant + * holding the minted row could decide a PENDING request belonging to a + * DIFFERENT tenant, holding no slot in its slate — see the cross-tenant arm. + * + * ## POPULATION OF THIS PIN — stated because a pin proves only what it covers + * + * Covers: `decideNode` (the approve/reject door) and `listRequests` + * (the console's participant-visibility read, `visibleRequestIds`), for a + * NON-slate-holding actor in a DIFFERENT organization from the request, in + * three shapes — a D4 row spelling the built-in name with no capability grant + * (`name-only`), a genuine unscoped `admin_full_access` grant (`genuine`), and + * a plain member (`plain`, the floor that proves the arms are not all passing + * for some unrelated reason). The `name-only` and `genuine` contexts are built + * by inserting rows and resolving them through the REAL + * `resolveUserAuthzGrants`. + * + * Does NOT cover: the `ADMIN_FULL_ACCESS` permission-name arm or the + * TENANT_ADMIN arms of `isOverrideActor` (untouched by this change; the + * `org_owner` / `org_admin` name-reads on the tenant arm are a NARROWER + * question left to #15972's write-side card and not silently widened here), + * the SLA/escalation paths, or the ADR-0091 validity window. Those are other + * suites' populations, and their passing is NOT evidence about this one. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { BUILTIN_IDENTITY_PLATFORM_ADMIN, ADMIN_FULL_ACCESS } from '@objectstack/spec/identity'; +import type { ExecutionContext } from '@objectstack/spec/kernel'; +import { hasPlatformAdminStanding, resolveUserAuthzGrants } from '@objectstack/core'; +// The engine's OWN dispatch predicates, so this double cannot be looser than +// the engine it stands in for (`check:engine-double-contract`). +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/metadata-core'; +import type { ApprovalRequestRow } from '@objectstack/spec/contracts'; +import { ApprovalService, type ApprovalNodeAutoOutcome } from './approval-service.js'; + +interface FakeRow { [k: string]: any } + +/** The same minimal engine shape `approval-override-audit.test.ts` uses. */ +function makeFakeEngine() { + const tables: Record = {}; + const ensure = (n: string) => (tables[n] ??= []); + function matches(row: FakeRow, filter: any): boolean { + if (!filter || typeof filter !== 'object') return true; + for (const [k, v] of Object.entries(filter)) { + if (k === '$or') { + if (!(v as any[]).some((sub) => matches(row, sub))) return false; + continue; + } + if (k.startsWith('$')) throw new Error(`fake engine: unsupported filter operator ${k}`); + const rv = row[k]; + if (v != null && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(rv)) return false; + continue; + } + if (rv !== v) return false; + } + return true; + } + return { + _tables: tables, + async find(object: string, options?: any) { + const rows = ensure(object).filter((r) => matches(r, options?.filter ?? options?.where)); + const start = options?.offset ?? 0; + return rows.slice(start, start + (options?.limit ?? 1000)); + }, + async insert(object: string, data: any) { ensure(object).push({ ...data }); return { ...data }; }, + // `update` / `delete` route through the ENGINE'S OWN dispatch predicates + // rather than a hand-mirrored copy: a double looser than the engine it + // stands in for is how #4434 shipped a dead REST route with its suite + // green, and `check:engine-double-contract` grades exactly this. + async update(object: string, data: any, options?: any) { + const dispatch = assertEngineUpdateDispatch(data, options); + const table = ensure(object); + if (dispatch.kind === 'multi') { + let n = 0; + for (let i = 0; i < table.length; i++) { + if (matches(table[i], options?.where)) { table[i] = { ...table[i], ...data }; n++; } + } + return { updated: n }; + } + const i = table.findIndex((r) => r.id === dispatch.id); + if (i >= 0) table[i] = { ...table[i], ...data }; + return i >= 0 ? { ...table[i] } : null; + }, + async delete(object: string, options?: any) { + const dispatch = assertEngineDeleteDispatch(options); + const table = ensure(object); + if (dispatch.kind === 'multi') { + const survivors = table.filter((r) => !matches(r, options?.where)); + const deleted = table.length - survivors.length; + table.splice(0, table.length, ...survivors); + return { deleted }; + } + const i = table.findIndex((r) => r.id === dispatch.id); + if (i >= 0) table.splice(i, 1); + return { id: dispatch.id }; + }, + registerHook() {}, unregisterHooksByPackage() { return 0; }, async fire() {}, + }; +} + +const VICTIM_ORG = 't_victim'; +const ATTACKER_ORG = 't_attacker'; +const ATTACKER = 'usr_attacker'; +const PS_ADMIN = 'ps_admin_full_access'; + +/** + * A minimal ObjectQL double for the AUTHZ resolver — the shape (and the + * top-level `$` refusal) of `resolve-authz-context.platform-admin-config.test.ts`. + */ +function makeAuthzQl(tables: Record>>) { + const matches = (row: Record, where: any): boolean => + Object.entries(where ?? {}).every(([k, v]) => { + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + if (v && typeof v === 'object' && '$in' in (v as any)) return (v as any).$in.includes(row[k]); + return row[k] === v; + }); + return { + async find(object: string, opts: any) { + const rows = (tables[object] ?? []).filter((r) => matches(r, opts?.where)); + return typeof opts?.limit === 'number' ? rows.slice(0, opts.limit) : rows; + }, + }; +} + +type Shape = 'name-only' | 'genuine' | 'plain'; + +function authzTables(shape: Shape) { + return { + sys_user: [{ id: ATTACKER, email: 'attacker@example.com', email_verified: true }], + sys_member: [{ organization_id: ATTACKER_ORG, user_id: ATTACKER, role: 'member' }], + sys_user_position: + shape === 'name-only' + ? [ + // Exactly what a tenant admin can write through the `apiEnabled` + // `sys_user_position` surface: a row whose NAME is the built-in. + { user_id: ATTACKER, position: BUILTIN_IDENTITY_PLATFORM_ADMIN, organization_id: null }, + ] + : [], + // An ACTIVE catalogue row, so ADR-0049's deactivated-position filter is not + // what carries the arm. + sys_position: + shape === 'name-only' + ? [{ id: 'pos_pa', name: BUILTIN_IDENTITY_PLATFORM_ADMIN, label: 'Platform Admin', active: true }] + : [], + sys_position_permission_set: [], + sys_user_permission_set: + shape === 'genuine' + ? [{ user_id: ATTACKER, permission_set_id: PS_ADMIN, organization_id: null }] + : [], + sys_permission_set: [{ id: PS_ADMIN, name: ADMIN_FULL_ACCESS, active: true }], + }; +} + +/** + * Resolve one principal through the REAL resolver, then build the context a + * transport would. `tenantId` is the ATTACKER's own organization — the request + * under test belongs to a different one, which is what makes the platform arm + * (and only the platform arm) able to reach it. + */ +async function resolve(shape: Shape) { + const ql = makeAuthzQl(authzTables(shape)); + const grants = await resolveUserAuthzGrants(ql as any, ATTACKER, { tenantId: ATTACKER_ORG }); + const context = { + userId: ATTACKER, + tenantId: ATTACKER_ORG, + positions: grants.positions, + permissions: grants.permissions, + systemPermissions: grants.systemPermissions, + ...(grants.posture ? { posture: grants.posture } : {}), + } as ExecutionContext; + return { context, grants, rung: await hasPlatformAdminStanding(ql as any, ATTACKER) }; +} + +/** + * Narrow `openNodeRequest`'s union, and REFUSE the auto-approval outcome. + * Same spelling as `business-unit-member-org-screen.test.ts` (#10230). It is + * load-bearing here: a slate that resolved empty would auto-approve with no + * request to attack, and every arm below would then be vacuously satisfied. + */ +function opened(result: ApprovalRequestRow | ApprovalNodeAutoOutcome): ApprovalRequestRow { + if ('autoApproved' in result) { + throw new Error('expected an OPENED approval request, got an auto-approval outcome'); + } + return result; +} + +const SYS = { isSystem: true, positions: [], permissions: [] } as any; +/** The victim tenant's submitter — the request being reached across the wall. */ +const SUBMITTER = { userId: 'usr_victim', tenantId: VICTIM_ORG, positions: [], permissions: [] } as any; + +describe('[#15981] a D4 `sys_user_position` row spelling `platform_admin` confers NO override authority', () => { + let engine: ReturnType; + let svc: ApprovalService; + let n = 0; + const baseTime = new Date('2026-01-15T10:00:00Z').getTime(); + + beforeEach(() => { + engine = makeFakeEngine(); + n = 0; + svc = new ApprovalService({ + engine: engine as any, + clock: { now: () => new Date(baseTime + (n++) * 1000) }, + }); + }); + + /** + * A request in the VICTIM tenant, with a staffed slate the attacker is not on. + * + * Each call takes its OWN record: `openNodeRequest` refuses a second pending + * request on the same one (`DUPLICATE_REQUEST`), and an arm that leaves the + * first request correctly PENDING would otherwise collide with itself. + */ + let seq = 0; + const openVictimRequest = () => { + const n = ++seq; + return svc.openNodeRequest( + { + object: 'opportunity', recordId: `opp${n}`, runId: `run_${n}`, nodeId: 'sign_off', + flowName: 'victim_flow', + config: { + approvers: [{ type: 'user' as const, value: 'usr_designated' }], + behavior: 'first_response' as const, + }, + record: { id: `opp${n}`, amount: 100 }, + } as any, + SUBMITTER, + ).then(opened); + }; + + it('the name IS in positions[] while the rung says MEMBER — the premise, without which the rest is vacuous', async () => { + const { context, grants, rung } = await resolve('name-only'); + + expect(context.positions, JSON.stringify(context.positions)).toContain( + BUILTIN_IDENTITY_PLATFORM_ADMIN, + ); + expect(grants.posture).not.toBe('PLATFORM_ADMIN'); + expect(rung).toBe(false); + // No capability arm of `isOverrideActor` is satisfied either, so the NAME + // is the only thing that could admit this actor. + expect(context.permissions).not.toContain(ADMIN_FULL_ACCESS); + }); + + /** + * Attempt a decision and report what it DID, not merely what it threw. + * + * ⭐ The thrown message is NOT a sound gate signal here, and reading it as one + * would have hidden the escalation. A cross-tenant decision that is ADMITTED + * writes the decision and then fails building its echo, throwing + * `READ_BACK_FAILED` — whose own message says "The write is NOT rolled back". + * A pin that asserted `rejects.toThrow()` would therefore have gone GREEN on + * a successful escalation. So the observable is STATE: did the request leave + * `pending`, and was a decision action recorded? + */ + const attemptDecide = async (ctx: ExecutionContext, id: string) => { + let threw: string | null = null; + try { + await svc.decideNode(id, { decision: 'approve', actorId: ATTACKER }, ctx); + } catch (e) { + threw = String((e as any)?.message ?? e).split(':')[0]; + } + const [row] = await engine.find('sys_approval_request', { where: { id } }); + const actions = (await svc.listActions(id, SYS)).filter((a: any) => a.actor_id === ATTACKER); + return { threw, status: row?.status, decided: actions.length > 0 }; + }; + + it('THREE-WAY AGREEMENT — the name says yes; the site gate and the rung both say no, and agree', async () => { + const { context, rung } = await resolve('name-only'); + const req = await openVictimRequest(); + + const nameRead = (context.positions ?? []).includes(BUILTIN_IDENTITY_PLATFORM_ADMIN); + const outcome = await attemptDecide(context, req.id); + // The gate ADMITTED the actor iff a decision was recorded for them. + const gate = outcome.decided; + + expect({ nameRead, gate, rung }).toEqual({ nameRead: true, gate: false, rung: false }); + }); + + it('refuses to decide ANOTHER tenant’s pending request, and leaves it pending', async () => { + const { context } = await resolve('name-only'); + const req = await openVictimRequest(); + + const outcome = await attemptDecide(context, req.id); + + expect(outcome.threw).toBe('FORBIDDEN'); + // The state half, asserted because a refusal that still moved the request + // would be no refusal at all — and because `READ_BACK_FAILED` is exactly + // what an ADMITTED cross-tenant write throws. + expect({ status: outcome.status, decided: outcome.decided }).toEqual({ + status: 'pending', + decided: false, + }); + }); + + it('answers the same as a PLAIN member — the minted row buys nothing', async () => { + const nameOnly = await resolve('name-only'); + const plain = await resolve('plain'); + + const a = await openVictimRequest(); + const viaName = await attemptDecide(nameOnly.context, a.id); + const b = await openVictimRequest(); + const viaPlain = await attemptDecide(plain.context, b.id); + + // The floor: a plain member of another tenant is refused. If this arm ever + // stops being FORBIDDEN, the comparison above is measuring nothing. + expect(viaPlain).toEqual({ threw: 'FORBIDDEN', status: 'pending', decided: false }); + expect(viaName).toEqual(viaPlain); + }); +}); + +describe('[#15981] CONTROL — a genuine unscoped admin_full_access grant still overrides', () => { + let engine: ReturnType; + let svc: ApprovalService; + let n = 0; + const baseTime = new Date('2026-01-15T10:00:00Z').getTime(); + + beforeEach(() => { + engine = makeFakeEngine(); + n = 0; + svc = new ApprovalService({ + engine: engine as any, + clock: { now: () => new Date(baseTime + (n++) * 1000) }, + }); + }); + + /** + * A request in the VICTIM tenant, with a staffed slate the attacker is not on. + * + * Each call takes its OWN record: `openNodeRequest` refuses a second pending + * request on the same one (`DUPLICATE_REQUEST`), and an arm that leaves the + * first request correctly PENDING would otherwise collide with itself. + */ + let seq = 0; + const openVictimRequest = () => { + const n = ++seq; + return svc.openNodeRequest( + { + object: 'opportunity', recordId: `opp${n}`, runId: `run_${n}`, nodeId: 'sign_off', + flowName: 'victim_flow', + config: { + approvers: [{ type: 'user' as const, value: 'usr_designated' }], + behavior: 'first_response' as const, + }, + record: { id: `opp${n}`, amount: 100 }, + } as any, + SUBMITTER, + ).then(opened); + }; + + it('all three answers are TRUE and agree — the stuck-request escape hatch survives', async () => { + const { context, rung } = await resolve('genuine'); + const req = await openVictimRequest(); + + const nameRead = (context.positions ?? []).includes(BUILTIN_IDENTITY_PLATFORM_ADMIN); + // ⚠️ The echo cannot be built across the tenancy wall, so the call throws + // `READ_BACK_FAILED` *after* recording the decision — pre-existing + // behaviour of `readBackRequest`, untouched here and deliberately NOT + // caught into a `null`. The decision itself is the observable. + await expect( + svc.decideNode(req.id, { decision: 'approve', actorId: ATTACKER }, context), + ).rejects.toThrow(/READ_BACK_FAILED/); + + const decision = (await svc.listActions(req.id, SYS)).at(-1)!; + const gate = decision?.actor_id === ATTACKER; + expect({ nameRead, gate, rung }).toEqual({ nameRead: true, gate: true, rung: true }); + // …and it is still recorded AS an override (#4466), not as an ordinary + // approval — the audit half the escape hatch is allowed to keep. + expect(decision).toMatchObject({ action: 'approve', via_override: true }); + }); + + it('the two shapes are INDISTINGUISHABLE by name and separable only by the rung', async () => { + const escalation = await resolve('name-only'); + const genuine = await resolve('genuine'); + + expect(escalation.context.positions).toContain(BUILTIN_IDENTITY_PLATFORM_ADMIN); + expect(genuine.context.positions).toContain(BUILTIN_IDENTITY_PLATFORM_ADMIN); + expect(escalation.rung).toBe(false); + expect(genuine.rung).toBe(true); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index e83aa47a3b..81d7226ff4 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -19,10 +19,13 @@ import { ExpressionEngine, collectCelRootIdentifiers } from '@objectstack/formul // a third answer to a question the codebase already answered two ways. import { createRecordOrganizationResolver, type RecordOrganizationResolver } from '@objectstack/metadata-core'; import { keysetWalk, strandedDecisionFailure } from '@objectstack/types'; +// [#15981] `BUILTIN_IDENTITY_PLATFORM_ADMIN` is deliberately absent: the +// platform arm of `isOverrideActor` reads the ADR-0095 rung, never the name. +// The two org-level built-ins below are a NARROWER question and are untouched +// here — see that predicate's doc block. import { ADMIN_FULL_ACCESS, ORGANIZATION_ADMIN_GRANTS, - BUILTIN_IDENTITY_PLATFORM_ADMIN, BUILTIN_IDENTITY_ORG_OWNER, BUILTIN_IDENTITY_ORG_ADMIN, } from '@objectstack/spec/identity'; @@ -1100,10 +1103,24 @@ export class ApprovalService implements IApprovalService { // unchecked `as any` on an enforcement input: a typo (`postures`, // `'PLATFORM-ADMIN'`) would have compiled and silently denied every // override, leaving a stuck approval with no in-product recovery. + // + // ⛔ [#15981] The rung and the capability name are the whole platform test: + // there is NO `positions.includes(BUILTIN_IDENTITY_PLATFORM_ADMIN)` arm any + // more, and reading the rung FIRST never made one safe — an OR is only as + // strong as its weakest arm. `positions[]` is the security axis and carries + // ADR-0057 D4 `sys_user_position` names; that table is `apiEnabled` with + // unconstrained `position` values, so a tenant could mint a row spelling + // that built-in and `resolveUserAuthzGrants` §4 would push it onto the + // array. `resolve-authz-context.ts` states the rule at + // `hasPlatformAdminStanding`, whose answer IS `posture === 'PLATFORM_ADMIN'`. + // + // This arm is the one that CROSSES THE TENANT WALL (the tenant-admin arm + // below is confined to the actor's own org), so the minted row bought a + // member of one tenant the power to decide another tenant's pending + // request. Driven in `approval-positions-name-authority.test.ts`. const posture = context.posture; const isPlatformAdmin = posture === 'PLATFORM_ADMIN' - || perms.includes(ADMIN_FULL_ACCESS) - || positions.includes(BUILTIN_IDENTITY_PLATFORM_ADMIN); + || perms.includes(ADMIN_FULL_ACCESS); if (isPlatformAdmin) return true; const isTenantAdmin = posture === 'TENANT_ADMIN' || ORGANIZATION_ADMIN_GRANTS.some((n) => perms.includes(n)) diff --git a/packages/plugins/plugin-security/src/explain-engine.test.ts b/packages/plugins/plugin-security/src/explain-engine.test.ts index d87944bc1f..82f1aa2d89 100644 --- a/packages/plugins/plugin-security/src/explain-engine.test.ts +++ b/packages/plugins/plugin-security/src/explain-engine.test.ts @@ -442,14 +442,27 @@ describe('explainAccess — record-grained (C2 / ADR-0095)', () => { expect(d.record).toMatchObject({ recordId: 'r1', visible: true }); }); - it('derives PLATFORM_ADMIN posture from the platform_admin position', async () => { + it('derives PLATFORM_ADMIN posture from the unscoped-grant flag', async () => { + // [#15981] Was "from the platform_admin position". The NAME is no longer + // that evidence: `positions[]` is the security axis and carries ADR-0057 D4 + // `sys_user_position` names from an `apiEnabled` table, so a tenant can mint + // one spelling the built-in. `hasPlatformAdminGrant` is what + // `buildContextForUser` sets from `grants.posture === 'PLATFORM_ADMIN'`. const d = await explainAccess( recDeps({ sets: [ADMIN], layered: { layer0: null, layer1: null } }), - { object: 'leave_request', operation: 'read', context: { userId: 'a1', tenantId: 'org1', positions: ['platform_admin', 'everyone'], permissions: [] }, recordId: 'r1' }, + { object: 'leave_request', operation: 'read', context: { userId: 'a1', tenantId: 'org1', positions: ['platform_admin', 'everyone'], permissions: [], hasPlatformAdminGrant: true }, recordId: 'r1' }, ); expect(d.principal.posture).toBe('PLATFORM_ADMIN'); }); + it('[#15981] the platform_admin NAME alone does not — it reports MEMBER', async () => { + const d = await explainAccess( + recDeps({ sets: [ADMIN], layered: { layer0: null, layer1: null } }), + { object: 'leave_request', operation: 'read', context: { userId: 'a1', tenantId: 'org1', positions: ['platform_admin', 'everyone'], permissions: [] }, recordId: 'r1' }, + ); + expect(d.principal.posture).toBe('MEMBER'); + }); + it('Layer 0 (the tenant wall) excludes a cross-org record — decidedBy tenant_isolation', async () => { const d = await explainAccess( recDeps({ layered: { layer0: { organization_id: 'org1' }, layer1: null }, record: { id: 'r1', organization_id: 'org2', owner_id: 'u1' } }), @@ -624,8 +637,27 @@ describe('posture derivation aligns with enforcement (label-drift elimination)', expect(await postureOf({ userId: 'a1', tenantId: 'org1', positions: ['everyone'], permissions: ['admin_full_access'], hasPlatformAdminGrant: true })).toBe('PLATFORM_ADMIN'); }); - it('the projected platform_admin built-in position still yields PLATFORM_ADMIN', async () => { - expect(await postureOf({ userId: 'a1', tenantId: 'org1', positions: ['platform_admin', 'everyone'], permissions: [] })).toBe('PLATFORM_ADMIN'); + it('[#15981] the platform_admin built-in position NAME no longer yields PLATFORM_ADMIN', async () => { + // This assertion is INVERTED from what it pinned before, deliberately. It + // used to read "the PROJECTED platform_admin built-in position still yields + // PLATFORM_ADMIN", whose premise was that the name is only ever projected + // from the unscoped `admin_full_access` grant. Ruling A made `positions[]` + // the security axis, so it now also carries ADR-0057 D4 + // `sys_user_position` names — and that table is `apiEnabled` with + // unconstrained values, so the name can be MINTED by a tenant. The genuine + // projection is not lost: a real operator's context carries + // `hasPlatformAdminGrant` (the case immediately above) and, off + // `buildContextForUser`, `posture` itself. + expect(await postureOf({ userId: 'a1', tenantId: 'org1', positions: ['platform_admin', 'everyone'], permissions: [] })).toBe('MEMBER'); + }); + + it('[#15981] …and the two are separable only by the rung, not by the array', async () => { + const minted = { userId: 'a1', tenantId: 'org1', positions: ['platform_admin', 'everyone'], permissions: [] }; + const genuine = { ...minted, hasPlatformAdminGrant: true }; + // Identical on the axis a name-reading predicate would consult … + expect(minted.positions).toEqual(genuine.positions); + // … and opposite on the axis that actually decides. + expect([await postureOf(minted), await postureOf(genuine)]).toEqual(['MEMBER', 'PLATFORM_ADMIN']); }); it('org_owner / org_admin better-auth role positions no longer confer TENANT_ADMIN (ADR-0095 D3)', async () => { diff --git a/packages/plugins/plugin-security/src/explain-engine.ts b/packages/plugins/plugin-security/src/explain-engine.ts index e9ea653f2b..aa05bdae78 100644 --- a/packages/plugins/plugin-security/src/explain-engine.ts +++ b/packages/plugins/plugin-security/src/explain-engine.ts @@ -27,7 +27,7 @@ import { resolveUserAuthzGrants, } from '@objectstack/core'; import { matchesFilterCondition } from '@objectstack/formula'; -import { BUILTIN_IDENTITY_PLATFORM_ADMIN, ORGANIZATION_ADMIN_GRANTS } from '@objectstack/spec'; +import { ORGANIZATION_ADMIN_GRANTS } from '@objectstack/spec'; import type { FieldMaskingRule } from '@objectstack/spec/data'; import type { PermissionSet } from '@objectstack/spec/security'; import type { @@ -96,12 +96,27 @@ function isAuthzPosture(v: unknown): v is AuthzPosture { * `resolveAuthzContext` uses — NOT the previous loose permission-set-NAME * match: * - `PLATFORM_ADMIN` ← the **unscoped `admin_full_access` USER grant** - * (`hasPlatformAdminGrant`, which `buildContextForUser` now READS OFF the - * resolver's own verdict rather than recomputing), OR the - * `platform_admin` built-in position (which is itself only ever - * PROJECTED from that same grant — ADR-0068 D2). A merely-SCOPED - * `admin_full_access` grant (name present in `permissions`, not held - * unscoped) no longer over-labels. + * (`hasPlatformAdminGrant`, which `buildContextForUser` READS OFF the + * resolver's own verdict rather than recomputing), and nothing else. A + * merely-SCOPED `admin_full_access` grant (name present in + * `permissions`, not held unscoped) does not over-label. + * + * ⛔ [#15981] It is NOT ALSO the `platform_admin` built-in POSITION. + * That arm rested on the position being "only ever PROJECTED from that + * same grant", which stopped being true when `positions[]` became the + * security axis: it now carries ADR-0057 D4 `sys_user_position` names, + * and that table is `apiEnabled` with unconstrained `position` values, + * so a tenant could mint a row spelling the built-in. ⚠️ Reachability + * here was NARROWER than at #15981's three sibling sites, and it is + * worth stating precisely rather than overclaiming: preference (2) + * above returns first for any principal `buildContextForUser` builds, + * so the SHIPPING path never reached this line and a D4 row never moved + * it. What the name-read did reach was a posture-less HAND-BUILT + * context — and there it made the panel REPORT `PLATFORM_ADMIN` for a + * principal enforcement treats as a MEMBER. That is a misreport, not an + * admission — this engine explains, it does not admit — but it is a + * misreport in the tool an administrator opens to check exactly this, + * so it answered reassuringly wrong at the worst moment. * - `TENANT_ADMIN` ← the `organization_admin` **capability** grant, exactly * like enforcement (ADR-0095 D3). The better-auth `org_owner`/`org_admin` * role positions are a provisioning source only and are no longer read @@ -110,11 +125,9 @@ function isAuthzPosture(v: unknown): v is AuthzPosture { function derivePosture(context: any): AuthzPosture { if (!context?.userId || context?.principalKind === 'guest') return 'EXTERNAL'; if (isAuthzPosture(context?.posture)) return context.posture; - const positions: string[] = Array.isArray(context?.positions) ? context.positions : []; const permissions: string[] = Array.isArray(context?.permissions) ? context.permissions : []; return deriveAdminPosture({ - isPlatformAdmin: - context?.hasPlatformAdminGrant === true || positions.includes(BUILTIN_IDENTITY_PLATFORM_ADMIN), + isPlatformAdmin: context?.hasPlatformAdminGrant === true, isTenantAdmin: ORGANIZATION_ADMIN_GRANTS.some((n) => permissions.includes(n)), }); } diff --git a/packages/plugins/plugin-security/src/explain-positions-name-authority.test.ts b/packages/plugins/plugin-security/src/explain-positions-name-authority.test.ts new file mode 100644 index 0000000000..d86b686c69 --- /dev/null +++ b/packages/plugins/plugin-security/src/explain-positions-name-authority.test.ts @@ -0,0 +1,233 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#15981] A `sys_user_position` row SPELLING `platform_admin` does not make the + * explain panel REPORT `PLATFORM_ADMIN`. + * + * ## The defect this pins, and how it differs from its three siblings + * + * `derivePosture` read a NAME as platform evidence: + * + * isPlatformAdmin: + * context?.hasPlatformAdminGrant === true || positions.includes(BUILTIN_IDENTITY_PLATFORM_ADMIN), + * + * ⚠️ MEASURED, and the reason this site's arms are shaped differently from the + * other three in #15981: this read sits BEHIND an early return — + * `if (isAuthzPosture(context?.posture)) return context.posture;` — and + * `buildContextForUser`, the explain API's own principal builder, always + * attaches `posture` for an authenticated principal. So on the SHIPPING path + * the name-read was already unreachable, and the D4 row changed nothing. That + * half is asserted below (`the shipping path was already gated`) and it was + * GREEN before this change as well as after: it is a regression guard, not + * evidence that anything was repaired here. + * + * What WAS reachable is the fallback branch itself, on a context carrying + * `positions` with no `posture` — the shape the doc block names ("a HAND-BUILT + * context: tests, an internal caller assembling `{ userId, positions, + * permissions }` itself"). There the name alone produced `PLATFORM_ADMIN`. + * That is a MISREPORT rather than an enforcement bypass — this engine explains, + * it does not admit — but it is a misreport in the one tool an administrator + * uses to check whether someone is a platform operator, so it answers + * "reassuringly wrong" exactly when someone is looking. + * + * The fallback's remaining evidence is `hasPlatformAdminGrant`, which + * `buildContextForUser` sets from `grants.posture === 'PLATFORM_ADMIN'` — the + * rung, byte-for-byte what `hasPlatformAdminStanding` returns. + * + * ## POPULATION OF THIS PIN — stated because a pin proves only what it covers + * + * Covers: `explainAccess`'s reported `principal.posture` for (a) contexts built + * by the REAL `buildContextForUser` over inserted rows, in the `name-only` and + * `genuine` shapes, and (b) hand-built contexts exercising the fallback branch + * directly — the name alone, the rung alone, and both together. + * + * Does NOT cover: the TENANT_ADMIN arm, the guest/EXTERNAL floor, or the + * record-grained layer attribution — those are `explain-engine.test.ts`'s + * population, and its passing is NOT evidence about this one. In particular it + * does not cover any ENFORCEMENT decision: no arm here admits or refuses + * anything, because this engine reports. + */ + +import { describe, it, expect } from 'vitest'; +import { BUILTIN_IDENTITY_PLATFORM_ADMIN, ADMIN_FULL_ACCESS } from '@objectstack/spec/identity'; +import { PermissionSetSchema } from '@objectstack/spec/security'; +import { hasPlatformAdminStanding } from '@objectstack/core'; +import { PermissionEvaluator } from './permission-evaluator'; +import { explainAccess, buildContextForUser, type ExplainEngineDeps } from './explain-engine'; + +const ADMIN = PermissionSetSchema.parse({ + name: 'admin_full_access', + objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true } }, + systemPermissions: ['manage_users'], +}); +const PRIVATE_SCHEMA = { name: 'leave_request', sharingModel: 'private' }; + +/** The base object-level deps — the posture value is independent of the record-grained ones. */ +function makeDeps(): ExplainEngineDeps { + return { + ql: { getSchema: () => PRIVATE_SCHEMA }, + resolveSets: async () => [ADMIN], + evaluator: new PermissionEvaluator(), + getObjectSecurityMeta: async () => ({ + isPrivate: false, + requiredPermissions: { all: [], read: [], create: [], update: [], delete: [] }, + fieldRequiredPermissions: {}, + }), + requiredCaps: (meta: any, op: string) => { + const bucket = op === 'find' ? 'read' : op === 'insert' ? 'create' : op; + return [...(meta.all ?? []), ...((meta as any)[bucket] ?? [])]; + }, + computeRlsFilter: async () => null, + getFieldMask: () => ({}), + getPartialMaskRules: async () => ({}), + baselinePermissionSets: ['member_default'], + }; +} + +/** `posture` is surfaced whenever a `recordId` is supplied. */ +const postureOf = async (context: any): Promise => { + const d = await explainAccess(makeDeps(), { + object: 'leave_request', operation: 'read', context, recordId: 'r1', + }); + return d.principal.posture; +}; + +const USER = 'usr_subject'; +const HOME_ORG = 'org_home'; +const PS_ADMIN = 'ps_admin_full_access'; + +/** + * A `where`-HONOURING ObjectQL double — load-bearing, not tidiness, for the + * reason `explain-engine.test.ts` states at its own `buildContextForUser` + * fixtures: the resolver delegates filtering to the engine, so a fake that + * ignored `where` would report grants nobody holds. Top-level `$` combinators + * are refused rather than approximated. + */ +function makeAuthzQl(tables: Record>>) { + const matches = (row: Record, where: any): boolean => + Object.entries(where ?? {}).every(([k, v]) => { + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + if (v && typeof v === 'object' && '$in' in (v as any)) return (v as any).$in.includes(row[k]); + return row[k] === v; + }); + return { + getSchema: () => PRIVATE_SCHEMA, + async find(object: string, opts: any) { + const rows = (tables[object] ?? []).filter((r) => matches(r, opts?.where)); + return typeof opts?.limit === 'number' ? rows.slice(0, opts.limit) : rows; + }, + }; +} + +function authzTables(shape: 'name-only' | 'genuine') { + return { + sys_user: [{ id: USER, email: 'subject@example.com', email_verified: true }], + sys_member: [{ organization_id: HOME_ORG, user_id: USER, role: 'member' }], + sys_user_position: + shape === 'name-only' + ? [{ user_id: USER, position: BUILTIN_IDENTITY_PLATFORM_ADMIN, organization_id: null }] + : [], + sys_position: + shape === 'name-only' + ? [{ id: 'pos_pa', name: BUILTIN_IDENTITY_PLATFORM_ADMIN, label: 'Platform Admin', active: true }] + : [], + sys_position_permission_set: [], + sys_user_permission_set: + shape === 'genuine' + ? [{ user_id: USER, permission_set_id: PS_ADMIN, organization_id: null }] + : [], + sys_permission_set: [{ id: PS_ADMIN, name: ADMIN_FULL_ACCESS, active: true }], + }; +} + +describe('[#15981] the SHIPPING explain path was already gated — a regression guard, not a repair', () => { + it('a D4 row spelling the built-in name is REPORTED as MEMBER, and the rung agrees', async () => { + const ql = makeAuthzQl(authzTables('name-only')); + const context = await buildContextForUser(ql as any, USER); + + // The premise: the name really is on the array the panel prints. + expect(context.positions, JSON.stringify(context.positions)).toContain( + BUILTIN_IDENTITY_PLATFORM_ADMIN, + ); + // …and the reported rung is the capability answer, not the name. + expect(context.posture).toBe('MEMBER'); + expect(context.hasPlatformAdminGrant).toBe(false); + expect(await hasPlatformAdminStanding(ql as any, USER)).toBe(false); + expect(await postureOf(context)).toBe('MEMBER'); + }); + + it('CONTROL — a genuine unscoped admin_full_access grant is still reported as PLATFORM_ADMIN', async () => { + const ql = makeAuthzQl(authzTables('genuine')); + const context = await buildContextForUser(ql as any, USER); + + expect(context.positions).toContain(BUILTIN_IDENTITY_PLATFORM_ADMIN); + expect(context.hasPlatformAdminGrant).toBe(true); + expect(await hasPlatformAdminStanding(ql as any, USER)).toBe(true); + expect(await postureOf(context)).toBe('PLATFORM_ADMIN'); + }); + + it('the two shapes are INDISTINGUISHABLE by name and separable only by the rung', async () => { + const nameOnly = makeAuthzQl(authzTables('name-only')); + const genuine = makeAuthzQl(authzTables('genuine')); + const a = await buildContextForUser(nameOnly as any, USER); + const b = await buildContextForUser(genuine as any, USER); + + expect(a.positions).toContain(BUILTIN_IDENTITY_PLATFORM_ADMIN); + expect(b.positions).toContain(BUILTIN_IDENTITY_PLATFORM_ADMIN); + expect([a.posture, b.posture]).toEqual(['MEMBER', 'PLATFORM_ADMIN']); + }); +}); + +describe('[#15981] the FALLBACK branch — reachable on a posture-less context, and it no longer reads the name', () => { + const base = { userId: 'a1', tenantId: 'org1', permissions: [] }; + + it('THREE-WAY AGREEMENT — the name says yes; the report and the rung both say no, and agree', async () => { + // No `posture` key: the early return is skipped and the fallback runs. This + // is the branch the shipping builder cannot produce, and the only one this + // change actually moves. + const context = { ...base, positions: [BUILTIN_IDENTITY_PLATFORM_ADMIN, 'everyone'] }; + const ql = makeAuthzQl(authzTables('name-only')); + + const nameRead = context.positions.includes(BUILTIN_IDENTITY_PLATFORM_ADMIN); + const reported = await postureOf(context); + const rung = await hasPlatformAdminStanding(ql as any, USER); + + expect({ nameRead, reported, rung }).toEqual({ + nameRead: true, reported: 'MEMBER', rung: false, + }); + }); + + it('the unscoped-grant flag alone STILL yields PLATFORM_ADMIN — the evidence that survives', async () => { + // `buildContextForUser` sets this from `grants.posture === 'PLATFORM_ADMIN'`, + // so the fallback's remaining input is the rung itself. + expect( + await postureOf({ ...base, positions: ['everyone'], hasPlatformAdminGrant: true }), + ).toBe('PLATFORM_ADMIN'); + }); + + it('the grant flag wins even when the name is absent, and the name loses even when the flag is false', async () => { + expect( + await postureOf({ ...base, positions: ['everyone'], hasPlatformAdminGrant: true }), + ).toBe('PLATFORM_ADMIN'); + expect( + await postureOf({ + ...base, + positions: [BUILTIN_IDENTITY_PLATFORM_ADMIN, 'everyone'], + hasPlatformAdminGrant: false, + }), + ).toBe('MEMBER'); + }); + + it('an explicit ctx.posture still wins over both — the early return is untouched', async () => { + expect( + await postureOf({ ...base, positions: ['everyone'], posture: 'PLATFORM_ADMIN' }), + ).toBe('PLATFORM_ADMIN'); + expect( + await postureOf({ + ...base, + positions: [BUILTIN_IDENTITY_PLATFORM_ADMIN, 'everyone'], + posture: 'MEMBER', + }), + ).toBe('MEMBER'); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-positions-name-authority.test.ts b/packages/plugins/plugin-sharing/src/sharing-rule-positions-name-authority.test.ts new file mode 100644 index 0000000000..fbf56421a1 --- /dev/null +++ b/packages/plugins/plugin-sharing/src/sharing-rule-positions-name-authority.test.ts @@ -0,0 +1,288 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#15981] A `sys_user_position` row SPELLING `platform_admin` confers no + * platform authority on the sharing-rule surface. + * + * ## The defect this pins + * + * `hasPlatformAuthority` derived PLATFORM standing from a NAME: + * + * const positions = Array.isArray(context?.positions) ? context.positions : []; + * return positions.includes(BUILTIN_IDENTITY_PLATFORM_ADMIN); + * + * `sys_user_position` is `apiEnabled` and its `position` values are + * unconstrained, so a tenant can mint an ADR-0057 D4 row spelling that exact + * built-in name. `resolveUserAuthzGrants` §4 pushes the row's `position` + * straight into `grants.positions`, and `resolveAuthzContext` copies that array + * onto `ExecutionContext.positions` verbatim. The capability rung + * (`grants.posture`, §6d) is derived from the unscoped `admin_full_access` + * grant and nothing else, so it stays `MEMBER` — the two answers genuinely + * disagree, and the name-read took the wrong one. + * + * ⭐ WHAT THE ESCALATION BUYS, driven rather than argued (see the arms below): + * `manage_sharing` is an ORG-scoped capability (ADR-0111 D6) that an ordinary + * tenant admin may grant. Holding it with no organization resolved is refused + * by `assertResolvableAdminScope` precisely because an unscoped answer "would + * expose every tenant's rules". The D4 name-read was the bypass: it satisfied + * that gate, `adminOrgScope` then returned the UNFILTERED `where`, and + * `listRules` answered with every organization's rows. The same read also + * carried `deleteRule` past `assertCanDeletePlatformGlobalRule`. + * + * ## POPULATION OF THIS PIN — stated because a pin proves only what it covers + * + * Covers: an ORG-LESS caller holding `manage_sharing`, in two shapes — a D4 + * row spelling the built-in name with no capability grant behind it + * (`name-only`), and a genuine unscoped `admin_full_access` grant + * (`genuine`, the control that proves the fix did not simply deny everyone). + * Both shapes are built by inserting rows and resolving them through the REAL + * `resolveUserAuthzGrants`, so the `positions` / `posture` disagreement under + * test is produced by the shipping resolver rather than hand-asserted. + * + * Does NOT cover: the `manage_platform_settings` spelling of platform + * authority (untouched by this change and asserted only as still-admitting + * below), tenant-SCOPED callers (a caller with an organization never reaches + * `assertResolvableAdminScope`'s refusal at all), the ADR-0091 validity window, + * or the ADR-0049 `active` flags. Those are other suites' populations, and + * their passing is NOT evidence about this one. + */ + +import { describe, it, expect } from 'vitest'; +import { BUILTIN_IDENTITY_PLATFORM_ADMIN } from '@objectstack/spec/identity'; +import { ADMIN_FULL_ACCESS } from '@objectstack/spec/identity'; +import type { ExecutionContext } from '@objectstack/spec/kernel'; +import { hasPlatformAdminStanding, resolveUserAuthzGrants } from '@objectstack/core'; +import { SharingRuleService } from './sharing-rule-service.js'; + +const USER = 'usr_subject'; +const OTHER_ORG = 'org_victim'; +const HOME_ORG = 'org_home'; +const PS_SHARING = 'ps_sharing_admin'; +const PS_ADMIN = 'ps_admin_full_access'; + +/** + * A minimal ObjectQL double for the AUTHZ resolver, copied in shape from + * `resolve-authz-context.platform-admin-config.test.ts`'s `makeQl` — including + * its refusal of top-level `$` combinators. The resolver issues none on this + * path, so a matcher that silently read `$or` as a field name would leave the + * suite asserting on an empty result with nothing erroring. + */ +function makeAuthzQl(tables: Record>>) { + const matches = (row: Record, where: any): boolean => + Object.entries(where ?? {}).every(([k, v]) => { + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + if (v && typeof v === 'object' && '$in' in (v as any)) return (v as any).$in.includes(row[k]); + return row[k] === v; + }); + return { + async find(object: string, opts: any) { + const rows = (tables[object] ?? []).filter((r) => matches(r, opts?.where)); + return typeof opts?.limit === 'number' ? rows.slice(0, opts.limit) : rows; + }, + }; +} + +/** The permission set that carries the ORG-scoped `manage_sharing` capability. */ +const sharingAdminSet = { + id: PS_SHARING, + name: 'sharing_admin', + system_permissions: ['manage_sharing'], + active: true, +}; + +function authzTables(shape: 'name-only' | 'genuine') { + const userPositions = + shape === 'name-only' + ? [ + // Exactly what a tenant admin can write through the `apiEnabled` + // `sys_user_position` surface: a row whose NAME is the built-in. + { user_id: USER, position: BUILTIN_IDENTITY_PLATFORM_ADMIN, organization_id: null }, + ] + : []; + const userSets: Array> = [ + // The ORG-scoped capability both shapes hold — the precondition, not the + // axis under test. Scoped to `HOME_ORG`, so it can never be mistaken for + // the unscoped grant that confers standing. + { user_id: USER, permission_set_id: PS_SHARING, organization_id: HOME_ORG }, + ]; + if (shape === 'genuine') { + userSets.push({ user_id: USER, permission_set_id: PS_ADMIN, organization_id: null }); + } + return { + sys_user: [{ id: USER, email: 'subject@example.com', email_verified: true }], + sys_member: [{ organization_id: HOME_ORG, user_id: USER, role: 'member' }], + sys_user_position: userPositions, + // An ACTIVE catalogue row for the minted position, so ADR-0049's + // deactivated-position filter cannot be what carries the arm. + sys_position: + shape === 'name-only' + ? [{ id: 'pos_pa', name: BUILTIN_IDENTITY_PLATFORM_ADMIN, label: 'Platform Admin', active: true }] + : [], + sys_position_permission_set: [], + sys_user_permission_set: userSets, + sys_permission_set: [ + sharingAdminSet, + { id: PS_ADMIN, name: ADMIN_FULL_ACCESS, active: true }, + ], + }; +} + +/** + * Resolve one principal through the REAL resolver and hand back both the + * context the transports would build and the rung, so every assertion below is + * about resolver output rather than a hand-written array. + * + * The caller is deliberately ORG-LESS (`tenantId` omitted): that is the shape + * `assertResolvableAdminScope` exists to refuse. + */ +async function resolve(shape: 'name-only' | 'genuine') { + const ql = makeAuthzQl(authzTables(shape)); + const grants = await resolveUserAuthzGrants(ql as any, USER); + const context = { + userId: USER, + positions: grants.positions, + permissions: grants.permissions, + systemPermissions: grants.systemPermissions, + ...(grants.posture ? { posture: grants.posture } : {}), + } as ExecutionContext; + return { context, grants, rung: await hasPlatformAdminStanding(ql as any, USER) }; +} + +/** Two tenants' rules plus one platform-global row, so a cross-tenant read is VISIBLE. */ +const RULE_ROWS = [ + { id: 'srule_home', name: 'home_rule', object_name: 'account', organization_id: HOME_ORG, active: true }, + { id: 'srule_victim', name: 'victim_rule', object_name: 'account', organization_id: OTHER_ORG, active: true }, + { id: 'srule_global', name: 'global_rule', object_name: 'account', organization_id: null, active: true }, +]; + +/** + * A sharing-engine double that RECORDS its reads. It refuses top-level `$` + * combinators for the same reason the authz double does; on this pin's + * org-less arms `adminOrgScope` never produces one, so a `$or` reaching it is + * itself a finding rather than something to teach the double. + */ +function makeSharingEngine() { + const finds: Array<{ object: string; where: any }> = []; + const deletes: Array<{ object: string; id: unknown }> = []; + const matches = (row: Record, where: any): boolean => + Object.entries(where ?? {}).every(([k, v]) => { + if (k.startsWith('$')) throw new Error(`fake engine: unsupported operator ${k}`); + return row[k] === v; + }); + return { + finds, + deletes, + async find(object: string, opts: any) { + finds.push({ object, where: opts?.where }); + if (object !== 'sys_sharing_rule') return []; + const rows = RULE_ROWS.filter((r) => matches(r, opts?.where)); + return typeof opts?.limit === 'number' ? rows.slice(0, opts.limit) : rows; + }, + async delete(object: string, id: unknown) { + deletes.push({ object, id }); + }, + }; +} + +function makeService(engine: ReturnType) { + return new SharingRuleService({ + engine: engine as any, + sharing: { revoke: async () => {} } as any, + logger: { warn: () => {} }, + }); +} + +describe('[#15981] a D4 `sys_user_position` row spelling `platform_admin` confers NO platform authority', () => { + it('the name IS in positions[] while the rung says MEMBER — the premise, without which the rest is vacuous', async () => { + const { context, grants, rung } = await resolve('name-only'); + + expect(context.positions, JSON.stringify(context.positions)).toContain( + BUILTIN_IDENTITY_PLATFORM_ADMIN, + ); + expect(grants.posture).not.toBe('PLATFORM_ADMIN'); + expect(rung).toBe(false); + // The org-scoped capability really is held — so a refusal below is about + // platform authority, not about a caller who could never manage rules. + expect(context.systemPermissions).toContain('manage_sharing'); + }); + + it('THREE-WAY AGREEMENT — the name says yes; the site gate and the rung both say no, and agree', async () => { + const { context, rung } = await resolve('name-only'); + const engine = makeSharingEngine(); + const service = makeService(engine); + + const nameRead = (context.positions ?? []).includes(BUILTIN_IDENTITY_PLATFORM_ADMIN); + // The site's own gate, read through the public verb it guards. + const gate = await service + .listRules({}, context) + .then(() => true) + .catch(() => false); + + expect({ nameRead, gate, rung }).toEqual({ nameRead: true, gate: false, rung: false }); + }); + + it('`listRules` refuses instead of answering with every organization’s rules', async () => { + const { context } = await resolve('name-only'); + const engine = makeSharingEngine(); + const service = makeService(engine); + + await expect(service.listRules({}, context)).rejects.toThrow(/PERMISSION_DENIED/); + // Refused BEFORE the read, so no other tenant's rows were ever fetched — + // the half a throw alone would not establish. + expect(engine.finds).toEqual([]); + }); + + it('`deleteRule` refuses to destroy a platform-global rule', async () => { + const { context } = await resolve('name-only'); + const engine = makeSharingEngine(); + const service = makeService(engine); + + await expect(service.deleteRule('global_rule', context)).rejects.toThrow(/PERMISSION_DENIED/); + expect(engine.deletes).toEqual([]); + }); +}); + +describe('[#15981] CONTROL — a genuine unscoped admin_full_access grant still admits', () => { + it('all three answers are TRUE and agree, and the cross-tenant read is served', async () => { + const { context, rung } = await resolve('genuine'); + const engine = makeSharingEngine(); + const service = makeService(engine); + + const nameRead = (context.positions ?? []).includes(BUILTIN_IDENTITY_PLATFORM_ADMIN); + const rows = await service.listRules({}, context); + expect({ nameRead, gate: true, rung }).toEqual({ nameRead: true, gate: true, rung: true }); + // A real platform operator still reads across tenants — the functional + // half, which a fix that simply denied everyone would break. + expect(rows.map((r) => r.id).sort()).toEqual(['srule_global', 'srule_home', 'srule_victim']); + }); + + it('the two shapes are INDISTINGUISHABLE by name and separable only by the rung', async () => { + const escalation = await resolve('name-only'); + const genuine = await resolve('genuine'); + + // Identical on the axis a name-reading predicate would consult … + expect(escalation.context.positions).toContain(BUILTIN_IDENTITY_PLATFORM_ADMIN); + expect(genuine.context.positions).toContain(BUILTIN_IDENTITY_PLATFORM_ADMIN); + // … and opposite on the axis that actually decides. + expect(escalation.rung).toBe(false); + expect(genuine.rung).toBe(true); + }); +}); + +describe('[#15981] the OTHER spelling of platform authority is untouched', () => { + it('`manage_platform_settings` still admits an org-less caller', async () => { + const engine = makeSharingEngine(); + const service = makeService(engine); + // A hand-built context on purpose: this arm pins the capability spelling + // this change does NOT touch, and it reaches contexts no resolver builds + // (ADR-0068 D2's second channel, per `hasPlatformAuthority`'s doc block). + const context = { + userId: 'usr_ops', + positions: [], + permissions: [], + systemPermissions: ['manage_sharing', 'manage_platform_settings'], + } as ExecutionContext; + + await expect(service.listRules({}, context)).resolves.toHaveLength(3); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts index dafa6fddf4..be016b7e73 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -12,10 +12,11 @@ import type { // has declared for every one of these context parameters since #6523 (the // #6206 ruling: no per-site subset contracts). import type { ExecutionContext } from '@objectstack/spec/kernel'; -// [#7795] The built-in platform-operator position (ADR-0068 D2) — one of the -// two spellings of platform authority the ruling names; see -// {@link SharingRuleService.assertCanDeletePlatformGlobalRule}. -import { BUILTIN_IDENTITY_PLATFORM_ADMIN } from '@objectstack/spec/identity'; +// [#15981] The built-in platform-operator position NAME is deliberately NOT +// imported here any more. It used to spell the second half of +// {@link SharingRuleService.hasPlatformAuthority}, and reading it as authority +// became an escalation channel once `positions[]` started carrying ADR-0057 D4 +// `sys_user_position` names; that predicate now reads the ADR-0095 rung. // [#8710] The ONE predicate for `sys_position.active` / `sys_permission_set.active` // (#8613). Reused rather than re-spelled: two notions of "is this row active" // — one honouring the 1/0 and 'false' storage shapes, one not — is how the @@ -316,18 +317,32 @@ export class SharingRuleService implements ISharingRuleService { * accepting either is the fail-safe reading: they are two independent * channels by which the same unscoped `admin_full_access` grant reaches an * `ExecutionContext` (a `scope: 'platform'` capability on - * `systemPermissions`; the ADR-0068 D2 built-in position on `positions`), - * and a hand-built context may carry only one. + * `systemPermissions`; the ADR-0095 posture RUNG). * * It is asked TWICE now — once to authorize destroying a platform-global * rule, once to decide whether an org-less caller may read across tenants — * so it is one predicate rather than two spellings that can drift apart. + * + * ⛔ [#15981] The second spelling is the RUNG, never + * `positions.includes(BUILTIN_IDENTITY_PLATFORM_ADMIN)`. The two stopped + * being the same channel when `positions[]` became the security axis: it + * carries ADR-0057 D4 `sys_user_position` names, that table is `apiEnabled` + * with unconstrained `position` values, and `resolveUserAuthzGrants` §4 + * pushes a minted row's name straight onto the array. `resolve-authz-context.ts` + * states the rule at `hasPlatformAdminStanding`, and + * `grants.posture === 'PLATFORM_ADMIN'` is byte-for-byte what that predicate + * returns — the unscoped `admin_full_access` evidence, which is what this + * doc block meant by the built-in position all along. + * + * Driven, not argued: with the minted row present and only the ORG-scoped + * `manage_sharing` capability held, the old read admitted an org-less caller + * to every tenant's rules through {@link listRules}. See + * `sharing-rule-positions-name-authority.test.ts`. */ private hasPlatformAuthority(context: ExecutionContext): boolean { const caps = Array.isArray(context?.systemPermissions) ? context.systemPermissions : []; if (caps.includes('manage_platform_settings')) return true; - const positions = Array.isArray(context?.positions) ? context.positions : []; - return positions.includes(BUILTIN_IDENTITY_PLATFORM_ADMIN); + return context?.posture === 'PLATFORM_ADMIN'; } /** diff --git a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts index 22d33a9ad0..8185bcf464 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts @@ -1377,10 +1377,34 @@ describe('[#7795] deleting a platform-global rule requires platform authority', userId: 'ops', organizationId: 'org1', systemPermissions: ['manage_sharing', 'manage_platform_settings'], } as any; - /** Platform authority, spelling 2: the built-in POSITION (ADR-0068 D2). */ - const PLATFORM_ADMIN_POSITION = { + /** + * Platform authority, spelling 2: the ADR-0095 posture RUNG. + * + * [#15981] This fixture used to spell the second channel as the built-in + * POSITION NAME alone, on the reading that the name is only ever projected + * from the unscoped `admin_full_access` grant. `positions[]` is now the + * security axis and also carries ADR-0057 D4 `sys_user_position` names, so + * the name no longer implies the grant — the rung does. A genuine operator's + * resolved context carries BOTH (the projection is still emitted), which is + * why this keeps `positions` as well: dropping it would make the fixture + * unrepresentative in the opposite direction. + */ + const PLATFORM_ADMIN_RUNG = { userId: 'root', organizationId: 'org1', systemPermissions: ['manage_sharing'], positions: ['platform_admin'], + posture: 'PLATFORM_ADMIN', + } as any; + /** + * [#15981] The ESCALATION shape: the same name with no rung behind it — what + * a tenant can mint through the `apiEnabled` `sys_user_position` surface. + * Refused. Driven end-to-end through the real resolver in + * `sharing-rule-positions-name-authority.test.ts`; this arm keeps the + * boundary visible in the suite that owns the gate. + */ + const PLATFORM_ADMIN_NAME_ONLY = { + userId: 'impostor', organizationId: 'org1', + systemPermissions: ['manage_sharing'], positions: ['platform_admin'], + posture: 'MEMBER', } as any; /** No `manage_sharing` at all — must still be refused by the OLDER gate. */ const MALLORY = { userId: 'mallory', organizationId: 'org1', systemPermissions: [] } as any; @@ -1511,12 +1535,19 @@ describe('[#7795] deleting a platform-global rule requires platform authority', expect(grantsOf(seededId)).toHaveLength(0); }); - it('the platform_admin position authorizes the delete', async () => { - await expect(rules.deleteRule(seededId, PLATFORM_ADMIN_POSITION)).resolves.toBeUndefined(); + it('the PLATFORM_ADMIN posture rung authorizes the delete', async () => { + await expect(rules.deleteRule(seededId, PLATFORM_ADMIN_RUNG)).resolves.toBeUndefined(); expect(engine._tables.sys_sharing_rule.find((r) => r.id === seededId)).toBeUndefined(); expect(grantsOf(seededId)).toHaveLength(0); }); + it('[#15981] the built-in position NAME without the rung does NOT', async () => { + expect(await refusalCodeOf(rules.deleteRule(seededId, PLATFORM_ADMIN_NAME_ONLY))) + .toBe('PERMISSION_DENIED'); + // The row survives: a refusal that still destroyed the rule would be none. + expect(engine._tables.sys_sharing_rule.find((r) => r.id === seededId)).toBeTruthy(); + }); + it('a system context (boot seeding, hooks, backfills) still deletes', async () => { await expect(rules.deleteRule(seededId, BOOT)).resolves.toBeUndefined(); expect(engine._tables.sys_sharing_rule.find((r) => r.id === seededId)).toBeUndefined(); @@ -1620,9 +1651,19 @@ describe('[#8158] a non-system caller with NO organization does not get the syst const ORG_LESS_PLATFORM_CAP = { userId: 'ops', systemPermissions: ['manage_sharing', 'manage_platform_settings'], } as any; - /** Platform authority with no active org, spelling 2: the ADR-0068 D2 position. */ - const ORG_LESS_PLATFORM_POSITION = { + /** + * Platform authority with no active org, spelling 2: the ADR-0095 rung. + * [#15981] Was the built-in position NAME alone — see the migration note on + * `PLATFORM_ADMIN_RUNG` above for why a name is no longer that evidence. + */ + const ORG_LESS_PLATFORM_RUNG = { userId: 'root', systemPermissions: ['manage_sharing'], positions: ['platform_admin'], + posture: 'PLATFORM_ADMIN', + } as any; + /** [#15981] The escalation shape: the minted name, no rung. Refused. */ + const ORG_LESS_NAME_ONLY = { + userId: 'impostor', systemPermissions: ['manage_sharing'], positions: ['platform_admin'], + posture: 'MEMBER', } as any; /** No capability at all — the OLDER gate must still fire first. */ const ORG_LESS_NOBODY = { userId: 'nobody', systemPermissions: [] } as any; @@ -1788,10 +1829,18 @@ describe('[#8158] a non-system caller with NO organization does not get the syst // bootstrapped, so refusing it would be a functional regression. expect(namesOf(await rules.listRules({}, ORG_LESS_PLATFORM_CAP))) .toEqual(['org1_rule', 'org2_rule', SEEDED].sort()); - expect(namesOf(await rules.listRules({}, ORG_LESS_PLATFORM_POSITION))) + expect(namesOf(await rules.listRules({}, ORG_LESS_PLATFORM_RUNG))) .toEqual(['org1_rule', 'org2_rule', SEEDED].sort()); expect((await rules.getRule(org2RuleId, ORG_LESS_PLATFORM_CAP))?.name).toBe('org2_rule'); - expect((await rules.getRule('org2_rule', ORG_LESS_PLATFORM_POSITION))?.id).toBe(org2RuleId); + expect((await rules.getRule('org2_rule', ORG_LESS_PLATFORM_RUNG))?.id).toBe(org2RuleId); + }); + + it('[#15981] an org-less caller holding only the minted NAME reads nothing', async () => { + // The whole point of the #8158 gate: `manage_sharing` is ORG-scoped, so an + // org-less holder of it must not answer unscoped. The name-read was the + // bypass — this is the arm that keeps it closed. + await expect(rules.listRules({}, ORG_LESS_NAME_ONLY)).rejects.toThrow(/PERMISSION_DENIED/); + await expect(rules.getRule(org2RuleId, ORG_LESS_NAME_ONLY)).rejects.toThrow(/PERMISSION_DENIED/); }); // ── the anti-vacuity control: scoped ≠ single-tenant ───────────────── diff --git a/packages/runtime/src/domains/action-activation-posture-gate.test.ts b/packages/runtime/src/domains/action-activation-posture-gate.test.ts index 0e10c4df8d..80ea2ec775 100644 --- a/packages/runtime/src/domains/action-activation-posture-gate.test.ts +++ b/packages/runtime/src/domains/action-activation-posture-gate.test.ts @@ -124,10 +124,36 @@ const PLATFORM_OPERATOR = (): HttpProtocolContext => ({ positions: ['platform_admin'], permissions: ['admin_full_access'], systemPermissions: ['manage_metadata'], + // [#15981] The ADR-0095 rung, which is what the gate now reads. A real + // operator's resolved context carries it AND the projected position name; + // this fixture used to carry only the NAME, and a name stopped being + // platform evidence when `positions[]` became the security axis (it also + // carries ADR-0057 D4 `sys_user_position` names, mintable by a tenant). + posture: 'PLATFORM_ADMIN', organizationId: null, }, } as unknown as HttpProtocolContext); +/** + * [#15981] The ESCALATION shape: a tenant org admin who ALSO holds a + * `sys_user_position` row spelling the built-in name. `sys_user_position` is + * `apiEnabled` with unconstrained `position` values, so a tenant can mint this + * for themselves; the rung stays `TENANT_ADMIN`, because it is derived from the + * unscoped `admin_full_access` grant they do not hold. Must be REFUSED. + */ +const MINTED_NAME_ONLY = (): HttpProtocolContext => ({ + request: {}, + environmentId: 'platform', + executionContext: { + userId: 'u_impostor', + positions: ['platform_admin', 'org_owner'], + permissions: ['organization_admin'], + systemPermissions: ['manage_metadata'], + posture: 'TENANT_ADMIN', + organizationId: 'org_northwind', + }, +} as unknown as HttpProtocolContext); + /** Engine self-invocation — never settable from the wire. */ const SYSTEM = (): HttpProtocolContext => ({ request: {}, @@ -221,6 +247,18 @@ describe('ADR-0126 §5 — the action activation write is operator-gated in wall expect(h.setActionActive).toHaveBeenCalled(); }); + it('[#15981] REFUSES a tenant admin holding only a MINTED `platform_admin` name', async () => { + const h = boot(posture); + + const r = await flip(h, MINTED_NAME_ONLY()); + + expect(statusOf(r)).toBe(403); + expect(codeOf(r)).toBe('PERMISSION_DENIED'); + // Refused BEFORE the install-wide write — the half a status + // assertion alone would not establish. + expect(h.setActionActive).not.toHaveBeenCalled(); + }); + it('ALLOWS engine self-invocation', async () => { const h = boot(posture); diff --git a/packages/runtime/src/domains/activation-gate-positions-name-authority.test.ts b/packages/runtime/src/domains/activation-gate-positions-name-authority.test.ts new file mode 100644 index 0000000000..ee6e926999 --- /dev/null +++ b/packages/runtime/src/domains/activation-gate-positions-name-authority.test.ts @@ -0,0 +1,254 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#15981] A `sys_user_position` row SPELLING `platform_admin` confers no +// operator authority on the ADR-0126 §5 activation gate. +// +// ## The defect this pins +// +// `refuseUngrantedActivationWrite` derived operator standing from a NAME: +// +// const positions: string[] = Array.isArray(ec?.positions) ? ec.positions : []; +// if (positions.includes(BUILTIN_IDENTITY_PLATFORM_ADMIN)) return undefined; +// +// The gate's own doc block argued for reading the position on the grounds that +// it is "unscoped, sourced from the unscoped `admin_full_access` grant". That +// premise stopped holding when `positions[]` became the security axis: it now +// also carries ADR-0057 D4 `sys_user_position` names, and that table is +// `apiEnabled` with unconstrained `position` values. So a tenant could mint a +// row spelling the built-in name, `resolveUserAuthzGrants` §4 pushed it into +// `grants.positions`, and this gate opened. +// +// ⭐ WHAT THE ESCALATION BUYS, driven rather than argued: this gate is the ONLY +// thing standing between a tenant org admin and the install-wide activation +// row under a walled posture. It is #10243 exactly — a tenant org owner +// switching a shipped flow off ENVIRONMENT-WIDE — except that ADR-0126 made +// the row DURABLE, so the same leak now survives a cold boot. The arms below +// drive the real `POST /automation/:name/toggle` route and assert on whether +// `toggleFlow` was entered, which is the write itself. +// +// ## POPULATION OF THIS PIN — stated because a pin proves only what it covers +// +// Covers: the `group` and `isolated` (wall-enforcing) postures, on the +// automation toggle door, for a tenant org admin who already holds +// `manage_metadata` — so the tier-above gate passes and THIS gate is the only +// thing left. Three shapes: a D4 row spelling the built-in name with no +// capability grant (`name-only`), a genuine unscoped `admin_full_access` grant +// (`genuine`), and a plain tenant admin (`plain`, the floor). All three +// contexts are produced by inserting rows and resolving them through the REAL +// `resolveUserAuthzGrants`, so `positions` / `posture` come from the shipping +// resolver rather than being hand-asserted. +// +// Does NOT cover: the `single` posture (where the gate is inert by design and +// this change cannot reach it), the actions door (same gate, same call — +// covered by `action-activation-posture-gate.test.ts`'s own population), the +// `manage_metadata` tier above, or engine self-invocation. Those are other +// suites' populations and their passing is NOT evidence about this one. + +import { describe, it, expect, vi } from 'vitest'; +import { BUILTIN_IDENTITY_PLATFORM_ADMIN, ADMIN_FULL_ACCESS } from '@objectstack/spec/identity'; +import { hasPlatformAdminStanding, resolveUserAuthzGrants } from '@objectstack/core'; + +import { HttpDispatcher } from '../http-dispatcher.js'; +import type { HttpProtocolContext } from '../http-dispatcher.js'; + +const FLOW = 'vendor_lead_router'; +const DEFINITION = { name: FLOW, label: 'Vendor Lead Router', type: 'autolaunched', nodes: [], edges: [] }; + +const TENANT_ORG = 'org_northwind'; +const ACTOR = 'usr_tenant_admin'; +const PS_ADMIN = 'ps_admin_full_access'; +const PS_METADATA = 'ps_metadata_author'; + +interface Harness { + dispatcher: HttpDispatcher; + toggleFlow: ReturnType; +} + +/** A dispatcher whose `tenancy` service reports the given posture. */ +function boot(posture: 'group' | 'isolated'): Harness { + const toggleFlow = vi.fn(async () => undefined); + const services: Record = { + automation: { + handlerReady: true, + toggleFlow, + getFlow: vi.fn(async (name: string) => (name === FLOW ? DEFINITION : undefined)), + }, + tenancy: { posture }, + }; + const resolve = (name: string): unknown => services[name]; + const kernel = { + getService: resolve, + getServiceAsync: async (name: string) => resolve(name), + context: { getService: resolve }, + }; + return { dispatcher: new HttpDispatcher(kernel as never), toggleFlow }; +} + +/** + * A minimal ObjectQL double for the AUTHZ resolver — the shape (and the + * top-level `$` refusal) of `resolve-authz-context.platform-admin-config.test.ts`. + */ +function makeAuthzQl(tables: Record>>) { + const matches = (row: Record, where: any): boolean => + Object.entries(where ?? {}).every(([k, v]) => { + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + if (v && typeof v === 'object' && '$in' in (v as any)) return (v as any).$in.includes(row[k]); + return row[k] === v; + }); + return { + async find(object: string, opts: any) { + const rows = (tables[object] ?? []).filter((r) => matches(r, opts?.where)); + return typeof opts?.limit === 'number' ? rows.slice(0, opts.limit) : rows; + }, + }; +} + +type Shape = 'name-only' | 'genuine' | 'plain'; + +function authzTables(shape: Shape) { + const userSets: Array> = [ + // The ORG-scoped authoring capability every shape holds, so the + // `manage_metadata` tier above always passes and this gate is the only + // thing under test. Scoped to the org, never unscoped. + { user_id: ACTOR, permission_set_id: PS_METADATA, organization_id: TENANT_ORG }, + ]; + if (shape === 'genuine') { + userSets.push({ user_id: ACTOR, permission_set_id: PS_ADMIN, organization_id: null }); + } + return { + sys_user: [{ id: ACTOR, email: 'tenant-admin@example.com', email_verified: true }], + sys_member: [{ organization_id: TENANT_ORG, user_id: ACTOR, role: 'owner' }], + sys_user_position: + shape === 'name-only' + ? [ + // Exactly what a tenant admin can write through the + // `apiEnabled` `sys_user_position` surface. + { user_id: ACTOR, position: BUILTIN_IDENTITY_PLATFORM_ADMIN, organization_id: null }, + ] + : [], + // An ACTIVE catalogue row, so ADR-0049's deactivated-position filter is + // not what carries the arm. + sys_position: + shape === 'name-only' + ? [{ id: 'pos_pa', name: BUILTIN_IDENTITY_PLATFORM_ADMIN, label: 'Platform Admin', active: true }] + : [], + sys_position_permission_set: [], + sys_user_permission_set: userSets, + sys_permission_set: [ + { id: PS_METADATA, name: 'metadata_author', system_permissions: ['manage_metadata'], active: true }, + { id: PS_ADMIN, name: ADMIN_FULL_ACCESS, active: true }, + ], + }; +} + +/** Resolve one principal through the REAL resolver into the protocol context. */ +async function resolve(shape: Shape) { + const ql = makeAuthzQl(authzTables(shape)); + const grants = await resolveUserAuthzGrants(ql as any, ACTOR, { tenantId: TENANT_ORG }); + const context = { + request: {}, + executionContext: { + userId: ACTOR, + tenantId: TENANT_ORG, + organizationId: TENANT_ORG, + positions: grants.positions, + permissions: grants.permissions, + systemPermissions: grants.systemPermissions, + ...(grants.posture ? { posture: grants.posture } : {}), + }, + } as HttpProtocolContext; + return { context, grants, rung: await hasPlatformAdminStanding(ql as any, ACTOR) }; +} + +const statusOf = (response: unknown): unknown => (response as any)?.status; +const codeOf = (response: unknown): unknown => { + const r = response as any; + return r?.body?.error?.code ?? r?.body?.error?.details?.code; +}; +const messageOf = (response: unknown): string => String((response as any)?.body?.error?.message ?? ''); + +const toggle = (h: Harness, ctx: HttpProtocolContext) => + h.dispatcher.handleAutomation(`/${FLOW}/toggle`, 'POST', { enabled: false }, ctx, undefined); + +for (const posture of ['group', 'isolated'] as const) { + describe(`[#15981] \`${posture}\` — a D4 row spelling \`platform_admin\` does NOT open the install-wide switch`, () => { + it('the name IS in positions[] while the rung says TENANT_ADMIN — the premise, without which the rest is vacuous', async () => { + const { context, grants, rung } = await resolve('name-only'); + const ec = (context as any).executionContext; + + expect(ec.positions, JSON.stringify(ec.positions)).toContain(BUILTIN_IDENTITY_PLATFORM_ADMIN); + expect(grants.posture).not.toBe('PLATFORM_ADMIN'); + expect(rung).toBe(false); + // The tier above really does pass, so a refusal below is this + // gate's and not `manage_metadata`'s. + expect(ec.systemPermissions).toContain('manage_metadata'); + }); + + it('THREE-WAY AGREEMENT — the name says yes; the site gate and the rung both say no, and agree', async () => { + const { context, rung } = await resolve('name-only'); + const h = boot(posture); + const ec = (context as any).executionContext; + + const nameRead = (ec.positions ?? []).includes(BUILTIN_IDENTITY_PLATFORM_ADMIN); + const { response } = await toggle(h, context); + const gate = statusOf(response) === 200; + + expect({ nameRead, gate, rung }).toEqual({ nameRead: true, gate: false, rung: false }); + }); + + it('REFUSES 403 PERMISSION_DENIED and never enters toggleFlow', async () => { + const { context } = await resolve('name-only'); + const h = boot(posture); + + const { response } = await toggle(h, context); + + expect(statusOf(response)).toBe(403); + expect(codeOf(response)).toBe('PERMISSION_DENIED'); + // The load-bearing assertion: refused BEFORE the write. A gate that + // wrote the install-wide row and then refused would satisfy the two + // above and still be #10243. + expect(h.toggleFlow).not.toHaveBeenCalled(); + }); + + it('the refusal still says nothing about the caller’s own positions (#7450)', async () => { + const { context } = await resolve('name-only'); + const h = boot(posture); + + const message = messageOf((await toggle(h, context)).response); + + expect(message).toContain(posture); + expect(message).toContain('ADR-0126 §5'); + expect(message).not.toContain(BUILTIN_IDENTITY_PLATFORM_ADMIN); + expect(message).not.toContain('org_owner'); + }); + + it('answers the same as a PLAIN tenant admin — the minted row buys nothing', async () => { + const nameOnly = await resolve('name-only'); + const plain = await resolve('plain'); + + const hName = boot(posture); + const hPlain = boot(posture); + const viaName = statusOf((await toggle(hName, nameOnly.context)).response); + const viaPlain = statusOf((await toggle(hPlain, plain.context)).response); + + // The floor: if this stops being 403, the comparison measures nothing. + expect(viaPlain).toBe(403); + expect(viaName).toBe(viaPlain); + expect(hName.toggleFlow).not.toHaveBeenCalled(); + }); + + it('CONTROL — a genuine unscoped admin_full_access grant still flips the switch', async () => { + const { context, rung } = await resolve('genuine'); + const h = boot(posture); + const ec = (context as any).executionContext; + + const nameRead = (ec.positions ?? []).includes(BUILTIN_IDENTITY_PLATFORM_ADMIN); + const { response } = await toggle(h, context); + + expect({ nameRead, gate: statusOf(response) === 200, rung }).toEqual({ + nameRead: true, gate: true, rung: true, + }); + expect(h.toggleFlow).toHaveBeenCalledWith(FLOW, false); + }); + }); +} diff --git a/packages/runtime/src/domains/activation-gate.ts b/packages/runtime/src/domains/activation-gate.ts index d24e3a7631..693ee3a7a2 100644 --- a/packages/runtime/src/domains/activation-gate.ts +++ b/packages/runtime/src/domains/activation-gate.ts @@ -32,19 +32,37 @@ * writable by tenants would be the same leak WITH persistence, which is * strictly worse than what was measured. * - * ## Why the operator test is a POSITION and not a capability + * ## Why the operator test is the POSTURE RUNG and not a capability * * ADR-0126 §5 says "the platform-operator capability"; the platform's actual - * operator identity is the ADR-0068 D2 built-in `platform_admin` POSITION, - * documented verbatim as "Platform operator (SaaS admin). NOT a tenant user - * role", unscoped, sourced from the unscoped `admin_full_access` grant. No - * capability in `PLATFORM_CAPABILITIES` carries that meaning: `manage_metadata` - * is the one the tier above already requires, and a tenant org admin can hold - * it — so spelling this gate as a capability check would either re-ask the - * question already answered or invent a capability name, which would be a - * `packages/spec` change ADR-0126 §9 walls this family out of. The position IS - * the platform's operator concept; this gate reads it rather than minting a - * synonym. + * operator identity is the ADR-0068 D2 platform operator — "Platform operator + * (SaaS admin). NOT a tenant user role", unscoped, sourced from the unscoped + * `admin_full_access` grant. No capability in `PLATFORM_CAPABILITIES` carries + * that meaning: `manage_metadata` is the one the tier above already requires, + * and a tenant org admin can hold it — so spelling this gate as a capability + * check would either re-ask the question already answered or invent a + * capability name, which would be a `packages/spec` change ADR-0126 §9 walls + * this family out of. + * + * ⛔ [#15981] What this gate reads is the ADR-0095 D2/D3 posture RUNG + * (`posture === 'PLATFORM_ADMIN'`), NEVER + * `positions.includes(BUILTIN_IDENTITY_PLATFORM_ADMIN)`. It used to read the + * name, on the reasoning above that the built-in position IS "sourced from the + * unscoped `admin_full_access` grant" — a premise that stopped holding when + * `positions[]` became the security axis. That array now also carries ADR-0057 + * D4 `sys_user_position` names; `sys_user_position` is `apiEnabled` with + * unconstrained `position` values, so a tenant could mint a row spelling that + * very built-in and `resolveUserAuthzGrants` §4 would push it onto the array. + * The rung is derived from the unscoped-grant evidence and nothing else, so it + * is what the paragraph above always MEANT — and it is byte-for-byte what + * `hasPlatformAdminStanding` returns. `resolve-authz-context.ts` states the + * rule at that predicate; this gate is one of the four sites #15981 found + * ignoring it. + * + * Driven, not argued: with the minted row present, a tenant org admin holding + * only the org-scoped `manage_metadata` capability flipped the install-wide + * switch under both walled postures — #10243 again, now with a DURABLE row. + * See `activation-gate-positions-name-authority.test.ts`. * * ## Fail-open on an ABSENT posture is deliberate, not a gap * @@ -57,11 +75,12 @@ // [ADR-0126 §5] The gate's two inputs: the deployment's EFFECTIVE tenancy // posture (the same resolver `resolve-execution-context.ts` uses, so admission -// and this gate can never disagree) and the built-in identity name that means -// "platform operator, NOT a tenant user role" (ADR-0068 D2). +// and this gate can never disagree) and — since #15981 — the caller's ADR-0095 +// authorization RUNG off the execution context, which is where "platform +// operator, NOT a tenant user role" (ADR-0068 D2) still means that. The +// built-in identity NAME is deliberately no longer imported: see the doc block. import { effectiveTenancyPosture } from '@objectstack/core'; import { postureEnforcesWall } from '@objectstack/spec/security'; -import { BUILTIN_IDENTITY_PLATFORM_ADMIN } from '@objectstack/spec/identity'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps } from '../domain-handler-registry.js'; @@ -145,8 +164,11 @@ export async function refuseUngrantedActivationWrite( } if (!posture || !postureEnforcesWall(posture)) return undefined; - const positions: string[] = Array.isArray(ec?.positions) ? ec.positions : []; - if (positions.includes(BUILTIN_IDENTITY_PLATFORM_ADMIN)) return undefined; + // [ADR-0095 D2/D3 · #15981] The platform-operator test is the posture RUNG. + // See the "Why the operator test is a POSITION" section above for why this + // is the SAME concept that section argues for, read through the one input + // that still means it. + if (ec?.posture === 'PLATFORM_ADMIN') return undefined; // The message names the posture and the sanctioned path — the loud-refusal // shape ADR-0126 §7 asks for throughout — and says nothing about the diff --git a/packages/runtime/src/domains/automation-activation-posture-gate.test.ts b/packages/runtime/src/domains/automation-activation-posture-gate.test.ts index 7ebd09e25c..9f45dd42f2 100644 --- a/packages/runtime/src/domains/automation-activation-posture-gate.test.ts +++ b/packages/runtime/src/domains/automation-activation-posture-gate.test.ts @@ -100,10 +100,35 @@ const PLATFORM_OPERATOR = (): HttpProtocolContext => ({ positions: ['platform_admin'], permissions: ['admin_full_access'], systemPermissions: ['manage_metadata'], + // [#15981] The ADR-0095 rung, which is what the gate now reads. A real + // operator's resolved context carries it AND the projected position name; + // this fixture used to carry only the NAME, and a name stopped being + // platform evidence when `positions[]` became the security axis (it also + // carries ADR-0057 D4 `sys_user_position` names, mintable by a tenant). + posture: 'PLATFORM_ADMIN', organizationId: null, }, } as HttpProtocolContext); +/** + * [#15981] The ESCALATION shape: a tenant org admin who ALSO holds a + * `sys_user_position` row spelling the built-in name. `sys_user_position` is + * `apiEnabled` with unconstrained `position` values, so a tenant can mint this + * for themselves; the rung stays `TENANT_ADMIN`, because it is derived from the + * unscoped `admin_full_access` grant they do not hold. Must be REFUSED. + */ +const MINTED_NAME_ONLY = (): HttpProtocolContext => ({ + request: {}, + executionContext: { + userId: 'u_impostor', + positions: ['platform_admin', 'org_owner'], + permissions: ['organization_admin'], + systemPermissions: ['manage_metadata'], + posture: 'TENANT_ADMIN', + organizationId: 'org_northwind', + }, +} as HttpProtocolContext); + /** Engine self-invocation — never settable from the wire. */ const SYSTEM = (): HttpProtocolContext => ({ request: {}, @@ -197,6 +222,18 @@ describe('ADR-0126 §5 — the activation write is operator-gated in walled post expect(h.toggleFlow).toHaveBeenCalledWith(FLOW, false); }); + it('[#15981] REFUSES a tenant admin holding only a MINTED `platform_admin` name', async () => { + const h = boot(posture); + + const { response } = await toggle(h, MINTED_NAME_ONLY()); + + expect(statusOf(response)).toBe(403); + expect(codeOf(response)).toBe('PERMISSION_DENIED'); + // Refused BEFORE the install-wide write — the half a status + // assertion alone would not establish. + expect(h.toggleFlow).not.toHaveBeenCalled(); + }); + it('ALLOWS engine self-invocation', async () => { const h = boot(posture); diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 81ef3511f3..38f5b095b8 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -2126,6 +2126,16 @@ "verb": "delete", "pinned": 1 }, + { + "file": "packages/plugins/plugin-approvals/src/approval-positions-name-authority.test.ts", + "verb": "delete", + "pinned": 1 + }, + { + "file": "packages/plugins/plugin-approvals/src/approval-positions-name-authority.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/plugins/plugin-approvals/src/backfill-platform-row-organizations.test.ts", "verb": "update",