Skip to content

Commit 917b87e

Browse files
claude[bot]claude
andauthored
fix(approvals): isOverrideActor resolves TENANT-admin standing from the ADR-0095 rung, never from a position name (#18252)
Fixes #16166 Clause-②: no `ApprovalService.isOverrideActor`'s TENANT arm derived override authority from a built-in identity NAME on `current_user.positions` — the half PR #16148 deliberately left when it closed the PLATFORM arm of the same predicate. This lands the tenant half the same way: **read the rung, never the name.** ## Step 1 was to DRIVE it — the three questions, in order, each with a reading The card and the triage comment both required the mechanism be measured before anything was built, and required each step be reported rather than concluded from shape. It was, on the tree at `68fea8bc` (the branch base). **Q1 — can a `manageAssignments`-only delegate mint an assignment row spelling `org_owner`?** Measured on both halves, in a throwaway harness (no permanent file: #15972's landed suite already pins this door). - The ADR-0090 D12 delegated-admin gate **approves** it, for `org_owner` and `org_admin` alike — the vacuity #15948 recorded, re-measured here for the org names rather than carried. Control: the same gate refuses when the scope does not carry `manageAssignments`. - The write is **refused anyway**, one layer up, by `plugin-security`'s `reserved_identity_position` object validation on a real engine over a real SQL driver — `VALIDATION_FAILED`, for both names. Negative control: an ordinary position name (`org_manager`) still writes, so the object is not simply refusing everything. ⇒ **The minting route the card names is gated upstream. That is a FINDING, not a failure** — and it is exactly the shape the triage comment reserved that word for. Two neighbouring routes were checked and are closed too: `sys_member.role` is a closed, write-enforced select whose HTTP surface is read-only, so the projection cannot be poisoned from there either. **Q2 — does such a row, once it exists, move `isOverrideActor`'s verdict?** Yes. Measured through the REAL `resolveUserAuthzGrants` with the actor resolved from stored rows, on three doors — `decideNode`, `recall`, and the console's participant-visibility read. All three admitted a non-slate, non-submitter actor whose resolved posture was `MEMBER` and who held no org-administration capability at all. The pin's premise legs assert exactly that separation before any door is driven, so the result cannot be an artifact of the actor accidentally holding standing. ⇒ **Q1 and Q2 together are why the reader still has to be fixed.** The write-side ruling refused NEW writes and explicitly declined a migration, so a row predating it resolves into `positions[]` on every request; and a reader that trusts a name is not an invariant in any case, which is the whole reason the write door was built. **Q3 — is any site already gated such that the name-read reaches only a misreport?** No. The console read here is not an explain panel: it returns the rows, with `can_override` set. ## The tenant rung, established from the resolver's own source — ⛔ not copied across The platform arm's expression is deliberately not reused. ADR-0095 D3 resolves the rung in `derivePosture` (`packages/core/src/security/posture-ladder.ts`) from held CAPABILITY grants: `PLATFORM_ADMIN` from the unscoped `admin_full_access` grant, and `TENANT_ADMIN` from `ORGANIZATION_ADMIN_GRANTS.some(n => permissions.includes(n))` **and from nothing else** — the identical expression the predicate's second arm already spelled. `packages/spec/src/identity/eval-user.zod.ts` declares those two grants "the source of truth for the `TENANT_ADMIN` posture rung". ⇒ The question the card left open — whether the `ORGANIZATION_ADMIN_GRANTS` capability arm is itself a tenant-authority rung or another weak arm — resolves to **rung**. It and the derived `posture` are one authority read in two spellings, kept apart only so a transport that never resolved `posture` still reads the held grant. Both survive. The two NAMES are the other thing entirely: ADR-0068 D2 declares them "a normalized PROJECTION into `current_user.positions`" whose sources of truth live elsewhere, while the same array also carries ADR-0057 D4 assignment values. A projection is not an authority. Both name arms are removed, and with them the predicate's last read of `positions[]` on either rung. **The whole predicate was read, not just the arm.** `posture === 'TENANT_ADMIN'` sat first in that OR and protected nothing — an OR is only as strong as its weakest arm. ## What does not change The #3424 stuck-approval escape hatch is intact for anyone who actually holds org-admin standing: a genuine `organization_admin` grant still overrides, still only within its own organization, and the decision is still audited as `via_override`. Three CONTROL legs assert that on all three doors, and they were green before this change as well as after — they are the floor, not the result. ## Verification **Reverse verification, both legs taken from COMMITTED states.** The pin was committed RED first (`bd157492c`), then the fix (`d40b9afd5`): - pin at `bd157492c` (fix absent): **`Tests 10 failed | 6 passed (16)`** - pin at `d40b9afd5` (fix present): **`Tests 16 passed (16)`** The 6 that passed RED are the premise legs and the three controls — i.e. the harness was already discriminating before the fix, and the 10 failures are the escalation itself, not a broken harness. **Package suites** (`@objectstack/plugin-approvals`): `pnpm test` → `Test Files 46 passed (46) · Tests 754 passed (754)`; `pnpm typecheck` → exit 0, including `check:test-typecheck`. **Gate families** — derived mechanically from the change set by `node scripts/pm/dispatch-gates.mjs`, then reconciled with `--ran` carrying each recorded exit code: **73 derived · 70 run green · 3 NOT MEASURED · 0 UNRUN.** The three are `check:dual-build-cjs-loads`, `check:i18n` and `check:type-check-debt`, each **exit 3 = PREREQUISITE NOT MET** (they read a full-repo build that this container does not hold). ⛔ Exit 3 is not a pass; those three are CI's. `check:engine-double-contract` failed first (exit 1) because the new pin declares its own `update`/`delete` doubles and the ledger had not learned about the file. Regenerated with `--write` — additive only, **2 rows added, 0 lost** — and the gate is green at `783180eba`. **Repo-wide lint**: `pnpm lint` (`eslint . --no-inline-config`) run in full, **exit 0**, 91s, at `783180eba`. No narrowing was used, so no narrowing evidence is owed. **Base freshness**: branched from `68fea8bca`; four commits have landed on `main` since and none touches `plugin-approvals`, `plugin-security` or the authz resolver, so `main` was not merged in. The merge queue rebuilds this as merged onto current `main` anyway. ## Clause-②: no — derived from the DELIVERED diff, with controls Derived by REACHABILITY FROM THE PUBLISHED ENTRY (the barrel's re-export list plus this package's `exports` / `files`), ⛔ not from the word `export` and ⛔ not from a bundle grep. The package was built at the branch base and at HEAD and the two `dist/index.d.ts` were compared: - **The only delta is TSDoc prose** attached to `private isOverrideActor;`. That declaration line is byte-identical, and it carries no signature to widen — nothing was added, removed or narrowed on the published surface. - **Positive control** — `ApprovalService`: 43 occurrences in both builds, so the artifact really is the published surface and the reading discriminates. - **Negative control** — `BUILTIN_IDENTITY_ORG_OWNER`: **0** in both builds. The constants this diff stopped importing were never reachable from the published entry, so dropping the import moves nothing published. - **Negative control** — a helper local to the new test file: **0**, so the test contributes nothing to the entry. The behavioural direction is a NARROWING — an authority path removed — which is the opposite of the widening tell. The changeset is `patch`, on `@objectstack/plugin-approvals`. The before/after measurement mutated the service file on disk and restored it; the restore is evidenced by `git hash-object` equalling the HEAD blob (`bbae6c8f…`) and by an empty `git diff HEAD`, and `dist/` was rebuilt at HEAD afterwards so nothing is left standing at the base build. ## Acceptance notes Two observations, noted and ⛔ not filed — neither is a reproducible defect, a contract violation, or a metadata-authoring trap: - `plugin-security`'s `sys_invitation_org_admin` RLS policy takes a `positions` domain on the same two built-in names. It is a different kind of read — a row-visibility WIDENING inside an already org-scoped select, with its rationale written out at the declaration (the domain only ever widens, so a principal it does not match fails closed). Its reach is bounded by Layer 0 and it grants no override authority. Successor: any future sweep of built-in-name reads. - The delegated-admin gate's `boundSets.every(...)` is vacuous for a position that distributes no permission set. That is a real property, it is already recorded in `plugin-security`'s own landed suite as the reason the gate cannot be where a reserved name is refused, and it is not this card's to change. Successor: #15972's write-side lane. ⛔ Per the card's family this body carries no reproduction recipe; the driving harness lives in the test suite, which is where it belongs. Authored by Claude Code in session https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj — recorded here in prose because this body's trailing footer block is the platform's to write. --- _Generated by [Claude Code](https://claude.ai/code)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent fd12471 commit 917b87e

4 files changed

Lines changed: 483 additions & 13 deletions

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
"@objectstack/plugin-approvals": patch
3+
---
4+
5+
`ApprovalService`'s privileged-override gate now resolves TENANT-admin standing from the ADR-0095 capability rung alone. Its tenant arm previously also admitted any principal whose `current_user.positions` contained the built-in identity names `org_owner` or `org_admin`, and a name on that array is not evidence of the capability behind it (#16166).
6+
7+
`positions[]` carries two different things at once: the ADR-0068 D2 **projection** of a membership role, whose source of truth is `sys_member.role`, and ADR-0057 D4 `sys_user_position` assignment values. A stored assignment row spelling one of those built-in names therefore arrived on the array with no org-administration grant behind it and satisfied the override gate anyway — for `decideNode`, `recall` and the console's participant-visibility read, within that organization. This is the tenant half of the same defect the platform arm of the same predicate had (#15981), and it lands the same way: **read the rung, never the name.**
8+
9+
- **The tenant rung is not the platform one.** ADR-0095 D3 resolves `TENANT_ADMIN` in `derivePosture` from the org-admin capability grants (`organization_admin` / `organization_admin_no_bypass`) and from nothing else, and those grants are what `packages/spec` declares that rung's source of truth. So the surviving two arms — the derived `posture` and the held capability — are one authority read in two spellings, kept apart only so a transport that never resolved `posture` still reads the grant.
10+
- **The #3424 stuck-approval escape hatch is unchanged** for anyone who actually holds org-admin standing: a genuine `organization_admin` grant still overrides, still only inside its own organization, and the decision is still audited as `via_override`.
11+
- **Who could notice.** A principal whose only claim to tenant-admin override was a stored `sys_user_position` row spelling `org_owner` / `org_admin` loses it. That row was never an assignment of the identity it spells — the platform refuses new ones on write — and the supported route to override standing is the org-admin capability grant, which the membership role provisions automatically for owners and admins.

‎packages/plugins/plugin-approvals/src/approval-service.ts‎

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -22,15 +22,14 @@ import { ExpressionEngine, collectCelRootIdentifiers } from '@objectstack/formul
2222
// a third answer to a question the codebase already answered two ways.
2323
import { createRecordOrganizationResolver, type RecordOrganizationResolver } from '@objectstack/metadata-core';
2424
import { keysetWalk, strandedDecisionFailure } from '@objectstack/types';
25-
// [#15981] `BUILTIN_IDENTITY_PLATFORM_ADMIN` is deliberately absent: the
26-
// platform arm of `isOverrideActor` reads the ADR-0095 rung, never the name.
27-
// The two org-level built-ins below are a NARROWER question and are untouched
28-
// here — see that predicate's doc block.
25+
// [#15981 / #16166] Every built-in identity NAME is deliberately absent here:
26+
// both arms of `isOverrideActor` read an ADR-0095 capability rung, never a name.
27+
// `BUILTIN_IDENTITY_PLATFORM_ADMIN` went with the platform arm's name read and
28+
// `BUILTIN_IDENTITY_ORG_OWNER` / `_ORG_ADMIN` with the tenant arm's — see that
29+
// predicate's doc block.
2930
import {
3031
ADMIN_FULL_ACCESS,
3132
ORGANIZATION_ADMIN_GRANTS,
32-
BUILTIN_IDENTITY_ORG_OWNER,
33-
BUILTIN_IDENTITY_ORG_ADMIN,
3433
} from '@objectstack/spec/identity';
3534
import type {
3635
IApprovalService,
@@ -1367,15 +1366,16 @@ export class ApprovalService implements IApprovalService {
13671366
* A platform admin crosses the tenant wall (matching the unscoped
13681367
* `admin_full_access` evidence); a tenant admin may override only within their
13691368
* own org (or an org-less request). A system context always passes. Signals are
1370-
* read defensively off the resolved exec context (`permissions` / `positions` /
1371-
* the derived `posture`, ADR-0095) so any transport that resolves through the
1372-
* shared authz resolver lights this up without extra wiring.
1369+
* read defensively off the resolved exec context (`permissions` and the derived
1370+
* `posture`, ADR-0095) so any transport that resolves through the shared authz
1371+
* resolver lights this up without extra wiring. ⛔ NOT `positions` — on BOTH
1372+
* rungs now: that array carries names, and a name is not an authority (see the
1373+
* two blocks inside).
13731374
*/
13741375
private isOverrideActor(context: ExecutionContext, requestOrg?: string | null): boolean {
13751376
if (!context) return false;
13761377
if (context.isSystem) return true;
13771378
const perms = Array.isArray(context.permissions) ? context.permissions : [];
1378-
const positions = Array.isArray(context.positions) ? context.positions : [];
13791379
// [#7135] A DECLARED read. `posture` (ADR-0095 D2) is resolved by
13801380
// `resolveAuthzContext` and is a field of the envelope the contract has
13811381
// named here since #6523 — the doc block above already says it is the
@@ -1402,10 +1402,29 @@ export class ApprovalService implements IApprovalService {
14021402
const isPlatformAdmin = posture === 'PLATFORM_ADMIN'
14031403
|| perms.includes(ADMIN_FULL_ACCESS);
14041404
if (isPlatformAdmin) return true;
1405+
// ⛔ [#16166] The tenant counterpart of the platform rule above — and
1406+
// deliberately NOT the same expression. ADR-0095 D3 derives `TENANT_ADMIN`
1407+
// in `derivePosture` from `ORGANIZATION_ADMIN_GRANTS.some(n =>
1408+
// permissions.includes(n))` and from nothing else, and
1409+
// `packages/spec/src/identity/eval-user.zod.ts` declares those two grants
1410+
// the source of truth for that rung. So the two arms below are ONE authority
1411+
// read in two spellings — kept apart only so a transport that never resolved
1412+
// `posture` still reads the held capability — and neither of them is the
1413+
// platform side's literal copied across.
1414+
//
1415+
// There is NO `positions.includes(BUILTIN_IDENTITY_ORG_OWNER | _ORG_ADMIN)`
1416+
// arm any more, for the same reason the platform arm lost its name read:
1417+
// ADR-0068 D2 declares those names a normalized PROJECTION into `positions`
1418+
// whose sources of truth are elsewhere (`sys_member.role`), while the same
1419+
// array also carries ADR-0057 D4 `sys_user_position` values — so a stored
1420+
// row spelling one arrived here with no authority behind it, and an OR is
1421+
// only as strong as its weakest arm. The write door refuses such a row today
1422+
// (`plugin-security`'s `reserved_identity_position` rule), but that ruling
1423+
// refused new writes WITHOUT a migration, so rows predating it still resolve
1424+
// on every request — and a reader that trusts a name is not an invariant in
1425+
// any case. Driven in `approval-tenant-positions-name-authority.test.ts`.
14051426
const isTenantAdmin = posture === 'TENANT_ADMIN'
1406-
|| ORGANIZATION_ADMIN_GRANTS.some((n) => perms.includes(n))
1407-
|| positions.includes(BUILTIN_IDENTITY_ORG_OWNER)
1408-
|| positions.includes(BUILTIN_IDENTITY_ORG_ADMIN);
1427+
|| ORGANIZATION_ADMIN_GRANTS.some((n) => perms.includes(n));
14091428
if (!isTenantAdmin) return false;
14101429
// A tenant admin's authority stops at their own org; a null-org request is
14111430
// global and any admin may release it.

0 commit comments

Comments
 (0)