Skip to content

Commit b61353f

Browse files
fix(plugin-approvals): an approval's status write-back runs its automation as the person who acted, in the record's organisation (#24634)
Fixes #24519 Clause-②: no (a producer fix inside `@objectstack/plugin-approvals`: no `@objectstack/spec` key, export, type or stored column moves; ruled by `6271152775`) The status write-back an approval makes (`approvalStatusField`) now runs the automation it fires as the person who acted, with that person's own request envelope in the record's organisation. Ruling `6271152775` on #24519 (maintainer: Gap 1 「A 带记录组织」, the cross-organisation approver 「x 按记录组织」, the live widening 「随 #24519 一起修」). **Landing order.** The claim made this PR wait for PR #24527 (#24321), which edits the same file at the decision to resume hand-off. PR #24527 has merged (`1b4078077`), and this branch merges `origin/main` past it at `8b118b6b3`. The two changes touch disjoint regions. The PR stays draft for the seat's review of record. ## What was wrong `ApprovalService.mirrorStatusField` wrote as `{ isSystem: true, userId, positions: [], permissions: [] }`, with no organisation, on every door that names a person: submit (through the approval node), approve, reject, recall, send back (returned and auto-reject), and the action link. Phase 1 (report `6270420946`) measured on a real stack what a `runAs: 'user'` consumer of that write received: - a `runAs: 'user'` hook kept the carried empty arrays verbatim (the rule PR #24518 keeps for a narrowed ceiling), so it ran on the member baseline instead of the person's own grants, and it saw only the person in `sys_user`; - a `runAs: 'user'` record-change flow fired by the write got no organisation, so its grants resolved organisation-less, which is not the person's request in the record's organisation. The docblock's reason for leaving the organisation off no longer held: since #10101 the request's organisation is the subject record's, and under `single` the driver scopes nothing. Its other claim, that the automation engine back-fills the run's `tenantId`, was false: the grants bridge answers a `tenantId` only when it was handed one. ## The change - **`mirrorStatusField`'s named-actor branch** writes under `attributedMirrorContext(...)`: `{ isSystem: true, userId, tenantId }` and nothing else. - `tenantId` is the subject record's LIVE organisation. It is read at write time, one by-id read of the organisation column, through `recordOrgResolver`, the resolver the request row was stamped by. The request's organisation is the fallback for an object (or a record) with none, as at the stamp. - No `positions` / `permissions`. The write itself is authorized by `isSystem` alone (the security middleware short-circuits before it reads a grant), so it still crosses the record lock and still lands a read-only status column. Every `runAs: 'user'` consumer now resolves the person's own envelope in that organisation. - **The user-less branch** (the dead-run sweep) keeps `SYSTEM_CTX`: a `runAs: 'user'` automation it fires is still refused. - **Every attributed write acts in the record's organisation,** an approver drawn from another organisation's directory (ADR-0105 D9) included. That is the reading PR #24527 applies to a `runAs: 'user'` decider. - **The six call sites** each pass the request organisation they already hold (`org` / `requestOrg`) as the fallback: one argument each. - **The docblock** is rewritten, the false back-fill sentence included. - **Untouched**, as the claim fences: `approval-node.ts`, `SYSTEM_CTX`, the action-link context (Phase 1 measured that no write is made under any of them), and PR #24527's resume hand-off. ## Pins `packages/plugins/plugin-approvals/src/status-mirror-envelope.integration.test.ts` (new, 10 cases). It uses a real kernel, the real ObjectQL engine and hook wrappers, the real record-change trigger and automation engine, the real `ApprovalService` and approval node executor, and driver-sql on better-sqlite3 `:memory:`. The authz tables the resolver reads are spelled locally (the `status-mirror-cascade` precedent: no new dependency, no lockfile change), and the security catalog is bound with the package's test kit. The fixture is synthetic: two organisations; a person whose memberships differ between them; a person holding a grant the baseline lacks; an approver from the other organisation. For each door that names a person, the envelope a `runAs: 'user'` hook and a `runAs: 'user'` record-change flow receive equals `resolveUserAuthzGrants(person, { tenantId: ORG })` (positions, permission sets, peers), and the write carries `isSystem`, the person, `ORG` and no arrays: - open through the approval node (the submitter), approve, reject, recall, send back, send back past the revision limit, action-link redeem; - direction 1: the grant held beyond the baseline is carried (approve, reject); - direction 2: the person whose memberships differ carries only the record organisation's (open, recall); - the approver from another organisation acts in the record's organisation, with its members as peers; - a record re-parented while its request was pending is written, and cascades, in its new organisation; - CONTROL: the dead-run sweep stays user-less, the hook is refused (`HOOK_UNSCOPED_DATA_ACCESS`), and the flow runs with no person. What the security plugin then enforces for an envelope stays pinned where the seed lives (`service-automation`'s `runas-user-hook-org-peers.integration.test.ts`). **Moved pin, one:** `approval-service.test.ts` › `status mirror identity (#3783)` asserted the mirror carries NO `tenantId`, which is the contract the ruling reverses. It now asserts the full context, `{ isSystem: true, userId: 'u1', tenantId: 't1' }`, on that suite's schema-less engine double (the request's organisation as the fallback). The other nine pins of that describe are unchanged and green. Kin sweep: no other package test reaches the real mirror. ## Ablation (fix committed first; `scripts/ablation-replace.mjs` on `approval-service.ts`; the suite imports the service from source, so no dist leg) Both legs were predicted before running: 9 red (every door that names a person), the CONTROL green. - **Leg 1, restore the empty arrays** (`{ ...SYSTEM_CTX, userId, tenantId }`; blob `9ceb29f2c7f9` → `d2258164b9de`): **9 failed / 1 passed**. Each fails at the hook's envelope: positions and permission sets `[]` where the person's own were expected. - **Leg 2, drop the organisation** (`{ isSystem: true, userId }`; blob `9ceb29f2c7f9` → `3522d1ac69f9`): **9 failed / 1 passed**. Each fails at the hook's envelope: `tenantId` undefined, peers = the person alone, and positions and permission sets that are not the record organisation's. - Each leg restored by the tool: blob after restore `9ceb29f2c7f9` == HEAD, `git diff HEAD` empty. ## Tests and gates (final head `8b118b6b3`, which merges `origin/main` at `65a8eeb7d`) - `pnpm --filter @objectstack/plugin-approvals test`: 83 files, 1132 tests passed (`TEST_EXIT=0`, under `scripts/pm/os-verify-lock.sh`). - `pnpm --filter @objectstack/plugin-approvals typecheck`: exit 0; `check:test-typecheck: OK` (324 errors / 27 signatures held, debt unchanged). - Derived gates (`node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack`, no paths, after the last commit): 69 commands, all 69 exit 0. `--ran`: `69 derived, 69 run, 0 NOT-MEASURED, 0 UNRUN`. - On the first pass, `check:query-options-erasure` and `check:slot-lookup` each flagged one new erasure in this diff. Both were fixed by typing (the record read no longer casts its options; the pin's engine lookup is typed `ObjectQL`), not by baselining. - `check:i18n` and `check:dual-build-cjs-loads` first exited 3 (`PREREQUISITE NOT MET`); after a full `turbo run build` both exit 0. - `node scripts/check-nul-bytes.mjs`: OK. - CI's families are read from this PR's check-runs. ## Acceptance notes - **Same family, out of this PR:** `service-automation`'s `resolveRunDataContext` system arm carries `positions: []` / `permissions: []` beside a carried `userId`. It is the shape PR #24527's `runAs: 'system'` post-decision segment writes with. The seat files it as its own family card under the same rule (ruling `6271152775` item 5). - **Visible to hook authors:** on a mirror write, `ctx.session.organizationId` now carries the organisation, and `ctx.session.positions` is absent instead of `[]`. The changeset says so. - **Cost:** one by-id read of the record's organisation column per named-actor mirror write, skipped for an object with no organisation column. - **Walled posture:** naming the record's live organisation scopes the mirror update to that organisation (and organisation-less rows), which the record always satisfies. A `runAs: 'system'` cascade of the mirror is now org-scoped there as well, the #5494 posture user-triggered system runs already have. --- _Generated by [Claude Code](https://claude.ai/code/session_01LR1cD9JqgkeqY7k9rrZH8s)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent af1b3f8 commit b61353f

4 files changed

Lines changed: 503 additions & 27 deletions

File tree

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
'@objectstack/plugin-approvals': patch
3+
---
4+
5+
fix(plugin-approvals): an approval's status write-back runs its automation as the person who acted, in the record's organisation
6+
7+
Clause-②: no
8+
9+
- **What changes.** The write that mirrors a request's status onto the record's `approvalStatusField` (submit, approve, reject, recall, send back, the action link) now names the organisation of the record it writes, and no longer carries an empty set of positions and permission sets. A `runAs: 'user'` hook on that object, and a `runAs: 'user'` record-change flow fired by that write, now run with the acting person's own request envelope in the record's organisation: their positions, their permission sets and their organisation's members, resolved exactly as their own request in that organisation resolves them.
10+
- **Before.** Such a hook ran on the member baseline and saw only the acting person in `sys_user`. Such a flow resolved the person's grants with no organisation, so it saw only the person as a peer and did not run with what the record's organisation grants them.
11+
- **Every acting person, one organisation.** An approver drawn from another organisation's directory also acts in the record's organisation. The organisation is read from the record when the status is written, so a record moved to another organisation while its approval was pending is written, and cascades, in its new organisation.
12+
- **Unchanged.** The write stays elevated (`isSystem`), so it still lands on a locked record and on a read-only status field. A status written by a sweep with no person behind it (the dead-run release) stays user-less, so a `runAs: 'user'` automation it fires is still refused. `ctx.session.organizationId` in a hook on the write now carries the organisation, and `ctx.session.positions` is absent instead of an empty list.
13+
14+
A patch: no export, type, accepted value or stored column changes.

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

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5121,13 +5121,20 @@ describe('status mirror identity (#3783)', () => {
51215121
expect(mirrorContext()).toMatchObject({ isSystem: true });
51225122
});
51235123

5124-
it('carries the actor WITHOUT org-scoping the write', async () => {
5125-
// `tenantId` on an ExecutionContext is a driver-scoping knob, not
5126-
// attribution: ObjectQL turns it into a tenant predicate on the update. The
5127-
// submitter's org (`t1` on CTX) must therefore not ride along, or the mirror
5128-
// would silently no-op on a record whose org differs from the request's.
5124+
it('names the organisation it acts in, and carries no grants of its own (#24519)', async () => {
5125+
// Ruling 6271152775 on #24519 reverses what this pin used to assert: a
5126+
// named actor's mirror names its organisation through `tenantId`, so a
5127+
// `runAs:'user'` consumer resolves the actor's own request envelope in
5128+
// it. The organisation is the subject record's, read live; this fake
5129+
// engine exposes no schema, so the object has none of its own and the
5130+
// request's organisation stands in (`t1`, from the opening context), as
5131+
// it does for the request row's stamp. No `positions` / `permissions`
5132+
// ride along — a consumer would keep them verbatim — and no
5133+
// `organizationId`, which is not an envelope field. The record-organisation
5134+
// read and the end-to-end envelope are pinned in
5135+
// `status-mirror-envelope.integration.test.ts`.
51295136
await open();
5130-
expect(mirrorContext()).not.toHaveProperty('tenantId');
5131-
expect(mirrorContext()).not.toHaveProperty('organizationId');
5137+
expect(engine._tables['sys_approval_request'][0].organization_id).toBe('t1');
5138+
expect(mirrorContext()).toEqual({ isSystem: true, userId: 'u1', tenantId: 't1' });
51325139
});
51335140
});

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

Lines changed: 78 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3183,39 +3183,97 @@ export class ApprovalService implements IApprovalService {
31833183
* most natural approvals automation there is, had to declare `runAs:'system'`
31843184
* and take blanket elevation for a case where a perfectly good scoped identity
31853185
* existed. Re-attaching the actor lets those cascades run as the deciding user
3186-
* with RLS enforced. Same shape the approval node already uses when it calls
3187-
* into this service (`approval-node.ts`).
3186+
* with RLS enforced.
31883187
*
31893188
* `actorId` is `null` for the genuinely machine-driven transitions (the SLA
31903189
* escalation's auto-decision, the dead-run sweep). There is no human to name
31913190
* there, and naming a sentinel would put a non-user in `updated_by` and in
3192-
* every downstream flow's identity. Those cascades stay user-less — a flow
3193-
* that wants to react to them still has to declare `runAs:'system'`, which is
3194-
* the honest answer rather than an oversight.
3195-
*
3196-
* Deliberately carries `userId` ONLY, not the request's org. On an
3197-
* ExecutionContext `tenantId` is a driver-scoping knob, not attribution
3198-
* (`buildDriverOptions` turns it into a tenant predicate on the update), so
3199-
* passing it would newly org-scope this write and silently no-op the mirror on
3200-
* a record whose org differs from the request's — while buying nothing: the
3201-
* automation engine back-fills the run's `tenantId` from the resolved user's
3202-
* own grants.
3191+
* every downstream flow's identity. Those cascades stay user-less, under
3192+
* {@link SYSTEM_CTX} — a flow that wants to react to them still has to
3193+
* declare `runAs:'system'`, which is the honest answer rather than an
3194+
* oversight.
3195+
*
3196+
* **A named actor's write acts in the record's organisation, and carries no
3197+
* grants of its own (#24519, ruling 6271152775).** Its context is
3198+
* {@link attributedMirrorContext}: `isSystem`, the actor's `userId`, and
3199+
* `tenantId` = the organisation of the record being written. Every consumer
3200+
* that runs as the actor then resolves the actor's own request envelope in
3201+
* that organisation, through the one resolver the request path uses
3202+
* (`resolveUserAuthzGrants`):
3203+
* - a `runAs:'user'` hook's pinned api is seeded with the actor's positions,
3204+
* permission sets and organisation peers. Positions and permission sets a
3205+
* context CARRIES are kept verbatim, so the empty arrays this write used to
3206+
* carry left such a hook on the member baseline instead of the actor's own
3207+
* grants;
3208+
* - a record-change flow receives the actor and the organisation as its
3209+
* `tenantId`, and a `runAs:'user'` run resolves the same envelope under it.
3210+
* Nothing downstream supplies a missing organisation: the automation
3211+
* engine's grants bridge answers a `tenantId` only when it was handed one,
3212+
* and an organisation-less resolution is not the actor's request in the
3213+
* record's organisation — its peers are the actor alone, and its grants
3214+
* are not the ones that organisation gives them.
3215+
* The write itself is authorized by `isSystem` alone — the security
3216+
* middleware short-circuits before it reads any grant — so leaving the arrays
3217+
* off changes nothing about what this write may do.
3218+
*
3219+
* The organisation is the record's, for every actor: an approver drawn from
3220+
* another organisation's directory (ADR-0105 D9) included, which is the
3221+
* reading #24321 applies to a `runAs:'user'` flow's decider. On an
3222+
* ExecutionContext `tenantId` is also the driver's scoping knob
3223+
* (`buildDriverOptions`): under a walled posture it narrows this update to that
3224+
* organisation's rows (and organisation-less ones), under `single` it scopes
3225+
* nothing (ADR-0131 D11). That is why the organisation is the record's LIVE
3226+
* one, read at write time, and not the request's stamp: the two agree by
3227+
* construction (#10101 stamps the request with the subject record's
3228+
* organisation), except for a record re-parented while its approval was
3229+
* pending, which the stamp would scope this write away from.
32033230
*/
32043231
private async mirrorStatusField(
32053232
object: string,
32063233
recordId: string,
32073234
field: string,
32083235
status: string,
32093236
actorId: string | null,
3237+
requestOrg: string | null | undefined,
32103238
): Promise<void> {
32113239
try {
3212-
const context = actorId ? { ...SYSTEM_CTX, userId: actorId } : SYSTEM_CTX;
3240+
const context = actorId
3241+
? await this.attributedMirrorContext(object, recordId, actorId, requestOrg)
3242+
: SYSTEM_CTX;
32133243
await this.engine.update(object, { id: recordId, [field]: status }, { context });
32143244
} catch (err: any) {
32153245
this.logger?.warn?.(`[approvals] mirrorStatusField failed: ${err?.message ?? err}`);
32163246
}
32173247
}
32183248

3249+
/**
3250+
* The context a named actor's status mirror writes under (see
3251+
* {@link mirrorStatusField}): `{ isSystem: true, userId, tenantId }` and
3252+
* nothing else. The organisation is the subject record's live one, read
3253+
* through {@link recordOrgResolver} — the resolver the request row was
3254+
* stamped by — and the request's own organisation for an object that has
3255+
* none (or a record that carries none), as at the stamp. ⛔ No `positions` /
3256+
* `permissions`: a carried array is kept verbatim by every `runAs:'user'`
3257+
* consumer, so an empty one would stand in for the actor's own grants.
3258+
*/
3259+
private async attributedMirrorContext(
3260+
object: string,
3261+
recordId: string,
3262+
actorId: string,
3263+
requestOrg: string | null | undefined,
3264+
): Promise<ExecutionContext> {
3265+
const column = this.recordOrgResolver.organizationFieldFor(object);
3266+
let organization: string | null = null;
3267+
if (column) {
3268+
const rows = await this.engine.find(object, {
3269+
where: { id: recordId }, fields: ['id', column], limit: 1, context: SYSTEM_CTX,
3270+
});
3271+
organization = this.recordOrgResolver.organizationOf(object, Array.isArray(rows) ? rows[0] : undefined);
3272+
}
3273+
if (!organization && requestOrg != null && String(requestOrg) !== '') organization = String(requestOrg);
3274+
return { isSystem: true, userId: actorId, ...(organization ? { tenantId: organization } : {}) };
3275+
}
3276+
32193277
/**
32203278
* Re-read a business record's CURRENT state by id so approver resolution binds
32213279
* to live data at node entry, not the trigger snapshot the flow froze into
@@ -3697,7 +3755,7 @@ export class ApprovalService implements IApprovalService {
36973755
// first-party path.
36983756
await this.mirrorStatusField(
36993757
input.object, input.recordId, input.config.approvalStatusField, 'pending',
3700-
row.submitter_id ?? null,
3758+
row.submitter_id ?? null, requestOrg,
37013759
);
37023760
}
37033761

@@ -4008,7 +4066,7 @@ export class ApprovalService implements IApprovalService {
40084066
if (config.approvalStatusField) {
40094067
await this.mirrorStatusField(
40104068
raw.object_name, raw.record_id, config.approvalStatusField, finalStatus,
4011-
actingUserId(context),
4069+
actingUserId(context), org,
40124070
);
40134071
}
40144072
const fresh = await this.readBackRequest(requestId, context);
@@ -4498,7 +4556,7 @@ export class ApprovalService implements IApprovalService {
44984556
if (config.approvalStatusField) {
44994557
await this.mirrorStatusField(
45004558
raw.object_name, raw.record_id, config.approvalStatusField, 'recalled',
4501-
actingUserId(context),
4559+
actingUserId(context), org,
45024560
);
45034561
}
45044562

@@ -4837,7 +4895,7 @@ export class ApprovalService implements IApprovalService {
48374895
if (config.approvalStatusField) {
48384896
await this.mirrorStatusField(
48394897
raw.object_name, raw.record_id, config.approvalStatusField, 'rejected',
4840-
actingUserId(context),
4898+
actingUserId(context), org,
48414899
);
48424900
}
48434901
let resumed = false;
@@ -4879,7 +4937,7 @@ export class ApprovalService implements IApprovalService {
48794937
if (config.approvalStatusField) {
48804938
await this.mirrorStatusField(
48814939
raw.object_name, raw.record_id, config.approvalStatusField, 'returned',
4882-
actingUserId(context),
4940+
actingUserId(context), org,
48834941
);
48844942
}
48854943

@@ -6552,7 +6610,7 @@ export class ApprovalService implements IApprovalService {
65526610
// No human did this — a sweep did. Left user-less on purpose (#3783): a
65536611
// flow that wants to react to a dead-run release declares runAs:'system'.
65546612
await this.mirrorStatusField(
6555-
raw.object_name, raw.record_id, config.approvalStatusField, 'recalled', null,
6613+
raw.object_name, raw.record_id, config.approvalStatusField, 'recalled', null, org,
65566614
);
65576615
}
65586616

0 commit comments

Comments
 (0)