diff --git a/.changeset/21794-lock-refusal-user-message.md b/.changeset/21794-lock-refusal-user-message.md new file mode 100644 index 00000000000..f1088750cc4 --- /dev/null +++ b/.changeset/21794-lock-refusal-user-message.md @@ -0,0 +1,13 @@ +--- +'@objectstack/plugin-security': minor +--- + +The packaged-permission-set lock refusal carries its guidance as `userMessage`, so the console tells the admin to clone the set instead of showing its generic "You don't have permission to save this record." (#21794). + +Clause-②: yes (widening) + +- **`PackagedPermissionSetLockedError.userMessage`**, a new `readonly` member. A save that targets a permission set an installed package ships still answers `403 NOT_OVERRIDABLE` with the same `message`. That holds at the data door (`PATCH` / `POST /api/v1/data/sys_permission_set`) on every kernel. It also holds at the metadata door (`PUT /api/v1/meta/permission/:name`) on a kernel with no environment id, such as a self-hosted app server, where this lock is the refusal that answers. On an environment kernel the metadata protocol's own package-door refusal answers that `PUT` first, and it is unchanged. The error envelope now also carries `userMessage`, the field `ApiErrorSchema` already declares and the console renders verbatim. An edit is told to clone the set with the Clone action and edit the clone. A new set named like a packaged one is told to choose a different name, or clone. +- **`PackagedPermissionSetProvenanceUnknownError.userMessage`**, the same member on the fail-closed refusal (the platform could not tell whether a package ships the set). It tells the admin to try again, or clone. The unreadable source stays in `message`. +- The texts name no set, package, id or API path; `message` keeps that diagnostic for logs and developers. They are English, like every platform refusal. + +Nothing that was accepted is refused now, and nothing that was refused is accepted. Both refusals keep their `code`, `status` and `message`. diff --git a/packages/plugins/plugin-security/src/packaged-permission-set-lock.test.ts b/packages/plugins/plugin-security/src/packaged-permission-set-lock.test.ts index 6947f61f787..bae48395acf 100644 --- a/packages/plugins/plugin-security/src/packaged-permission-set-lock.test.ts +++ b/packages/plugins/plugin-security/src/packaged-permission-set-lock.test.ts @@ -76,8 +76,13 @@ import { describe, it, expect } from 'vitest'; import { PermissionSetSchema } from '@objectstack/spec/security'; import { assertEngineDeleteDispatch, assertEngineUpdateDispatch, assertEngineFindOnePredicate } from '@objectstack/metadata-core'; +import { mapDataError, resolveThrownHttpError } from '@objectstack/types'; import { SysPermissionSet } from './objects/sys-permission-set.object.js'; -import { classifyPackagedPermissionSet } from './packaged-permission-set-lock.js'; +import { + classifyPackagedPermissionSet, + PackagedPermissionSetLockedError, + PackagedPermissionSetProvenanceUnknownError, +} from './packaged-permission-set-lock.js'; import { registerPackagedPermissionSetLockGate } from './packaged-permission-set-lock-gate.js'; import { createPermissionSetWriteThrough, @@ -985,3 +990,149 @@ describe('[#21789] the lock judges "shipped by a code package" from the row\'s p .rejects.toMatchObject({ code: 'NOT_OVERRIDABLE', status: 403 }); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// The refusal's guidance reaches the END USER — `userMessage` on the wire +// ───────────────────────────────────────────────────────────────────────────── + +/** + * The console renders a refusal's `userMessage` verbatim and substitutes its + * generic "You don't have permission to save this record." for every 403 that + * carries none (the objectui#5210 producer-side opt-in; `declaredUserMessage` + * in `@objectstack/types` is the one read every door applies). An unmarked + * lock refusal therefore told the admin nothing about the clone path, which is + * the whole point of the ruling the lock implements. + * + * The wire envelope is computed by the producer's own mapping, not restated: + * + * - `mapDataError` is the call the REST `/data` door makes in its catch + * (`PATCH` and `POST /data/:object[/:id]`, `rest-server.ts`) — the door that + * serves Setup's save, measured on a booted showcase as the flat + * `{ error, code, object }` body; + * - `resolveThrownHttpError` is the dispatcher door's resolution of the same + * throw. + * + * Every case asserts the envelope (`status` + `code`) AND the `userMessage`: + * the code and status are the refusal's identity and must not move, and the + * marked text is what this block is about. The text itself is not pinned word + * for word — what is pinned is what the end user must and must not be shown: + * the guidance (clone it), and none of the diagnostic's machine names, package + * ids or API paths (the console's friendly-copy rule, objectstack#3821). + */ +describe('the lock refusal carries its guidance as `userMessage` on the wire envelope', () => { + const PACKAGE_ID = 'com.example.ehr'; + const SET_NAME = 'ehr_quality_inspector'; + + /** End-user copy: a non-empty string carrying none of the diagnostic's machine identifiers. */ + const expectEndUserCopy = (text: unknown, ...machineNames: string[]) => { + expect(typeof text, 'userMessage must be present on the wire').toBe('string'); + expect(String(text).trim().length).toBeGreaterThan(0); + for (const machineName of machineNames) { + expect(text, `userMessage must not name '${machineName}'`).not.toContain(machineName); + } + expect(text, 'userMessage must not carry an API path').not.toMatch(/\/api\//); + }; + + const refusedUpdate = async () => { + const ql = makeQl([packagedSet()]); + const protocol = makeHatchOpenProtocol(ql, { [SET_NAME]: packagedSet() }); + registerPermissionSetProjection(protocol, { ql }); + ql.permRows.push(packagedRow()); + return run(makeMiddleware(ql, protocol), { + object: 'sys_permission_set', operation: 'update', context: userCtx, + data: { id: 'ps_pkg', description: 'edit' }, + }).then(() => null, (e: any) => e); + }; + + it('UPDATE at the data door: 403 NOT_OVERRIDABLE with the clone guidance in `userMessage`', async () => { + const rejection = await refusedUpdate(); + expect(rejection).toBeInstanceOf(PackagedPermissionSetLockedError); + + const wire = mapDataError(rejection, 'sys_permission_set'); + expect(wire.status).toBe(403); + expect(wire.body.code).toBe('NOT_OVERRIDABLE'); + expectEndUserCopy(wire.body.userMessage, SET_NAME, PACKAGE_ID, 'sys_permission_set'); + expect(String(wire.body.userMessage)).toMatch(/clone it instead/i); + // The diagnostic channel is untouched: it still names the set and the + // package for logs and developers. The mark never replaces `message`. + expect(String(wire.body.error)).toContain(SET_NAME); + expect(String(wire.body.error)).toContain(PACKAGE_ID); + }); + + it('INSERT of a packaged name at the data door: 403 NOT_OVERRIDABLE with its own guidance (choose another name, or clone)', async () => { + const ql = makeQl([packagedSet()]); + const protocol = makeHatchOpenProtocol(ql, { [SET_NAME]: packagedSet() }); + registerPermissionSetProjection(protocol, { ql }); + const rejection = await run(makeMiddleware(ql, protocol), { + object: 'sys_permission_set', operation: 'insert', context: userCtx, + data: { name: SET_NAME, label: 'Mine', object_permissions: '{}' }, + }).then(() => null, (e: any) => e); + expect(rejection).toBeInstanceOf(PackagedPermissionSetLockedError); + + const wire = mapDataError(rejection, 'sys_permission_set'); + expect(wire.status).toBe(403); + expect(wire.body.code).toBe('NOT_OVERRIDABLE'); + expectEndUserCopy(wire.body.userMessage, SET_NAME, PACKAGE_ID, 'sys_permission_set'); + expect(String(wire.body.userMessage)).toMatch(/different name/i); + expect(String(wire.body.userMessage)).toMatch(/clone/i); + + // Each operation carries the guidance that fits it. + const update = mapDataError(await refusedUpdate(), 'sys_permission_set'); + expect(wire.body.userMessage).not.toBe(update.body.userMessage); + }); + + it('the dispatcher door resolves the same throw to the same status, code and `userMessage`', async () => { + const rejection = await refusedUpdate(); + const rest = mapDataError(rejection, 'sys_permission_set'); + const resolved = resolveThrownHttpError(rejection); + expect(resolved.status).toBe(403); + expect(resolved.code).toBe('NOT_OVERRIDABLE'); + expectEndUserCopy(resolved.userMessage, SET_NAME, PACKAGE_ID); + expect(resolved.userMessage).toBe(rest.body.userMessage); + }); + + it('the metadata door\'s registered lock throws the same class with the same `userMessage`', async () => { + const ql = makeQl([shippedArtifact()]); + let gate: ((ctx: { type: string; name: string; body: unknown }) => Promise) | undefined; + const protocol = { + registerAuthoringGate: (_type: string, g: typeof gate) => { gate = g; }, + getMetaItemLayered: async ({ name }: { name: string }) => ({ type: 'permission', name, code: null, overlay: null, effective: null }), + }; + expect(registerPackagedPermissionSetLockGate(protocol, ql)).toBe(true); + + const rejection = await gate!({ type: 'permission', name: SET_NAME, body: packagedSet() }) + .then(() => null, (e: any) => e); + expect(rejection).toBeInstanceOf(PackagedPermissionSetLockedError); + const resolved = resolveThrownHttpError(rejection); + expect(resolved.status).toBe(403); + expect(resolved.code).toBe('NOT_OVERRIDABLE'); + expectEndUserCopy(resolved.userMessage, SET_NAME, PACKAGE_ID); + expect(resolved.userMessage).toBe(mapDataError(await refusedUpdate(), 'sys_permission_set').body.userMessage); + }); + + it('the fail-closed sibling (provenance unknown): 403 NOT_OVERRIDABLE with retry-or-clone guidance, and no diagnostic reason', async () => { + const ql = makeQl([]); + ql.registry = { listItems: () => { throw new Error('registry unavailable'); } }; + const protocol = makeHatchOpenProtocol(ql, {}); + protocol.getMetaItemLayered = async () => { throw new Error('metadata store unavailable'); }; + registerPermissionSetProjection(protocol, { ql }); + ql.permRows.push({ id: 'ps_x', name: 'unknown_provenance', managed_by: 'admin', system_permissions: '[]' }); + const rejection = await run(makeMiddleware(ql, protocol), { + object: 'sys_permission_set', operation: 'update', context: userCtx, + data: { id: 'ps_x', system_permissions: '["whatever"]' }, + }).then(() => null, (e: any) => e); + expect(rejection).toBeInstanceOf(PackagedPermissionSetProvenanceUnknownError); + + const wire = mapDataError(rejection, 'sys_permission_set'); + expect(wire.status).toBe(403); + expect(wire.body.code).toBe('NOT_OVERRIDABLE'); + expectEndUserCopy( + wire.body.userMessage, 'unknown_provenance', 'sys_permission_set', + 'registry unavailable', 'metadata store unavailable', + ); + expect(String(wire.body.userMessage)).toMatch(/try again/i); + expect(String(wire.body.userMessage)).toMatch(/clone/i); + // The reason stays where it was: the diagnostic channel. + expect(String(wire.body.error)).toContain('registry unavailable'); + }); +}); diff --git a/packages/plugins/plugin-security/src/packaged-permission-set-lock.ts b/packages/plugins/plugin-security/src/packaged-permission-set-lock.ts index 020d604935e..483e5e6062d 100644 --- a/packages/plugins/plugin-security/src/packaged-permission-set-lock.ts +++ b/packages/plugins/plugin-security/src/packaged-permission-set-lock.ts @@ -262,6 +262,44 @@ export function classifyPackagedPermissionSet( }; } +/** + * The refusals' guidance, addressed to the END USER — the `userMessage` each + * refusal below declares. + * + * A thrown `userMessage` is the producer-side opt-in every HTTP door reads + * through `declaredUserMessage` (`@objectstack/types`; see + * `ThrownHttpError.userMessage` there): it rides the wire envelope beside + * `code`, and the console renders it verbatim in place of the generic sentence + * it substitutes for every unmarked 403 ("You don't have permission to save + * this record."). Unmarked, the clone guidance in `message` never reached the + * admin who hit the lock in Setup. + * + * The two channels have two audiences, so they carry different text: + * + * - `message` is the DIAGNOSTIC, for logs and developers. It names the set, + * the package and the API path, and it is unchanged. + * - `userMessage` is the GUIDANCE, for the admin. It names no machine name, + * id, package id or API path (the console's friendly-copy rule), so it + * carries no host state either: a sandboxed body that catches this error + * receives static text. + * + * English, like every platform refusal: there is no localization path for a + * thrown `userMessage`, and these texts do not invent one. + */ +const LOCKED_UPDATE_USER_MESSAGE = + `This permission set is provided by an installed package and can't be edited here. Clone it instead ` + + `with the Clone action, then edit the clone. The clone is your organization's own permission set, ` + + `and package upgrades keep reaching the original.`; + +const LOCKED_INSERT_USER_MESSAGE = + `This name belongs to a permission set provided by an installed package. Choose a different name for ` + + `your permission set, or clone the packaged one with the Clone action and edit the clone.`; + +const PROVENANCE_UNKNOWN_USER_MESSAGE = + `This permission set can't be saved right now, because the system couldn't confirm whether an installed ` + + `package provides it. Try again in a moment. To customize a permission set that a package provides, ` + + `clone it with the Clone action and edit the clone.`; + /** * The refusal a write door throws for a package-declared set. * @@ -287,11 +325,16 @@ export function classifyPackagedPermissionSet( * (`mapDataError` passes a domain error through on `.status`; the runtime * dispatcher's `errorFromThrown` reads `.status` then falls back to * `.statusCode`), and this throws on the DATA path, which reaches both. + * + * `userMessage` carries the same guidance for the end user, per operation — + * see the note on the texts above. */ export class PackagedPermissionSetLockedError extends Error { readonly code = 'NOT_OVERRIDABLE'; readonly status = 403; readonly statusCode = 403; + /** The guidance addressed to the end user; `message` stays the diagnostic. */ + readonly userMessage: string; constructor(name: string, packageId: string, operation: 'insert' | 'update') { super( `[Security] Permission set '${name}' is declared by package '${packageId}' and is locked in this ` @@ -305,14 +348,22 @@ export class PackagedPermissionSetLockedError extends Error { + `flowing to '${name}' untouched.`), ); this.name = 'PackagedPermissionSetLockedError'; + this.userMessage = operation === 'insert' ? LOCKED_INSERT_USER_MESSAGE : LOCKED_UPDATE_USER_MESSAGE; } } -/** Thrown when provenance could not be determined at all — fail-closed. */ +/** + * Thrown when provenance could not be determined at all — fail-closed. + * + * Its `userMessage` says to retry or clone and leaves the unreadable source's + * `reason` in `message`, where the diagnostic belongs. + */ export class PackagedPermissionSetProvenanceUnknownError extends Error { readonly code = 'NOT_OVERRIDABLE'; readonly status = 403; readonly statusCode = 403; + /** The guidance addressed to the end user; `message` stays the diagnostic. */ + readonly userMessage: string; constructor(name: string, reason: string) { super( `[Security] Permission set '${name}' cannot be saved right now: this environment could not determine ` @@ -321,6 +372,7 @@ export class PackagedPermissionSetProvenanceUnknownError extends Error { + `once the metadata layer is readable; if you meant to customize a packaged set, clone it instead.`, ); this.name = 'PackagedPermissionSetProvenanceUnknownError'; + this.userMessage = PROVENANCE_UNKNOWN_USER_MESSAGE; } } diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 60fecbc93af..e350395700c 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -12157,9 +12157,11 @@ export class RestServer { // derivation and one reason with two copies is this lane's own // recurring defect). `userMessage` has NO invariant left for a // caller to re-derive. `declaredUserMessage` already decided - // PRESENCE — the field exists on an error only because an - // author deliberately wrote caller-facing text onto it, and - // platform and driver code never set it — and + // PRESENCE — the field exists on an error only because its + // producer deliberately wrote end-user text with no host state + // onto it (an application hook, or a platform refusal carrying + // static guidance such as the packaged-permission-set lock's; + // platform and driver diagnostics never set it) — and // `truncateClientMessage` already applied #5423's bound to the // value. Reading `refusal.body.userMessage` IS the rule; there // is no second function to run it through, and running one diff --git a/packages/runtime/src/http-dispatcher.permission-denied-user-message.test.ts b/packages/runtime/src/http-dispatcher.permission-denied-user-message.test.ts index b998c724acb..bee1794e7c9 100644 --- a/packages/runtime/src/http-dispatcher.permission-denied-user-message.test.ts +++ b/packages/runtime/src/http-dispatcher.permission-denied-user-message.test.ts @@ -35,9 +35,11 @@ * `object` is derived from the ROUTE, never from `details.object` (which, on a * cascade delete, names a CHILD the caller never addressed). `§3` drives that * exact fixture **with a mark present**, so the repair cannot be read as - * loosening the withhold: the marked channel is authored end-user text, - * carried as a declared top-level sibling of `code`/`message`, and platform and - * driver code never set it. + * loosening the withhold: the marked channel is authored end-user text with + * no host state (an application hook's, or a platform refusal's static + * guidance such as the packaged-permission-set lock's), carried as a declared + * top-level sibling of `code`/`message`; platform and driver diagnostics never + * set it. * * That same ruling is also why the change is owed: it makes REST's shape the * contract for BOTH transports, and REST's shape has carried the mark on this diff --git a/packages/runtime/src/http-dispatcher.ts b/packages/runtime/src/http-dispatcher.ts index 5944b36f25f..c12bd1bf77a 100644 --- a/packages/runtime/src/http-dispatcher.ts +++ b/packages/runtime/src/http-dispatcher.ts @@ -2828,7 +2828,10 @@ export class HttpDispatcher { // SERVER-side diagnostics attached by `plugin-security` and are // still dropped above. `userMessage` is the opposite by // construction: it exists only because a producer wrote text - // FOR the caller, platform and driver code never set it, and it + // FOR the caller with no host state in it (an application + // hook, or a platform refusal carrying static guidance such as + // the packaged-permission-set lock's); platform and driver + // diagnostics never set it, and it // lands as a declared top-level sibling of `code`/`message` // (`ApiErrorSchema.userMessage`), never inside `details`. // The mark never moves the status or the `code` (commit 79c46da90). diff --git a/packages/runtime/src/sandbox/quickjs-runner.ts b/packages/runtime/src/sandbox/quickjs-runner.ts index 9c35efba2a6..93dfa9af669 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.ts @@ -1275,9 +1275,12 @@ function safeJsonStringify(v: unknown): string { * writes `const e = new Error(msg); e.userMessage = msg; throw e`, and the * text must survive the VM flattening the throw to a string, or the marking * dies exactly where its primary producers live. Crossing INTO the VM is safe - * for the same reason `code` is: the value is author-written user-facing text - * by construction (platform and driver code never sets the field), so it - * carries no host state a sandboxed body could exfiltrate — and it keeps the + * for the same reason `code` is: the value is text its producer authored for + * the end user (an application hook's, or a platform refusal's static + * guidance such as the packaged-permission-set lock's, which names no set, + * package, id or path), and platform and driver diagnostics never set the + * field, so it carries no host state a sandboxed body could exfiltrate — and + * it keeps the * established property that a body which catches, inspects and re-throws a * host error does not lose the structured payload. */ diff --git a/packages/spec/src/api/contract.zod.ts b/packages/spec/src/api/contract.zod.ts index 862667a8b0e..924d611713c 100644 --- a/packages/spec/src/api/contract.zod.ts +++ b/packages/spec/src/api/contract.zod.ts @@ -80,9 +80,12 @@ export const ApiErrorSchema = lazySchema(() => z.object({ * and the marked text are one value, so a boundary that rewraps or * substitutes `message` (sanitisation, truncation, the sandbox debug * wrapper) can never accidentally promote platform prose into the marked - * channel. Platform/driver code never sets it. Same audience-split - * precedent as `developerMessage` on the DELETE_RESTRICTED envelope - * (#7307), pointed the other way. + * channel. It is set only by a producer that authors end-user text with + * no host state: an application hook, or a platform refusal that carries + * static guidance (the packaged-permission-set lock's refusals in + * `@objectstack/plugin-security`). Platform and driver diagnostics never + * set it. Same audience-split precedent as `developerMessage` on the + * DELETE_RESTRICTED envelope (#7307), pointed the other way. * * It never replaces `message` — the diagnostic channel keeps its own wording * for logs and developers. diff --git a/packages/types/src/data-error-classification.ts b/packages/types/src/data-error-classification.ts index 2632bb1637b..af9155a5334 100644 --- a/packages/types/src/data-error-classification.ts +++ b/packages/types/src/data-error-classification.ts @@ -672,11 +672,13 @@ export function mapDataError(error: any, object?: string): { status: number; bod * Why riding it across the FAULT terminals is safe rather than a #5437/#7543 * regression: those disciplines withhold text the producer never addressed to * the caller — driver prose, a crash's `TypeError: …`. `userMessage` is the - * opposite by construction: it exists on an error only because an author - * deliberately wrote user-facing text onto it (`declaredUserMessage` answers - * `undefined` for everything else — platform and driver code never sets the - * field), so carrying it discloses nothing that was not authored for exactly - * this audience. A genuine crash carries no marking and its envelope is + * opposite by construction: it exists on an error only because its producer + * deliberately wrote end-user text with no host state onto it — an + * application hook, or a platform refusal carrying static guidance such as + * the packaged-permission-set lock's (`declaredUserMessage` answers + * `undefined` for everything else; platform and driver diagnostics never set + * the field) — so carrying it discloses nothing that was not authored for + * exactly this audience. A genuine crash carries no marking and its envelope is * byte-identical to before. The one thing the marking never does is move the * STATUS or the `code` — a marked crash is still the sanitised 500. */