Skip to content

Commit 65759ba

Browse files
os-litantclaude
andauthored
fix(plugin-sharing): render the by-id write denial through the operation-message catalog (#12976)
* fix(plugin-sharing): render the by-id write denial through the operation-message catalog The sharing middleware's by-id write gate refused with one hardcoded English sentence naming the object's API name and the row's opaque id. `@objectstack/rest` ships it as the 403 body's human-readable `error` and clients show it verbatim, so a user in a fully Chinese deployment read English prose they could not act on. The refusal now renders through the shared Operation Message Catalog in `@objectstack/spec/system` under the `record_write_denied` key that landed for it — the same mechanism plugin-security's record-level denial uses, which is the comparison the report drew. One key serves both write verbs; the verb, object and row id move to `developerMessage`, `details` and the log. `buildSharingMiddleware` gains an optional third argument, a lazily resolved `II18nService.t`-compatible lookup wired by `SharingServicePlugin`, because the i18n service is contributed by another plugin and may start later. Not changed: who may write, the `FORBIDDEN:` prefix the REST layer classifies 403 on, and the ADR-0111 D10 delete-verb diagnostic breadcrumb. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5 * docs(skills): stop printing a refusal string the sharing gate no longer emits `data-hooks.md` quoted the by-id write gate's message verbatim. That sentence is now end-user copy rendered in the caller's locale, so a hook author reading the doc would string-match prose that varies by locale. The fence names the shape and points at the stable channel (the error's code) instead. Token-neutral by construction: the published-bundle ratchet reads 12611 against a ceiling of 12611 (+0), so this correction spends no context budget. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5 * Revert "docs(skills): stop printing a refusal string the sharing gate no longer emits" This reverts commit ae4cfcc. Not a change of mind about the edit — it is correct and necessary, and it lands unchanged in its own PR. `skills/**` is a governed surface on the `GOVERNED_SURFACES` register in scripts/pm/check-governed-merges.mjs, and Prime Directive #14 judges a PR on its FILE LIST, not its description: a mixed diff is not a proportion question, one path hit forks the whole PR and reserves the landing for a human. Keeping the doc here would have made this branch unmergeable by anything but a hand merge. The doc half now rides claude/issue-12260-skills-data-hooks-prose, with the replacement text byte-identical. The two want landing together; until the doc one lands, data-hooks.md briefly prints a string the gate no longer emits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent b46773c commit 65759ba

5 files changed

Lines changed: 617 additions & 4 deletions

