fix(service-storage,plugin-audit,plugin-security)!: the attachment and comment parent gates judge a controlled_by_parent parent through its master

.changeset/22455-cbp-parent-gates-judge-master.md

@@ -0,0 +1,22 @@
+---
+'@objectstack/service-storage': minor
+'@objectstack/plugin-audit': minor
+---
+
+fix(service-storage,plugin-audit)!: the `sys_attachment` and `sys_comment` parent gates judge a `controlled_by_parent` parent through its master, so a member can no longer attach files to, or delete other people's files or comments from, a child record they may not edit
+
+Clause-②: no (narrowing)
+
+<!-- adr-0087: not-required (no-migration-prescription) No metadata moves: no spec key, authorable spelling or stored shape is removed, renamed or re-shaped, and no stored row is read, rewritten, converted or dropped, so there is nothing for `objectstack migrate meta` to rewrite. What narrows is a runtime accept set: the attachment parent gate (attach, the update and delete row rule, the re-point) and the comment moderation gate now refuse a write on a controlled_by_parent parent whose master the caller may not edit. The remedy is an access decision the app owner makes on the master record, not a rewrite. The two exported port types (AttachmentSharingLike, CommentSharingLike) now pick the sharing service's tri-state member instead of its two-state projection; the registered sharing service serves both, and a hand-written double meets the compiler at its own call site. The other categories are closed on facts: both bumped packages publish (not unpublished); no ADR-0087 id covers these paths and this diff adds none (not registered / already-registered); and the port types were concretely typed before, not erased (not type-surface-only). -->
+
+**BREAKING** (an accept-set narrowing), shipped as `minor` under the launch-window convention for breaking changes. It follows ADR-0055.
+
+**What stops being accepted.**
+
+- A `controlled_by_parent` record takes its access from its MASTER. Record sharing does not enforce on it, so the sharing service's edit verdict abstains on every such record, and both gates used to take that abstention as permission. A member holding the `sys_attachment` create or delete bit, or the `sys_comment` delete bit, could therefore attach files to, and delete other people's files and comments from, every master-detail child record of the organization. That included records whose own `PATCH` refuses the member, and records the member cannot read.
+- Both gates now ask the sharing service's tri-state `checkEdit`. `allow` admits and `deny` refuses, as before. On an abstention they ask the security service's master-detail write check (`ISecurityService.checkControlledByParentWrite`), which is the answer a by-id update of the parent gets. Its `allow` and `not_applicable` admit. Its `deny` refuses, and so does an `unresolvable` outcome: a broken master-detail declaration, a record that does not exist, or an empty master reference. A datasource fault propagates with its `503`.
+- The refusals keep today's envelopes. An attach or a re-point answers `403 ATTACHMENT_PARENT_ACCESS`. A delete of another user's file answers `403 ATTACHMENT_DELETE_DENIED`, and an update of it answers `403 RECORD_NOT_ACCESSIBLE`. A delete or update of another user's comment answers `403 RECORD_NOT_ACCESSIBLE`. A caller who cannot read the parent still gets the not-visible `403 PERMISSION_DENIED`.
+
+**What is unchanged.** An uploader still deletes their own file, and an author still deletes their own comment. Posting a comment still needs only read access to the record. A parent that is not `controlled_by_parent` is judged exactly as before, and a `public_read_write` parent still admits every member. A kernel whose security service does not serve the master-detail write check composes no master check, and there the gates admit on the abstention as before, which is also what the parent's own update gets.
+
+**What changes for you.** A member who was attaching or moderating on a child record through this gap now gets the refusal above. To give them that access, grant them edit access on the MASTER record: ownership of it, an `edit`-level record share, the `modifyAllRecords` bypass on the master object, or an app-authored `update` row-level security policy on the master that admits the row. Then their `PATCH` of the child and their attachment and moderation rights on it agree. The port types `AttachmentSharingLike` and `CommentSharingLike` now pick `checkEdit` rather than `canEdit`. The registered sharing service serves both, so a caller passing it is unaffected. `installAttachmentAccessHooks` and `installCommentAccessHooks` take an optional trailing resolver for the security service (new port types `AttachmentSecurityLike` and `CommentSecurityLike`). The two plugins pass it themselves.

.changeset/22455-security-serves-controlled-by-parent-write.md

