fix(sharing): defer to an app-authored RLS widener before hard-refusing a by-id write (#5493)

.changeset/sharing-defer-authored-row-write-widener.md

@@ -0,0 +1,57 @@
+---
+"@objectstack/plugin-sharing": patch
+---
+
+fix(sharing): the by-id write gate defers to an app-authored RLS widener instead of hard-refusing (#5493)
+
+HotCRM's 17.0 GA acceptance sweep declared two RLS update-wideners on one
+profile and measured one of them working. On the junction object
+`crm_campaign_member` a non-owner PATCH returned 200; on `crm_campaign` the
+identical shape returned 403 `FORBIDDEN: insufficient privileges to update
+crm_campaign`. That sentence is the sharing middleware's, not the row gate's
+`(row-level security)` — so the refusal landed **before** RLS was consulted and
+the declared widener was never asked.
+
+The discriminator was never "carries sharing rules". It is whether **record
+sharing enforces on the object at all**: `checkEdit` abstains — and `canEdit`
+therefore answers `true`, letting the write through to RLS — when the effective
+sharing model is `public` (which `controlled_by_parent` maps onto) or when the
+schema has no `owner_id` field. A junction lands in that set; an ordinary owned
+business object does not. Same declaration, opposite outcome, split by a
+property no author writes down.
+
+Row-level write authority is ONE composite determination (maintainer ruling on
+#5492), so the middleware no longer ends the decision by itself. Before it
+hard-refuses a **by-id** update or delete, it asks the security service's
+`ISecurityService.checkAuthoredRowWrite` — the fail-closed verdict landed by
+#5493 step 1 (PR #6841) — whether an **app-authored** (non-floor) row-level
+policy admits this row for this operation. `admit` retracts this authority's
+refusal and hands the row to the security pre-image gate, which composes per
+#6684/#5492 and makes the final row decision. It does not authorize anything on
+its own.
+
+Everything else is unchanged, deliberately:
+
+- **The guarded surface does not shrink.** A member with no authored policy, no
+  share and no bypass is still refused; a read-level share still never widens a
+  write; an `edit`-level share still widens update and not delete (ADR-0111 D3),
+  and an update-only authored widener does not open delete either — the verb is
+  threaded through to the verdict, not collapsed.
+- **Fail-closed on every non-`admit` outcome.** No security service (a
+  deployment without `@objectstack/plugin-security`), a service predating the
+  method, a throwing probe, a principal-less context, an on-behalf-of context
+  (ADR-0090 D10) and any unrecognised verdict all leave today's refusal
+  byte-for-byte intact. The probe is reached through the same structural
+  late-binding this plugin already uses for `hasWriteBypass`; no runtime
+  dependency on `plugin-security` is introduced.
+- **A creator who is no longer the owner gets nothing back.** The platform's own
+  ownership floor (`created_by == current_user.id`, shipped on the additive
+  `member_default` baseline) matches a record transferred away from its creator,
+  so a deferral keyed on "the composed RLS admits this row" would return
+  transferred records to former creators. The verdict is provenance-aware and
+  abstains there; the deferral does not widen it.
+- **The bulk path is untouched** — it composes a filter rather than a verdict,
+  and is tracked separately (#6736).
+- **Objects with no owner field are untouched** (#6698): sharing abstains, the
+  gate never refuses, so the deferral is never reached and the platform
+  `created_by` write floor remains their only row-level write gate.

packages/plugins/plugin-security/src/row-write-widener-composition.test.ts

@@ -136,9 +136,10 @@ const CRM_REP: PermissionSet = PermissionSetSchema.parse({
  * — which is the shape #5493 reports from the other side: the security gate
  * admits the row and the SHARING middleware refuses it first.
  *
- * Present as a CONTROL, not a fix. This PR does not touch the sharing
- * middleware; the case at the bottom of this file measures that #5493's symptom
- * is unchanged by the composition landed here.
+ * It arrived as a CONTROL of the OLD position (#5492 left #5493's symptom
+ * standing deliberately). #5493 step 2 fixed it, so the case at the bottom of
+ * this file now measures the DEFERRAL: sharing still refuses on its own terms,
+ * consults `checkAuthoredRowWrite`, gets `admit`, and the write lands.
  */
 const CRM_REP_WIDENED: PermissionSet = PermissionSetSchema.parse({
   name: 'crm_rep_widened',
@@ -485,33 +486,43 @@ describe('[#5492] the platform ownership floor still stands where nothing replac
   });
 });
 
-describe('[#5493 control] the sharing middleware still refuses on its own — unchanged by this PR', () => {
+describe('[#5493] the sharing middleware DEFERS to an app-authored RLS widener instead of hard-refusing', () => {
   let stack: Stack;
   beforeEach(async () => { stack = await makeStack(); });
 
-  it('an APP-AUTHORED RLS update-widener passes the security gate and is still refused by sharing', async () => {
-    // #5493 is this composition's mirror image: there the RLS layer admits and
-    // the SHARING middleware answers FORBIDDEN first. This case reproduces that
-    // shape so the answer to "did #5492's fix change #5493?" is measured rather
-    // than reasoned: the app policy `stage == 'prospecting'` OR-combines past the
-    // platform ownership floor, so the security pre-image gate admits the row —
-    // and the write is still refused, by the other authority, exactly as before.
+  it('an APP-AUTHORED RLS update-widener passes the security gate AND the write now lands', async () => {
+    // ── what this case used to assert, and why it flipped ──────────────────
+    // #5492 (this file's subject) landed the composition and left #5493's
+    // symptom standing on purpose, so this case was written as a CONTROL of
+    // the old position: the security pre-image gate admitted the row and the
+    // SHARING middleware refused it first with `FORBIDDEN`. #5493 step 2 is
+    // the fix for exactly that, so the control flips — the two authorities are
+    // ONE composite determination (maintainer ruling, #5492 comment
+    // 5219846435; mirrored for this card in comment 5217346436), and this
+    // middleware may not hard-refuse a by-id write an app-authored widener
+    // admits by declaration.
+    //
+    // Everything the old case measured is still measured here, and the load
+    // is the same load: sharing STILL refuses on its own terms (`checkEdit` →
+    // `deny`, unchanged — nothing widened the sharing verdict), the security
+    // gate STILL admits the row through the app policy `stage ==
+    // 'prospecting'` OR-combining past the platform ownership floor. What
+    // changed is the composition between them: the middleware consults
+    // `checkAuthoredRowWrite` before refusing, gets `admit`, and hands the row
+    // to the pre-image gate that makes the final decision. The write lands and
+    // the ROW REALLY CHANGES — a completed write with an unchanged row would
+    // mean the middleware chain was bypassed, not that the fix works.
     await expect(
       stack.sharing.checkEdit('crm_opportunity', OPP_THEIRS.id, WIDENED_CTX as any),
     ).resolves.toBe('deny');
+    // The deferral predicate is the SECURITY service's verdict, not anything
+    // this middleware re-derived (ruling Q1 = A, #5493 comment 5226389104).
+    await expect(
+      stack.security.checkAuthoredRowWrite('crm_opportunity', OPP_THEIRS.id, 'update', WIDENED_CTX),
+    ).resolves.toBe('admit');
 
     const out = await stack.write('update', 'crm_opportunity', OPP_THEIRS.id, WIDENED_CTX);
-    expect(out.ok, 'still refused').toBe(false);
-    // The refusal is NOT the row-level pre-image gate's — that one admitted.
-    // Naming which authority refused is the whole point of the control.
-    expect(out.message, 'refused by the sharing middleware, not the RLS pre-image gate').not.toContain(
-      '(row-level security)',
-    );
-    // …and POSITIVELY the other authority's envelope, so this case cannot pass
-    // by refusing for some third reason (a missing CRUD bit, a thrown probe):
-    //   FORBIDDEN: insufficient privileges to update crm_opportunity opp_theirs
-    expect(out.code, "the sharing middleware's own code").toBe('FORBIDDEN');
-    expect(out.message).toContain('insufficient privileges to update crm_opportunity');
-    expect(rowById(stack, 'crm_opportunity', OPP_THEIRS.id)?.next_step).toBe('call');
+    expect(out, out.message).toMatchObject({ ok: true });
+    expect(rowById(stack, 'crm_opportunity', OPP_THEIRS.id)?.next_step).toBe('updated');
   });
 });