diff --git a/.changeset/automation-toggle-deny-message.md b/.changeset/automation-toggle-deny-message.md new file mode 100644 index 0000000000..4e71fdff5c --- /dev/null +++ b/.changeset/automation-toggle-deny-message.md @@ -0,0 +1,36 @@ +--- +"@objectstack/runtime": patch +--- + +fix(runtime): a refused `POST /automation/:name/toggle` is told what it attempted (#11666) + +The enablement door refuses in its own words now. A caller without +`manage_metadata` that hit `POST /api/v1/automation/:name/toggle` was answered +with the refusal the three definition writes share: + +```text +before: Authoring automation flows requires the `manage_metadata` capability. +after: Enabling or disabling an automation flow requires the `manage_metadata` capability. +``` + +They were disabling a flow, not authoring one. The sentence was accurate about +the policy — #10243's ruling classified toggle into the `manage_metadata` +authoring write set — and it named a verb the caller did not use. + +⛔ **Copy only; no policy moved.** The accept set is bit-identical: the same +callers are refused on the same four routes, `POST /` / `PUT /:name` / +`DELETE /:name` keep the shared sentence they read correctly with, and the +envelope is untouched — `PERMISSION_DENIED` / **403** on every arm, as #11660's +pins and the ADR-0112 vocabulary assert. Nothing becomes newly accepted or +newly rejected. + +Shaped on this domain's own precedent (`SCREEN_READ_DENY_MESSAGE` beside +`RUN_READ_DENY_MESSAGE`, #7968): a second constant for a second question, +rather than a reworded shared one. Rewording the shared sentence to cover both +was considered and declined — it would degrade the message for the three +definition writes in order to fix one arm. Both sentences still satisfy #7450: +each names the capability that would admit any caller, and nothing about this +one. + +A client branching on the human-readable prose of a 403 (rather than on +`error.code`) is the only thing that can notice. diff --git a/packages/runtime/src/domains/automation-toggle-deny-message.test.ts b/packages/runtime/src/domains/automation-toggle-deny-message.test.ts new file mode 100644 index 0000000000..6e6b750707 --- /dev/null +++ b/packages/runtime/src/domains/automation-toggle-deny-message.test.ts @@ -0,0 +1,328 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11666] A refused `POST /automation/:name/toggle` is told what IT attempted. + * + * ## The defect, exactly + * + * #10243's ruling put the enablement door into the `manage_metadata` authoring + * write set, and #11660 landed it by adding one arm to `isFlowAuthoringWrite`. + * The refusal that arm reached was the shared one: + * + * > `Authoring automation flows requires the \`manage_metadata\` capability.` + * + * A caller DISABLING a shipped flow was not authoring one. The sentence is + * accurate about the policy — the ruling classified toggle as an authoring + * write — and it names a verb the caller did not use. + * + * ## Why a pin on the SENTENCE, and why it could not be a pin on the envelope + * + * ⭐ The defect ships a 403 today. Every status- and code-only assertion in + * `automation-write-capability-gate.test.ts` passes over it, in both the + * before and the after state, which is precisely why this file exists and + * asserts the prose. Those assertions are not weakened here; they are the + * envelope half, and this is the copy half. + * + * ⛔ `code` and `status` do NOT move — `PERMISSION_DENIED` / 403 is what + * #11660's pins and the ADR-0112 vocabulary assert — and they are re-asserted + * on every case below so that a future edit to the copy cannot drag the + * envelope with it. + * + * ⛔ The POLICY is untouched. The accept set is bit-identical: the same callers + * are refused on the same four routes. Nothing here is allowed to become newly + * accepted or newly rejected, and the `still refused / still admitted` cases at + * the foot of this file are that guard, not decoration. + * + * ## Both directions, or the change is unpinned where it matters + * + * A one-sided pin ("toggle says the new thing") would sit green if someone + * later reworded the SHARED constant to match — option C, which was considered + * and declined because it degrades the sentence for the authoring writes that + * read correctly today. So each arm asserts its own sentence AND the + * absence of the other's. + * + * ## Driven through the registered route, not the shortcut + * + * Every case goes through `dispatcher.dispatch('POST', '/automation/…')`, which + * resolves the domain out of the registry `createAutomationDomain` registers — + * the path a real request takes, including the prefix slicing. Identity is + * supplied by stubbing `timedResolveExecutionContext`, the seam `dispatch` + * itself writes `context.executionContext` from + * (`automation-resume-envelope.test.ts` drives the same seam the same way). + */ + +import { describe, it, expect, vi } from 'vitest'; + +import { HttpDispatcher } from '../http-dispatcher.js'; +import type { HttpProtocolContext } from '../http-dispatcher.js'; + +const FLOW = 'lead_auto_assignment'; +const CAPABILITY = 'manage_metadata'; + +/** A legal flow definition — so nothing below is refused for its shape. */ +const DEFINITION = { name: FLOW, label: 'Lead Auto Assignment', type: 'autolaunched', nodes: [], edges: [] }; + +/** The two sentences, spelled out here rather than imported — a pin that reads + * its expectation from the module under test cannot see the module change. */ +const AUTHORING_SENTENCE = 'Authoring automation flows requires the `manage_metadata` capability.'; +const ENABLEMENT_SENTENCE = 'Enabling or disabling an automation flow requires the `manage_metadata` capability.'; + +/** The filer's principal: authenticated, an org owner, and NOT an author. */ +const UNENTITLED = { + userId: 'u_northwind_owner', + positions: ['organization_admin'], + permissions: ['org_admin'], + systemPermissions: [] as string[], +}; + +/** A metadata author — the positive control. */ +const AUTHOR = { userId: 'u_author', systemPermissions: [CAPABILITY] }; + +interface Harness { + dispatch: (method: string, path: string, body?: unknown) => Promise; + registerFlow: ReturnType; + unregisterFlow: ReturnType; + toggleFlow: ReturnType; + seed: (name: string) => void; +} + +function boot(principal: Record = UNENTITLED): Harness { + const flows = new Map([[FLOW, { ...DEFINITION }]]); + + const registerFlow = vi.fn((name: string, definition: unknown) => { flows.set(name, definition); }); + const unregisterFlow = vi.fn((name: string) => { flows.delete(name); }); + const toggleFlow = vi.fn(async () => undefined); + const getFlow = vi.fn(async (name: string) => flows.get(name)); + const listFlows = vi.fn(async () => [...flows.keys()]); + const execute = vi.fn(async () => ({ success: true, runId: 'run_1', status: 'completed' })); + + const services: Record = { + automation: { handlerReady: true, registerFlow, unregisterFlow, toggleFlow, getFlow, listFlows, execute }, + }; + const resolve = (name: string): unknown => services[name]; + const kernel: any = { + getService: resolve, + getServiceAsync: async (name: string) => resolve(name), + context: { getService: resolve }, + }; + + const dispatcher = new HttpDispatcher(kernel); + // The seam `dispatch` resolves identity through; a context handed in is + // overwritten by it, so the principal is supplied here. + (dispatcher as any).timedResolveExecutionContext = async () => ({ ...principal }); + + return { + dispatch: (method: string, path: string, body?: unknown) => + dispatcher.dispatch(method, path, body, {}, { request: {} } as HttpProtocolContext), + registerFlow, unregisterFlow, toggleFlow, + seed: (name: string) => { flows.set(name, { ...DEFINITION, name }); }, + }; +} + +const statusOf = (r: any): unknown => r?.response?.status; +const codeOf = (r: any): unknown => r?.response?.body?.error?.code ?? r?.response?.body?.error?.details?.code; +const messageOf = (r: any): string => String(r?.response?.body?.error?.message ?? ''); + +/** The three DEFINITION writes — the arm whose sentence must NOT move. */ +const DEFINITION_WRITES = [ + { + name: 'POST /automation (createFlow)', + drive: (h: Harness) => h.dispatch('POST', '/automation', { ...DEFINITION, name: 'probe_flow_x' }), + spy: (h: Harness) => h.registerFlow, + }, + { + name: 'PUT /automation/:name (updateFlow)', + drive: (h: Harness) => h.dispatch('PUT', `/automation/${FLOW}`, { ...DEFINITION, label: 'clobbered' }), + spy: (h: Harness) => h.registerFlow, + }, + { + name: 'DELETE /automation/:name (deleteFlow)', + drive: (h: Harness) => h.dispatch('DELETE', `/automation/${FLOW}`), + spy: (h: Harness) => h.unregisterFlow, + }, +] as const; + +describe('#11666 — the enablement door refuses in its own words', () => { + describe('the arm that was refused: POST /:name/toggle', () => { + it('names enabling/disabling, not authoring', async () => { + const h = boot(); + const result = await h.dispatch('POST', `/automation/${FLOW}/toggle`, { enabled: false }); + + // THE POINT of this file. + expect(messageOf(result)).toBe(ENABLEMENT_SENTENCE); + // ⛔ The defect's sentence, gone from this arm — asserted rather + // than implied, because `toBe` above would also pass if the shared + // constant had merely been reworded in place (option C). + expect(messageOf(result)).not.toContain('Authoring'); + expect(messageOf(result)).not.toBe(AUTHORING_SENTENCE); + }); + + it('⛔ carries the same envelope it always did — 403 PERMISSION_DENIED', async () => { + const h = boot(); + const result = await h.dispatch('POST', `/automation/${FLOW}/toggle`, { enabled: false }); + + expect(statusOf(result)).toBe(403); + expect(codeOf(result)).toBe('PERMISSION_DENIED'); + }); + + it('⛔ still refuses — the copy change admits nobody new', async () => { + const h = boot(); + const result = await h.dispatch('POST', `/automation/${FLOW}/toggle`, { enabled: false }); + + expect(statusOf(result)).toBe(403); + expect(h.toggleFlow).not.toHaveBeenCalled(); + }); + + it('says the same thing in both directions — enabling and disabling', async () => { + // #10243's measurement was symmetric, and a caller switching a flow + // ON is no more "authoring" than one switching it off. + const h = boot(); + + const off = await h.dispatch('POST', `/automation/${FLOW}/toggle`, { enabled: false }); + const on = await h.dispatch('POST', `/automation/${FLOW}/toggle`, { enabled: true }); + + expect(messageOf(off)).toBe(ENABLEMENT_SENTENCE); + expect(messageOf(on)).toBe(ENABLEMENT_SENTENCE); + }); + + it('answers before the body is read, in its own words', async () => { + // The gate is ahead of #3899's body checks, so `{ enable: false }` + // is a 403 rather than the 400 that names the key — and the 403 it + // gets is still the enablement sentence, not the shared one. + const h = boot(); + const result = await h.dispatch('POST', `/automation/${FLOW}/toggle`, { enable: false }); + + expect(statusOf(result)).toBe(403); + expect(messageOf(result)).toBe(ENABLEMENT_SENTENCE); + expect(h.toggleFlow).not.toHaveBeenCalled(); + }); + + it('a deeper spelling is gated AND told the same thing — `/:name/toggle/anything`', async () => { + // The router's toggle arm has no depth bound, so this path still + // reaches `toggleFlow`; the gate matches it, and the sentence must + // follow the gate rather than a narrower reading of the path. + const h = boot(); + const result = await h.dispatch('POST', `/automation/${FLOW}/toggle/x`, { enabled: false }); + + expect(statusOf(result)).toBe(403); + expect(messageOf(result)).toBe(ENABLEMENT_SENTENCE); + expect(h.toggleFlow).not.toHaveBeenCalled(); + }); + }); + + describe('the arms that were NOT refused here keep the sentence they read correctly with', () => { + for (const route of DEFINITION_WRITES) { + it(`${route.name}: still "Authoring automation flows …"`, async () => { + const h = boot(); + const result = await route.drive(h); + + expect(messageOf(result)).toBe(AUTHORING_SENTENCE); + // The other direction of the same pin: the enablement wording + // must not bleed onto a definition write. + expect(messageOf(result)).not.toContain('Enabling or disabling'); + expect(statusOf(result)).toBe(403); + expect(codeOf(result)).toBe('PERMISSION_DENIED'); + expect(route.spy(h)).not.toHaveBeenCalled(); + }); + } + + it('a flow literally NAMED `toggle` is still a definition write on PUT /automation/toggle', async () => { + // The sentence is chosen by the OPERATION, never by a substring of + // the flow name: `parts[1]` is what the enablement arm reads, and a + // one-segment PUT has no `parts[1]` at all. + const h = boot(); + h.seed('toggle'); + const result = await h.dispatch('PUT', '/automation/toggle', { ...DEFINITION, name: 'toggle' }); + + expect(statusOf(result)).toBe(403); + expect(messageOf(result)).toBe(AUTHORING_SENTENCE); + expect(h.registerFlow).not.toHaveBeenCalled(); + }); + + it('POST /:name/clone — the arm that arrived AFTER this card — still reads "Authoring …"', async () => { + // [#12156] The clone door joined `isFlowAuthoringWrite` on `main` + // while this branch held the file. This case could not have existed + // when the ones above were written; the merge created the + // population, so the merge owes the pin. + // + // A clone AUTHORS a flow — it registers a new definition under a new + // name — so the shared sentence is the correct one for it. It is also + // the arm most able to inherit the WRONG one: like the enablement + // door it is a two-segment POST whose verb lives in `parts[1]`, so a + // future loosening of `isFlowEnablementWrite` (dropping + // `parts[1] === 'toggle'`, or reading only the method and the depth) + // would hand it "Enabling or disabling an automation flow …" — this + // card's own defect, reproduced on the arm that arrived after it. + const h = boot(); + const result = await h.dispatch('POST', `/automation/${FLOW}/clone`, { + name: 'lead_auto_assignment_copy', + label: 'Lead Auto Assignment (copy)', + }); + + expect(messageOf(result)).toBe(AUTHORING_SENTENCE); + // The other direction, as every arm here does it: the enablement + // wording must not bleed onto an authoring write. + expect(messageOf(result)).not.toContain('Enabling or disabling'); + expect(messageOf(result)).not.toBe(ENABLEMENT_SENTENCE); + expect(statusOf(result)).toBe(403); + expect(codeOf(result)).toBe('PERMISSION_DENIED'); + expect(h.registerFlow).not.toHaveBeenCalled(); + }); + }); + + describe('what both sentences must go on doing (#7450)', () => { + it('each names the capability that would admit ANY caller', async () => { + const h = boot(); + + const toggle = await h.dispatch('POST', `/automation/${FLOW}/toggle`, { enabled: false }); + const write = await h.dispatch('DELETE', `/automation/${FLOW}`); + + expect(messageOf(toggle)).toContain(CAPABILITY); + expect(messageOf(write)).toContain(CAPABILITY); + }); + + it('neither answers the caller\'s own authorization topology', async () => { + const h = boot(); + const result = await h.dispatch('POST', `/automation/${FLOW}/toggle`, { enabled: false }); + + const serialized = JSON.stringify(result?.response?.body); + expect(serialized).not.toContain('organization_admin'); + expect(serialized).not.toContain('u_northwind_owner'); + expect(serialized).not.toContain('org_admin'); + }); + }); + + describe('⛔ the policy classification is untouched — the accept set is bit-identical', () => { + it('an entitled caller still toggles, in both directions', async () => { + const h = boot(AUTHOR); + + const off = await h.dispatch('POST', `/automation/${FLOW}/toggle`, { enabled: false }); + expect(statusOf(off)).toBe(200); + expect(h.toggleFlow).toHaveBeenLastCalledWith(FLOW, false); + + const on = await h.dispatch('POST', `/automation/${FLOW}/toggle`, { enabled: true }); + expect(statusOf(on)).toBe(200); + expect(h.toggleFlow).toHaveBeenLastCalledWith(FLOW, true); + }); + + it('an entitled caller still writes definitions', async () => { + const h = boot(AUTHOR); + const result = await h.dispatch('DELETE', `/automation/${FLOW}`); + + expect(statusOf(result)).toBe(200); + expect(h.unregisterFlow).toHaveBeenCalledWith(FLOW); + }); + + it('the legacy EXECUTION door stays out of the gate — even for a flow named `toggle`', async () => { + // `POST /automation/trigger/:name` is the run door. The enablement + // helper excludes `parts[0] === 'trigger'` for exactly this path, + // and extracting that helper must not have moved the exclusion. + const h = boot(); + h.seed('toggle'); + const result = await h.dispatch('POST', '/automation/trigger/toggle', {}); + + expect(statusOf(result)).not.toBe(403); + expect(h.toggleFlow).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/packages/runtime/src/domains/automation.ts b/packages/runtime/src/domains/automation.ts index 3bc7782ac3..a088b6afed 100644 --- a/packages/runtime/src/domains/automation.ts +++ b/packages/runtime/src/domains/automation.ts @@ -308,6 +308,48 @@ const FLOW_WRITE_DENY_CODE = 'PERMISSION_DENIED'; const FLOW_WRITE_DENY_MESSAGE = `Authoring automation flows requires the \`${FLOW_AUTHORING_CAPABILITY}\` capability.`; +/** + * [#11666] The enablement arm's own refusal text — the same capability, the + * same `code` and the same `status` as {@link FLOW_WRITE_DENY_MESSAGE}, and a + * different sentence, because a different operation was attempted. + * + * ⛔ Copy, not policy. [#10243]'s ruling put `POST /:name/toggle` into the + * authoring write set and that classification is untouched here: the same + * callers are refused, with the same `PERMISSION_DENIED` and the same 403. + * What moves is only what a refused caller is TOLD. Switching a shipped flow + * OFF was answered with "Authoring automation flows requires …" — accurate + * about the policy, and naming a verb the caller did not use. + * + * Shaped on {@link SCREEN_READ_DENY_MESSAGE} (#7968) one screen up: a second + * constant for a second question, rather than a reworded shared one. Rewording + * the shared sentence was considered and declined — it reads correctly for the + * authoring writes that reach it (`POST /`, `PUT /:name`, `DELETE /:name`, and + * [#12156]'s `POST /:name/clone`), and widening it to cover both would degrade + * it for every one of them in order to fix one. + * + * It satisfies #7450 exactly as its sibling does: it names the capability that + * would admit ANY caller, and nothing about this one. + */ +const FLOW_ENABLEMENT_DENY_MESSAGE = + `Enabling or disabling an automation flow requires the \`${FLOW_AUTHORING_CAPABILITY}\` capability.`; + +/** + * [#11666] Is THIS request the enablement door, `POST /automation/:name/toggle`? + * + * Extracted so the question is asked once. {@link isFlowAuthoringWrite} needs + * it to decide whether the route is gated at all, and + * {@link refuseUngrantedFlowWrite} needs the SAME answer to decide which + * sentence the refusal carries — and this file's own rule is that a question + * spelled at two call sites is two questions that happen to agree today. + * + * ⛔ The truth table is [#10243]'s, moved nowhere: the exclusion of + * `parts[0] === 'trigger'` and the absence of any depth bound are that arm's, + * for that arm's reasons, restated below where they are read. + */ +function isFlowEnablementWrite(parts: string[], method: string): boolean { + return method === 'POST' && parts[1] === 'toggle' && parts[0] !== 'trigger'; +} + /** * [#10145] Which `/automation` routes the `manage_metadata` write set covers. * @@ -378,7 +420,11 @@ function isFlowAuthoringWrite(parts: string[], method: string): boolean { // toggle arm, so for a flow literally named `toggle` the path // `/automation/trigger/toggle` RUNS that flow. Gating it would over-block // an execution door, which is the one thing the ruling did not do. - if (method === 'POST' && parts[1] === 'toggle') return parts[0] !== 'trigger'; + // + // [#11666] Delegated to `isFlowEnablementWrite` rather than inlined, because + // the refusal text now asks the same question; under this guard the helper + // reduces to exactly the `parts[0] !== 'trigger'` it replaces. + if (method === 'POST' && parts[1] === 'toggle') return isFlowEnablementWrite(parts, method); // [#12156] `POST /automation/:name/clone` — the ADR-0126 §7.1 clone door. // // It CREATES a flow, so it belongs to this set for the same reason @@ -456,7 +502,11 @@ function isFlowActivationWrite(parts: string[], method: string): boolean { * not permission-SET names, which ride `permissions` (#4705). * * The message names the CAPABILITY it wants and nothing about the caller (no - * positions, no permission-set names — #7450). + * positions, no permission-set names — #7450). [#11666] There are two of them, + * picked by the arm that was actually refused — the definition writes get + * {@link FLOW_WRITE_DENY_MESSAGE}, the enablement door gets + * {@link FLOW_ENABLEMENT_DENY_MESSAGE}. ⛔ `code` and `status` are shared and + * do not vary: one policy, one envelope, two sentences. * * ⚠️ Callers MUST run this BEFORE the automation service is resolved and before * any body validation, so (a) an unentitled caller cannot use the 501-vs-403 @@ -488,14 +538,22 @@ const refuseUngrantedFlowActivationWrite = ( function refuseUngrantedFlowWrite( deps: DomainHandlerDeps, context: HttpProtocolContext, + parts: string[], + method: string, ): HttpDispatcherResult | undefined { const ec: any = context?.executionContext; if (ec?.isSystem) return undefined; if (new Set(ec?.systemPermissions ?? []).has(FLOW_AUTHORING_CAPABILITY)) return undefined; + // [#11666] Which sentence, decided from the SAME predicate that decided the + // route is gated — never a second reading of the path. + const message = isFlowEnablementWrite(parts, method) + ? FLOW_ENABLEMENT_DENY_MESSAGE + : FLOW_WRITE_DENY_MESSAGE; + return { handled: true, - response: deps.error(FLOW_WRITE_DENY_MESSAGE, FLOW_WRITE_DENY_STATUS, { code: FLOW_WRITE_DENY_CODE }), + response: deps.error(message, FLOW_WRITE_DENY_STATUS, { code: FLOW_WRITE_DENY_CODE }), }; } @@ -876,6 +934,8 @@ async function respondToFlowTrigger( * ⚑ authoring write — `manage_metadata` (#10243): * enablement is environment-wide, so an * unentitled toggle reached every organization + * ⚑ refused with its OWN sentence (#11666) — + * same capability, code and status * POST /:name/clone → clone the whole definition under a NEW machine * name (ADR-0126 §7.1, #12156). Body * `{ name, label }`, both mandatory; unknown source @@ -967,7 +1027,7 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str // /:name/toggle` moved INSIDE it by ruling, and moved by editing that one // predicate rather than by adding a check here. if (isFlowAuthoringWrite(parts, m)) { - const refusal = refuseUngrantedFlowWrite(deps, context); + const refusal = refuseUngrantedFlowWrite(deps, context, parts, m); if (refusal) return refusal; }