@@ -0,0 +1,12 @@
+---
+'@objectstack/plugin-security': minor
+---
+
+The registered `security` service serves `checkControlledByParentWrite(object, recordId, context)`, the ADR-0055 master-detail write check that `ISecurityService` declares as optional.
+
+- **What it answers.** It answers what a by-id update of the record gets from the master-detail write check. It runs the write path's own composition and never re-derives it: the engine middleware's context prologue (a principal-less context, permission sets that cannot be resolved and a missing on-behalf-of delegator are refused with the same `403 PERMISSION_DENIED`), and then the check for the principal's permission sets and, on a delegated context, for the delegator's (ADR-0090 D10). The first refusal is the answer. A refusal resolves `deny` with the leg it came from, the three non-verdicts resolve `unresolvable` with their reason, and an object that is not `controlled_by_parent` resolves `not_applicable`. A system context resolves `allow`. A store fault rejects with the engine's own error, so a datasource outage keeps its `503`.
+- **Where it differs from the write path's guard.** A principal whose permission sets resolve to none is judged over that empty list, so it answers `deny` on `object_permission` as the contract declares. A principal with positions or sets and no user id is also judged, where the write path skips the check. It answers `deny` when it holds no `update` grant on the master, and its own update is refused by the object-level gate anyway.
+- **Who asks it.** The `sys_attachment` parent gate in `@objectstack/service-storage` and the `sys_comment` moderation gate in `@objectstack/plugin-audit` ask it, so they judge a `controlled_by_parent` parent through its master.
+- The boot line that announces the `security` service now lists the members the registered service actually serves, read from the service object. The hand-kept list it replaced had fallen behind by five members.
+
+Nothing an author writes changes, and no by-id write is judged differently. The middleware's context prologue moved into one method that the middleware and the member both call. Its refusals, their order and their wording are unchanged.

content/docs/permissions/system-context.mdx

@@ -10,7 +10,7 @@ the seed loader replaying package fixtures, a plugin's boot reconciler, a
 service self-write, a migration.
 
 This page is **the authority** for what that flag actually does. It exists
-because the flag is not one concept: it is a single boolean read at **121
+because the flag is not one concept: it is a single boolean read at **122
 distinct sites across 20 packages**, and knowing three of those behaviours gives
 no hint that the other hundred-and-four exist. Every documented app-side bug
 traced to `isSystem` had the same shape — the metadata was complete and correct,
@@ -106,6 +106,7 @@ that silently does not happen.
 | 7 | Write bypass = `true`, effective write scope = `org` | plugin-security | Get: widest write scope without holding any capability | `packages/plugins/plugin-security/src/security-plugin.ts#start` |
 | 8 | Metadata-plane schema masking exempt (ADR-0106 D4) | metadata-core | Get: unmasked object schema. Note: the exemption is a **caller** property — it short-circuits before the security service is consulted | `packages/metadata-core/src/object-schema-fls.ts#isObjectSchemaMaskExempt` |
 | 9 | `explain()` may target a principal other than the caller | plugin-security | Get: no `manage_users` / delegated-admin check | `packages/plugins/plugin-security/src/security-plugin.ts#explainAccessForCaller` |
+| 9b | The master-detail write check the `security` service serves (`checkControlledByParentWrite`, ADR-0055) answers `allow` | plugin-security | Get: `allow` for any record, with no master judged — the write path's own first exit, so a gate composing it admits exactly what the record's own system-context update would. Lose: nothing a caller reaches today: its two consumers, the `sys_attachment` and `sys_comment` parent gates, return before they ask for a system session | `packages/plugins/plugin-security/src/security-plugin.ts#checkControlledByParentWrite` |
 | 10 | Anonymous-deny treats the caller as authenticated | core | Get: passes the 401 seam with no `userId` | `packages/core/src/security/anonymous-deny.ts#shouldDenyAnonymous` |
 | 11 | Permission-set projection middleware skipped | plugin-security | Lose: projection of permission-set-derived columns | `packages/plugins/plugin-security/src/permission-set-projection.ts#createPermissionSetWriteThrough` |
 | 11b | Position write-through skipped — a `sys_position` write is not mirrored into the environment ledger | plugin-security | Get: the seeders and the package door write rows for definitions that already have their home, untouched. Lose: a system-written row has no environment definition, so the security catalog read does not resolve it until a data-door edit, or the one-time row-only position backfill, gives it one | `packages/plugins/plugin-security/src/position-write-through.ts#createPositionWriteThrough` |
