Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions .changeset/record-delete-share-cascade.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
---
"@objectstack/plugin-sharing": patch
---

fix(sharing): deleting a record now revokes every `sys_record_share` row on it, whatever the source (#5103)

A share row says "principal P has level L on (object O, record R)". Delete R and
the row describes nothing — yet until now 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 it binds only on objects that appear in `sys_sharing_rule`. So an
object that uses nothing but MANUAL shares — a `sharingModel: 'private'` object
with no rule ever configured — had no delete hook at all, and **manual share +
record delete = a permanent orphan**.

Today the harm is bounded, and only because record ids are never reused: the
`record_id IN (…)` predicate `buildReadFilter` emits matches nothing. Nothing
enforces that assumption. A custom primary key, an import that preserves ids, or
any future id recycling turns every one of those rows into a real privilege
escalation — a new record landing on a recycled id inherits the dead record's
recipients outright. Secondarily, `sys_record_share` grew without bound and
Setup's Record Shares list showed rows pointing at nothing.

**What changed**

- **A record-delete cascade on every sharing-capable object.** `plugin-sharing`
binds one `beforeDelete`/`afterDelete` pair with no object filter and judges
the object's sharing posture from its `sharingModel` metadata *per delete*.
Nothing is enumerated at boot, so nothing goes stale: an object that gains
`sharingModel` at runtime is covered on its very next delete, with no rebind.
Bounded deletes (a scalar id, an `$in` list, or a predicate matching at most
1000 rows) are revoked synchronously and set-based; an unbounded one queues an
object-scoped orphan sweep instead. System-context deletes cascade too.
- **A boot-time orphan sweep keyed on record existence.** On
`kernel:bootstrapped`, share rows whose RECORD no longer exists are revoked —
historical orphans, rows a failed hook missed, and the one posture the cascade
deliberately skips (an unmarked system object). This is a different question
from the existing `sweepOrphanedRuleGrants`, which asks whether the RULE row
still exists and therefore can never see a manual share. Bounded per boot:
keyset pages, one batched existence probe per object per page, and a scan cap
that reports when it stopped early. An object whose existence probe FAILS has
its rows left in place — "could not ask" is never read as "the record is gone".

**What did not change**

Rule *recompute* still never touches a manual share. That boundary (#5102) is
the point: while the record exists, a manual grant is a human decision no rule
evaluation may overrule. Only the record's DELETION revokes it, and only because
there is no longer anything to have access to.

New exports for hosts that compose the plugin by hand:
`bindRecordShareCascade` / `unbindRecordShareCascade`,
`objectCanCarryRecordShares`, `SharingService.revokeSharesForDeletedRecords`,
`SharingService.sweepOrphanedRecordShares`, and `effectiveSharingModel`. Nothing
was removed or renamed; the standard `SharingServicePlugin` composition needs no
changes.
64 changes: 64 additions & 0 deletions packages/plugins/plugin-sharing/src/bulk-recompute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,70 @@ export async function resolveAffectedRows(
}
}

/**
* [#4779] Shared-`HookContext` key holding the row set the write is about to
* change, stashed by the `before` hook for the `after` hook to consume.
*
* The stash is necessary, not a convenience: an update that moves rows OUT of
* a rule's criteria makes them unfindable by the write's own predicate the
* instant it lands, and a delete removes them outright — so `afterUpdate` /
* `afterDelete` are structurally too late to ask "which rows was this?".
* `ObjectQL.update()` / `.delete()` reuse ONE `HookContext` instance across
* each before/after pair (they mutate `ctx.event` in place), which is the same
* seam `primary-bu-projection.ts`'s `__primaryBuUserId` rides on.
*
* [#5103] Lives HERE, next to the resolver, rather than in `rule-hooks.ts`
* where it started: two independent hook packages now need the same answer for
* the same write (the rule recompute, and the record-delete share cascade),
* and each resolving it separately would double the predicate query on every
* bulk write for no gain — the row set is a property of the WRITE, not of
* either subscriber.
*/
export const AFFECTED_ROWS_STASH_KEY = '__sharingAffectedRows';

/**
* Resolve (or reuse) the row set a `before` hook's write is about to change and
* park it on the shared `HookContext`.
*
* **Reuse is the point.** The first plugin-sharing `before` hook to run on a
* write resolves; every later one reads that answer back. Recomputing would be
* wasteful and — worse — could disagree, because a resolve issued after an
* earlier hook has already changed something is answering a different question.
*
* Never throws: `resolveAffectedRows` already fails safe to `unbounded`, and
* this adds the belt for a genuinely unexpected throw. "Unknown" must never
* degrade to "no rows" — that is the direction that silently skips cleanup.
*/
export async function stashAffectedRows(
engine: RecomputeEngine | { find?: RecomputeEngine['find'] },
objectName: string,
hookCtx: any,
logger?: MinimalLogger,
): Promise<AffectedRows> {
const already = hookCtx?.[AFFECTED_ROWS_STASH_KEY] as AffectedRows | undefined;
if (already) return already;
let resolved: AffectedRows;
try {
resolved = typeof engine?.find === 'function'
? await resolveAffectedRows(engine as RecomputeEngine, objectName, hookCtx, logger)
: { kind: 'unbounded', reason: 'resolve-failed', detail: 'engine has no find()' };
} catch (err: any) {
resolved = { kind: 'unbounded', reason: 'resolve-failed', detail: err?.message };
}
if (hookCtx && typeof hookCtx === 'object') hookCtx[AFFECTED_ROWS_STASH_KEY] = resolved;
return resolved;
}

/**
* What an `after` hook should act on. A missing stash means no `before` hook of
* ours ran for this write, which is not "nothing changed" — it is "we do not
* know", and it reads as `unbounded` so the caller takes its safe branch.
*/
export function readAffectedRows(hookCtx: any): AffectedRows {
return (hookCtx?.[AFFECTED_ROWS_STASH_KEY] as AffectedRows | undefined)
?? { kind: 'unbounded', reason: 'resolve-failed', detail: 'no before-hook stash' };
}

/**
* The asynchronous half of the ruling: re-grant after the synchronous revoke.
*
Expand Down
14 changes: 14 additions & 0 deletions packages/plugins/plugin-sharing/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@ export { SysRecordShare, SysSharingRule, SysShareLink } from './objects/index.js
export { SysBusinessUnit, SysBusinessUnitMember } from '@objectstack/platform-objects/identity';
export {
SharingService,
effectiveSharingModel,
type SharingEngine,
type SharingServiceOptions,
type OrphanShareSweepOptions,
type OrphanShareSweepResult,
} from './sharing-service.js';
export {
SharingRuleService,
Expand Down Expand Up @@ -43,10 +46,21 @@ export {
RuleRegrantQueue,
resolveAffectedRows,
idsFromHookInput,
stashAffectedRows,
readAffectedRows,
AFFECTED_ROWS_STASH_KEY,
type AffectedRows,
type UnboundedReason,
type RecomputeEngine,
} from './bulk-recompute.js';
export {
bindRecordShareCascade,
unbindRecordShareCascade,
objectCanCarryRecordShares,
orphanShareSweepQueue,
RECORD_SHARE_CASCADE_PACKAGE,
type CascadeEngine,
} from './record-share-cascade.js';
export {
parseCriteria,
isMatchAllCriteria,
Expand Down
Loading
Loading