Skip to content

Commit 7f955e5

Browse files
os-zhuangclaude
andauthored
fix(sharing): 记录删除时撤销该记录的全部 sys_record_share(不分 source)+ boot 期按记录存在性的孤儿清扫 (#5103) (#5196)
* fix(sharing): revoke every share on a deleted record, not just rule grants (#5103) A `sys_record_share` row says "principal P has level L on (object O, record R)". Delete R and the row describes nothing — yet it stayed in the table forever. #4779 (PR #5102) bound an `afterDelete` for this, but inside the sharing-RULE package, where two conditions fenced it in: it revokes only `source: 'rule'` rows, and `bindRuleHooks` binds only on objects that appear in `sys_sharing_rule`. So an object using nothing but MANUAL shares had no delete hook at all, and manual share + record delete = a permanent orphan. Harm is bounded today only because record ids are never reused — an assumption no gate enforces. A custom primary key, an import preserving ids, or any future recycling turns those rows into real escalation: a new record on a recycled id inherits the dead record's recipients. Maintainer ruling (2026-08-04, on the issue): option A. Option B (a platform polymorphic weak-reference cascade) is a separate engine-lane design card (#5180); when it lands these hooks collapse into it. - `record-share-cascade.ts` binds ONE global `beforeDelete`/`afterDelete` pair and judges the object's sharing posture from `sharingModel` metadata PER DELETE. Nothing is enumerated at boot, so nothing goes stale — an object that gains sharing at runtime is covered on its next delete with no rebind, which is a stronger answer to the ruling's hot-update requirement than a metadata subscription would have been. Bounded row sets are revoked synchronously and set-based; an unbounded delete queues an object-scoped orphan sweep rather than the rule path's revoke-then-regrant, which is unavailable here because nothing can re-create a manual share. System-context deletes cascade too. - `SharingService.sweepOrphanedRecordShares` is the record-existence twin of `sweepOrphanedRuleGrants` (#4433) — that one asks whether the RULE row still exists and therefore can never see a manual share. Runs on `kernel:bootstrapped`, keyset-paged with one batched existence probe per object per page and a scan cap that reports itself. An object whose probe FAILS keeps its rows: "could not ask" is not "the record is gone". - The `beforeDelete` row-set stash moves from `rule-hooks.ts` into `bulk-recompute.ts` beside its resolver, so both hook packages share one answer per write instead of resolving the same predicate twice. Rule recompute still never touches a manual share (#5102's pin, re-asserted in this branch's tests). Only the record's DELETION revokes it, and only because there is no longer anything to have access to. Fixes #5103 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015W6nhsDrz6zWQc8je12a1t * fix(sharing): the deferred-sweep breadcrumb is info, not warn (#5103) The rule path warns on its unbounded branch because recipients visibly lose access to records they still qualify for until the re-grant lands. Nothing equivalent happens here: the sweep only removes rows whose record is gone, so a deferred reclaim takes nothing from a surviving record and has no user-visible consequence. A warn on every predicate delete would only erode the level (AGENTS.md's own caution against over-applying it). The sweep still warns when it actually revokes rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015W6nhsDrz6zWQc8je12a1t * docs(sharing): record why referential cascades are already covered (#5103) `cascadeDeleteRelations` removes a `deleteBehavior: 'cascade'` child through the public `delete()` rather than the driver, so a detail record swept away with its master reaches this hook like any other delete. That is the fact behind treating `controlled_by_parent` as sharing-capable: manual grants are refused there, but the rule evaluator can still materialise rows under system context, and this is the path that reclaims them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015W6nhsDrz6zWQc8je12a1t --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent e96ad55 commit 7f955e5

8 files changed

Lines changed: 1408 additions & 30 deletions

File tree

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
---
2+
"@objectstack/plugin-sharing": patch
3+
---
4+
5+
fix(sharing): deleting a record now revokes every `sys_record_share` row on it, whatever the source (#5103)
6+
7+
A share row says "principal P has level L on (object O, record R)". Delete R and
8+
the row describes nothing — yet until now it stayed in the table forever.
9+
10+
#4779 (PR #5102) bound an `afterDelete` for this, but inside the sharing-RULE
11+
package, where two conditions fenced it in: it revokes only `source: 'rule'`
12+
rows, and it binds only on objects that appear in `sys_sharing_rule`. So an
13+
object that uses nothing but MANUAL shares — a `sharingModel: 'private'` object
14+
with no rule ever configured — had no delete hook at all, and **manual share +
15+
record delete = a permanent orphan**.
16+
17+
Today the harm is bounded, and only because record ids are never reused: the
18+
`record_id IN (…)` predicate `buildReadFilter` emits matches nothing. Nothing
19+
enforces that assumption. A custom primary key, an import that preserves ids, or
20+
any future id recycling turns every one of those rows into a real privilege
21+
escalation — a new record landing on a recycled id inherits the dead record's
22+
recipients outright. Secondarily, `sys_record_share` grew without bound and
23+
Setup's Record Shares list showed rows pointing at nothing.
24+
25+
**What changed**
26+
27+
- **A record-delete cascade on every sharing-capable object.** `plugin-sharing`
28+
binds one `beforeDelete`/`afterDelete` pair with no object filter and judges
29+
the object's sharing posture from its `sharingModel` metadata *per delete*.
30+
Nothing is enumerated at boot, so nothing goes stale: an object that gains
31+
`sharingModel` at runtime is covered on its very next delete, with no rebind.
32+
Bounded deletes (a scalar id, an `$in` list, or a predicate matching at most
33+
1000 rows) are revoked synchronously and set-based; an unbounded one queues an
34+
object-scoped orphan sweep instead. System-context deletes cascade too.
35+
- **A boot-time orphan sweep keyed on record existence.** On
36+
`kernel:bootstrapped`, share rows whose RECORD no longer exists are revoked —
37+
historical orphans, rows a failed hook missed, and the one posture the cascade
38+
deliberately skips (an unmarked system object). This is a different question
39+
from the existing `sweepOrphanedRuleGrants`, which asks whether the RULE row
40+
still exists and therefore can never see a manual share. Bounded per boot:
41+
keyset pages, one batched existence probe per object per page, and a scan cap
42+
that reports when it stopped early. An object whose existence probe FAILS has
43+
its rows left in place — "could not ask" is never read as "the record is gone".
44+
45+
**What did not change**
46+
47+
Rule *recompute* still never touches a manual share. That boundary (#5102) is
48+
the point: while the record exists, a manual grant is a human decision no rule
49+
evaluation may overrule. Only the record's DELETION revokes it, and only because
50+
there is no longer anything to have access to.
51+
52+
New exports for hosts that compose the plugin by hand:
53+
`bindRecordShareCascade` / `unbindRecordShareCascade`,
54+
`objectCanCarryRecordShares`, `SharingService.revokeSharesForDeletedRecords`,
55+
`SharingService.sweepOrphanedRecordShares`, and `effectiveSharingModel`. Nothing
56+
was removed or renamed; the standard `SharingServicePlugin` composition needs no
57+
changes.

‎packages/plugins/plugin-sharing/src/bulk-recompute.ts‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,70 @@ export async function resolveAffectedRows(
194194
}
195195
}
196196

197+
/**
198+
* [#4779] Shared-`HookContext` key holding the row set the write is about to
199+
* change, stashed by the `before` hook for the `after` hook to consume.
200+
*
201+
* The stash is necessary, not a convenience: an update that moves rows OUT of
202+
* a rule's criteria makes them unfindable by the write's own predicate the
203+
* instant it lands, and a delete removes them outright — so `afterUpdate` /
204+
* `afterDelete` are structurally too late to ask "which rows was this?".
205+
* `ObjectQL.update()` / `.delete()` reuse ONE `HookContext` instance across
206+
* each before/after pair (they mutate `ctx.event` in place), which is the same
207+
* seam `primary-bu-projection.ts`'s `__primaryBuUserId` rides on.
208+
*
209+
* [#5103] Lives HERE, next to the resolver, rather than in `rule-hooks.ts`
210+
* where it started: two independent hook packages now need the same answer for
211+
* the same write (the rule recompute, and the record-delete share cascade),
212+
* and each resolving it separately would double the predicate query on every
213+
* bulk write for no gain — the row set is a property of the WRITE, not of
214+
* either subscriber.
215+
*/
216+
export const AFFECTED_ROWS_STASH_KEY = '__sharingAffectedRows';
217+
218+
/**
219+
* Resolve (or reuse) the row set a `before` hook's write is about to change and
220+
* park it on the shared `HookContext`.
221+
*
222+
* **Reuse is the point.** The first plugin-sharing `before` hook to run on a
223+
* write resolves; every later one reads that answer back. Recomputing would be
224+
* wasteful and — worse — could disagree, because a resolve issued after an
225+
* earlier hook has already changed something is answering a different question.
226+
*
227+
* Never throws: `resolveAffectedRows` already fails safe to `unbounded`, and
228+
* this adds the belt for a genuinely unexpected throw. "Unknown" must never
229+
* degrade to "no rows" — that is the direction that silently skips cleanup.
230+
*/
231+
export async function stashAffectedRows(
232+
engine: RecomputeEngine | { find?: RecomputeEngine['find'] },
233+
objectName: string,
234+
hookCtx: any,
235+
logger?: MinimalLogger,
236+
): Promise<AffectedRows> {
237+
const already = hookCtx?.[AFFECTED_ROWS_STASH_KEY] as AffectedRows | undefined;
238+
if (already) return already;
239+
let resolved: AffectedRows;
240+
try {
241+
resolved = typeof engine?.find === 'function'
242+
? await resolveAffectedRows(engine as RecomputeEngine, objectName, hookCtx, logger)
243+
: { kind: 'unbounded', reason: 'resolve-failed', detail: 'engine has no find()' };
244+
} catch (err: any) {
245+
resolved = { kind: 'unbounded', reason: 'resolve-failed', detail: err?.message };
246+
}
247+
if (hookCtx && typeof hookCtx === 'object') hookCtx[AFFECTED_ROWS_STASH_KEY] = resolved;
248+
return resolved;
249+
}
250+
251+
/**
252+
* What an `after` hook should act on. A missing stash means no `before` hook of
253+
* ours ran for this write, which is not "nothing changed" — it is "we do not
254+
* know", and it reads as `unbounded` so the caller takes its safe branch.
255+
*/
256+
export function readAffectedRows(hookCtx: any): AffectedRows {
257+
return (hookCtx?.[AFFECTED_ROWS_STASH_KEY] as AffectedRows | undefined)
258+
?? { kind: 'unbounded', reason: 'resolve-failed', detail: 'no before-hook stash' };
259+
}
260+
197261
/**
198262
* The asynchronous half of the ruling: re-grant after the synchronous revoke.
199263
*

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,11 @@ export { SysRecordShare, SysSharingRule, SysShareLink } from './objects/index.js
1313
export { SysBusinessUnit, SysBusinessUnitMember } from '@objectstack/platform-objects/identity';
1414
export {
1515
SharingService,
16+
effectiveSharingModel,
1617
type SharingEngine,
1718
type SharingServiceOptions,
19+
type OrphanShareSweepOptions,
20+
type OrphanShareSweepResult,
1821
} from './sharing-service.js';
1922
export {
2023
SharingRuleService,
@@ -43,10 +46,21 @@ export {
4346
RuleRegrantQueue,
4447
resolveAffectedRows,
4548
idsFromHookInput,
49+
stashAffectedRows,
50+
readAffectedRows,
51+
AFFECTED_ROWS_STASH_KEY,
4652
type AffectedRows,
4753
type UnboundedReason,
4854
type RecomputeEngine,
4955
} from './bulk-recompute.js';
56+
export {
57+
bindRecordShareCascade,
58+
unbindRecordShareCascade,
59+
objectCanCarryRecordShares,
60+
orphanShareSweepQueue,
61+
RECORD_SHARE_CASCADE_PACKAGE,
62+
type CascadeEngine,
63+
} from './record-share-cascade.js';
5064
export {
5165
parseCriteria,
5266
isMatchAllCriteria,

0 commit comments

Comments
 (0)