@@ -142,7 +143,7 @@ that silently does not happen.
 
 ### 3. Sharing (`plugin-sharing`)
 
-The largest single consumer — **17 of the 121 sites**.
+The largest single consumer — **17 of the 122 sites**.
 
 | # | Behaviour when `isSystem` | What you get / what you lose | Anchor |
 |:--|:---|:---|:---|
@@ -285,7 +286,7 @@ Ownership injection, `readonly` bypass and sharing materialisation are
 independent decisions, and a seed loader plausibly wants the first two but not
 the third. The concept is nevertheless **staying as one boolean**:
 
-- **Shipped semantics.** `isSystem` is a published contract with 121 read sites
+- **Shipped semantics.** `isSystem` is a published contract with 122 read sites
   in 20 packages. Splitting it is a breaking contract change across all of them.
   (The ruling was taken when the census read 80 sites in 18 packages; the count
   has grown, which strengthens rather than weakens the argument.)
@@ -359,16 +360,16 @@ still holds equal to the census on every pull request:
 | Appearances of the bare identifier `isSystem` in non-test sources | 813 | — |
 | — parsed as a declaration | 28 | ✅ |
 | — parsed as an object-literal / type key (producers and option objects) | 310 | — |
-| — parsed as a property **read** | 127 | ✅ |
+| — parsed as a property **read** | 128 | ✅ |
 | — parsed in some other syntactic position (a local, a cast, a conditional) | 9 | ✅ |
 | — the remainder: text inside comments and string literals | 358 | — |
 | Of those reads: reads of one of the unrelated metadata fields | 6 | ✅ |
-| Of those reads: reads of `ExecutionContext.isSystem` | **121** | ✅ |
-| — behaviour-bearing (rows 1–61 above) | 118 | ✅ |
+| Of those reads: reads of `ExecutionContext.isSystem` | **122** | ✅ |
+| — behaviour-bearing (rows 1–61 above) | 119 | ✅ |
 | — carry the flag onward only (rows 62–64 above) | 3 | ✅ |
 | Packages containing at least one elevation read | **20** | ✅ |
 | Files containing at least one elevation read | 57 | ✅ |
-| — the distinct symbols those reads live in — what this page anchors | 103 | ✅ |
+| — the distinct symbols those reads live in — what this page anchors | 104 | ✅ |
 | — of those files, the ones holding more than one read in one symbol | 8 | ✅ |
 
 The six rows marked — are a **dated decomposition, not a live claim**: they were

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

@@ -2,7 +2,7 @@
 
 import type { Plugin, PluginContext } from '@objectstack/core';
 import { resolveLocalizationContext } from '@objectstack/core';
-import type { IDataEngine, II18nService, ISharingService } from '@objectstack/spec/contracts';
+import type { IDataEngine, II18nService, ISecurityService, ISharingService } from '@objectstack/spec/contracts';
 import { SysAuditLog, SysActivity, SysComment } from './objects/index.js';
 // `sys_notification` was parked here "until that [ADR-0030] migration lands".
 // It has landed, so the contribution moved to @objectstack/service-messaging —
@@ -355,7 +355,7 @@ export class AuditPlugin implements Plugin {
           () => {
             try {
               // Typed with the slot's contract (#4251): the gate consults
-              // `canEdit` only, but it consults the REAL interface.
+              // `checkEdit` only, but it consults the REAL interface.
               return ctx.getService<ISharingService>('sharing');
             } catch {
               return null;
@@ -372,6 +372,17 @@ export class AuditPlugin implements Plugin {
             if (typeof t !== 'function') return undefined;
             return (key: string, loc: string, params?: Record<string, unknown>) => t.call(i18n, key, loc, params);
           },
+          // [ADR-0055] The security service, for its master-detail write check:
+          // a `controlled_by_parent` parent is judged through its master, as its
+          // own update is. Resolved per check (the security plugin may start
+          // after this one); a kernel without it composes no master check.
+          () => {
+            try {
+              return ctx.getService<ISecurityService>('security');
+            } catch {
+              return null;
+            }
+          },
         );
         if (typeof (engine as any).registerMiddleware === 'function') {
           installCommentReadVisibility(engine as any, ctx.logger);