Record-scoped write authorization: per-record allowDelete on query deletes, per-element allow* on array puts#1842
Record-scoped write authorization: per-record allowDelete on query deletes, per-element allow* on array puts#1842kriszyp wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces record-scoped write authorization as a write-side corollary to record-scoped read authorization. It defers permission checks for collection-shaped writes (array PUTs and query-shaped DELETEs) to evaluate them per-record or per-element when application-overridden allowDelete, allowUpdate, or allowCreate hooks are present. Query-shaped deletes now utilize filter semantics (skipping denied rows), while array PUTs enforce fail-fast semantics (aborting the transaction on any denial). The feedback recommends improving the thenable check in Resource.ts by verifying that then is a function rather than just checking its truthiness.
|
Reviewed; no blockers found. Prior blocker (insert-RBAC regression on post/publish/copy) is resolved — kept intentionally with a regression test and updated PR description. New commits since last review (insert-RBAC/async-batch-authorization split, and reject-writes-after-abort) both check out with solid regression tests. |
…letes, per-element allow* on array puts Write-side corollary to #1786 (record-scoped allowRead), closing the gaps where allowUpdate/allowCreate/allowDelete were enforced once per request on multi-record writes: - Query-shaped delete with an application-overridden allowDelete defers its entry check (gated on the framework delete's enforcesCheckPermission marker, so a subclass overriding delete() keeps the entry check) and is enforced once per matching record with FILTER semantics: denied/throwing rows are skipped; limit/offset count allowed rows. `this` is a per-row resource instance, so schema properties and super.allowDelete composition both work, and the target-supplied permission operand is preserved for the RBAC baseline. - Array puts (static collection put and direct instance put) are authorized per element — create-vs-update chosen by that element's existence — with FAIL semantics: one denial throws AccessViolation, aborting the whole transaction. The deferred check runs in the static put action against the RESOLVED body, so a promise body can't bypass it and enforcement is framework-owned for every resource class. - The write paths are async, so unlike the record-scoped allowRead, async overrides participate in per-record evaluation (awaited, fail-closed). - transactional() argument parsing now carries the collection-ness it determined onto the fallback-constructed query, so programmatic `put(records, context)` resolves a collection resource like the equivalent REST target. Operations-API/SQL writes deliberately keep operation-level RBAC only (they never arm checkPermission; raw tables have no override surface). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… params in finally The limit gate ran after _writeDelete, so limit=0 deleted one row (the default query-delete path deletes zero). Check before writing. Also restore select/limit/offset/checkPermission on the target in a finally, so the per-record hooks (and an erroring search) see the original target. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…able record-scoped allowDelete Review finding (cross-model adjudication): the un-gated put deferral moved default-table array-PUT create authorization from the collection-scope INSERT operand to the per-element row-resource path (allowCreate delegating to allowUpdate = UPDATE operand) — a reachable RBAC behavior change. Defer only when allowUpdate/allowCreate is application-overridden, mirroring the delete gate; default-RBAC tables keep today's single entry check exactly (test pins the insert operand). Also warn once per class when a record-scoped allowDelete coexists with an overridden delete() — that combination silently keeps the collection-scope entry check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ture lint CI exposed a latent bug the per-element path exercises: the static put loop's async branch called put(element, request) — the context is not a URLSearchParams, so put()'s legacy argument shift never engaged and the CONTEXT was written as the record body whenever getResource resolved asynchronously (cyclic object → encoder stack overflow). Both branches now pass the query, matching the sync branch. Deterministic regression test forces the async branch via a getResource override. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t resources Review comments: cover both legs of the overridden-delete() gate (fail-closed entry denial with the custom delete never reached; collection-permissive override → documented downgrade to entry-scope semantics), and detect async element resources via typeof .then — a record attribute named `then` reads through the resource attribute proxy, so truthiness alone could misroute. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lain Resource keeps its entry check Record scoping is only meaningful where records exist. The put deferral had no table gate, so a plain Resource subclass overriding allowUpdate/allowCreate on a collection array put would have its entry check rerouted into per-element evaluation — N calls with each element instead of one call with the array, on classes that define their own class/instance semantics. The delete branch was behavior-safe (enforcesCheckPermission is table-only) but logged a misleading record-scoping warning for plain Resource subclasses. Both write branches now gate on a class-level supportsRecordScopedWriteAuth marker (the write-side analogue of supportsRowLevelAllowRead), set only by makeTable. Tests pin the plain-Resource contract: single entry check receiving the whole array, per-element put dispatch unchanged, no deferral on delete.
44df133 to
c59f38f
Compare
heskew
left a comment
There was a problem hiding this comment.
Both notes are specific to tables that override an allow* hook — the default path is correct and covered by t8–t14. An override re-routes authorization through the per-element resource, and that's where the two issues live. Left inline.
🤖 Draft prepared with Claude (cross-model review pipeline); reviewed and posted by @heskew.
Write-side corollary to #1786 (Record-scoped allowRead):
allowUpdate/allowCreate/allowDeletewere enforced once per request on multi-record writes, so a record-scoped override was silently under-enforced on conditional deletes and array puts.The model
Mirrors #1786's marker/deferral design, with the write-path differences:
allowDelete(markerisDefaultAllowDelete) defers its entry check and is enforced once per matching record — FILTER semantics: denied (or throwing/rejecting — fail closed) rows are skipped, permitted rows deleted;limit/offsetcount allowed rows, mirroring the read guard filtering before the query limit.allowUpdate/allowCreateare authorized per element — create-vs-update chosen by that element's existence — with FAIL semantics: one denial throwsAccessViolation, aborting the whole transaction (no partial batch).thisbinding: mutatingallow*hooks stay on the resource (no RecordObject delegate); per-record evaluation bindsthisto a per-row resource instance with the record loaded, sothis.ownerIdandsuper.allow*composition both work (the SWR-instance idiom from allowStaleWhileRevalidate is never consulted for query-driven revalidation #1578).allow*overrides are awaited per record (fail-closed).allowCreateconsistently means INSERT: record creation and record-targeted publish both check INSERT RBAC; publish creates an audit/message entry and does not update the record. Framework-default array puts still keep their single collection-entry check.putaction against the resolved body, so a promise body can't bypass it and enforcement is framework-owned for every resource class. A subclass overridingdelete()loses theenforcesCheckPermissionmarker → keeps today's entry check (warned once per class).Deliberately out of scope: operations-API/SQL writes stay on operation-level RBAC (they never arm
checkPermission; raw tables have no override surface).Where to look
resources/Resource.ts— the write-deferral gate in the authorize wrapper (the security-critical predicate set) and the per-element checks in the staticputaction.resources/Table.ts—#deleteWithRecordScopedCheck(scan-param save/restore aroundsearch()preserves thecheckPermissionpermission operand forsuper.allowDelete), and the per-element branch in instanceput.resources/DatabaseTransaction.ts/LMDBTransaction.ts— aborted transactions are terminally poisoned:addWrite()/save()/commit()reject, while successfully completed transactions retain their existing post-commit behavior; abort cascades across the multi-database chain.transactional()arg parsing now carries collection-ness onto the fallback-constructed query (programmaticput(records, context)previously never resolved a collection resource — latent inconsistency).Cross-model review (Codex + Opus adjudication; Gemini leg failed — agy hung 3×)
Findings fixed in-branch: limit=0 off-by-one; scan-param restore in
finally; default-table array-PUT INSERT→UPDATE operand shift; insert RBAC on record-scoped creates; two-phase async batch authorization; aborted-transaction write rejection. Remaining accepted limitations:limitscan the full matching set — same class of limitation as the Record-scoped allowRead: unified row-level read access control (#1422 gap 2) #1786 read filter.Docs: companion PR HarperFast/documentation#593 (extends the #1786 allowRead docs page with the write-side model).
Generated by KrAIs (Claude Fable 5).