Skip to content

Commit c7a60e1

Browse files
fix(plugin-security): explain credits read depth, not sharing, for a row only the depth admits (#21754)
Fixes #21726 Clause-②: no The explain answer now produces `decidedBy: 'depth'`, a member the published `decidedBy` enum (`packages/spec/src/security/explain.zod.ts`, about `:331`-`:341`) already declares. No access decision changes. ## What changed All runtime changes are in `packages/plugins/plugin-security/src/explain-engine.ts`, in the record-grained pass (`applyRecordAttribution`). - **A row only read depth admits now explains as `decidedBy: 'depth'`.** The decision tree ranks `depth` with `sharing`: after a decisive bypass, after a share, and before `rls`. These are the two layers that widen a private baseline past its owner. - **The `depth` layer carries a `record` block on record-grained requests.** It is `admitted` when the read depth admits the row, and it names the depth and, below `org`, the owner it reached. It is `excluded` when the owner is outside the depth. It is `not_evaluated` with a reason when the depth adds nothing (`own`, a non-private baseline, no record), when the depth cannot be separated (a share admits the row too, below `org`), or when the depth is not in play (an on-behalf-of read, a write). - **The sharing layer reports `admitted` only when a share or rule matched.** For a row that only the depth admits, it is `not_evaluated`, and its detail says that no share grants the row and that read depth already admits it. - **The sharing detail states a grant only when one exists.** "N share(s) attached; access is granted for this record." is kept for a share that names the caller. When the sharing filter admits a row with no such share, the detail names the part of the sharing service that let it through. - `visible` comes from the same disjunction as before. `depthAdmitsRow` is the part of the old `sharingMatches === true` admission that the sharing layer no longer claims, so `sharingAdmitted || depthAdmitsRow` equals the old `sharingAdmitted`. ## How the depth admission is judged (measured, not re-derived) The sharing read filter is one predicate with two grants: the owner-match, which read depth widens, OR-ed with the ids of the caller's own shares. Measured in plugin-sharing's `buildReadFilter`: at `org` depth it returns `null` and composes no owner-match at all. Below `org` it returns the owner-match over the depth's owners, OR-ed with `id IN (granted ids)` when the caller holds shares. The engine does not parse that shape and does not call the filter a second time. It reads facts the pass already holds: - the depth the `depth` layer published (`depthScope`); - ownership; - the share rows naming the caller (the grant disjunct's population); - whether the filter matched. Two cases are deliberately not credited to the depth. On an on-behalf-of read, `security-plugin.ts` hands the filter the delegated context with no depth stamped. Below `org`, a `null` filter means the sharing service imposes no owner-match on that object. ## Precedence when a share and the depth both admit This case is not named on the card. `decidedBy`'s published contract decides it: "the layer whose `record.outcome` excluded it, or the last one to admit it" (`explain.zod.ts` about `:390`, and the `.describe` at `:342`). In pipeline order, depth comes before sharing, so a row a share admits keeps `decidedBy: 'sharing'`. That is also the answer before this change. The hand-written explain page (`content/docs/permissions/explain.mdx`) does not describe record verdict precedence, so no docs sentence became false. ## Tests Tests were added or updated in `plugin-security`: - `explain-engine.test.ts`, unit harness: - org depth and unit depth explain as `depth`, with a depth record block; - an owner outside the unit is excluded by both layers; - a criteria rule's share still explains as `sharing` with exactly "1 share(s) attached; access is granted for this record."; - share plus depth, at org and at unit; - the two guards (a null filter below org, and an on-behalf-of read). - `explain-engine.test.ts`, enumeration: `decidedBy` is read from `ExplainDecisionSchema` in `@objectstack/spec/security`. The test fails by name for any member that no scenario produces and no ledger entry explains. Today `principal` and `fls` are ledgered with reasons, and every other member is produced by a scenario that drives its branch. - `explain-read-verdict-inputs.test.ts`, real SecurityPlugin plus real SharingService: - decider cells: depth for org and unit readers; sharing for a manual share, a rule-sourced share row, and share plus depth; owd_baseline for the owner; - the depth record block; - the rule row's exact detail. The existing test that asserted the sharing layer `admitted` an org-depth row was re-judged and replaced: it pinned the behaviour this change removes. Each cell still asserts explain `visible` equals the find's outcome. - `explain-dependency-fault.test.ts`: the "a null filter is an answer, not a fault" test now asserts `decidedBy: 'depth'`, with the filter published and judged. A faulted layer publishes neither. Runs at head `8998a647`: - `pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2` (full package): 164 files, 3562 passed, 45 skipped, 0 failed. - `pnpm --filter @objectstack/plugin-security typecheck`: exit 0. This includes `check:test-typecheck` over `tsconfig.test.json`, which compiles the test layer. ## Ablations (each red, then restored and proven: blob equals HEAD, `git diff HEAD` empty) - A0, the whole file put back at the merge base (`ff29410e`): 15 failed out of 161 in the three explain files. Examples: `expected 'sharing' to be 'depth'`, and `expected '0 share(s) attached; access is grante…' to contain …`. - A1, `depthAdmitsRow` forced false: 9 red. These are every depth decider cell and the `'depth' is produced` enumeration case. - A2, the detail names a grant without a share: 2 red (the null-filter and on-behalf-of guards' detail assertions). - A3, the on-behalf-of guard removed: 1 red. - A4, the null-below-org guard removed: 1 red. - A5, the `fls` ledger entry renamed: 1 red, with `decidedBy member 'fls' is produced by no branch and carries no ledger reason`. The subject resolves from `src` (relative imports), so no dist rebuild was involved. ## Gates `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derived 66 commands from this branch, and all 66 ran at `8998a647` with exit 0. `check:i18n` and `check:dual-build-cjs-loads` first answered PREREQUISITE NOT MET (exit 3). Each was re-run green after its stated build (`turbo` cache hits). The `--ran` reconciliation: 66 derived, 66 run, 0 NOT-MEASURED, 0 UNRUN. Lint was narrowed to the 4 touched `.ts` files. ① They are in the population of `eslint.config.mjs`'s `packages/**` objects (`--print-config` resolves for each). ② `--format json` reports 4 files, 0 errors, 0 warnings. ③ The config enables no type-aware linting (no `parserOptions.project`, no typed rules), so this diff cannot move any untouched file's verdict. ## Acceptance notes - **The write path has the same attribution shape and is left as it is.** For `update`/`delete`, the row verdict is the sharing service's per-record gate, which judges write depth, ownership and shares together. A write admitted by write depth still reads `decidedBy: 'sharing'`, with the gate detail naming ownership or a share. The new `depth` record block on a write says that this report does not separate the write depth. Carrier: none named. - **The owner's row keeps its sharing-layer detail**, "Caller owns the record — visible without a share." It still decides as `owd_baseline`, which was already the case. --- _Generated by [Claude Code](https://claude.ai/code/session_011K3zqE8Pv1Evw5hc8tZCnN)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 1c3a4d9 commit c7a60e1

5 files changed

Lines changed: 398 additions & 25 deletions

File tree

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
'@objectstack/plugin-security': patch
3+
---
4+
5+
`POST /api/v1/security/explain` with `{ object, operation: 'read', recordId }` now credits read depth for a row that only the caller's read depth admits.
6+
7+
Clause-②: no
8+
9+
- **Before.** The object's OWD is private. The caller's read depth is wider than `own` (`own_and_reports`, `unit`, `unit_and_below` or `org`). The caller reads a row they do not own and hold no share on. Explain answered `decidedBy: 'sharing'` for that row, with the sharing layer `admitted` and the detail "0 share(s) attached; access is granted for this record." No share granted anything; the read depth did. `depth` is a member of the published `decidedBy` enum, and no answer ever produced it.
10+
- **Now.** That row answers `decidedBy: 'depth'`. The `depth` layer carries a `record` block: `admitted`, naming the depth and, below `org`, the owner it reached. The sharing layer is `not_evaluated`, and its detail says that no share grants the row and that read depth already admits it.
11+
- **The sharing detail names a grant only when one exists**, meaning a share that names the caller. A filter that admits a row with no such share now says which part of the sharing service let the row through.
12+
- **A row a share admits keeps `decidedBy: 'sharing'`** and its "N share(s) attached; access is granted for this record." detail, also when the read depth admits it too. Of the two layers that admit it, sharing comes last in the pipeline, and the `decidedBy` contract names the last layer to admit.
13+
- **No access decision changes.** `visible` is the same for every record; only the layer the report credits moves. No key is added. `depth` and `not_evaluated` are existing values.
14+
- **The `depth` layer's `record` block is new on every record-grained request.** On a write it is `not_evaluated`, because the write depth is judged inside the sharing service's per-record write gate together with ownership and shares, and this report does not separate them.

‎packages/plugins/plugin-security/src/explain-dependency-fault.test.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -359,10 +359,16 @@ describe('healthy sharing layer — reported exactly as before', () => {
359359
const stack = await makeStack({ storeFaults: ['sys_record_share'] });
360360
const decision = await stack.explain('read', 'unshared', HR_CTX);
361361
expect((await stack.request('read', 'unshared', HR_CTX)).ok).toBe(true);
362-
expect(decision.record.visible).toBe(true);
362+
// The org depth admits the row, so the report credits the depth layer;
363+
// a fault would have answered `visible: false`, decided by sharing.
364+
expect(decision.record).toEqual({ recordId: ROWS.unshared.id, visible: true, decidedBy: 'depth' });
363365
const sharingRecord = layerOf(decision, 'sharing').record;
364-
expect(sharingRecord.outcome).toBe('admitted');
366+
// An answer, not a fault: the filter is published (null, unrestricted) and
367+
// judged against the row. A faulted layer publishes neither.
365368
expect(sharingRecord.rowFilter).toBeNull();
369+
expect(sharingRecord.matchesRecord).toBe(true);
370+
expect(sharingRecord.detail).not.toContain('could not be evaluated');
371+
expect(layerOf(decision, 'depth').record.outcome).toBe('admitted');
366372
});
367373
});
368374

