Document operation authorization and explicit row filters#593
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds documentation and release notes for the new record-scoped write authorization hooks (allowDelete, allowUpdate, and allowCreate) introduced in version 5.2.0. The feedback suggests updating a caveat mention of delete() to static delete() in the schema documentation to maintain consistency with Harper Resource documentation standards, which define HTTP verb handlers as static methods.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-593 This preview will update automatically when you push new commits. |
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-593 This preview will update automatically when you push new commits. |
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-593 This preview will update automatically when you push new commits. |
- Scope the upgrade-checklist 403->200 note to collection reads and conditional DELETE; array PUT denials still return 403 (the change there is only that the hook may now run at all). - Clarify that `user` is undefined (not an object with a falsy id) for an unauthenticated caller, and how the allowDelete vs allowUpdate/allowCreate examples differ in how they fail closed for that case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-593 This preview will update automatically when you push new commits. |
|
|
||
| One caveat: a class that overrides `delete()` itself replaces the framework's record-scoped delete path — its query-shaped deletes keep the request-entry check (a warning is logged when this combination is detected), and the custom `delete()` is responsible for any row-level enforcement. | ||
|
|
||
| For an unauthenticated caller, `user` itself is `undefined` — not an object with a falsy `id`. Leading with `super.allow*(user, ...)`, as the `allowDelete` example does, denies before any `user.*` access runs, since the default checks are written with `user?.`. An override that skips that call and dereferences `user.id` directly, as the `allowUpdate`/`allowCreate` examples do, will throw for an unauthenticated caller instead — which still fails closed (denies), just via an exception rather than an explicit `false`. |
There was a problem hiding this comment.
Medium: this clarification documents a side effect of the RBAC-bypass gap without closing it
This paragraph explains that skipping super.allow*(...) makes an unauthenticated caller throw instead of returning false — accurate, but it only addresses that narrow edge case. It doesn't fix why the allowUpdate/allowCreate examples above (lines 575-582) omit super.allowUpdate(user, ...) / super.allowCreate(user, ...) in the first place: any authenticated user who satisfies the ownership check (this.ownerId === user.id) is granted the write regardless of their role/RBAC permission on that table, because the RBAC baseline that super.allow*(...) composes (per the prose two paragraphs above) is never consulted for those two hooks. That's the privilege-bypass risk flagged in the prior review round — this addition explains a symptom of the gap rather than removing it.
Suggested fix: add if (!super.allowUpdate(user, updates, context)) return false; and if (!super.allowCreate(user, record, context)) return false; to the two examples, matching the allowDelete example immediately above. This paragraph can then be trimmed to the still-true note about user being undefined for unauthenticated callers.
—
Generated by Barber AI
There was a problem hiding this comment.
The allow* methods are intended to programmatically provide "grants" to users otherwise not allowed (hence "allow"), not restricting users who already have permission (and could otherwise access the content through other means).
Ethan-Arrowood
left a comment
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
Requesting changes on one substantive gap. #1842 doesn't only add the record-scoped overrides documented here — it also tightens a default RBAC rule: it removed the allowCreate branch where creating within a record delegated to allowUpdate, so a record-targeted publish/post now requires the table's insert permission instead of update. That's a silent, user-visible change for apps that override nothing — recordScopedWriteAuth.test.js ("record-targeted publish uses insert RBAC because it creates a message entry") pins it: insert:false, update:true → 403, insert:true, update:false → 200.
Both this reference page and the 5.2 release note currently say framework defaults are "unchanged," which is misleading on exactly this point. A caller that had update but not insert and relied on publish/post to a record id will start getting 403s after upgrading, so it needs to be called out explicitly. Inline suggestions on both files below.
sent with Claude Opus 4.8
| - **Array `PUT` into a collection** — with an overridden `allowUpdate`/`allowCreate`, each element is authorized individually: an element whose record already exists is checked with `allowUpdate` (`this` = the existing record's resource), a new element with `allowCreate`. Any denial fails the **whole request** with a 403 and aborts the transaction — no partial batch is committed (each element was an explicit write target, unlike a query's incidental matches). | ||
| - **Single-record writes** (`PUT`/`PATCH`/`DELETE` on an id) — unchanged: evaluated at request entry with the record loaded, so the same override works there too. | ||
|
|
||
| The default (non-overridden) hooks are table-level RBAC checks and keep their single request-entry evaluation with no per-record cost — including the collection-scope insert permission governing array `PUT`s. Operations API and SQL writes are governed by role permissions only; the `allow*` hooks apply to the Resource APIs (REST and JavaScript). |
There was a problem hiding this comment.
Defaults aren't fully unchanged: the create-within-record permission moved from update to insert in #1842. Folding that into this sentence, which is the natural home since it already describes default write-hook RBAC:
| The default (non-overridden) hooks are table-level RBAC checks and keep their single request-entry evaluation with no per-record cost — including the collection-scope insert permission governing array `PUT`s. Operations API and SQL writes are governed by role permissions only; the `allow*` hooks apply to the Resource APIs (REST and JavaScript). | |
| The default (non-overridden) hooks are table-level RBAC checks and keep their single request-entry evaluation with no per-record cost — including the collection-scope insert permission governing array `PUT`s. One default did change in 5.2, independent of any override: a record-targeted `publish`/`post` (creating _within_ an existing record) is now authorized against the table's **`insert`** permission, where it previously delegated to **`update`** (a create inside a record was treated as an update to that record). A caller with `update` but not `insert` that relied on `publish`/`post` to a record id will now receive a `403`. Operations API and SQL writes are governed by role permissions only; the `allow*` hooks apply to the Resource APIs (REST and JavaScript). |
|
|
||
| ## Record-Scoped Authorization | ||
|
|
||
| The `allow*` authorization hooks on tables are now **record-scoped when overridden**: instead of a single collection-scope verdict per request, an application-overridden hook is evaluated once per record, with access to that record's fields. Framework defaults are unchanged — a table that does not override a hook keeps its single request-entry RBAC check with no per-record cost — and the change applies only to classes that extend a table; a plain `Resource` subclass defines its own semantics and keeps the single entry check. |
There was a problem hiding this comment.
"Framework defaults are unchanged" isn't quite right — the create-within-record RBAC operand changed (see #1842). Suggested rewording that keeps the record-scoping point but adds the operand change:
| The `allow*` authorization hooks on tables are now **record-scoped when overridden**: instead of a single collection-scope verdict per request, an application-overridden hook is evaluated once per record, with access to that record's fields. Framework defaults are unchanged — a table that does not override a hook keeps its single request-entry RBAC check with no per-record cost — and the change applies only to classes that extend a table; a plain `Resource` subclass defines its own semantics and keeps the single entry check. | |
| The `allow*` authorization hooks on tables are now **record-scoped when overridden**: instead of a single collection-scope verdict per request, an application-overridden hook is evaluated once per record, with access to that record's fields. The record-scoping change applies only to classes that extend a table — framework defaults keep their single request-entry RBAC check with no per-record cost, and a plain `Resource` subclass defines its own semantics and keeps the single entry check. One default RBAC operand did change: a record-targeted `publish`/`post` (creating _within_ an existing record) now requires the table's **`insert`** permission instead of **`update`**, so a caller with `update` but not `insert` on that table will now get a `403`. |
4693838 to
a501b0f
Compare
kriszyp
changed the title
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-593 This preview will update automatically when you push new commits. |
|
This companion PR has been fully rewritten for the rescoped authorization model. The prior record-scoped write-hook documentation and the behavior behind the existing changes-requested review are gone. It now documents deprecated one-time allow* operation gates, explicit rowFilter/eventFilter predicates, built-in versus custom loadAsInstance=false ownership, and the corrected allowDelete/allowCreate false-mode behavior. Local format and production docs builds pass; I’ll re-request review after CI finishes. |
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-593 This preview will update automatically when you push new commits. |
4 participants
Companion documentation for HarperFast/harper#1915 and HarperFast/harper#1842.
Summary
allowReaddocumentation from #1786 with explicitrowFilterand subscriptioneventFilterguidanceallow*hooks are deprecated one-time operation gatesloadAsInstance = falsebehavior, and custom false-mode handlers that retain responsibility for authorizationcheckPermission = trueuse, and collision-safe event-owner ID prefixesValidation
5.1#deployment-operationsanchor warning remainsGenerated with GPT-5.6 Codex.