diff --git a/.changeset/23618-approval-title-family-served-fields.md b/.changeset/23618-approval-title-family-served-fields.md new file mode 100644 index 00000000000..4f6bd47f15d --- /dev/null +++ b/.changeset/23618-approval-title-family-served-fields.md @@ -0,0 +1,12 @@ +--- +'@objectstack/plugin-approvals': patch +--- + +fix(plugin-approvals): the inbox's referenced-record titles (`payload_display`) and the record-lock refusal's title are served only from fields the caller may read + +Clause-②: no + +- **Referenced-record titles follow the reader's field-level security on the referenced object.** `listRequests` and `getRequest` now pick each `payload_display` title from the referenced record as served to the reading caller: the same answer the payload snapshot and `record_title` are narrowed by (readable, and not masked for that caller), asked for the referenced object, once per object per read. A display field the reader may not read on the referenced object, or is served masked, gives that key no display value, so the snapshot's stored id stands, as it already does for a deleted referenced record. A title-eligible field the reader may read still names the record, and a reader who may read the display field gets the same title as before. +- **The `RECORD_LOCKED` refusal names the record only from what the writer may read.** The per-record refusal sentence takes its title from the record as served to the refused writer, asked with the writer's own execution context. When the writer may not read the display field, or is served it masked, the sentence names the object's label alone (`This Opportunity is locked …`), as it already does for a record with no title; it never names the id. A writer who may read the field is told the title as before. The code (`RECORD_LOCKED`) and status (409) are unchanged. + +A patch: a narrowing of served values only. Nothing is granted, and no schema, route, error code, translation key or export moves. With no field-visibility source wired, every title is served as before. diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index daeac5d639c..0a8b5fc6121 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -1535,8 +1535,8 @@ export class ApprovalService implements IApprovalService { /** * [#10749 / #23594] The subject fields the READING caller may be served, per - * subject object of `rows`: {@link resolveReadableSnapshotFields}' answer - * (readable, and not masked for this caller), asked as the caller. + * subject object of `rows` — {@link ApprovalService.servedFields} for the + * rows' subject objects. * * It is the ONE answer both serve-time narrowings of a row read: the payload * snapshot ({@link ApprovalService.redactPayloads}) and the subject's title @@ -1546,13 +1546,31 @@ export class ApprovalService implements IApprovalService { * pass-through, and it passes the title through exactly as it passes the * snapshot. */ - private async servedSubjectFields( + private servedSubjectFields( rows: readonly ApprovalRequestRow[], context: ExecutionContext, ): Promise> { - const served = new Map(); - for (const r of rows) { - const object = String((r as any).object_name ?? ''); + return this.servedFields(rows.map((r) => String((r as any).object_name ?? '')), context); + } + + /** + * [#10749 / #23594 / #23618] The fields the READING caller may be served on + * each of `objects`: {@link resolveReadableSnapshotFields}' answer (readable, + * and not masked for this caller), asked as the caller — the one place a row + * read asks it. + * + * `served` holds the answers this read has already asked; an object in it is + * not asked again. So one read asks once per object, whether that object is a + * request's subject or the target of a lookup in a snapshot + * ({@link ApprovalService.enrichRows}), and a target that is also a subject + * object is narrowed by the same answer as the subject. + */ + private async servedFields( + objects: Iterable, + context: ExecutionContext, + served: Map = new Map(), + ): Promise> { + for (const object of objects) { if (served.has(object)) continue; served.set(object, await resolveReadableSnapshotFields( this.fieldVisibility, object, context, this.logger, @@ -6672,6 +6690,8 @@ export class ApprovalService implements IApprovalService { * (ADR-0049) for `get`, the read `GET /data/{target}/{id}` performs and the * operation the data door's `$expand` and the dataset door's labels ask of * the same target. It takes no caller: the read runs under a system context. + * [#23618] What is SERVED from that read is the reader's: the title is picked + * from the target as served to the reader ({@link titlesAsServed}). * * A refused target is not read, so its key gets no display value and the * snapshot's stored id stands, the answer a deleted target already gets. A @@ -6745,23 +6765,48 @@ export class ApprovalService implements IApprovalService { for (const [object, idSet] of byObject) { const label = this.objectLabelOf(object); if (label) objectLabels.set(object, label); - const ids = Array.from(idSet); - const displayField = this.resolveDisplayField(object); - const readable = served.get(object); try { - const recs = await this.engine.find(object, { - where: { id: { $in: ids } }, limit: ids.length, context: SYSTEM_CTX, - }); - for (const rec of (recs ?? []) as any[]) { - const asServed = readable === undefined ? rec : redactSnapshot(rec, readable).payload; - const title = ApprovalService.pickTitle(asServed, displayField); - if (rec?.id && title) titles.set(`${object} ${rec.id}`, title); - } + await this.titlesAsServed(object, idSet, served.get(object), titles); } catch { /* object may be unregistered — the caller's fallback applies */ } } return { titles, objectLabels }; } + /** + * [#23594 / #23618] Title the `ids` of `object` into `into` (keyed + * `" "`), each picked from the record AS SERVED to the reader: + * `readable` is the reader's served fields for `object` + * ({@link ApprovalService.servedFields}), and `redactSnapshot` keeps exactly + * the fields the payload snapshot keeps, so a display field the reader may + * not read, or is served masked, gives no title, while a field the reader may + * be served still names the record. `undefined` is the seam's pass-through. + * + * The ONE spelling of how an approval read titles a record, shared by the + * subject's `record_title` ({@link ApprovalService.resolveSubjectDisplays}) + * and the lookup targets' `payload_display` ({@link ApprovalService.enrichRows}). + * The records are read under the system context, so the read narrows nothing + * (#7497 ruling C: record access stays with object permissions, sharing and + * row-level rules); only what is served from it is narrowed. Throws what the + * read throws; each caller keeps its own best-effort catch. + */ + private async titlesAsServed( + object: string, + ids: Iterable, + readable: string[] | undefined, + into: Map, + ): Promise { + const list = Array.from(ids); + const displayField = this.resolveDisplayField(object); + const recs = await this.engine.find(object, { + where: { id: { $in: list } }, limit: list.length, context: SYSTEM_CTX, + }); + for (const rec of (recs ?? []) as any[]) { + const asServed = readable === undefined ? rec : redactSnapshot(rec, readable).payload; + const title = ApprovalService.pickTitle(asServed, displayField); + if (rec?.id && title) into.set(`${object} ${rec.id}`, title); + } + } + /** * An object's schema label, or `undefined` — the one spelling of "the * object's label" the inbox row ({@link ApprovalService.resolveSubjectDisplays}) @@ -6787,7 +6832,8 @@ export class ApprovalService implements IApprovalService { * reader to narrow for. */ private nameSubjectRecord(object: string, recordId: string): string { - // [#23594] The title would go here; withheld until resolveSubjectDisplays narrows it by each reader's FLS. + // [#23594 / #23556 ruling B] The inbox serves the title narrowed per reader (resolveSubjectDisplays); this body is + // written once at send time, with no per-recipient reader to narrow for, so it stays label-and-id. return `${this.objectLabelOf(object) ?? object} ${recordId}`; } @@ -6804,10 +6850,20 @@ export class ApprovalService implements IApprovalService { * does not serve `get` is not read ({@link servesPayloadDisplayTarget}). * [#23594] `record_title` is narrowed by the reader's `served` fields, the * answer the payload was redacted by ({@link resolveSubjectDisplays}). + * [#23618] So is every `payload_display` title, by the reader's served fields + * on the TARGET object: the lookup keys come from the already-redacted + * snapshot, and the record a readable key points at is titled from the target + * as served to `context`'s caller ({@link titlesAsServed}). A target display + * field the reader may not be served gives that key no display value, so the + * snapshot's stored id stands — the answer a deleted or exposure-refused + * target gets. The targets' answers are asked through + * {@link servedFields} into `served`, once per object per read, so a target + * that is also a subject object reuses the subject's answer. */ private async enrichRows( rows: ApprovalRequestRow[], - served: ReadonlyMap, + served: Map, + context: ExecutionContext, ): Promise { if (!rows.length) return; @@ -6845,18 +6901,11 @@ export class ApprovalService implements IApprovalService { } } const refTitles = new Map(); - for (const [object, idSet] of refIds) { - if (!this.servesPayloadDisplayTarget(object)) continue; - const ids = Array.from(idSet); - const displayField = this.resolveDisplayField(object); + const targets = Array.from(refIds.keys()).filter((object) => this.servesPayloadDisplayTarget(object)); + await this.servedFields(targets, context, served); + for (const object of targets) { try { - const recs = await this.engine.find(object, { - where: { id: { $in: ids } }, limit: ids.length, context: SYSTEM_CTX, - }); - for (const rec of (recs ?? []) as any[]) { - const title = ApprovalService.pickTitle(rec, displayField); - if (rec?.id && title) refTitles.set(`${object} ${rec.id}`, title); - } + await this.titlesAsServed(object, refIds.get(object)!, served.get(object), refTitles); } catch { /* referenced object unreadable — leave unresolved */ } } @@ -7527,10 +7576,11 @@ export class ApprovalService implements IApprovalService { const rows = await this.engine.find('sys_approval_request', findOpts); const list = Array.isArray(rows) ? rows.map(rowFromRequest) : []; // [#10749] Redact before enrichment — see `redactPayloads`. [#23594] The - // snapshot and the subject title are narrowed by ONE answer for the reader. + // snapshot and the subject title are narrowed by ONE answer for the reader; + // [#23618] the lookup targets' titles by the same reader's answers for them. const served = await this.servedSubjectFields(list, context); this.redactPayloads(list, served); - await this.enrichRows(list, served); + await this.enrichRows(list, served, context); this.attachViewers(list, context, caller); return list; } @@ -7640,10 +7690,11 @@ export class ApprovalService implements IApprovalService { } const row = rowFromRequest(raw); // [#10749] Redact before enrichment — see `redactPayloads`. [#23594] The - // snapshot and the subject title are narrowed by ONE answer for the reader. + // snapshot and the subject title are narrowed by ONE answer for the reader; + // [#23618] the lookup targets' titles by the same reader's answers for them. const served = await this.servedSubjectFields([row], context); this.redactPayloads([row], served); - await this.enrichRows([row], served); + await this.enrichRows([row], served, context); await this.attachFlowSteps(row); await this.attachDecisionProgress(row, raw); this.attachViewers([row], context, caller); diff --git a/packages/plugins/plugin-approvals/src/approvals-plugin.ts b/packages/plugins/plugin-approvals/src/approvals-plugin.ts index 6915ea259f0..18760fa904a 100644 --- a/packages/plugins/plugin-approvals/src/approvals-plugin.ts +++ b/packages/plugins/plugin-approvals/src/approvals-plugin.ts @@ -305,7 +305,9 @@ export class ApprovalsServicePlugin implements Plugin { if (!this.options.disableAutoHooks) { try { unbindAllHooks(engine); - bindApprovalLockHook(engine, ctx.logger); + // [#23618] The refusal's title is narrowed by the writer's served fields, + // through the same lazily-resolved authority the snapshot redaction reads. + bindApprovalLockHook(engine, ctx.logger, fieldVisibility); bindRecordDeleteCancelHook(engine, this.service, ctx.logger); bindDelegationWriteGuard(engine, ctx.logger); // [#10749] Generic-door snapshot redaction. Registered here so the diff --git a/packages/plugins/plugin-approvals/src/implicit-reference-target-lookup.test.ts b/packages/plugins/plugin-approvals/src/implicit-reference-target-lookup.test.ts index ed3be977f63..70b1573cf7b 100644 --- a/packages/plugins/plugin-approvals/src/implicit-reference-target-lookup.test.ts +++ b/packages/plugins/plugin-approvals/src/implicit-reference-target-lookup.test.ts @@ -73,12 +73,13 @@ function makeService( const resolve = (object: string) => (service as unknown as { resolveLookupFields(o: string): Array<{ key: string; reference: string }> }) .resolveLookupFields(object); - // [#23594] No field-visibility answer for any object: the reader's served - // fields narrow nothing here, as with no security service wired. + // [#23594 / #23618] No security service wired, so no field-visibility answer + // for any object, subject or target: the reader's served fields narrow + // nothing here. const enrich = (rows: InboxRow[]) => (service as unknown as { - enrichRows(r: InboxRow[], served: ReadonlyMap): Promise; - }).enrichRows(rows, new Map()); + enrichRows(r: InboxRow[], served: Map, context: unknown): Promise; + }).enrichRows(rows, new Map(), { userId: 'u_reader', positions: [], permissions: [] }); return { resolve, enrich, warn, finds }; } diff --git a/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts b/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts index 334a20ac809..25e7a598c35 100644 --- a/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts +++ b/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts @@ -110,11 +110,28 @@ * that. When no title is in hand the sentence degrades to the object's label, * and when that is missing too, to "This record" — ⛔ never back to the id. * + * ## The title is the WRITER's to be served (#23618) + * + * `ctx.previous` is the row as STORED (a raw driver read, declared columns + * only), narrowed by no caller's field-level security — and the sentence is + * served to the writer. So the title is picked from the pre-image AS SERVED to + * the writer, by the rule every approval-path title answers to + * (`payload-redaction.ts`, #10749: keyed on the caller): the served-fields + * answer, `resolveReadableSnapshotFields` (readable, and not masked for this + * caller), asked with the writer's own envelope ({@link writerEnvelope}). A + * display field the writer may not be served gives no title, and the sentence + * degrades as above. Still no record read: the cost is one field-visibility + * question, on the deny path only, so a write the lock does not refuse asks + * nothing. With no field-visibility authority wired, the title passes through + * as before, exactly as the payload snapshot does. + * * Registered under `packageId: 'plugin-approvals:lock'` so it can be cleanly * unbound on plugin stop. */ +import { withoutOperationPrivateKeys } from '@objectstack/core'; import { resolveDisplayField } from '@objectstack/spec/data'; +import { redactSnapshot, resolveReadableSnapshotFields, type FieldVisibilitySource } from './payload-redaction.js'; export const APPROVALS_HOOK_PACKAGE = 'plugin-approvals:lock'; @@ -238,6 +255,23 @@ function recordTitleOf( return title; } +/** + * [#23618] The WRITER's own execution envelope — the one riding on the + * operation (`input.options.context`), minus its operation-private keys — or + * `undefined` when the write carried none. + * + * ⛔ Never rebuilt from `ctx.session`: the session is a projection of the + * envelope that drops `permissions` (and spells the org `organizationId`), so a + * field-visibility question asked with it would be a second derivation of the + * writer's authority beside the envelope the security middleware judged the + * write by. The `__` keys go for the reason `withoutOperationPrivateKeys` + * states, and the copy keeps a callee from writing back into the operation. + */ +function writerEnvelope(ctx: any): unknown { + const exec = ctx?.input?.options?.context; + return exec && typeof exec === 'object' ? withoutOperationPrivateKeys(exec as Record) : undefined; +} + /** * The END-USER sentence for a record held by a live approval (#18153). * @@ -439,8 +473,17 @@ async function gatingRequests( /** * Bind the global record-lock hook. Caller is responsible for calling * {@link unbindAllHooks} first if re-binding. + * + * `fieldVisibility` resolves the field-visibility authority the refusal's title + * is narrowed by (#23618) — lazily, per refusal, because the security plugin may + * start after this one. Absent, or resolving nothing, the title is served as + * before, exactly as the payload snapshot is (`resolveReadableSnapshotFields`). */ -export function bindApprovalLockHook(engine: MinimalEngine, logger?: MinimalLogger): void { +export function bindApprovalLockHook( + engine: MinimalEngine, + logger?: MinimalLogger, + fieldVisibility?: () => FieldVisibilitySource | undefined, +): void { engine.registerHook('beforeUpdate', async (ctx: any) => { const object = (ctx?.object ?? ctx?.objectName) as string | undefined; // No object name (shouldn't happen) or our own bookkeeping objects → skip. @@ -518,12 +561,25 @@ export function bindApprovalLockHook(engine: MinimalEngine, logger?: MinimalLogg const previous = ctx?.previous as Record | undefined; const previousId = previous ? String(previous.id ?? previous._id ?? '') : ''; const record = previousId && previousId === recordId ? previous : undefined; + // [#23618] …and it is the row as STORED, narrowed by no caller's + // field-level security, while this sentence is served to the WRITER. So + // the title is picked from it AS SERVED to the writer: the served-fields + // answer the approval inbox narrows its titles and snapshots by (readable, + // and not masked for this caller), asked with the writer's own envelope. + // A display field the writer may not be served gives no title, and the + // sentence degrades to the label — ⛔ never to the id. + const served = record + ? await resolveReadableSnapshotFields(fieldVisibility?.(), object, writerEnvelope(ctx), logger) + : undefined; + const asServed = record && served !== undefined + ? redactSnapshot(record, served).payload as Record + : record; logger?.info?.( `[approvals] update refused RECORD_LOCKED: record '${recordId}' of '${object}' ` + `is held by pending approval request '${String(pending?.id ?? '')}'`, ); - lockedError(recordLockRefusal(objectLabel, recordTitleOf(schema, record, recordId))); + lockedError(recordLockRefusal(objectLabel, recordTitleOf(schema, asServed, recordId))); } }, { packageId: APPROVALS_HOOK_PACKAGE, priority: 50 }); diff --git a/packages/plugins/plugin-approvals/src/lookup-target-title-reader-fls.integration.test.ts b/packages/plugins/plugin-approvals/src/lookup-target-title-reader-fls.integration.test.ts new file mode 100644 index 00000000000..41bb909ee4e --- /dev/null +++ b/packages/plugins/plugin-approvals/src/lookup-target-title-reader-fls.integration.test.ts @@ -0,0 +1,281 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#23618] A `payload_display` title — the title of the record a snapshot's + * lookup points at — is served to a reader only from fields that reader may be + * served on the TARGET object. + * + * ## The contract + * + * `payload-redaction.ts` (#10749): "Redaction is applied at SERVE time, keyed + * on the reading caller, so the same row answers an admin with the whole + * snapshot and a restricted approver with only the fields they may read." The + * lookup keys already come from the reader's redacted snapshot, so a withheld + * lookup field lends nothing. The title of the record a readable lookup points + * at is a field of a SECOND object, read beside that snapshot under the system + * context, so it answers to the same rule asked of that object: the served-fields + * answer (`resolveReadableSnapshotFields`, readable and not masked for this + * reader) that #23594 applies to the subject's own title, asked as the reader, + * once per object per read. The title is picked from the target AS SERVED to the + * reader, so a withheld display field gives the reader nothing for that key and + * the snapshot's stored id stands — the answer a deleted or exposure-refused + * target already gets (#22738). + * + * ## What this pins + * + * Driven through the real `ApprovalService` on a real `ObjectQL` over + * `SqlDriver` (better-sqlite3, in-memory), with a field-visibility source in + * place of `plugin-security`'s (the three answers it gives are its contract): + * + * (a) a reader who may not read a target's display field gets no title for + * that key, on the single read and on the list, and the stored id stands; + * a target that is the subject's own object (a self-lookup) is narrowed by + * the same reader's answer for that object; + * (a-masked) a target display field served MASKED to the reader is withheld + * the same way (#20964); + * (b) the control: a reader who may read every target field gets every title; + * (c) a withheld target display field never lends its value: the title is + * picked from what the reader is served on the target; + * (d) one rule, asked once: one field-visibility ask per object per read, + * subject and targets together, on the single read and on a multi-row + * list — a target that is also a subject object is not asked twice. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { ApprovalService } from './approval-service.js'; +import { ApprovalsServicePlugin } from './approvals-plugin.js'; +import type { FieldVisibilitySource } from './payload-redaction.js'; + +const SYSTEM = { isSystem: true, positions: [], permissions: [] } as any; +const asUser = (userId: string) => ({ userId, positions: [], permissions: [] }) as any; + +/** May not read the account's or the deal's display field, nor the vendor's `code`. */ +const RESTRICTED = 'u_restricted'; +/** Served the account's display field MASKED: readable, not queryable (#20964). */ +const MASKED = 'u_masked'; +/** May read every field (the control). */ +const READER = 'u_reader'; +const SUBMITTER = 'u_sub'; + +/** Target display values a restricted reader must never be served. */ +const ACCOUNT_TITLE = 'Globex private account'; +const PARENT_TITLE = 'Parent deal secret'; +const VENDOR_CODE = 'VENDOR-SECRET-9'; +/** The vendor's other title-eligible field, readable to every reader. */ +const VENDOR_NAME = 'Initech Supplies'; + +const account = { + name: 'crm_account', + label: 'Account', + nameField: 'name', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + region: { name: 'region', label: 'Region', type: 'text' as const }, + }, +}; +/** Its display field is `code`; `name` is another title-eligible field. */ +const vendor = { + name: 'crm_vendor', + label: 'Vendor', + nameField: 'code', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + code: { name: 'code', label: 'Code', type: 'text' as const }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + }, +}; +const deal = { + name: 'crm_deal', + label: 'Deal', + nameField: 'name', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + amount: { name: 'amount', label: 'Amount', type: 'number' as const }, + account: { name: 'account', label: 'Account', type: 'lookup' as const, reference: 'crm_account' }, + vendor: { name: 'vendor', label: 'Vendor', type: 'lookup' as const, reference: 'crm_vendor' }, + parent: { name: 'parent', label: 'Parent deal', type: 'lookup' as const, reference: 'crm_deal' }, + }, +}; +const sysUser = { + name: 'sys_user', + label: 'User', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + email: { name: 'email', label: 'Email', type: 'text' as const }, + }, +}; + +const ALL: Record = { + crm_deal: ['id', 'name', 'amount', 'account', 'vendor', 'parent'], + crm_account: ['id', 'name', 'region'], + crm_vendor: ['id', 'code', 'name'], +}; +/** reader → object → [readable, queryable]. */ +const ANSWERS: Record> = { + [RESTRICTED]: { + crm_deal: [['id', 'amount', 'account', 'vendor', 'parent'], ['id', 'amount', 'account', 'vendor', 'parent']], + crm_account: [['id', 'region'], ['id', 'region']], + crm_vendor: [['id', 'name'], ['id', 'name']], + }, + [MASKED]: { + crm_deal: [ALL.crm_deal, ALL.crm_deal], + crm_account: [ALL.crm_account, ['id', 'region']], + crm_vendor: [ALL.crm_vendor, ALL.crm_vendor], + }, + [READER]: { + crm_deal: [ALL.crm_deal, ALL.crm_deal], + crm_account: [ALL.crm_account, ALL.crm_account], + crm_vendor: [ALL.crm_vendor, ALL.crm_vendor], + }, +}; + +/** Stands in for `plugin-security`: the system context reads every field, as its `isSystem` bypass does. */ +function makeVisibility(): FieldVisibilitySource & { calls: Array<{ object: string; userId?: string }> } { + const calls: Array<{ object: string; userId?: string }> = []; + const answer = (object: string, context: any, which: 0 | 1) => { + if (context?.isSystem) return ALL[object]; + return ANSWERS[String(context?.userId ?? '')]?.[object]?.[which] ?? []; + }; + return { + calls, + async getReadableFields(object: string, context?: any) { + calls.push({ object, userId: context?.userId }); + return answer(object, context, 0); + }, + async getQueryableFields(object: string, context?: any) { + return answer(object, context, 1); + }, + }; +} + +/** The plugin's own object definitions, as its manifest registers them. */ +async function packageObjects(): Promise { + let manifest: any; + const ctx: any = { + getService: (name: string) => { + if (name === 'manifest') return { register: (m: any) => { manifest = m; } }; + throw new Error(`no service '${name}'`); + }, + logger: { info: () => {}, warn: () => {}, error: () => {}, debug: () => {} }, + }; + await new ApprovalsServicePlugin().init(ctx); + return manifest.objects; +} + +describe('[#23618] a lookup target\'s title is served from what its reader may read on the target', () => { + let engine: ObjectQL; + let svc: ApprovalService; + let vis: ReturnType; + let n = 0; + + /** One pending request on a fresh deal whose snapshot points at all three targets. */ + const openDeal = async () => { + n += 1; + const recordId = `D${n}`; + const record = { id: recordId, name: `Deal ${n}`, amount: 100 * n, account: 'A1', vendor: 'V1', parent: 'D_PARENT' }; + await engine.insert('crm_deal', record, { context: SYSTEM } as any); + const row: any = await svc.openNodeRequest({ + object: 'crm_deal', recordId, runId: `run_${n}`, nodeId: 'review', flowName: 'deal_review', + config: { + approvers: [RESTRICTED, MASKED, READER].map((value) => ({ type: 'user', value })), + behavior: 'first_response', + } as any, + submitterId: SUBMITTER, record, + }, asUser(SUBMITTER)); + return { requestId: String(row.id), recordId }; + }; + + beforeAll(async () => { + engine = new ObjectQL(); + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, + }), true); + await engine.init(); + for (const def of [sysUser, account, vendor, deal, ...(await packageObjects())]) { + engine.registry.registerObject(def as any, 'approvals-test', 'approvals-test'); + } + await engine.syncSchemas(); + for (const id of [RESTRICTED, MASKED, READER, SUBMITTER]) { + await engine.insert('sys_user', { id, name: id, email: `${id}@example.com` }, { context: SYSTEM } as any); + } + await engine.insert('crm_account', { id: 'A1', name: ACCOUNT_TITLE, region: 'EMEA' }, { context: SYSTEM } as any); + await engine.insert('crm_vendor', { id: 'V1', code: VENDOR_CODE, name: VENDOR_NAME }, { context: SYSTEM } as any); + await engine.insert('crm_deal', { id: 'D_PARENT', name: PARENT_TITLE, amount: 1 }, { context: SYSTEM } as any); + svc = new ApprovalService({ + engine: engine as any, + logger: { warn: () => {}, info: () => {}, error: () => {}, debug: () => {} } as any, + }); + vis = makeVisibility(); + svc.attachFieldVisibility(vis); + }); + + afterAll(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + }); + + it('(a) a reader who may not read a target\'s display field gets no title for it — single read and list', async () => { + const { requestId, recordId } = await openDeal(); + + const one: any = await svc.getRequest(requestId, asUser(RESTRICTED)); + const [listed]: any[] = await svc.listRequests({ recordId }, asUser(RESTRICTED)); + for (const [door, row] of [['getRequest', one], ['listRequests', listed]] as const) { + expect(row, door).toBeTruthy(); + expect(row.payload_display ?? {}, door).not.toHaveProperty('account'); + // A self-lookup is narrowed by the same reader's answer for the subject object. + expect(row.payload_display ?? {}, door).not.toHaveProperty('parent'); + // The stored ids stand, as for a deleted or exposure-refused target. + expect(row.payload, door).toMatchObject({ account: 'A1', parent: 'D_PARENT' }); + expect(JSON.stringify(row), door).not.toContain(ACCOUNT_TITLE); + expect(JSON.stringify(row), door).not.toContain(PARENT_TITLE); + } + }); + + it('(a-masked) a target display field served MASKED to the reader is withheld the same way (#20964)', async () => { + const { requestId } = await openDeal(); + const row: any = await svc.getRequest(requestId, asUser(MASKED)); + expect(row.payload_display ?? {}).not.toHaveProperty('account'); + expect(JSON.stringify(row)).not.toContain(ACCOUNT_TITLE); + // The targets this reader is served unmasked still name their records. + expect(row.payload_display).toMatchObject({ vendor: VENDOR_CODE, parent: PARENT_TITLE }); + }); + + it('(b) control: a reader who may read every target field gets every title — single read and list', async () => { + const { requestId, recordId } = await openDeal(); + const one: any = await svc.getRequest(requestId, asUser(READER)); + const [listed]: any[] = await svc.listRequests({ recordId }, asUser(READER)); + for (const [door, row] of [['getRequest', one], ['listRequests', listed]] as const) { + expect(row.payload_display, door).toEqual({ account: ACCOUNT_TITLE, vendor: VENDOR_CODE, parent: PARENT_TITLE }); + } + }); + + it('(c) a withheld target display field never lends its value: the title is picked from what the reader is served', async () => { + const { requestId } = await openDeal(); + const row: any = await svc.getRequest(requestId, asUser(RESTRICTED)); + expect(row.payload_display?.vendor).toBe(VENDOR_NAME); + expect(JSON.stringify(row)).not.toContain(VENDOR_CODE); + }); + + it('(d) one rule, asked once: one answer per object per read, subject and targets together', async () => { + const first = await openDeal(); + await openDeal(); + const expected = [ + { object: 'crm_deal', userId: RESTRICTED }, + { object: 'crm_account', userId: RESTRICTED }, + { object: 'crm_vendor', userId: RESTRICTED }, + ]; + + vis.calls.length = 0; + await svc.getRequest(first.requestId, asUser(RESTRICTED)); + expect(vis.calls).toEqual(expected); + + vis.calls.length = 0; + const rows = await svc.listRequests({ object: 'crm_deal', approverId: RESTRICTED }, asUser(RESTRICTED)); + expect(rows.length).toBeGreaterThan(1); + expect(vis.calls).toEqual(expected); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/payload-display-target-exposure.test.ts b/packages/plugins/plugin-approvals/src/payload-display-target-exposure.test.ts index 94a2d3efca7..4f39e09defa 100644 --- a/packages/plugins/plugin-approvals/src/payload-display-target-exposure.test.ts +++ b/packages/plugins/plugin-approvals/src/payload-display-target-exposure.test.ts @@ -60,11 +60,11 @@ function makeService(unreadable?: string) { const { engine, reads } = makeEngine(tables, unreadable); const service = new ApprovalService({ engine: engine as any, logger: { info() {}, warn, error() {}, debug() {} } }); // `enrichRows` is private; its public callers drag the whole request lifecycle in. - // [#23594] No field-visibility answer for any object: the reader's served fields - // narrow nothing here, as with no security service wired. + // [#23594 / #23618] No security service wired, so no field-visibility answer for + // any object, subject or target: the reader's served fields narrow nothing here. const enrich = (rows: Row[]) => (service as unknown as { - enrichRows(r: Row[], served: ReadonlyMap): Promise; - }).enrichRows(rows, new Map()); + enrichRows(r: Row[], served: Map, context: unknown): Promise; + }).enrichRows(rows, new Map(), { userId: 'u_reader', positions: [], permissions: [] }); return { enrich, reads, warn }; } diff --git a/packages/plugins/plugin-approvals/src/record-lock-refusal-writer-fls.integration.test.ts b/packages/plugins/plugin-approvals/src/record-lock-refusal-writer-fls.integration.test.ts new file mode 100644 index 00000000000..f158a652a8b --- /dev/null +++ b/packages/plugins/plugin-approvals/src/record-lock-refusal-writer-fls.integration.test.ts @@ -0,0 +1,190 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#23618] The record lock's per-row refusal names the record by a title the + * WRITER may be served, or by the object's label alone. + * + * ## The contract + * + * The refusal (`RECORD_LOCKED`, #18153) names the record from the engine's + * update pre-image (`ctx.previous`). That pre-image is the row as STORED — a raw + * driver read (`readUpdatePriorRow` in `@objectstack/objectql`), shaped to the + * declared columns and narrowed by no caller's field-level security. The + * sentence is served to the writer, so it answers to the rule every other + * approval-path title answers to (`payload-redaction.ts`, #10749: keyed on the + * caller): the served-fields answer (`resolveReadableSnapshotFields`, readable + * and not masked for this caller), asked as the writer, with the writer's own + * execution envelope — the one riding on the operation — never one rebuilt from + * the hook session. A display field the writer may not be served gives no + * title: the sentence degrades to the object's label, ⛔ never to the id. + * + * ## What this pins + * + * Driven end to end — real `ObjectQL`, real sqlite driver, the real + * `beforeUpdate` hook — with a field-visibility source in place of + * `plugin-security`'s: + * + * (a) a writer who may not read the display field is refused without the + * title, on a by-id write and on a predicate write (the per-row dispatch); + * (a-masked) a display field served MASKED to the writer is withheld the same + * way (#20964); + * (b) the control: a writer who may read the field is told the title; + * (c) one ask per refusal, made with the writer's own envelope, and none for + * a write the lock does not refuse. + * + * `RECORD_LOCKED` / 409 is asserted on every refusal: the wire vocabulary does + * not move (ADR-0112). + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { bindApprovalLockHook } from './lifecycle-hooks.js'; +import type { FieldVisibilitySource } from './payload-redaction.js'; + +const SYSTEM = { isSystem: true, positions: [], permissions: [] } as any; +/** The writer's envelope carries `permissions`, which the hook session does not. */ +const asUser = (userId: string) => ({ userId, positions: [], permissions: [`grant_${userId}`] }) as any; + +/** May not read the display field. */ +const RESTRICTED = 'u_restricted'; +/** Served the display field MASKED: readable, not queryable (#20964). */ +const MASKED = 'u_masked'; +/** May read every field (the control). */ +const WRITER = 'u_writer'; + +const TITLE = 'Acme secret renewal'; +const LOCKED_SENTENCE = 'is locked while an approval is in progress, and cannot be edited until that approval is complete'; + +const OPPORTUNITY = { + name: 'opportunity', + label: 'Opportunity', + nameField: 'name', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const }, + amount: { name: 'amount', type: 'number' as const }, + approval_status: { name: 'approval_status', type: 'text' as const }, + }, +}; + +const APPROVAL_REQUEST = { + name: 'sys_approval_request', + label: 'Approval Request', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + object_name: { name: 'object_name', type: 'text' as const }, + record_id: { name: 'record_id', type: 'text' as const }, + status: { name: 'status', type: 'text' as const }, + flow_run_id: { name: 'flow_run_id', type: 'text' as const }, + node_config_json: { name: 'node_config_json', type: 'text' as const }, + }, +}; + +const ALL = ['id', 'name', 'amount', 'approval_status']; +/** writer → [readable, queryable] on `opportunity`. */ +const ANSWERS: Record = { + [RESTRICTED]: [['id', 'amount', 'approval_status'], ['id', 'amount', 'approval_status']], + [MASKED]: [ALL, ['id', 'amount', 'approval_status']], + [WRITER]: [ALL, ALL], +}; + +interface Ask { object: string; userId?: string; permissions?: unknown } + +/** Stands in for `plugin-security`: the system context reads every field, as its `isSystem` bypass does. */ +function makeVisibility(): FieldVisibilitySource & { calls: Ask[] } { + const calls: Ask[] = []; + const answer = (context: any, which: 0 | 1) => { + if (context?.isSystem) return ALL; + return ANSWERS[String(context?.userId ?? '')]?.[which] ?? []; + }; + return { + calls, + async getReadableFields(object: string, context?: any) { + calls.push({ object, userId: context?.userId, permissions: context?.permissions }); + return answer(context, 0); + }, + async getQueryableFields(_object: string, context?: any) { + return answer(context, 1); + }, + }; +} + +describe('[#23618] the RECORD_LOCKED refusal names the record from what its writer may read', () => { + let engine: ObjectQL; + let vis: ReturnType; + let lockedId: string; + let freeId: string; + + afterEach(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + }); + + beforeEach(async () => { + engine = new ObjectQL(); + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }), true); + await engine.init(); + for (const o of [OPPORTUNITY, APPROVAL_REQUEST]) { + engine.registry.registerObject(o as any, 'com.objectstack.test.23618'); + } + await engine.syncSchemas(); + + lockedId = String((await engine.insert('opportunity', { name: TITLE, amount: 100 }, { context: SYSTEM } as any)).id); + freeId = String((await engine.insert('opportunity', { name: 'Unlocked deal', amount: 5 }, { context: SYSTEM } as any)).id); + await engine.insert('sys_approval_request', { + object_name: 'opportunity', record_id: lockedId, status: 'pending', flow_run_id: 'run_1', + node_config_json: JSON.stringify({ lockRecord: true, approvalStatusField: 'approval_status' }), + }, { context: SYSTEM } as any); + + vis = makeVisibility(); + bindApprovalLockHook(engine as any, { warn: () => {}, info: () => {} }, () => vis); + }); + + async function refusal(writer: string, options: Record = { where: { id: lockedId } }): Promise { + let thrown: any = null; + try { + await engine.update('opportunity', { amount: 999 }, { ...options, context: asUser(writer) } as any); + } catch (e) { thrown = e; } + expect(thrown, `${writer} was not refused`).not.toBeNull(); + expect(thrown.code).toBe('RECORD_LOCKED'); + expect(thrown.statusCode).toBe(409); + return thrown; + } + + it('(a) a writer who may not read the display field is refused without the title — by-id write', async () => { + const thrown = await refusal(RESTRICTED); + expect(thrown.message).toBe(`RECORD_LOCKED: This Opportunity ${LOCKED_SENTENCE}`); + expect(thrown.message).not.toContain(TITLE); + expect(thrown.message).not.toContain(lockedId); + }); + + it('(a) …and on a predicate write, where the hook runs once per matched row', async () => { + const thrown = await refusal(RESTRICTED, { where: { amount: 100 }, multi: true }); + expect(thrown.message).toBe(`RECORD_LOCKED: This Opportunity ${LOCKED_SENTENCE}`); + expect(thrown.message).not.toContain(TITLE); + }); + + it('(a-masked) a display field served MASKED to the writer is withheld the same way (#20964)', async () => { + const thrown = await refusal(MASKED); + expect(thrown.message).toBe(`RECORD_LOCKED: This Opportunity ${LOCKED_SENTENCE}`); + expect(thrown.message).not.toContain(TITLE); + }); + + it('(b) control: a writer who may read the display field is told the title', async () => { + const thrown = await refusal(WRITER); + expect(thrown.message).toBe(`RECORD_LOCKED: Opportunity '${TITLE}' ${LOCKED_SENTENCE}`); + }); + + it('(c) one ask per refusal, with the writer\'s own envelope — and none for a write the lock does not refuse', async () => { + vis.calls.length = 0; + await engine.update('opportunity', { amount: 6 }, { where: { id: freeId }, context: asUser(RESTRICTED) } as any); + expect(vis.calls).toEqual([]); + + await refusal(RESTRICTED); + expect(vis.calls).toEqual([{ object: 'opportunity', userId: RESTRICTED, permissions: [`grant_${RESTRICTED}`] }]); + }); +});