diff --git a/.changeset/runas-system-stamping.md b/.changeset/runas-system-stamping.md new file mode 100644 index 0000000000..bc23f9057c --- /dev/null +++ b/.changeset/runas-system-stamping.md @@ -0,0 +1,33 @@ +--- +"@objectstack/service-automation": patch +--- + +fix(service-automation): runAs:'system' 的 create_record 按 ADR-0118 染全三列——组织、属主、创建者禁 NULL (#5494) + +修的是缺陷,不是新语义——契约是 ADR-0118(#4608)既有的:显式 `isSystem`、fail-closed、 +禁 NULL 歧义;`runAs` 声明的是授权姿态而非身份(ADR-0073 D2),提权不等于匿名。 + +根因:`resolveRunDataContext` 的 system 分支把触发上下文的 `userId` / `tenantId` 整个丢弃, +而三列的平台盖章恰好全部键在被丢弃的信息上——`created_by` 键在写上下文的 `userId` +(ObjectQL 审计钩子)、`owner_id` 键在安全中间件的 acting user(而整条中间件含盖章步骤在 +`isSystem` 上短路)、`organization_id` 键在上下文 `tenantId`(驱动层租户机制)。于是用户 +触发的 system 清扫流程建出的每一行三列全 NULL:落在组织分区之外(唯一索引跨 NULL 不生效、 +org 作用域查询看不见),也落在所有 owner/creator 作用域授权之外——issue 里"admin 都 +403"的由来。 + +修复(writer 侧,`packages/services/service-automation`): + +- system 分支把触发身份原样带过去(`userId` + `tenantId`),与 action-body 缝的 + `{ ...caller, isSystem: true }` 信封(hotcrm#548 同族修复)同形:`isSystem` 独自决定 + 授权(中间件在读到 `userId` 之前就短路),身份只驱动归因盖章(`created_by`/`updated_by`、 + 审计 actor)、驱动层的 `organization_id` 填充,以及下游 record-change 级联的触发身份; +- `create_record` 对 system 运行补 `owner_id` 填充(fill-only、schema 存在才染):所有权锚 + 的平台盖章在 `isSystem` 上被短路,payload 是唯一通道;染的是 acting user——与同一触发在 + `runAs:'user'` 下会得到的默认一致,不是把系统身份塞进 owner(ADR-0118 D6 / ADR-0073 D3); +- 流程 `fields` 显式给值一律优先;真正无用户的运行(schedule)保持三列不染——没有 acting + user 时按 ADR-0118 D1,哨兵串与伪用户都是被禁的替代品,`svc:flow:*` actor 标签 + + `flowRunId` 继续承担溯源。 + +行为变化:`runAs:'system'` 且触发上下文带 org 的运行,其数据操作在驱动层按 +`(org = 触发 org OR org IS NULL)` 作用域——与 action-body 缝一致的姿态;schedule 触发的 +运行不带 org,行为不变。 diff --git a/packages/services/service-automation/package.json b/packages/services/service-automation/package.json index e1e6964ff0..55aa17f403 100644 --- a/packages/services/service-automation/package.json +++ b/packages/services/service-automation/package.json @@ -23,7 +23,9 @@ "@objectstack/spec": "workspace:*" }, "devDependencies": { + "@objectstack/driver-sql": "workspace:*", "@objectstack/objectql": "workspace:*", + "@objectstack/plugin-security": "workspace:*", "@types/node": "^26.1.2", "typescript": "^6.0.3", "vitest": "^4.1.10" diff --git a/packages/services/service-automation/src/builtin/crud-nodes.ts b/packages/services/service-automation/src/builtin/crud-nodes.ts index e416dcafe5..7921fcc4ba 100644 --- a/packages/services/service-automation/src/builtin/crud-nodes.ts +++ b/packages/services/service-automation/src/builtin/crud-nodes.ts @@ -20,7 +20,7 @@ import type { AutomationEngine } from '../engine.js'; import { interpolate, interpolateFilter, type VariableMap } from './template.js'; import { refuseNode } from '../guard-refusal.js'; import { parseNodeConfig } from './parse-config.js'; -import { resolveRunDataContext } from '../runtime-identity.js'; +import { resolveRunDataContext, stampSystemInsertOwner } from '../runtime-identity.js'; /** * A filter condition that an author WROTE but that interpolation erased @@ -300,7 +300,17 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext): } // #1888 — honor flow.runAs (system → RLS-bypassing; user → trigger user). + // #5494 — a BORN row must not escape the platform stamps. The run + // context now carries the trigger's user + org even under system + // elevation (so the audit hook stamps `created_by` and the driver's + // tenant machinery fills `organization_id`, exactly like a user-path + // insert); the ownership anchor has no such engine-side channel for + // system writes — the security middleware that stamps it + // short-circuits on `isSystem` — so the writer fills it here. + // Fill-only — flow-authored `fields` win. Policy + rationale live + // beside `resolveRunDataContext` in runtime-identity.ts. const dataCtx = resolveRunDataContext(context); + stampSystemInsertOwner(fields, dataCtx, data, objectName); try { // #3407 — symmetric with update_record. Today the engine's // insert path strips nothing (INSERT is readonly-exempt and diff --git a/packages/services/service-automation/src/builtin/crud-runas.test.ts b/packages/services/service-automation/src/builtin/crud-runas.test.ts index 7617f40cbc..e88ec563ba 100644 --- a/packages/services/service-automation/src/builtin/crud-runas.test.ts +++ b/packages/services/service-automation/src/builtin/crud-runas.test.ts @@ -81,17 +81,24 @@ describe('flow.runAs identity enforcement at the data layer (#1888)', () => { engine.registerFlow('sys', allOpsFlow('sys', 'system')); // Triggered by a normal user — `runAs:'system'` must still elevate. - const res = await engine.execute('sys', { userId: 'u1' }); + const res = await engine.execute('sys', { userId: 'u1', tenantId: 'org1' }); expect(res.success).toBe(true); expect(calls.map((c) => c.op).sort()).toEqual(['delete', 'find', 'findOne', 'insert', 'update']); for (const c of calls) { expect(c.ctx, `${c.op} got no context`).toBeTruthy(); expect(c.ctx.isSystem, `${c.op} not elevated`).toBe(true); - // An elevated run is NOT attributed to the triggering user… - expect(c.ctx.userId).toBeUndefined(); - // …but it IS attributed to the flow, so audit rows never read - // "Unknown user" (ADR-0014 D2, #4366). + // #5494 — elevation is not anonymity: the triggering user and org are + // CARRIED THROUGH (attribution: created_by/updated_by stamps, audit + // actor, downstream record-change identity; the org drives the driver's + // organization_id fill on born rows), while `isSystem` alone decides + // authorization. Dropping them here is what inserted rows with all + // three platform columns NULL — untouchable even by the triggering + // member, and outside the org partition. + expect(c.ctx.userId, `${c.op} lost the acting user (#5494)`).toBe('u1'); + expect(c.ctx.tenantId, `${c.op} lost the trigger org (#5494)`).toBe('org1'); + // …and it stays attributed to the flow as well, so audit rows name + // WHICH automation wrote them (ADR-0014 D2, #4366; ADR-0118 D5). expect(c.ctx.actor, `${c.op} lost the service-principal label`).toBe('svc:flow:sys'); } }); @@ -174,19 +181,22 @@ describe('flow.runAs identity enforcement at the data layer (#1888)', () => { edges: [{ id: 'e1', source: 'start', target: 'mk' }, { id: 'e2', source: 'mk', target: 'end' }], } as any); - // Trigger as a restricted user. If the engine ignored runAs, the insert would - // carry that user's identity (or none) instead of the elevated principal. + // Trigger as a restricted user. If the engine ignored runAs, the insert + // would run under that user's AUTHORIZATION (isSystem false) instead of the + // elevated principal. Since #5494 the user still rides the context — as + // attribution, which grants nothing under the isSystem short-circuit — so + // the regression tell is the `isSystem` flag, never the userId's absence. await engine.execute('reg', { userId: 'restricted' }); const insert = calls.find((c) => c.op === 'insert'); expect(insert?.ctx?.isSystem, 'runAs:system did not elevate the data op (#1888 regressed)').toBe(true); - expect(insert?.ctx?.userId).not.toBe('restricted'); + expect(insert?.ctx?.userId, 'the acting user must ride the elevated context (#5494)').toBe('restricted'); }); }); describe('resolveRunDataContext (#1888 unit)', () => { - it("maps runAs:'system' to an elevated context attributed to the flow (#4366)", () => { + it("maps runAs:'system' to an elevated context attributed to the flow AND the acting user (#4366, #5494)", () => { expect(resolveRunDataContext({ runAs: 'system', userId: 'u1', flowName: 'mirror_status' })).toEqual({ - isSystem: true, actor: 'svc:flow:mirror_status', positions: [], permissions: [], + isSystem: true, actor: 'svc:flow:mirror_status', userId: 'u1', positions: [], permissions: [], }); }); diff --git a/packages/services/service-automation/src/index.ts b/packages/services/service-automation/src/index.ts index 7bebb0d3f6..70a08f24c9 100644 --- a/packages/services/service-automation/src/index.ts +++ b/packages/services/service-automation/src/index.ts @@ -74,7 +74,11 @@ export type { AutomationServicePluginOptions } from './plugin.js'; // outright (#3760). Exported for hosts building custom data nodes: call // `resolveRunDataContext` and let the error propagate, so a custom node inherits // the same posture as the built-ins instead of re-opening the fail-open. -export { resolveRunDataContext, UnscopedRunDataAccessError } from './runtime-identity.js'; +export { + resolveRunDataContext, + stampSystemInsertOwner, + UnscopedRunDataAccessError, +} from './runtime-identity.js'; export type { RunDataContext, RunIdentityContext, RunProvenanceContext } from './runtime-identity.js'; // Built-in node executors (ADR-0018). These are seeded by AutomationServicePlugin diff --git a/packages/services/service-automation/src/record-lookup-expand.integration.test.ts b/packages/services/service-automation/src/record-lookup-expand.integration.test.ts index 968923c327..670390e72a 100644 --- a/packages/services/service-automation/src/record-lookup-expand.integration.test.ts +++ b/packages/services/service-automation/src/record-lookup-expand.integration.test.ts @@ -109,7 +109,10 @@ describe('record-change lookup expansion (#3475)', () => { const read = crud.find((c) => c.op === 'findOne' && c.obj === 'lead'); expect(read!.ctx?.isSystem).toBe(true); - expect(read!.ctx?.userId).toBeUndefined(); + // #5494 — the acting user rides the elevated context as attribution; what + // makes this read ELEVATED is `isSystem` (the middleware short-circuits + // before any gate reads `userId`), not the absence of a user. + expect(read!.ctx?.userId).toBe('u1'); await kernel.shutdown(); }); diff --git a/packages/services/service-automation/src/runas-grant-resolution.integration.test.ts b/packages/services/service-automation/src/runas-grant-resolution.integration.test.ts index ba50477788..e26a3490f2 100644 --- a/packages/services/service-automation/src/runas-grant-resolution.integration.test.ts +++ b/packages/services/service-automation/src/runas-grant-resolution.integration.test.ts @@ -119,7 +119,13 @@ describe("AutomationServicePlugin bridges the runAs:'user' grant resolver (#3356 await automation.execute('sys', { userId: 'u1', params: { noteId: 'n1' } }); const update = crud.find((c) => c.op === 'update' && c.obj === 'runas_thing'); expect(update!.ctx.isSystem).toBe(true); - expect(update!.ctx.userId).toBeUndefined(); + // #5494 — the acting user rides the elevated context as ATTRIBUTION (it + // drives the created_by/updated_by stamps and the audit actor; the + // isSystem short-circuit precedes every gate that reads it). What proves + // "the resolver is not consulted" is the untouched authz envelope: + expect(update!.ctx.userId).toBe('u1'); + expect(update!.ctx.positions).toEqual([]); + expect(update!.ctx.permissions).toEqual([]); await kernel.shutdown(); }); diff --git a/packages/services/service-automation/src/runas-system-stamping.integration.test.ts b/packages/services/service-automation/src/runas-system-stamping.integration.test.ts new file mode 100644 index 0000000000..0a374051e2 --- /dev/null +++ b/packages/services/service-automation/src/runas-system-stamping.integration.test.ts @@ -0,0 +1,313 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5494 — a `runAs:'system'` flow's `create_record` must not insert rows with + * `owner_id` / `organization_id` / `created_by` all NULL. + * + * The defect: `resolveRunDataContext`'s system branch DISCARDED the trigger's + * identity (`userId` / `tenantId`), and every platform stamp for the three + * columns keys on exactly what was discarded — `created_by` on the write + * context's `userId` (ObjectQL audit hook), `owner_id` on the acting user in + * the security middleware (whose whole chain, stamp included, short-circuits + * on `isSystem`), `organization_id` on the context `tenantId` (driver-level + * tenant machinery; the org middleware also skips system writes). So every + * record a user-triggered system sweep created was born with all three NULL: + * outside the org partition (unique indexes don't bite across NULL, org + * queries don't see it) and outside every owner/creator-scoped grant — the + * issue's "born untouchable even by admin", flipped to 200 only after a demo + * crutch stamped the row. + * + * These tests run the REAL stack — ObjectKernel, ObjectQLPlugin (audit-stamp + * hooks), driver-sql on better-sqlite3 `:memory:` (tenant-column injection), + * the real AutomationEngine + CRUD nodes, and (for the flip) the real + * SecurityPlugin middleware — because the defect lived precisely in the + * hand-off between those layers, invisible to any single-layer unit test. + * + * Directions decided before running (reverse-verification discipline): + * - user-triggered system run → all three columns land (was: all NULL); + * - user-LESS system run (schedule shape) → columns stay NULL — there is no + * acting user, and ADR-0118 D1 forbids a sentinel/pseudo-user in its + * place; the actor label + flowRunId remain the provenance channels. This + * is also the structural "before" twin that proves the columns come from + * the carried identity, not from some new global default; + * - the #5494 step-3→5 flip: the SAME caller is denied on the NULL-born row + * and admitted on the stamped row — row content, not caller, decides. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { ObjectKernel } from '@objectstack/core'; +import { ObjectQLPlugin, type ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { SecurityPlugin, securityDefaultPermissionSets } from '@objectstack/plugin-security'; +import { PermissionSetSchema } from '@objectstack/spec/security'; +import { AutomationServicePlugin } from './plugin.js'; +import type { AutomationEngine } from './engine.js'; + +/** Real backend: better-sqlite3 `:memory:` through driver-sql (PR #5715 shape). */ +function makeSqliteDriver() { + return new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); +} + +/** + * A plain business object. `organization_id`, `owner_id` and the audit family + * (`created_by` / `updated_by`) are NOT declared — the registry injects them + * (`applySystemFields`), exactly like a production app object, so the test + * proves the stamps land on the injected platform columns. + */ +const crmTask = { + name: 'crm_task', + label: 'Task', + fields: { + title: { name: 'title', label: 'Title', type: 'text' }, + status: { name: 'status', label: 'Status', type: 'text' }, + }, +}; + +/** start → create_record(crm_task) → end, under runAs:'system'. */ +const sweepFlow = (name: string, fields: Record) => ({ + name, + label: name, + type: 'autolaunched', + runAs: 'system', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'mk', type: 'create_record', label: 'Create', config: { objectName: 'crm_task', fields } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'mk' }, + { id: 'e2', source: 'mk', target: 'end' }, + ], +}); + +/** The manual-trigger envelope the issue reproduced with: a real user + org. */ +const ADMIN_TRIGGER = { + userId: 'usr_admin', + tenantId: 'org_1', + positions: [] as string[], + permissions: [] as string[], +}; + +describe("runAs:'system' create_record stamps organization_id / owner_id / created_by (#5494)", () => { + let kernel: ObjectKernel; + let ql: ObjectQL; + let automation: AutomationEngine; + + afterEach(async () => { + try { await kernel?.shutdown(); } catch { /* noop */ } + }); + + async function boot(extraPlugins: any[] = []) { + kernel = new ObjectKernel({ logger: { level: 'fatal' } }); + await kernel.use(new ObjectQLPlugin()); + await kernel.use(new AutomationServicePlugin({ suspendedRunStore: 'memory' })); + for (const p of extraPlugins) await kernel.use(p); + await kernel.bootstrap(); + + ql = kernel.getService('objectql'); + automation = kernel.getService('automation'); + + const driver = makeSqliteDriver(); + await driver.connect(); + ql.registerDriver(driver, true); + ql.registry.registerObject(crmTask as any, 'stamping-test', 'stamping-test'); + await ql.syncSchemas(); + } + + const SYS = { isSystem: true } as const; + const taskByTitle = (title: string) => + ql.findOne('crm_task', { where: { title }, context: SYS }); + + it('a USER-TRIGGERED system run lands all three columns from the trigger identity', async () => { + await boot(); + automation.registerFlow('renewal_sweep', sweepFlow('renewal_sweep', { title: 'renew A', status: 'open' }) as any); + + const res = await automation.execute('renewal_sweep', { ...ADMIN_TRIGGER }); + expect(res.success, `run failed: ${JSON.stringify(res)}`).toBe(true); + + const row = await taskByTitle('renew A'); + expect(row, 'the sweep must have created the row').toBeTruthy(); + // The issue's step 2, inverted: the three platform columns are all + // non-NULL, and each equals what the trigger context knew. + expect(row.created_by, 'created_by must be the triggering user').toBe('usr_admin'); + expect(row.owner_id, 'owner_id must be the acting user (the runAs:user default, restored)').toBe('usr_admin'); + expect(row.organization_id, "organization_id must be the trigger context's org").toBe('org_1'); + }); + + it("flow-authored ownership wins: an explicit `fields.owner_id` is never overwritten", async () => { + await boot(); + automation.registerFlow( + 'assigned_sweep', + sweepFlow('assigned_sweep', { title: 'renew B', owner_id: 'usr_assignee' }) as any, + ); + + const res = await automation.execute('assigned_sweep', { ...ADMIN_TRIGGER }); + expect(res.success).toBe(true); + + const row = await taskByTitle('renew B'); + // ADR-0073 D3 — flow logic sets ownership explicitly; the stamp is the + // fill-only default underneath it, exactly like the security middleware's + // own "empty means stamp" rule on the user path. + expect(row.owner_id).toBe('usr_assignee'); + // Attribution is untouched by the ownership choice. + expect(row.created_by).toBe('usr_admin'); + expect(row.organization_id).toBe('org_1'); + }); + + it('a USER-LESS system run (schedule shape) stamps nothing — and that is the contract, not a gap', async () => { + await boot(); + automation.registerFlow('night_sweep', sweepFlow('night_sweep', { title: 'renew C' }) as any); + + // What ScheduleTrigger actually supplies: an event and params, NO user, NO + // org (schedule-trigger.ts builds `{ event: 'schedule', params }`). + const res = await automation.execute('night_sweep', { event: 'schedule', params: {} } as any); + expect(res.success).toBe(true); + + const row = await taskByTitle('renew C'); + expect(row).toBeTruthy(); + // No acting user exists. ADR-0118 D1: the system actor's representation in + // user-lookup columns IS null — a sentinel string or pseudo-user is the + // banned alternative. ADR-0073's automation principal (a real identity for + // these runs) is M2, gated on its first consumer. Provenance still names + // the writer: the run's `svc:flow:*` actor label and flowRunId (#4366/#3712). + expect(row.created_by ?? null).toBeNull(); + expect(row.owner_id ?? null).toBeNull(); + // The schedule trigger supplies no org today, so there is nothing to + // stamp; a schedule-run in an org-partitioned deployment needs the flow's + // `fields` to place rows (or a future org-aware schedule binding). + expect(row.organization_id ?? null).toBeNull(); + }); + + it("REGRESSION: the runAs:'user' path is unchanged — audit + org stamps still land", async () => { + await boot(); + const userFlow = { ...sweepFlow('user_flow', { title: 'renew D' }), runAs: 'user' }; + automation.registerFlow('user_flow', userFlow as any); + + const res = await automation.execute('user_flow', { ...ADMIN_TRIGGER }); + expect(res.success, `run failed: ${JSON.stringify(res)}`).toBe(true); + + const row = await taskByTitle('renew D'); + expect(row.created_by).toBe('usr_admin'); + expect(row.organization_id).toBe('org_1'); + // (`owner_id` on the user path is the security middleware's stamp; the + // full-security composition below covers it. This harness pins that the + // audit + tenant machinery behave identically before and after the fix.) + }); +}); + +/** + * The issue's step 3→5 flip, replayed against the REAL SecurityPlugin: the + * same non-elevated caller PATCHes and DELETEs the flow-created row — denied + * (403-shape: PermissionDeniedError) while the row's stamp columns are NULL, + * admitted once they carry the acting user. Nothing about the caller changes + * between the two, exactly the issue's tell. + * + * The framework's own default grants make the mechanism concrete: member + * writes are scoped by the `owner_only_writes` / `owner_only_deletes` RLS + * policies (`created_by == current_user.id`, bound to `org_member`), enforced + * on by-id writes via the middleware's pre-image re-read — so a row born with + * `created_by = NULL` matches nobody's write scope, and no caller-side change + * can ever reach it (the HotCRM report's admin hit the owner-keyed variant of + * the same class). + */ +describe('the #5494 admission flip: row content, not caller, decides (real SecurityPlugin)', () => { + let kernel: ObjectKernel; + let ql: ObjectQL; + let automation: AutomationEngine; + + afterEach(async () => { + try { await kernel?.shutdown(); } catch { /* noop */ } + }); + + /** Members get no baseline delete (ADR-0090 D5) — grant it explicitly. */ + const taskDeleteSet = PermissionSetSchema.parse({ + name: 'task_delete', + label: 'Flip proof — task delete', + objects: { crm_task: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } }, + }); + + /** The rank-and-file member who triggered the sweep. */ + const MEMBER_CTX = { + userId: 'usr_member', + tenantId: 'org_1', + positions: ['org_member'], + permissions: ['member_default', 'task_delete'], + }; + + async function bootWithSecurity() { + kernel = new ObjectKernel({ logger: { level: 'fatal' } }); + await kernel.use(new ObjectQLPlugin()); + await kernel.use(new AutomationServicePlugin({ suspendedRunStore: 'memory' })); + await kernel.use( + new SecurityPlugin({ + defaultPermissionSets: [...securityDefaultPermissionSets, taskDeleteSet], + }), + ); + await kernel.bootstrap(); + + ql = kernel.getService('objectql'); + automation = kernel.getService('automation'); + + const driver = makeSqliteDriver(); + await driver.connect(); + ql.registerDriver(driver, true); + ql.registry.registerObject(crmTask as any, 'stamping-test', 'stamping-test'); + await ql.syncSchemas(); + } + + const SYS = { isSystem: true } as const; + const rowByTitle = (title: string) => + ql.findOne('crm_task', { where: { title }, context: SYS }); + + it('the member can PATCH and DELETE the row their own trigger created (was: denied)', async () => { + await bootWithSecurity(); + automation.registerFlow('renewal_sweep', sweepFlow('renewal_sweep', { title: 'flip A', status: 'open' }) as any); + + // The member triggers the system sweep — the issue's reproduction shape. + const res = await automation.execute('renewal_sweep', { ...MEMBER_CTX }); + expect(res.success, `run failed: ${JSON.stringify(res)}`).toBe(true); + + const row = await rowByTitle('flip A'); + expect(row.created_by).toBe('usr_member'); + expect(row.owner_id).toBe('usr_member'); + expect(row.organization_id).toBe('org_1'); + + // Step 3, inverted: the same member — no elevation, no transfer grant — + // repairs and completes the record the sweep made for them. + await expect( + ql.update('crm_task', { id: row.id, status: 'done' }, { context: { ...MEMBER_CTX } }), + ).resolves.toBeDefined(); + expect((await rowByTitle('flip A')).status).toBe('done'); + + await expect( + ql.delete('crm_task', { where: { id: row.id }, context: { ...MEMBER_CTX } }), + ).resolves.toBeDefined(); + expect(await rowByTitle('flip A')).toBeFalsy(); + }); + + it('the NULL-born row (user-less run) is still untouchable by the same caller — the 403 half of the flip', async () => { + await bootWithSecurity(); + automation.registerFlow('night_sweep', sweepFlow('night_sweep', { title: 'flip B', status: 'open' }) as any); + + // A user-less firing creates the row with NULL stamps (nothing to carry). + const res = await automation.execute('night_sweep', { event: 'schedule', params: {} } as any); + expect(res.success).toBe(true); + const row = await rowByTitle('flip B'); + expect(row.created_by ?? null).toBeNull(); + + // The SAME member context that succeeded above is denied here: the only + // difference between the two attempts is the row's stamp columns — the + // issue's step-4/5 tell, reproduced in one build. (`owner_only_writes` + // matches no one on a NULL `created_by`; the pre-image check fails closed.) + await expect( + ql.update('crm_task', { id: row.id, status: 'done' }, { context: { ...MEMBER_CTX } }), + ).rejects.toThrow(/denied|permission/i); + await expect( + ql.delete('crm_task', { where: { id: row.id }, context: { ...MEMBER_CTX } }), + ).rejects.toThrow(/denied|permission/i); + }); +}); diff --git a/packages/services/service-automation/src/runtime-identity.ts b/packages/services/service-automation/src/runtime-identity.ts index 309f5b6012..27a0301001 100644 --- a/packages/services/service-automation/src/runtime-identity.ts +++ b/packages/services/service-automation/src/runtime-identity.ts @@ -16,13 +16,27 @@ export interface RunIdentityContext { isSystem: boolean; /** * Service-principal label for audit attribution (`ExecutionContext.actor`, - * ADR-0014 D2): `svc:flow:` on a `runAs:'system'` run, which - * resolves no user. Without it the audit writer records `user_id=null, - * actor=null` and the history UI renders "Unknown user" (#4366). - * Attribution only — no security middleware keys on it. + * ADR-0014 D2): `svc:flow:` on a `runAs:'system'` run. It names + * WHICH automation performed the write (ADR-0118 D5's "related field", not a + * second actor spelling); the acting human, when the trigger resolved one, + * rides {@link userId} beside it. Without at least one of the two the audit + * writer records `user_id=null, actor=null` and the history UI renders + * "Unknown user" (#4366). Attribution only — no security middleware keys on + * it. */ actor?: string; - /** Acting user id — drives owner/role RLS for `runAs:'user'` runs. */ + /** + * Acting user id. On a `runAs:'user'` run it drives owner/role RLS. On a + * `runAs:'system'` run it is carried through UNCHANGED from the trigger when + * one resolved a user (#5494): elevation and anonymity are separate choices + * (ADR-0073's authorization-vs-attribution axes; the #3783 approvals-mirror + * `{ isSystem: true, userId }` shape), and the security middleware's + * `isSystem` short-circuit precedes every gate that reads `userId`, so under + * elevation it grants nothing — it only keeps the run's writes attributable + * (`created_by`/`updated_by` audit stamps, `sys_audit_log` actor) and lets + * record-change flows fired by those writes resolve the same triggering + * human instead of being refused user-less (#3760). + */ userId?: string; /** Acting user's role names (RLS parity with a direct REST request). */ positions: string[]; @@ -113,10 +127,25 @@ export type RunDataContext = RunIdentityContext | RunProvenanceContext; * Translate a flow run's {@link AutomationContext} into the ObjectQL `context` * its CRUD nodes must pass, honoring `runAs` (ADR-0049 / #1888): * - * - `runAs:'system'` → `{ isSystem: true, actor: 'svc:flow:' }` — - * the security middleware short-circuits, so the run reads/writes with full - * access, bypassing RLS; the `actor` label keeps those writes attributable - * in the audit log (ADR-0014 D2, #4366). + * - `runAs:'system'` → `{ isSystem: true, actor: 'svc:flow:' }`, + * PLUS the trigger's `userId` / `tenantId` when the trigger resolved them + * (#5494) — the security middleware short-circuits on `isSystem`, so the + * run reads/writes with full access, bypassing RLS; the `actor` label and + * the carried-through acting user keep those writes attributable in the + * audit log and in the rows' own `created_by`/`updated_by` stamps + * (ADR-0014 D2, #4366), and the carried org lets the driver's tenant + * machinery fill `organization_id` on born rows like any user-path insert. + * `runAs` declares the run's AUTHORIZATION posture, not its identity + * (ADR-0073 D2) — discarding the trigger identity here is what used to + * insert rows with all three platform columns NULL: outside the org + * partition, and untouchable even by the triggering member (the default + * `owner_only_writes` RLS keys on `created_by`). Same envelope shape as + * the action-body seam's `{ ...caller, isSystem: true }` (the + * hotcrm#548-family fix in @objectstack/runtime) — one platform posture + * for "elevated, user-triggered" writers. A run whose trigger genuinely + * resolved no user (a schedule) stays user-less: the actor label + + * `flowRunId` are its provenance, and ADR-0118 D1 forbids inventing a + * sentinel or pseudo-user in its place. * - `runAs:'user'` (default) → the triggering user's identity * (`{ userId, positions, permissions, tenantId? }`), so the security middleware * enforces that user's row-level security. The run can never exceed the @@ -149,12 +178,43 @@ export type RunDataContext = RunIdentityContext | RunProvenanceContext; export function resolveRunDataContext(context: AutomationContext | undefined): RunDataContext | undefined { const flowRunId = context?.flowRunId; if (context?.runAs === 'system') { - // A system run resolves no user, so name the flow as the acting service - // principal (`svc:flow:`, ADR-0014 D2) — the audit writer falls back - // to `session.actor` when `userId` is absent, which is what keeps these + // Name the flow as the acting service principal (`svc:flow:`, + // ADR-0014 D2) — the audit writer falls back to `session.actor` when + // `userId` is absent, which is what keeps a genuinely user-less run's // writes attributable instead of "Unknown user" (#4366). const actor = `svc:flow:${context.flowName ?? 'automation'}`; - return { isSystem: true, actor, positions: [], permissions: [], ...(flowRunId ? { flowRunId } : {}) }; + return { + isSystem: true, + actor, + // #5494 — elevation is not anonymity. When the trigger resolved a user + // (a manual run, a record-change fired by a user's write), carry them + // through: `isSystem` alone decides authorization (the middleware + // short-circuits before any gate reads `userId`), while the user drives + // the platform's attribution stamps — `created_by`/`updated_by` via the + // engine's audit hook — and downstream record-change dispatch, exactly + // like the #3783 approvals mirror's `{ isSystem: true, userId }` writes. + // A schedule-shaped trigger resolves no user and stays user-less; per + // ADR-0118 D1 its actor representation IS null (never a sentinel). + ...(context.userId ? { userId: context.userId } : {}), + // #5494 — and elevation is not org-lessness either. The trigger's org + // rides along, which is what makes the driver's tenant machinery treat + // this run's INSERTs exactly like a user-path insert: `organization_id` + // filled on rows that omit it (per-table opt-outs respected), autonumber + // sequences org-scoped. Without it, every row a user-triggered system + // sweep created was born org-NULL — outside the org partition, where + // unique indexes don't bite and org-scoped queries don't look. Reads / + // updates / deletes org-scope to `(org = trigger-org OR org IS NULL)` at + // the driver — the same posture the action-body seam ships for its + // `{ ...caller, isSystem: true }` envelope (the hotcrm#548-family fix): + // the tenant wall survives elevation for an org-triggered run, while + // org-NULL (pre-fix / global) rows stay reachable via the OR-NULL arm. + // A run that must genuinely cross orgs is a user-less one (a schedule + // carries no org, nothing narrows) or sets explicit fields. + ...(context.tenantId ? { tenantId: context.tenantId } : {}), + positions: [], + permissions: [], + ...(flowRunId ? { flowRunId } : {}), + }; } if (!context?.userId) { // #3760 — FAIL CLOSED. There is no identity to present, and presenting none @@ -182,6 +242,65 @@ export function resolveRunDataContext(context: AutomationContext | undefined): R return out; } +/** + * Fill `owner_id` on a `runAs:'system'` create's payload from the run's acting + * user, when the run has one and the flow did not set an owner itself (#5494). + * + * The ownership anchor is normally stamped by the security middleware ("an + * empty `owner_id` is auto-stamped to the acting user", #3004 step 3.5), but + * that middleware — including the stamp — short-circuits on `isSystem`, so for + * a system-elevated write the payload is the only channel. Left NULL, the row + * is born outside every owner-scoped grant: nobody's `own`-depth write scope + * matches it, and repairing it needs a transfer grant the affected users may + * not hold — #5494's "born untouchable even by admin". Stamping the ACTING + * USER restores exactly the default the same trigger would have produced under + * `runAs:'user'`; it does not put a system identity in the column (ADR-0118 D6 + * keeps `owner_id` a business-ownership axis — the system never owns records, + * and ADR-0073 D3 forbids force-owning rows to an automation principal). + * + * Fill-only and schema-guarded: + * - an author-set `owner_id` in the node's `fields` wins (flow logic sets + * ownership explicitly — ADR-0073 D3), matching the middleware's own + * "empty means stamp" test (`null`/`''` count as empty); + * - objects that do not carry the column (`ownership: 'org' | 'none'`, + * platform-managed tables) are left alone — stamping would make the driver + * INSERT a column the table does not have. Same duck-typed `getSchema` + * posture as the engine's audit hook and plugin-security: no schema + * surface → no stamp (today's behavior, not an error). + * - a user-less system run (a schedule) stamps nothing: there is no acting + * user, and ADR-0118 D1 forbids a sentinel or pseudo-user in its place. + */ +export function stampSystemInsertOwner( + fields: Record, + dataCtx: RunDataContext | undefined, + data: unknown, + objectName: string, +): void { + if (!dataCtx || (dataCtx as RunIdentityContext).isSystem !== true) return; + const userId = (dataCtx as RunIdentityContext).userId; + if (!userId) return; + // Own-property test + the middleware's own "empty" definition: a present, + // non-empty owner is the author's explicit choice and is never overwritten. + if ( + Object.prototype.hasOwnProperty.call(fields, 'owner_id') && + fields.owner_id != null && + fields.owner_id !== '' + ) { + return; + } + const getSchema = (data as { getSchema?: (o: string) => unknown } | null | undefined)?.getSchema; + if (typeof getSchema !== 'function') return; + try { + const schema = getSchema.call(data, objectName) as { fields?: Record } | null | undefined; + const schemaFields = schema?.fields; + if (!schemaFields || typeof schemaFields !== 'object') return; + if (!Object.prototype.hasOwnProperty.call(schemaFields, 'owner_id')) return; + } catch { + return; + } + fields.owner_id = userId; +} + /** * Node types that perform an ObjectQL data operation — the ones that thread * {@link resolveRunDataContext} into the data engine as `options.context`. A diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 99808482aa..17895a4298 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2039,9 +2039,15 @@ importers: specifier: workspace:* version: link:../../spec devDependencies: + '@objectstack/driver-sql': + specifier: workspace:* + version: link:../../drivers/driver-sql '@objectstack/objectql': specifier: workspace:* version: link:../../objectql + '@objectstack/plugin-security': + specifier: workspace:* + version: link:../../plugins/plugin-security '@types/node': specifier: ^26.1.2 version: 26.1.2