‎packages/plugins/plugin-security/src/explain-engine.test.ts‎

Lines changed: 173 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
import { describe, it, expect } from 'vitest';
55
import { resolveUserAuthzGrants, resetPlatformAdminEmailMemo } from '@objectstack/core';
6-
import { PermissionSetSchema } from '@objectstack/spec/security';
6+
import { ExplainDecisionSchema, PermissionSetSchema } from '@objectstack/spec/security';
77
import { PermissionEvaluator } from './permission-evaluator';
88
import { explainAccess, buildContextForUser, resolveDelegatorContext, type ExplainEngineDeps } from './explain-engine';
99
import { RLS_DENY_FILTER } from './rls-compiler';
@@ -594,6 +594,178 @@ describe('explainAccess — record-grained (C2 / ADR-0095)', () => {
594594
expect(d.record).toMatchObject({ visible: true, decidedBy: 'vama_bypass' });
595595
});
596596

597+
// ── read depth, at row granularity ──────────────────────────────────────
598+
// The sharing read filter is one predicate carrying two grants: the
599+
// owner-match the read depth widens, OR-ed with the caller's own shares. A
600+
// row the filter admits with no share and no ownership behind it was admitted
601+
// by the depth, and the report names the depth layer, not sharing.
602+
describe('read depth admits a row that no share does', () => {
603+
const ORG_READER = PermissionSetSchema.parse({
604+
name: 'org_reader',
605+
objects: { leave_request: { allowRead: true, readScope: 'org' } },
606+
});
607+
const NOT_MINE = { id: 'r1', organization_id: 'org1', owner_id: 'u_other' };
608+
const RULE_SHARE = {
609+
id: 'shr_rule', recipient_type: 'user', recipient_id: 'u1', access_level: 'read' as const,
610+
source: 'rule', source_id: 'rule_open_requests',
611+
};
612+
const layerRecord = (d: { layers: Array<{ layer: string; record?: any }> }, layer: string) =>
613+
d.layers.find((l) => l.layer === layer)!.record!;
614+
const read = (deps: ExplainEngineDeps, context: any = REC_CTX) =>
615+
explainAccess(deps, { object: 'leave_request', operation: 'read', context, recordId: 'r1' });
616+
617+
it("org depth: the row explains as decidedBy 'depth', with a depth-layer record block", async () => {
618+
// At org depth plugin-sharing composes no owner-match: the filter is null.
619+
const d = await read(recDeps({ sets: [ORG_READER], record: NOT_MINE, shares: [], sharingFilter: null }));
620+
expect(d.record).toMatchObject({ visible: true, decidedBy: 'depth' });
621+
const depth = layerRecord(d, 'depth');
622+
expect(depth.outcome).toBe('admitted');
623+
expect(depth.detail).toContain("read depth 'org'");
624+
const sharing = layerRecord(d, 'sharing');
625+
expect(sharing.outcome).not.toBe('admitted');
626+
expect(sharing.detail).not.toContain('access is granted');
627+
expect(sharing.detail).toContain('see the depth layer');
628+
});
629+
630+
it("unit depth: an owner inside the caller's unit explains as 'depth', naming the owner", async () => {
631+
const d = await read(recDeps({ record: NOT_MINE, shares: [], sharingFilter: { owner_id: { $in: ['u1', 'u_other'] } } }));
632+
expect(d.record).toMatchObject({ visible: true, decidedBy: 'depth' });
633+
expect(layerRecord(d, 'depth').outcome).toBe('admitted');
634+
expect(layerRecord(d, 'depth').detail).toContain("owner (u_other) is inside the caller's 'unit' read depth");
635+
expect(layerRecord(d, 'sharing').outcome).toBe('not_evaluated');
636+
});
637+
638+
it("unit depth: an owner outside the caller's unit is excluded by the depth layer too", async () => {
639+
const d = await read(recDeps({ record: NOT_MINE, shares: [], sharingFilter: { owner_id: { $in: ['u1'] } } }));
640+
expect(d.record).toMatchObject({ visible: false, decidedBy: 'sharing' });
641+
expect(layerRecord(d, 'depth').outcome).toBe('excluded');
642+
expect(layerRecord(d, 'sharing').outcome).toBe('excluded');
643+
});
644+
645+
it("a criteria rule's share still explains as 'sharing', with '1 share(s) attached'", async () => {
646+
// A criteria sharing rule materialises as a `sys_record_share` row with
647+
// `source: 'rule'` naming the matched user (sharing-rule-service.ts).
648+
const d = await read(recDeps({
649+
record: NOT_MINE,
650+
shares: [RULE_SHARE],
651+
sharingFilter: { $or: [{ owner_id: { $in: ['u1'] } }, { id: { $in: ['r1'] } }] },
652+
}));
653+
expect(d.record).toMatchObject({ visible: true, decidedBy: 'sharing' });
654+
const sharing = layerRecord(d, 'sharing');
655+
expect(sharing.outcome).toBe('admitted');
656+
expect(sharing.detail).toBe('1 share(s) attached; access is granted for this record.');
657+
expect(sharing.rules?.[0]).toMatchObject({ kind: 'sharing_rule', name: 'rule_open_requests', effect: 'admits' });
658+
});
659+
660+
it("a share AND org depth: 'sharing' decides, the last layer to admit; the depth layer admits as well", async () => {
661+
const d = await read(recDeps({ sets: [ORG_READER], record: NOT_MINE, shares: [RULE_SHARE], sharingFilter: null }));
662+
expect(d.record).toMatchObject({ visible: true, decidedBy: 'sharing' });
663+
expect(layerRecord(d, 'depth').outcome).toBe('admitted');
664+
});
665+
666+
it("a share AND unit depth: 'sharing' decides; the depth layer says it cannot separate the two", async () => {
667+
const d = await read(recDeps({
668+
record: NOT_MINE,
669+
shares: [RULE_SHARE],
670+
sharingFilter: { $or: [{ owner_id: { $in: ['u1', 'u_other'] } }, { id: { $in: ['r1'] } }] },
671+
}));
672+
expect(d.record).toMatchObject({ visible: true, decidedBy: 'sharing' });
673+
expect(layerRecord(d, 'depth').outcome).toBe('not_evaluated');
674+
expect(layerRecord(d, 'depth').detail).toContain('not separated');
675+
});
676+
677+
it('below org, a null filter is the sharing service imposing nothing — not the depth', async () => {
678+
const d = await read(recDeps({ record: NOT_MINE, shares: [], sharingFilter: null }));
679+
expect(d.record).toMatchObject({ visible: true, decidedBy: 'sharing' });
680+
const sharing = layerRecord(d, 'sharing');
681+
expect(sharing.detail).toContain('imposes no owner-match');
682+
expect(sharing.detail).not.toContain('access is granted');
683+
expect(layerRecord(d, 'depth').outcome).toBe('not_evaluated');
684+
});
685+
686+
it('an on-behalf-of read never credits the depth: its filter is asked with no depth stamped', async () => {
687+
const d = await read(
688+
{ ...recDeps({ record: NOT_MINE, shares: [], sharingFilter: { owner_id: { $in: ['u1', 'u_other'] } } }), ql: DELEGATOR_QL },
689+
{ ...REC_CTX, onBehalfOf: { userId: 'u_boss' } },
690+
);
691+
expect(d.record?.visible).toBe(true);
692+
expect(d.record?.decidedBy).not.toBe('depth');
693+
expect(layerRecord(d, 'depth').outcome).toBe('not_evaluated');
694+
expect(layerRecord(d, 'sharing').detail).not.toContain('access is granted');
695+
});
696+
});
697+
698+
// ── every published decider is reachable ────────────────────────────────
699+
// The enum is read from the spec, never restated here: a member the spec
700+
// gains, and no branch of the decision tree produces, fails by name until a
701+
// scenario produces it or the ledger below says why none can.
702+
describe('decision.record.decidedBy: each published member is produced by a branch, or ledgered', () => {
703+
const DECIDED_BY: readonly string[] =
704+
ExplainDecisionSchema.shape.record.unwrap().shape.decidedBy.unwrap().options;
705+
const VIEW_ALL = PermissionSetSchema.parse({
706+
name: 'compliance_auditor',
707+
objects: { '*': { allowRead: true, viewAllRecords: true } },
708+
});
709+
const CAPS_GATED = {
710+
isPrivate: false,
711+
requiredPermissions: { all: [], read: ['manage_metadata'], create: [], update: [], delete: [] },
712+
fieldRequiredPermissions: {},
713+
};
714+
const NOT_MINE = { id: 'r1', organization_id: 'org1', owner_id: 'u_other' };
715+
const ask = (deps: ExplainEngineDeps, operation: 'read' | 'delete' = 'read') =>
716+
explainAccess(deps, { object: 'leave_request', operation, context: REC_CTX, recordId: 'r1' });
717+
718+
const PRODUCERS: Record<string, () => Promise<{ record?: { decidedBy?: string } }>> = {
719+
tenant_isolation: () => ask(recDeps({
720+
layered: { layer0: { organization_id: 'org1' }, layer1: null },
721+
record: { ...NOT_MINE, organization_id: 'org2' },
722+
})),
723+
required_permissions: () => ask({ ...recDeps(), getObjectSecurityMeta: async () => CAPS_GATED }),
724+
object_crud: () => ask(recDeps(), 'delete'),
725+
owd_baseline: () => ask(recDeps({ record: { id: 'r1', organization_id: 'org1', owner_id: 'u1' } })),
726+
depth: () => ask(recDeps({ record: NOT_MINE, shares: [], sharingFilter: { owner_id: { $in: ['u1', 'u_other'] } } })),
727+
sharing: () => ask(recDeps({
728+
record: NOT_MINE,
729+
shares: [{ id: 'shr_1', recipient_type: 'user', recipient_id: 'u1', access_level: 'read', source: 'manual' }],
730+
sharingFilter: { $or: [{ owner_id: { $in: ['u1'] } }, { id: { $in: ['r1'] } }] },
731+
})),
732+
vama_bypass: () => ask(recDeps({ sets: [VIEW_ALL], record: NOT_MINE, shares: [], sharingFilter: { owner_id: 'u1' } })),
733+
rls: () => ask(recDeps({
734+
layered: { layer0: null, layer1: { status: 'open' } },
735+
record: { ...NOT_MINE, owner_id: 'u1', status: 'closed' },
736+
})),
737+
};
738+
const NOT_PRODUCED: Record<string, string> = {
739+
principal:
740+
'The principal layer resolves who is asking and which sets they hold; it never judges a row. A principal ' +
741+
'whose grants fail to resolve (a missing or unresolvable delegator) is refused at object_crud, which is ' +
742+
'the layer the tree names for it.',
743+
fls:
744+
'Field-level security masks fields of a row that is served; it never admits or removes the row, so it ' +
745+
'cannot decide whether the row is visible.',
746+
};
747+
748+
it('the producers and the ledger partition the enum exactly', () => {
749+
for (const member of DECIDED_BY) {
750+
const produced = member in PRODUCERS;
751+
const ledgered = member in NOT_PRODUCED;
752+
expect(produced || ledgered, `decidedBy member '${member}' is produced by no branch and carries no ledger reason`).toBe(true);
753+
expect(produced && ledgered, `decidedBy member '${member}' is both produced and ledgered`).toBe(false);
754+
}
755+
for (const key of [...Object.keys(PRODUCERS), ...Object.keys(NOT_PRODUCED)]) {
756+
expect(DECIDED_BY, `'${key}' is not a member of the published decidedBy enum`).toContain(key);
757+
}
758+
for (const [member, reason] of Object.entries(NOT_PRODUCED)) {
759+
expect(reason.trim().length, `the ledger entry for '${member}' carries no reason`).toBeGreaterThan(0);
760+
}
761+
});
762+
763+
it.each(Object.keys(PRODUCERS))("'%s' is produced by the branch its scenario drives", async (member) => {
764+
const d = await PRODUCERS[member]();
765+
expect(d.record?.decidedBy).toBe(member);
766+
});
767+
});
768+
597769
it('degrades gracefully with no record-grained deps — object-level layers plus a best-effort verdict', async () => {
598770
// Only the base object-level deps: recordId is given but fetchRecord /
599771
// computeLayeredRlsFilter etc. are absent (e.g. no plugin-sharing).

0 commit comments

Comments
 (0)