diff --git a/.changeset/approval-status-mirror-names-the-actor.md b/.changeset/approval-status-mirror-names-the-actor.md new file mode 100644 index 0000000000..6fe10bfb09 --- /dev/null +++ b/.changeset/approval-status-mirror-names-the-actor.md @@ -0,0 +1,56 @@ +--- +"@objectstack/plugin-approvals": minor +"@objectstack/service-automation": patch +--- + +fix(approvals): the status mirror names the human who caused the transition (#3783) + +When an approval moves, the service writes the new status onto the business +record (`approvalStatusField`). That write is what fires the record-change flows +bound to that object — so it is the seam "when the invoice is approved, do X" +runs through. It presented a bare `{ isSystem: true }` context with **no +`userId`**, at six call sites that each know exactly who acted: a submitter +submitting, an approver approving, rejecting, sending back, recalling. + +Combined with #3760 — which stopped letting a `runAs:'user'` run with no trigger +user touch data — that identity gap made the most natural approvals automation +there is unwritable in its obvious form. The cascade inherited no user, so its +data nodes were refused, and the author's only way forward was to declare +`runAs: 'system'` and take blanket elevation for a case where a perfectly good +scoped identity existed at the call site all along. + +The mirror now carries the acting user. It stays `isSystem` — the record is +normally locked while its approval is live, so only a platform write can land the +status — because elevation and anonymity are separate choices, and this write +only ever needed the first. Cascades now run as the deciding user with RLS +enforced. + +- **The identity is the authenticated principal, never the request body's + `actorId`.** `actorId` arrives from the caller (`body.actorId ?? context.userId`) + and is only checked against the pending approver slate, never against the + caller. That is tolerable on an audit row; promoting it to the identity of an + RLS-scoped write would have turned a mislabelled audit trail into identity + spoofing. +- **Approval-by-email-link is attributed too.** ADR-0043 action links carry no + session, so they used to decide as pure system. The single-use hashed token + binds exactly one approver and is re-checked against the live slate at + redemption — that is an authentication — so the redeemed decision now presents + that approver, and an emailed approval cascades identically to one made in the + UI. +- **The two machine-driven transitions stay user-less on purpose**: the SLA + escalation's auto-decision and the dead-run sweep. `system:sla` and + `system:dead-run` are reserved audit actors, not users, and presenting one as a + user would put a non-user in `updated_by` and in every downstream flow's + identity. A flow that wants to react to those declares `runAs:'system'` — the + honest answer, and now a deliberate one rather than an artefact. +- **Attribution only — the write is not newly org-scoped.** On an + ExecutionContext `tenantId` is a driver-scoping knob, not attribution + (ObjectQL turns it into a tenant predicate), so passing the request's org would + have silently no-op'd the mirror on a record whose org differs. The automation + engine already back-fills a run's `tenantId` from the resolved user's grants. + +**Visible change:** the mirrored record's `updated_by` now names the acting user +instead of retaining its previous value — ObjectQL's audit stamping is gated on +the write context's `userId` alone, and `isSystem` buys no exemption. That is the +attribution this fix is for: the approver who set the record to `approved` is now +its last modifier. diff --git a/packages/plugins/plugin-approvals/package.json b/packages/plugins/plugin-approvals/package.json index ee21745681..653d09e273 100644 --- a/packages/plugins/plugin-approvals/package.json +++ b/packages/plugins/plugin-approvals/package.json @@ -26,6 +26,7 @@ "devDependencies": { "@objectstack/objectql": "workspace:*", "@objectstack/service-automation": "workspace:*", + "@objectstack/trigger-record-change": "workspace:*", "@types/node": "^26.1.1", "typescript": "^6.0.3", "vitest": "^4.1.10" diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index a56a547a80..6c2c7a0cb3 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -45,9 +45,13 @@ function makeFakeEngine() { return true; } + /** Every `update` the service made, with the context it presented (#3783). */ + const writes: Array<{ object: string; data: any; context: any }> = []; + return { _tables: tables, _hooks: hooks, + _writes: writes, async find(object: string, options?: any) { const rows = ensure(object).filter(r => matches(r, options?.filter ?? options?.where)); if (options?.orderBy?.[0]) { @@ -73,6 +77,7 @@ function makeFakeEngine() { async update(object: string, idOrData: any, _opts?: any) { const data = typeof idOrData === 'object' ? idOrData : _opts; const id = typeof idOrData === 'object' ? idOrData.id : idOrData; + writes.push({ object, data, context: _opts?.context }); const table = ensure(object); const i = table.findIndex(r => r.id === id); if (i >= 0) table[i] = { ...table[i], ...data }; @@ -2538,3 +2543,135 @@ describe('in-band transitions finalise before they resume (#3456 invariant)', () expectCleanHandoffs(); }); }); + +/** + * #3783 — the status mirror names the human who caused the transition. + * + * The mirror write lands on the CUSTOMER's object, so it is what fires that + * object's record-change flows. It has to stay `isSystem` (the record is locked + * while its approval is live), but dropping the actor left every one of those + * cascades with no trigger user — which #3760 now refuses outright, forcing + * "when the invoice is approved, do X" to declare `runAs:'system'`. + * + * Each case therefore asserts BOTH halves: the elevation survives (or the lock + * hook stops mirroring at all) and the identity is present. + */ +describe('status mirror identity (#3783)', () => { + let engine: ReturnType; + let svc: ApprovalService; + let n = 0; + const baseTime = new Date('2026-01-15T10:00:00Z').getTime(); + + const REVISE_FLOW = { + name: 'deal_approval', + edges: [{ id: 'e_rev', source: 'approve_step', target: 'wait_revision', label: 'revise' }], + }; + + /** The context the service presented on the mirror write, or undefined. */ + const mirrorContext = () => + engine._writes.filter(w => w.object === 'opportunity').at(-1)?.context as any; + + const open = (configExtra: Record = {}, ctx: any = CTX) => + svc.openNodeRequest(openInput(['u9'], {}, { approvalStatusField: 'approval_status', ...configExtra }), ctx); + + beforeEach(() => { + engine = makeFakeEngine(); + n = 0; + svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(baseTime + (n++) * 1000) } }); + svc.attachAutomation({ + async resume() {}, + async cancelRun() {}, + async getFlow() { return REVISE_FLOW; }, + } as any); + engine._tables['opportunity'] = [{ id: 'opp1', amount: 100 }]; + }); + + it('submit: mirrors as the submitter, still elevated', async () => { + await open(); + expect(mirrorContext()).toMatchObject({ isSystem: true, userId: 'u1' }); + }); + + it('decide: mirrors as the deciding user', async () => { + const req = await open(); + const approver = { ...CTX, userId: 'u9' }; + await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, approver as any); + expect(engine._tables['opportunity'][0].approval_status).toBe('approved'); + expect(mirrorContext()).toMatchObject({ isSystem: true, userId: 'u9' }); + }); + + it('recall: mirrors as the recalling user', async () => { + const req = await open(); + await svc.recall(req.id, { actorId: 'u1' }, CTX); + expect(mirrorContext()).toMatchObject({ isSystem: true, userId: 'u1' }); + }); + + it('sendBack: mirrors as the approver who returned it', async () => { + const req = await open(); + const approver = { ...CTX, userId: 'u9' }; + await svc.sendBack(req.id, { actorId: 'u9', comment: 'redo the totals' }, approver as any); + expect(engine._tables['opportunity'][0].approval_status).toBe('returned'); + expect(mirrorContext()).toMatchObject({ isSystem: true, userId: 'u9' }); + }); + + it('sendBack past the revision budget: the auto-reject mirror names the approver too', async () => { + const req = await open({ maxRevisions: 0 }); + const approver = { ...CTX, userId: 'u9' }; + const out = await svc.sendBack(req.id, { actorId: 'u9' }, approver as any); + expect(out.autoRejected, 'expected the auto-reject branch').toBe(true); + expect(engine._tables['opportunity'][0].approval_status).toBe('rejected'); + expect(mirrorContext()).toMatchObject({ isSystem: true, userId: 'u9' }); + }); + + it('action link: mirrors as the approver the token is bound to', async () => { + // ADR-0043 email approval — no session at all, but the single-use hashed + // token names exactly one approver, and `resolveActionToken` has just + // re-checked they still hold a pending slot. That IS an authenticated act. + const req = await open(); + const { approve } = await svc.issueActionTokens(req.id, 'u9'); + expect(await svc.redeemActionToken(approve)).toMatchObject({ ok: true }); + expect(mirrorContext()).toMatchObject({ isSystem: true, userId: 'u9' }); + }); + + it('never takes the identity from the caller-supplied actorId', async () => { + // `actorId` arrives in the REST body (`body.actorId ?? context.userId`) and + // is only checked against the pending slate, never against the caller. It is + // fine on an audit row; making it the identity of an RLS-scoped write would + // let any authenticated caller borrow a slot holder's identity. + const req = await open(); + const someoneElse = { ...CTX, userId: 'intruder' }; + await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, someoneElse as any); + expect(mirrorContext()?.userId).toBe('intruder'); + }); + + it('SLA auto-decision: stays user-less — no human did it', async () => { + const req = await open({ escalation: { timeoutHours: 1, action: 'auto_approve', notifySubmitter: false } }); + const raw = engine._tables['sys_approval_request'].find((r: any) => r.id === req.id)!; + raw.created_at = new Date(baseTime - 3 * 60 * 60 * 1000).toISOString(); + await svc.runEscalations(); + expect(engine._tables['opportunity'][0].approval_status).toBe('approved'); + // `system:sla` is a reserved audit actor, not a user — it must never be + // presented as one. The cascade stays user-less on purpose; a flow that + // wants to react to an SLA auto-decision declares runAs:'system'. + expect(mirrorContext()?.userId).toBeUndefined(); + expect(mirrorContext()).toMatchObject({ isSystem: true }); + }); + + it('dead-run sweep: stays user-less — no human did it', async () => { + await open(); + svc.attachAutomation({ getRun: async () => ({ status: 'failed' }) } as any); + expect(await svc.releaseDeadRunRequests()).toMatchObject({ released: 1 }); + expect(engine._tables['opportunity'][0].approval_status).toBe('recalled'); + expect(mirrorContext()?.userId).toBeUndefined(); + expect(mirrorContext()).toMatchObject({ isSystem: true }); + }); + + it('carries the actor WITHOUT org-scoping the write', async () => { + // `tenantId` on an ExecutionContext is a driver-scoping knob, not + // attribution: ObjectQL turns it into a tenant predicate on the update. The + // submitter's org (`t1` on CTX) must therefore not ride along, or the mirror + // would silently no-op on a record whose org differs from the request's. + await open(); + expect(mirrorContext()).not.toHaveProperty('tenantId'); + expect(mirrorContext()).not.toHaveProperty('organizationId'); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 3732d5d3f8..ba489c534d 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -143,6 +143,27 @@ export type ActionTokenOutcome = const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; +/** + * Who is acting, for the purpose of a data write made on their behalf (#3783). + * + * Reads the AUTHENTICATED principal off the execution context — deliberately not + * `input.actorId`. Every public entrypoint takes `actorId` from the request body + * (`body.actorId ?? context.userId`, see the REST approval routes) and the + * service only checks that it names a pending approver, never that it is the + * caller. Tolerable on an audit row; promoting it to the identity of an + * RLS-scoped write would turn a mislabelled audit trail into identity spoofing. + * A caller holding a trustworthy actor with no session behind it — the ADR-0043 + * action link, whose token cryptographically binds exactly one approver — puts + * that actor ON the context instead of relying on this. + * + * `null` for a machine caller (the SLA sweep passes {@link SYSTEM_CTX}), so a + * reserved sentinel like {@link SLA_ACTOR_ID} can never surface as a `userId`. + */ +function actingUserId(context: SharingExecutionContext | undefined): string | null { + const userId = (context as { userId?: unknown } | undefined)?.userId; + return typeof userId === 'string' && userId ? userId : null; +} + /** * Max hops when following an OOO delegation chain (#1322 M1): A out → B, B out * → C, … Bounds the walk so a mis-configured chain can't loop or resolve @@ -971,10 +992,50 @@ export class ApprovalService implements IApprovalService { return active[0]; } - /** Mirror a request status onto a business-object field, if configured. */ - private async mirrorStatusField(object: string, recordId: string, field: string, status: string): Promise { + /** + * Mirror a request status onto a business-object field, if configured. + * + * **Elevated, but not anonymous (#3783).** The write stays `isSystem`: the + * record is normally LOCKED while its approval is live and the submitter + * cannot edit it, so only a platform write can land the status — that is what + * the lock hook's system exemption (`lifecycle-hooks.ts`) is for. What it must + * NOT do is throw away *who* caused the transition. Every status below is + * something a specific human just did — a submitter submitting or recalling, + * an approver deciding or sending back — and this write is what fires the + * target object's record-change flows. With no `userId` on it those cascades + * inherit no trigger user, and since #3760 a `runAs:'user'` run with no trigger + * user has its data ops REFUSED — so "when the invoice is approved, do X", the + * most natural approvals automation there is, had to declare `runAs:'system'` + * and take blanket elevation for a case where a perfectly good scoped identity + * existed. Re-attaching the actor lets those cascades run as the deciding user + * with RLS enforced. Same shape the approval node already uses when it calls + * into this service (`approval-node.ts`). + * + * `actorId` is `null` for the genuinely machine-driven transitions (the SLA + * escalation's auto-decision, the dead-run sweep). There is no human to name + * there, and naming a sentinel would put a non-user in `updated_by` and in + * every downstream flow's identity. Those cascades stay user-less — a flow + * that wants to react to them still has to declare `runAs:'system'`, which is + * the honest answer rather than an oversight. + * + * Deliberately carries `userId` ONLY, not the request's org. On an + * ExecutionContext `tenantId` is a driver-scoping knob, not attribution + * (`buildDriverOptions` turns it into a tenant predicate on the update), so + * passing it would newly org-scope this write and silently no-op the mirror on + * a record whose org differs from the request's — while buying nothing: the + * automation engine back-fills the run's `tenantId` from the resolved user's + * own grants. + */ + private async mirrorStatusField( + object: string, + recordId: string, + field: string, + status: string, + actorId: string | null, + ): Promise { try { - await this.engine.update(object, { id: recordId, [field]: status }, { context: SYSTEM_CTX }); + const context = actorId ? { ...SYSTEM_CTX, userId: actorId } : SYSTEM_CTX; + await this.engine.update(object, { id: recordId, [field]: status }, { context }); } catch (err: any) { this.logger?.warn?.(`[approvals] mirrorStatusField failed: ${err?.message ?? err}`); } @@ -1227,7 +1288,17 @@ export class ApprovalService implements IApprovalService { // Record lock (when `lockRecord !== false`) is enforced by the beforeUpdate // hook keyed on the now-pending request; no extra write needed here. if (input.config.approvalStatusField) { - await this.mirrorStatusField(input.object, input.recordId, input.config.approvalStatusField, 'pending'); + // Attributed to whoever the row itself calls the submitter (#3783), so + // there is exactly one answer to "who submitted this". Not the + // {@link actingUserId} route: `submitterId` is server-supplied here (the + // approval node passes the run's own trigger user) and unreachable from a + // request body, so it carries none of the caller-controlled risk that rule + // exists for — and it already resolves to `context.userId` in every + // first-party path. + await this.mirrorStatusField( + input.object, input.recordId, input.config.approvalStatusField, 'pending', + row.submitter_id ?? null, + ); } return rowFromRequest(row); @@ -1429,7 +1500,10 @@ export class ApprovalService implements IApprovalService { }, { context: SYSTEM_CTX }); await this.syncApproverIndex(requestId, [], org, now); if (config.approvalStatusField) { - await this.mirrorStatusField(raw.object_name, raw.record_id, config.approvalStatusField, finalStatus); + await this.mirrorStatusField( + raw.object_name, raw.record_id, config.approvalStatusField, finalStatus, + actingUserId(context), + ); } const fresh = await this.readBackRequest(requestId, context); return { request: fresh!, runId, nodeId, finalized: true, decision: input.decision, outputs: mergedOutputs }; @@ -1536,7 +1610,10 @@ export class ApprovalService implements IApprovalService { }, { context: SYSTEM_CTX }); await this.syncApproverIndex(requestId, [], org, now); if (config.approvalStatusField) { - await this.mirrorStatusField(raw.object_name, raw.record_id, config.approvalStatusField, 'recalled'); + await this.mirrorStatusField( + raw.object_name, raw.record_id, config.approvalStatusField, 'recalled', + actingUserId(context), + ); } let resumed = false; @@ -1636,7 +1713,10 @@ export class ApprovalService implements IApprovalService { }, { context: SYSTEM_CTX }); await this.syncApproverIndex(requestId, [], org, now); if (config.approvalStatusField) { - await this.mirrorStatusField(raw.object_name, raw.record_id, config.approvalStatusField, 'rejected'); + await this.mirrorStatusField( + raw.object_name, raw.record_id, config.approvalStatusField, 'rejected', + actingUserId(context), + ); } let resumed = false; if (runId && typeof this.automation?.resume === 'function') { @@ -1674,7 +1754,10 @@ export class ApprovalService implements IApprovalService { }, { context: SYSTEM_CTX }); await this.syncApproverIndex(requestId, [], org, now); if (config.approvalStatusField) { - await this.mirrorStatusField(raw.object_name, raw.record_id, config.approvalStatusField, 'returned'); + await this.mirrorStatusField( + raw.object_name, raw.record_id, config.approvalStatusField, 'returned', + actingUserId(context), + ); } let resumed = false; @@ -2087,7 +2170,14 @@ export class ApprovalService implements IApprovalService { decision: res.token.action, actorId: res.token.approver_id, comment: 'Via action link', - }, SYSTEM_CTX as unknown as SharingExecutionContext); + // The token IS the authentication (#3783): it is single-use, hashed at + // rest and bound to one approver, who `resolveActionToken` has just + // re-checked still holds a pending slot. So this decision has a real + // acting user even though no session carried it — name them on the + // context, so the status mirror and every flow it cascades into are + // attributed exactly like a decision made through the UI. Elevation is + // unchanged: `isSystem` still stands in for the missing session. + }, { ...SYSTEM_CTX, userId: res.token.approver_id } as unknown as SharingExecutionContext); return { ok: true, action: res.token.action, request: out.request, approverId: res.token.approver_id }; } @@ -2332,7 +2422,11 @@ export class ApprovalService implements IApprovalService { raw.node_config_json, { approvers: [], behavior: 'first_response' } as any, ); if (config.approvalStatusField) { - await this.mirrorStatusField(raw.object_name, raw.record_id, config.approvalStatusField, 'recalled'); + // No human did this — a sweep did. Left user-less on purpose (#3783): a + // flow that wants to react to a dead-run release declares runAs:'system'. + await this.mirrorStatusField( + raw.object_name, raw.record_id, config.approvalStatusField, 'recalled', null, + ); } this.logger?.warn?.('[approvals] released a record held by a dead approval run', { diff --git a/packages/plugins/plugin-approvals/src/status-mirror-cascade.integration.test.ts b/packages/plugins/plugin-approvals/src/status-mirror-cascade.integration.test.ts new file mode 100644 index 0000000000..5ce64eeb53 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/status-mirror-cascade.integration.test.ts @@ -0,0 +1,224 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3783 — an approval decision cascades as the deciding user. + * + * "When the invoice is approved, do X" is the single most natural approvals + * automation there is, and until now it could not be written the obvious way. + * The status mirror — the write that puts `approved` on the business record, and + * therefore the write that fires that object's record-change flows — presented a + * bare `{ isSystem: true }` context with no `userId`. `isSystem` does not + * suppress trigger dispatch, so the flow DID fire; it just fired with no trigger + * user, and since #3760 a `runAs:'user'` run with no trigger user has its data + * operations refused. Authors were pushed to declare `runAs:'system'` — blanket + * elevation — for a case where a perfectly good scoped identity existed all + * along, sitting right there at the call site. + * + * The seam is invisible in a unit test of any single hop, so this one refuses to + * stub any of them: a real {@link ObjectKernel} with the real ObjectQL engine, + * the real record-change trigger, the real automation engine, and the real + * {@link ApprovalService}. The mirror is produced by an actual decision, not + * hand-written. + * + * The negative case is load-bearing, not decoration. It fires the SAME flow off + * the dead-run sweep's mirror, which has no human behind it and stays user-less + * on purpose — and shows it is still refused. Without it, the positive case + * would prove only that the flow runs, never that it runs *because* the identity + * arrived. + * + * The record lock is deliberately not bound here: it is a separate concern with + * its own end-to-end coverage (`record-lock-schedule-run.integration.test.ts`). + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectKernel } from '@objectstack/core'; +import { ObjectQLPlugin } from '@objectstack/objectql'; +import { AutomationServicePlugin, type AutomationEngine } from '@objectstack/service-automation'; +import { RecordChangeTriggerPlugin } from '@objectstack/trigger-record-change'; +import { ApprovalService } from './approval-service.js'; +import { SysApprovalRequest } from './sys-approval-request.object.js'; +import { SysApprovalAction } from './sys-approval-action.object.js'; +import { SysApprovalApprover } from './sys-approval-approver.object.js'; + +const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); + +const SUBMITTER = { userId: 'submitter', positions: [], permissions: [] } as any; +const APPROVER = { userId: 'approver', positions: [], permissions: [] } as any; + +/** Equality-WHERE in-memory driver — the same shape the trigger's own e2e uses. */ +function makeMemoryDriver(): any { + const stores = new Map>>(); + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + let nextId = 0; + const matches = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + if (Array.isArray(where.$and)) return where.$and.every((w: any) => matches(row, w)); + if (Array.isArray(where.$or)) return where.$or.some((w: any) => matches(row, w)); + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const expected = v && typeof v === 'object' && '$eq' in (v as any) ? (v as any).$eq : v; + const a = row[k] === undefined ? null : row[k]; + const b = expected === undefined ? null : expected; + if (a !== b) return false; + } + return true; + }; + return { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, async syncSchema() {}, + async find(object: string, ast: any) { + return Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)); + }, + findStream() { throw new Error('not implemented'); }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) throw new Error(`not found: ${object}/${id}`); + const updated = { ...cur, ...data, id }; + s.set(id, updated); + return updated; + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(object).has(id)) return this.update(object, id, data); + return this.create(object, data); + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: any) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; +} + +const opportunity = { + name: 'opportunity', + label: 'Opportunity', + fields: { + amount: { name: 'amount', label: 'Amount', type: 'number' }, + approval_status: { name: 'approval_status', label: 'Approval Status', type: 'text' }, + cascaded: { name: 'cascaded', label: 'Cascaded', type: 'text' }, + }, +}; + +/** + * The automation an author actually wants to write: react to the approval, no + * `runAs` — the spec default `'user'`. That default is the whole point; a flow + * forced to say `runAs:'system'` to work at all is the bug being fixed. + */ +const onApprovedFlow = { + name: 'on_approved', + label: 'On Approved', + type: 'record_change', + nodes: [ + { + id: 'start', type: 'start', label: 'Start', + config: { + objectName: 'opportunity', + triggerType: 'record-after-update', + // Fires on BOTH terminal mirrors, so the two cases below differ only in + // whether the mirror that fired it carried a user — nothing else. + condition: "approval_status == 'approved' || approval_status == 'recalled'", + }, + }, + { + id: 'stamp', type: 'update_record', label: 'Stamp', + config: { objectName: 'opportunity', filter: { id: '{record.id}' }, fields: { cascaded: 'yes' } }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'stamp' }, + { id: 'e2', source: 'stamp', target: 'end' }, + ], +}; + +const nodeConfig = { + approvers: [{ type: 'user' as const, value: 'approver' }], + behavior: 'first_response' as const, + lockRecord: false, + approvalStatusField: 'approval_status', +}; + +describe('an approval decision cascades as the deciding user (#3783)', () => { + let data: any; + let svc: ApprovalService; + + beforeEach(async () => { + const kernel = new ObjectKernel({ logLevel: 'silent' }); + await kernel.use(new ObjectQLPlugin()); + await kernel.use(new AutomationServicePlugin()); + await kernel.use(new RecordChangeTriggerPlugin()); + await kernel.bootstrap(); + + const objectql = kernel.getService('objectql') as any; + data = kernel.getService('data') as any; + const automation = kernel.getService('automation'); + + objectql.registerDriver(makeMemoryDriver(), true); + for (const def of [opportunity, SysApprovalRequest, SysApprovalAction, SysApprovalApprover]) { + objectql.registry.registerObject(def as any, 'approvals-test', 'approvals-test'); + } + automation.registerFlow('on_approved', onApprovedFlow as any); + + svc = new ApprovalService({ engine: objectql }); + await data.insert('opportunity', { id: 'opp1', amount: 100 }, { context: { isSystem: true } }); + }); + + const readBack = () => data.findOne('opportunity', { where: { id: 'opp1' } }); + + const openRequest = () => svc.openNodeRequest({ + object: 'opportunity', recordId: 'opp1', runId: 'run_1', nodeId: 'approve_step', + flowName: 'deal_approval', config: nodeConfig, submitterId: 'submitter', + record: { id: 'opp1', amount: 100 }, + }, SUBMITTER); + + it('the approve mirror hands the flow a trigger user, so its data node runs', async () => { + const req = await openRequest(); + await svc.decide(req.id as string, { decision: 'approve', actorId: 'approver' }, APPROVER); + await sleep(300); + + const row = await readBack(); + expect(row?.approval_status, 'the mirror itself must still land').toBe('approved'); + // Before #3783 this stayed undefined: the run inherited no trigger user, so + // `resolveRunDataContext` refused its `update_record` outright. + expect(row?.cascaded).toBe('yes'); + // ObjectQL's audit stamp is gated on the write context's `userId` alone — + // `isSystem` buys no exemption — so this is direct evidence the elevated + // mirror named a user rather than nobody. + expect(row?.updated_by).toBe('approver'); + }); + + it('a dead-run release cascades user-less, and is still refused', async () => { + await openRequest(); + svc.attachAutomation({ getRun: async () => ({ status: 'failed' }) } as any); + await svc.releaseDeadRunRequests(); + await sleep(300); + + const row = await readBack(); + expect(row?.approval_status, 'the sweep must still release the record').toBe('recalled'); + // No human abandoned this request — a sweep did. The cascade therefore has + // no identity to inherit and stays refused; an author who wants to react to + // a dead-run release declares `runAs:'system'` and means it. + expect(row?.cascaded).toBeFalsy(); + }); +}); diff --git a/packages/services/service-automation/src/runtime-identity.ts b/packages/services/service-automation/src/runtime-identity.ts index 7b1aef7204..8d94ecb470 100644 --- a/packages/services/service-automation/src/runtime-identity.ts +++ b/packages/services/service-automation/src/runtime-identity.ts @@ -112,10 +112,17 @@ export type RunDataContext = RunIdentityContext | RunProvenanceContext; * provenance-only envelope and let the run proceed UNSCOPED — the #1888 * fail-open. A schedule is only the most obvious source of a user-less run; * the commonest is a record-change flow fired by a write that carried no - * user (any `isSystem` plugin/service write, the approvals status mirror, or - * a `runAs:'system'` flow's own data node — `isSystem` does NOT suppress - * trigger dispatch, only `skipTriggers` does). None of those are decidable at - * authoring time, which is why the refusal has to live here. + * user (any `isSystem` plugin/service write, or a `runAs:'system'` flow's own + * data node — `isSystem` does NOT suppress trigger dispatch, only + * `skipTriggers` does). None of those are decidable at authoring time, which + * is why the refusal has to live here. + * + * Elevation and anonymity are separate choices, and a service that elevates + * for a reason usually still knows who it is acting for. The approvals status + * mirror was the motivating example on both sides: it has to stay `isSystem` + * (the record is locked while its approval is live) but it now names the + * deciding user, so approvals cascades resolve here instead of being refused + * (#3783). Only its machine-driven sweeps stay user-less. * * The engine sets {@link AutomationContext.runAs} on the run context at setup; * this function is the single place that maps it to an ObjectQL context, shared diff --git a/packages/triggers/trigger-record-change/src/record-change-integration.test.ts b/packages/triggers/trigger-record-change/src/record-change-integration.test.ts index 8c6e753aa1..c9dd7abe64 100644 --- a/packages/triggers/trigger-record-change/src/record-change-integration.test.ts +++ b/packages/triggers/trigger-record-change/src/record-change-integration.test.ts @@ -185,9 +185,9 @@ const objectDef = (name: string) => ({ * * `isSystem` does NOT suppress trigger dispatch (only `skipTriggers` does), and * the trigger forwards `session.userId` with no fallback. So a write made with a - * system context — any plugin/service write, the approvals status mirror, a - * `runAs:'system'` flow's own data node — fires the record-change flows bound to - * that object with `userId: undefined`. A flow left at the spec default + * system context — any plugin/service write, a `runAs:'system'` flow's own data + * node — fires the record-change flows bound to that object with + * `userId: undefined`. A flow left at the spec default * `runAs:'user'` then presented NO principal to ObjectQL, and the data security * middleware skips when there is no principal: the flow read and wrote every row. * @@ -216,7 +216,10 @@ describe('a system write must not fire a record-change flow UNSCOPED (#3760)', ( automation.registerFlow('sysw_stamp', stampFlow('sysw_stamp', 'sysw') as any); // A SYSTEM write: elevated, no userId, and NOT skipTriggers — so it still - // dispatches. This is the approvals-status-mirror shape. + // dispatches. Elevation is not what makes it user-less; presenting no + // principal is. A service that elevates *and* knows its actor can name one + // (the approvals status mirror does since #3783) and lands in the scoped + // branch instead — this is the shape that genuinely has nobody behind it. const created = await data.insert( 'sysw', { status: 'new' }, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4554337251..cdfc1d0467 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1261,6 +1261,9 @@ importers: '@objectstack/service-automation': specifier: workspace:* version: link:../../services/service-automation + '@objectstack/trigger-record-change': + specifier: workspace:* + version: link:../../triggers/trigger-record-change '@types/node': specifier: ^26.1.1 version: 26.1.1