|
| 1 | +--- |
| 2 | +"@objectstack/plugin-sharing": minor |
| 3 | +--- |
| 4 | + |
| 5 | +fix(plugin-sharing): one refused grant no longer aborts a sharing rule's reconcile pass — its stale-row revocations still run (#14754) |
| 6 | + |
| 7 | +After #14484 `sys_record_share` is `tenant-scoped` in the #13491 ledger, so on a |
| 8 | +walled install an organization-less system insert on it is refused loudly with |
| 9 | +`ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED` (#8844). `SharingService.grant` |
| 10 | +resolves the organization on every path that can; a platform-global sharing |
| 11 | +rule (`organization_id = null`, its sweep unscoped) materialising a grant onto |
| 12 | +an organization-LESS record resolves none, and meets that refusal. |
| 13 | + |
| 14 | +`SharingRuleService.reconcile` / `reconcileForRecord` had no per-grant catch, so |
| 15 | +the refusal propagated and **that rule's pass aborted mid-loop**. Two things |
| 16 | +were lost, and they are not equally serious: |
| 17 | + |
| 18 | +- the remaining grants — recoverable, the next pass writes them; |
| 19 | +- **the stale-row revocations of that pass** — not recoverable by waiting, |
| 20 | + because every subsequent pass meets the same organization-less record and |
| 21 | + dies in the same place. A stale over-grant of that rule therefore persisted |
| 22 | + indefinitely, and the record kept aborting the pass until it was repaired by |
| 23 | + hand. That is the security-relevant half. |
| 24 | + |
| 25 | +Measured while pinning this, and it sharpens the point: the engine returns |
| 26 | +organization-less rows **last** in a rule's criteria sweep (the driver's |
| 27 | +NULL-org compatibility arm is appended to the scoped arm). So a refused grant |
| 28 | +is nearly always one of the final attempts of a pass, and what an abort |
| 29 | +destroyed was hardly ever "the remaining grants" — it was almost entirely the |
| 30 | +revoke loop that runs after the whole upsert loop. |
| 31 | + |
| 32 | +Both loops now attempt each grant individually. A refusal is logged with the |
| 33 | +rule, object, record, recipient and the engine's own code, counted, and the |
| 34 | +pass **continues** — the remaining grants and, above all, the stale-row |
| 35 | +revocations still run. |
| 36 | + |
| 37 | +**The catch is deliberately narrow.** Only |
| 38 | +`ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED` is absorbed; every other error |
| 39 | +rethrows unchanged. A catch-all would swallow real defects and report a pass |
| 40 | +that "completed" having written nothing. It would also silently retire a |
| 41 | +reviewed decision: `record-share-organization-stamp.test.ts` deliberately pins |
| 42 | +the abort on the OTHER error a reconcile pass can meet here — the scoped update |
| 43 | +half answering `RECORD_NOT_FOUND` for a row stamped with a different |
| 44 | +organization — which the 2026-09-02 contract review left standing on "loud |
| 45 | +beats a wrong count". Those three pins are unchanged and still green. |
| 46 | + |
| 47 | +**Why `minor` rather than `patch`.** The repair is a bug fix, but it reports |
| 48 | +through a new key. `reconcile` / `reconcileForRecord` / `evaluateRule` / |
| 49 | +`evaluateAllForRecord` now return `SharingRuleReconcilePassResult` — the spec's |
| 50 | +`SharingRuleEvaluationResult` plus `grantsRefused: number` — and that type is |
| 51 | +newly exported from the package index. Purely additive: the contract in |
| 52 | +`@objectstack/spec` is untouched, its six declared fields are unchanged, and a |
| 53 | +consumer typed against `ISharingRuleService` keeps compiling as it did. Same |
| 54 | +shape as `fix(runtime): tell an action handler when its caller-scope record load |
| 55 | +was refused` (#14143), which shipped `minor` for the same reason. |
| 56 | + |
| 57 | +`grantsRefused > 0` does **not** mean the pass failed. It means the pass met a |
| 58 | +record it cannot grant on and carried on — which is the whole point. |
| 59 | + |
| 60 | +**Wire surface — declared, not lifted.** `grantsRefused` reaches the wire. |
| 61 | +`POST /api/v1/sharing/rules/:idOrName/evaluate` is a ledgered **SDK** route — |
| 62 | +`packages/rest/src/rest-route-ledger.ts:390`, the row carrying |
| 63 | +`disposition: 'sdk'` and `client: 'shares.rules.evaluate'` — and its REST handler |
| 64 | +passes the service return value through **unfiltered** |
| 65 | +(`packages/rest/src/rest-server.ts:11108`–`:11109`: |
| 66 | +`const result = await svc.evaluateRule(req.params.idOrName, context ?? {})` |
| 67 | +followed by `res.json(result)`). So the seventh key is on the response body every |
| 68 | +caller of that route already receives. The SDK method declares |
| 69 | +`SharingRuleEvaluationResult` as its resolved type |
| 70 | +(`packages/client/src/index.ts:4766`, unwrapped at `:4771` through |
| 71 | +`unwrapResponse` parameterised on that same type), and that type is the spec's |
| 72 | +six-field contract — so the **declared client type cannot name the seventh key**. |
| 73 | +That is a client-type **lag**, not a contract break: the key is additive on the |
| 74 | +wire, every declared field is unchanged, and a consumer typed against |
| 75 | +`SharingRuleEvaluationResult` keeps compiling exactly as before. Lifting the type |
| 76 | +is not this PR's to do — `SharingRuleEvaluationResult` lives in |
| 77 | +`@objectstack/spec`, a `domain:spec` single-owner file — so the lag is declared |
| 78 | +here and tracked as the follow-up #14969, which lifts `grantsRefused?: number` |
| 79 | +(optional) into `SharingRuleEvaluationResult`. |
0 commit comments