File tree

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
---
2+
"@objectstack/plugin-sharing": minor
3+
---
4+
5+
fix(plugin-sharing): the by-id write denial renders through the Operation
6+
Message Catalog instead of a hardcoded English sentence (#12260, the consumer
7+
half of the key #12493 landed)
8+
9+
A user holding object-level allowRead + allowEdit — and no `modifyAllRecords`
10+
PATCHed a record they do not own on an object declaring
11+
`sharingModel: 'public_read'` with `access: { default: 'private' }`. The sharing
12+
middleware refused, correctly, and the client showed the server's reason
13+
verbatim to the end user: one hardcoded English sentence naming the object's API
14+
name and the row's opaque id. In a fully Chinese deployment that was the only
15+
thing the user was told about why their save failed.
16+
17+
The refusal now renders through the shared Operation Message Catalog in
18+
`@objectstack/spec/system` under the key `record_write_denied` that #12493
19+
landed for it — the same mechanism `plugin-security`'s record-level denial
20+
already uses, which is exactly the comparison the report drew: the same "I can
21+
see this record but cannot change it" situation showed human language or raw
22+
English depending on which layer refused. Same resolution ladder (deployment
23+
override → the caller's locale → `en` → the key), same guarantee that a
24+
misbehaving i18n service cannot turn a 403 into a 500. All four platform
25+
locales (`en`, `zh-CN`, `ja-JP`, `es-ES`) ship copy that sends the reader to the
26+
record's owner or an administrator instead of dead-ending them.
27+
28+
`record_write_denied` is deliberately not `record_access_denied`: this gate
29+
fires on a row the READ path already admitted, so "You do not have access to
30+
this record" would be false the moment it rendered. It is one key for BOTH write
31+
verbs — the user's situation and remedy are identical for `update` and `delete`.
32+
33+
`buildSharingMiddleware` gains an optional third argument, a lazily resolved
34+
`II18nService.t`-compatible lookup wired by `SharingServicePlugin`, because the
35+
i18n service is contributed by another plugin and may start later. It is what
36+
makes the override address the catalog documents,
37+
`errors.record_write_denied`, take effect for this emitter. The argument is
38+
additive: every existing caller passes two and is unchanged, and a stack with no
39+
i18n service still renders the built-in catalog in the caller's locale.
40+
41+
**Not changed: who may write.** The gate is byte-identical — ownership, write
42+
depth, an edit-level share for `update`, Modify All Data — and the app-authored
43+
RLS deferral ahead of it is untouched. The `FORBIDDEN:` prefix the REST layer
44+
classifies 403 on is untouched, and so is the ADR-0111 D10 `delete`-verb
45+
diagnostic breadcrumb. The verb, object and row id the old sentence carried are
46+
now developer facts on the error's `developerMessage` and `details` and in the
47+
log, where a developer reads them and a user never does.

packages/plugins/plugin-sharing/src/authored-row-write-deferral.test.ts

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,13 @@
1111
// middleware's, not the row-gate's `(row-level security)` — so the refusal
1212
// landed BEFORE RLS was consulted and the declared widener was never asked.
1313
//
14+
// [#12260] That English sentence is HISTORY as of this card: the refusal's
15+
// user-facing half now renders from the Operation Message Catalog
16+
// (`record_write_denied`) and the verb/object/id it used to name moved to
17+
// `developerMessage`. The tell is unchanged in substance — `[sharing] …` vs
18+
// the row gate's `(row-level security)` — only its channel moved. See
19+
// `write-denial-user-copy.test.ts`.
20+
//
1421
// The discriminator is not "carries sharing rules" (#5493's own wording) but
1522
// **whether record sharing enforces on the object at all** (round-2 refinement,
1623
// issue comment 5226364929): `checkEdit` abstains — and `canEdit` therefore
@@ -52,6 +59,7 @@
5259
// and that everything that is not a literal `admit` leaves the refusal intact.
5360
import { describe, it, expect, beforeEach, vi } from 'vitest';
5461
import { assertEngineDeleteDispatch, assertEngineUpdateDispatch, assertEngineFindOnePredicate } from '@objectstack/objectql';
62+
import { BUILTIN_OPERATION_MESSAGES } from '@objectstack/spec/system';
5563
import { SharingService, type SharingSecurityProbe } from './sharing-service.js';
5664
import { buildSharingMiddleware } from './sharing-plugin.js';
5765

@@ -311,6 +319,8 @@ interface WriteOutcome {
311319
code?: string;
312320
status?: number;
313321
message: string;
322+
/** [#12260] The developer half the user-facing sentence no longer carries. */
323+
developerMessage?: string;
314324
}
315325

316326
interface Stack {
@@ -376,7 +386,10 @@ function makeStack(opts: {
376386
reached = true;
377387
});
378388
} catch (e: any) {
379-
return { ok: false, code: e?.code, status: e?.status, message: String(e?.message ?? e) };
389+
return {
390+
ok: false, code: e?.code, status: e?.status,
391+
message: String(e?.message ?? e), developerMessage: e?.developerMessage,
392+
};
380393
}
381394
return reached
382395
? { ok: true, message: 'written' }
@@ -405,7 +418,15 @@ function expectSharingRefusal(out: WriteOutcome, operation: 'update' | 'delete',
405418
expect(out.ok, `expected a refusal, got a completed ${operation}`).toBe(false);
406419
expect(out.code, 'ADR-0112 error code').toBe('FORBIDDEN');
407420
expect(out.status, 'ADR-0112 HTTP status').toBe(403);
408-
expect(out.message).toContain(`FORBIDDEN: insufficient privileges to ${operation} ${object} ${id}`);
421+
// [#12260] The SENTENCE moved onto the Operation Message Catalog (key
422+
// `record_write_denied`), so the discriminator this file turns on moved with
423+
// it: the verb, the object's API name and the row id are now developer copy.
424+
// Both halves are asserted, because both are how this refusal is told apart
425+
// from `plugin-security`'s row gate — which answers `PERMISSION_DENIED` with
426+
// its own `(row-level security)` breadcrumb and never writes `[sharing]`.
427+
// The `FORBIDDEN:` prefix is wire contract and is unchanged.
428+
expect(out.message).toBe(`FORBIDDEN: ${BUILTIN_OPERATION_MESSAGES.en.record_write_denied}`);
429+
expect(out.developerMessage).toContain(`[sharing] ${operation} denied on ${object} ${id}`);
409430
}
410431

411432
const rowById = (stack: Stack, object: string, id: string) =>

packages/plugins/plugin-sharing/src/sharing-plugin.ts

Lines changed: 109 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,12 @@ import type { ExecutionContext } from '@objectstack/spec/kernel';
2222
// mark `plugin-security` and `service-analytics` stamp at theirs — never a
2323
// local flag, never a second spelling of the same idea.
2424
import { markFilterSubtreeProvenance } from '@objectstack/spec/data';
25+
// [#12260] The SANCTIONED renderer for OPERATION-level refusal copy. The
26+
// Operation Message Catalog is the ONE seat for these sentences — its own
27+
// header bars both a package-local string table and a second rendering
28+
// mechanism for a second producer, and #12493 landed this middleware's key
29+
// (`record_write_denied`) into it ahead of this consumer half.
30+
import { renderOperationMessage, type ValidationMessageTranslator } from '@objectstack/spec/system';
2531
import { SysRecordShare, SysSharingRule, SysShareLink } from './objects/index.js';
2632
import { SysBusinessUnit, SysBusinessUnitMember } from '@objectstack/platform-objects/identity';
2733
import {
@@ -610,7 +616,7 @@ export class SharingServicePlugin implements Plugin {
610616
if (this.options.enforce === false) {
611617
ctx.logger.info('SharingServicePlugin: enforcement disabled (enforce=false) — share-link service still registered');
612618
} else {
613-
const mw = buildSharingMiddleware(this.service, ctx.logger as any);
619+
const mw = buildSharingMiddleware(this.service, ctx.logger as any, pluginMessageTranslator(ctx));
614620
if (typeof engine.registerMiddleware === 'function') {
615621
engine.registerMiddleware(mw, { object: '*' });
616622
ctx.logger.info('SharingServicePlugin: enforcement middleware installed');
@@ -954,6 +960,73 @@ export class SharingServicePlugin implements Plugin {
954960
}
955961
}
956962

963+
/**
964+
* [#12260] The END USER's half of the by-id write refusal.
965+
*
966+
* This middleware's refusal declares `{ code: 'FORBIDDEN', status: 403 }`, so
967+
* `@objectstack/rest` answers it through the DECLARED-status arm and ships
968+
* `error.message` to the client as the body's human-readable `error` — which
969+
* Console renders verbatim in a toast. One hardcoded English sentence
970+
* therefore reached a business user in a fully Chinese deployment as the only
971+
* thing they were told about why their save failed.
972+
*
973+
* Rendered through the SHARED Operation Message Catalog
974+
* (`@objectstack/spec/system`), not a second mechanism: same `errors.<key>`
975+
* override address, same resolution ladder (deployment override -> locale
976+
* catalog -> `en` -> the key), same guarantee that a misbehaving i18n service
977+
* cannot turn a 403 into a 500. `plugin-security`'s `userFacingDenialMessage`
978+
* is the sibling consumer this mirrors, and `plugin-approvals`'
979+
* `userFacingRefusal` (#11993) is the same conversion one card earlier.
980+
*
981+
* ⛔ ONE key for BOTH write verbs, which is the catalog's own ruling and not a
982+
* shortcut taken here: the user's situation (they can see this record, they
983+
* cannot change it) and their remedy (ask its owner, or an administrator) are
984+
* identical for `update` and `delete`. WHICH verb was refused is a developer
985+
* fact and stays on `developerMessage`, on the structured `details`, and — for
986+
* `delete` — on the ADR-0111 D10 breadcrumb that keeps its own wording.
987+
*
988+
* The `FORBIDDEN:` prefix is NOT part of what this renders. It is wire
989+
* contract (ADR-0111's `CODE: message` idiom, which the share routes read and
990+
* strip) and it is applied by the caller around this sentence.
991+
*
992+
* The translator is resolved LAZILY, per refusal, for the reason ADR-0029 D8
993+
* makes structural: the i18n service is contributed by a different plugin that
994+
* may start after this one, so a lookup captured when the middleware was built
995+
* would pin `undefined` for the life of the process. Absent is a SUPPORTED
996+
* stack, not a degraded one — the built-in catalog still renders the caller's
997+
* locale; what the translator adds is the documented override address
998+
* `errors.record_write_denied`.
999+
*/
1000+
function userFacingWriteDenial(
1001+
locale: string | undefined,
1002+
messageTranslator?: () => ValidationMessageTranslator | undefined,
1003+
): string {
1004+
let translate: ValidationMessageTranslator | undefined;
1005+
try {
1006+
translate = messageTranslator?.();
1007+
} catch {
1008+
// i18n is optional and late-bound; the built-in catalog still renders the
1009+
// caller's locale without it.
1010+
translate = undefined;
1011+
}
1012+
return renderOperationMessage({ messageKey: 'record_write_denied' }, { locale, translate });
1013+
}
1014+
1015+
/**
1016+
* [#12260] The deployment i18n lookup this plugin hands its middleware, read
1017+
* through `PluginContext` on every refusal rather than captured at start().
1018+
* See {@link userFacingWriteDenial} for why late binding is the requirement
1019+
* and not a defensive habit.
1020+
*/
1021+
function pluginMessageTranslator(ctx: PluginContext): () => ValidationMessageTranslator | undefined {
1022+
return () => {
1023+
const i18n = ctx.getService<II18nService>('i18n');
1024+
const t = i18n?.t;
1025+
if (typeof t !== 'function') return undefined;
1026+
return (key: string, loc: string, params?: Record<string, unknown>) => t.call(i18n, key, loc, params);
1027+
};
1028+
}
1029+
9571030
/**
9581031
* Build the engine middleware that injects read filters and gates
9591032
* write operations. Exported so it can be unit-tested without booting
@@ -971,6 +1044,15 @@ export class SharingServicePlugin implements Plugin {
9711044
export function buildSharingMiddleware(
9721045
service: SharingService,
9731046
log?: { warn?: (msg: string, meta?: any) => void },
1047+
/**
1048+
* [#12260] Deployment i18n lookup for the by-id write refusal's user-facing
1049+
* sentence — an `II18nService.t`-compatible function, resolved LAZILY per
1050+
* refusal. Optional and additive: every existing caller (six suites in
1051+
* `plugin-security`, two here) passes two arguments and is unchanged, and a
1052+
* stack without it still renders the caller's locale from the built-in
1053+
* catalog. See {@link userFacingWriteDenial}.
1054+
*/
1055+
messageTranslator?: () => ValidationMessageTranslator | undefined,
9741056
): EngineMiddleware {
9751057
return async function sharingMiddleware(ctx: OperationContext, next: () => Promise<void>) {
9761058
const op = ctx.operation;
@@ -1116,11 +1198,36 @@ export function buildSharingMiddleware(
11161198
{ object: ctx.object, recordId: String(id), userId: exec?.userId },
11171199
);
11181200
}
1201+
// [#12260] The DEVELOPER's half — the verb, the object's API name and
1202+
// the row id. This USED TO BE the whole message, which is how it
1203+
// reached an end user's toast in English; the catalog sentence
1204+
// deliberately names none of it (the only spellings available here
1205+
// are an API name and an opaque id, the #7414 vocabulary that must
1206+
// not reach a toast). It is kept where a developer reads it and a
1207+
// user never does: on the error, and in the log line below. REST
1208+
// ships neither `developerMessage` nor `details` on a FORBIDDEN
1209+
// body — only `DELETE_RESTRICTED` forwards a `developerMessage` —
1210+
// so this adds nothing to the wire.
1211+
const developerMessage =
1212+
`[sharing] ${verb} denied on ${ctx.object} ${id}: the caller holds no ${verb} authority ` +
1213+
`over this row (owner match, share depth and Modify All Data all answered no)`;
1214+
log?.warn?.(developerMessage, {
1215+
object: ctx.object,
1216+
recordId: String(id),
1217+
operation: verb,
1218+
userId: exec?.userId,
1219+
});
1220+
// The `FORBIDDEN:` PREFIX STAYS. It is not user copy — it is the
1221+
// ADR-0111 `CODE: message` idiom the share routes read and strip,
1222+
// and it sits beside the `code`/`status` the `/data` door
1223+
// classifies on. Only the SENTENCE after it moved.
11191224
const err: any = new Error(
1120-
`FORBIDDEN: insufficient privileges to ${op} ${ctx.object} ${id}`,
1225+
`FORBIDDEN: ${userFacingWriteDenial(exec?.locale, messageTranslator)}`,
11211226
);
11221227
err.code = 'FORBIDDEN';
11231228
err.status = 403;
1229+
err.developerMessage = developerMessage;
1230+
err.details = { operation: verb, object: ctx.object, recordId: String(id) };
11241231
throw err;
11251232
}
11261233
return next();

0 commit comments

Comments
 (0)