Skip to content

Commit 55cd8d4

Browse files
fix(plugin-security): explain's record update/delete verdict uses the by-id write path's inputs (#19984)
Fixes #19963 Clause-②: no ## What changes `POST /api/v1/security/explain` with `{ object, operation: 'update' | 'delete', recordId }` now builds its record verdict from the by-id write path's own inputs. `decision.record.visible` equals what the by-id `PATCH` / `DELETE` does for the same caller. Enforcement admits and refuses exactly what it did before. Only the explain report changes. Landing: `packages/plugins/plugin-security/src/security-plugin.ts` (the `explainAccessForCaller` wiring, plus one helper extracted from the step-2.7 gate), a new test file, the changeset, and three generated rows in `scripts/engine-double-contract.pinned.json` (see "Surface note"). ## The two mechanisms, reproduced on `origin/main` `2c1011b01b` before the change The old bindings, quoted from `2c1011b01b`: - `computeLayeredRlsFilter: (sets, o, engineOp, c) => this.computeLayeredRlsFilter(sets as any, o, engineOp, c)`: no options, so the platform floor stays (M1). - `canEditRecord: (o: string, rid: string, c: any) => sharing.canEdit(o, rid, c)`: bare context, no `__writeScope` (M2). `canDeleteRecord` had the same shape. One-variable causal legs. Each ran on the unfixed tree through the real `SecurityPlugin`, the real `SharingService` and sharing middleware, and the real platform `member_default`. The object has an unset OWD (private). | leg | the one variable | explain `record` | by-id PATCH | |:--|:--|:--|:--| | M1: `dept_reporter` × edit-shared row | `created_by` = admin | `visible: false`, `decidedBy: rls` | admitted | | M1: same principal, row, share and owner | `created_by` = `dept_reporter` | `visible: true` | admitted | | M2: `hr_reviewer` (write depth `org`, no `org_member`, so the floor is out of play) × unshared row | bare context | `visible: false`, `decidedBy: sharing` | admitted | | M2: same call | `__writeScope: 'org'` stamped on the context | `visible: true` | admitted | Delete was unmeasured on the card, and it has the same shape. On `2c1011b01b`, `hr_reviewer` × unshared and `dept_reporter` × owned (owner, not creator) both read `visible: false` (`rls`), while the by-id DELETE admitted. This PR covers delete too. ## The change - **`resolvePreImageFloorDrop(rlsOperation, object, recordId, context, sets, delegated)`** is step 2.7's floor decision, extracted clause for clause. The floor must be in play, the sharing tri-state verdict must be `allow`, and the on-behalf-of path is excluded. The gate calls it with the arguments it used before. The whole plugin-security suite stays green (2341 tests), including the #5492 / #8757 / ADR-0055 composition suites that drive this gate. - **Explain's `computeLayeredRlsFilter` binding** passes `{ dropPlatformOwnershipFloor }` from that helper for `update` / `delete`. It does not pass `masterGateCoversThisWrite`: that option vouches that ADR-0055's master gate runs after the filter, and explain runs no master gate. An unanswerable decision keeps the floor standing. - **Explain's `canEditRecord` / `canDeleteRecord` bindings** stamp `__writeScope` from `resolveWriteScopeForSharing`. The stamp always overwrites the context's value, the way step 2.6 overwrites it. - **On-behalf-of contexts keep their previous inputs** on both halves (see "Acceptance notes"). ## Tests: `explain-write-verdict-inputs.test.ts` (25 cases) Every cell asserts that `record.visible` equals the write's outcome. An admitted write must really change the row. A refused write is checked by its ADR-0112 `code` + `status`: `PERMISSION_DENIED`/403 for the pre-image gate, `FORBIDDEN`/403 for the sharing gate. - update matrix, private OWD: builtin admin × shared (control); `hr_reviewer` × shared / owned / unshared; `dept_reporter` × edit-shared, × owned-not-created. Negative control: `dept_reporter` × unshared, where both deny. - delete matrix: `hr_reviewer` × unshared and `dept_reporter` × owned admit. `dept_reporter` × edit-shared (ADR-0111 D3) and × unshared are refused. - M1: `created_by` moved only, and the verdict must not move. - M2: an `org` writer with no floor is admitted. A `__writeScope` that the caller brings does not decide the verdict (negative pin). - OWD control `public_read_write`: all nine principal × row cells admit on both sides. Unfixed tree `2c1011b01b`: `Tests 10 failed | 15 passed (25)`. Fixed: `Tests 25 passed (25)`. ## Ablations The fix was committed first (`81dba2597`). Each mutation used `scripts/ablation-replace.mjs`: the anchor went from 1 hit to 0, and the blob changed on disk. After each restore the blob matched HEAD (`be403e07a5ae`) and `git diff HEAD` was empty. A driver trap also re-verified both. | ablation | mutation | result | |:--|:--|:--| | A1 (M1) | old `computeLayeredRlsFilter` binding, no options | `8 failed / 17 passed`: `hr_reviewer` × 3 update, `dept_reporter` shared and owned update, 2 delete cells, the M1 created-by-admin leg. The M2 pins stay green. | | A2 (M2) | old `canEditRecord` binding, bare context | `5 failed`: `hr_reviewer` × 3 update (`decidedBy: sharing`), the M2 `org`-writer pin, and the forged-stamp negative pin (explain `true`, PATCH refused). | | A3 (delete) | old `canDeleteRecord` binding | `1 failed`: `hr_reviewer` × unshared delete. | | A4 (every negative pin) | explain forced wide: `__writeScope: 'org'` stamped and the floor always dropped | `5 failed`: the update negative control (both cases), delete × edit-shared, delete × unshared, and the forged stamp. | The first A4 attempt was a no-op on its second mutation. The driver read that anchor file after a `cd`, and `ablation-replace` refused the empty anchor with exit 2. The re-run with absolute paths is the row above. The test imports `./security-plugin.js` from source, so there is no `dist/` hop and `ablation-dist-preflight` does not apply. ## Local verification: final head `aa9013ffb` - `pnpm --filter @objectstack/plugin-security build` exit 0. `typecheck` exit 0: the new test is in `tsconfig.test.json`'s program (`--listFiles`: 1 hit). - Full plugin-security `vitest run`: `Test Files 123 passed`, `Tests 2341 passed`. This ran at `81dba2597`; `aa9013ffb` adds only the ledger rows. - `dispatch-gates --commands` union at `aa9013ffb`, reconciled with `--ran`: 70 derived, 69 ran with exit 0, 1 NOT MEASURED. `check:dual-build-cjs-loads` exits 3 (PREREQUISITE NOT MET): it needs every workspace package's `dist/`, and CI's Build Core owns that. Four more roster gates keep their roster under a directory this diff touches: `check-changeset-fixed`, `check:authz-resolver`, `check:error-code-casing`, `check:filter-alias-parity`. All four exit 0. - Narrowed eslint (`--no-inline-config --format json`) over the two changed TS files: 2 files, 0 errors, 0 warnings. Both files are in the population of `eslint.config.mjs`'s `**/*.{ts,…}` block. That config enables no type-aware linting (no `parserOptions.project`), so this diff cannot move the verdict of any untouched file. - `origin/main` was still `2c1011b01b` when this PR opened, so the pre-PR merge was a no-op. ## Surface note `scripts/engine-double-contract.pinned.json` (+15 lines: the new test file's `delete` / `findOne` / `update` doubles) was written by `node scripts/check-engine-double-contract.mjs --write`, which is what that gate prescribes for new pinned doubles ("0 seams added or lost"). It lies outside the claim's declared file surface. The step-3.6 post-image block is untouched. ## Acceptance notes - **The read-side twin reproduces, and this PR does not change it.** Explain binds `sharingReadFilter` as `sharing.buildReadFilter(o, c)` with no `__readScope`. On `2c1011b01b`, `explain(read)` for `hr_reviewer` (read depth `org`) answered `visible: false`, `decidedBy: sharing`, on all three rows. The same `find` through the real security and sharing middleware returned each row, because enforcement stamps `__readScope: 'org'` and the sharing read filter is then null. The `dept_reporter` (`own`) control agrees on both sides. This is reported for the seat to file. - **Explain's record path does not model these, before or after this PR.** None of them was measured here: - the ADR-0090 D10 on-behalf-of legs (the agent-leg depth intersection, and the second gate call as the delegator); - ADR-0055's master gate for `controlled_by_parent` details; - the sharing middleware's `probeAuthoredRowWrite` deferral; - the lifecycle verbs `transfer` / `restore` / `purge`, whose Layer 1 is computed with the raw verb rather than the gate's `rlsOperation` mapping. - **The test double's nested reads are not scoped by plugin-sharing's read filter.** That is the same boundary `row-write-widener-composition.test.ts` records. In every admitted cell, the caller can read the row on the real stack too: through `org` read depth, an edit share, or ownership. - **Parallel work in the same file.** The fold of #19950 and #19964 edits the step-3.6 block of the same file. It had not landed when this PR opened. Whichever of the two lands second merges main and re-runs. --- _Generated by [Claude Code](https://claude.ai/code/session_01Evb5jFDZGKQE9KG4jbMfMF)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent a7581b3 commit 55cd8d4

4 files changed

Lines changed: 526 additions & 30 deletions

File tree

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
---
2+
'@objectstack/plugin-security': patch
3+
---
4+
5+
fix(plugin-security): `security/explain` computes a record's `update` / `delete` verdict from the by-id write path's own inputs, so `record.visible` matches what the by-id PATCH / DELETE does (#19963)
6+
7+
Clause-②: no
8+
9+
`POST /api/v1/security/explain` with `{ object, operation: 'update' | 'delete', recordId }` answered `decision.record.visible: false` (`decidedBy: 'rls'` or `'sharing'`) on rows that the by-id `PATCH` / `DELETE /api/v1/data/{object}/{id}` then admitted for the same caller. It happened on every object whose OWD is private (set explicitly, or left unset). A console that gates Edit on `record.visible` hid Edit and inline edit from users who were allowed to edit. Two inputs differed from the write path:
10+
11+
- **The platform ownership floor.** The by-id write gate drops `owner_only_writes` / `owner_only_deletes` (`created_by == current_user.id`) when the sharing service answers `allow` for the row. Explain kept the floor, so it excluded every row the caller did not create. That covers a row shared to them with `edit` access, a row they own but did not create, and a row an `org`-depth writer may edit.
12+
- **The write depth.** The write path hands the sharing service's per-record gate (`canEdit` / `canDelete`) the caller's effective write depth. Explain asked the same gate without it, so a caller with `org` or unit write depth was judged owner-only.
13+
14+
Explain now asks the same floor decision the write gate asks. It also passes the same write depth to the per-record gate.
15+
16+
Unchanged:
17+
18+
- Enforcement: the by-id write gate admits and refuses exactly what it did before. Its floor decision moved into one method that both paths call.
19+
- Reads (`operation: 'read'`) and object-level explanations (no `recordId`).
20+
- A caller acting on behalf of another user (`onBehalfOf`): its record-level write explanation uses the same inputs as before.
21+
- Objects whose OWD is `public_read_write` already matched and still do.

0 commit comments

Comments
 (0)