docs(roles): state the limits of the operations allowlist - #631
Draft
dawsontoth wants to merge 1 commit into
Draft
docs(roles): state the limits of the operations allowlist#631dawsontoth wants to merge 1 commit into
dawsontoth wants to merge 1 commit into
Conversation
The two-gate description reads as universal, which leads people to expect a 'restricted super user' — a combination add_role/alter_role actually reject, and which authorization would ignore anyway. Documents that, the structure_user DDL carve-out (scoped to the listed databases for the array form), and that sql is authorized on its own path so listing it neither grants nor restricts SQL — worth stating since both permission groups include the name. Also notes that a non-array value can wedge the user-cache load (harper#2194). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the documentation in reference/users-and-roles/overview.md to clarify the scoping behavior and limitations of the operations field in permission objects. The review feedback suggests clarifying the explanation of how super_user roles interact with the allowlist to avoid misleading phrasing, and recommends using a VersionBadge component to properly document the behavior change introduced in version 5.0.0.
|
|
||
| The field scopes an ordinary role; it is not a way to narrow an administrator. Three limits follow from that, and each one surprises people: | ||
|
|
||
| - **`super_user` and `cluster_user` roles cannot carry an allowlist at all.** `add_role` and `alter_role` reject any permission that sets either flag alongside other keys, so the combination is a validation error rather than a restricted admin. Authorization also clears a `super_user` role before the allowlist is consulted. |
Contributor
There was a problem hiding this comment.
The phrase 'clears a super_user role' is misleading as it might imply that the role or its permissions are being deleted or emptied. Based on the actual authorization logic, it would be clearer to state that authorization bypasses the allowlist check or returns early for super_user roles.
Suggested change
| - **`super_user` and `cluster_user` roles cannot carry an allowlist at all.** `add_role` and `alter_role` reject any permission that sets either flag alongside other keys, so the combination is a validation error rather than a restricted admin. Authorization also clears a `super_user` role before the allowlist is consulted. | |
| - **<code>super_user</code> and <code>cluster_user</code> roles cannot carry an allowlist at all.** <code>add_role</code> and <code>alter_role</code> reject any permission that sets either flag alongside other keys, so the combination is a validation error rather than a restricted admin. Authorization also bypasses the allowlist check for a <code>super_user</code> role. |
| - **`structure_user` roles bypass the allowlist for DDL only.** `create_table`, `create_attribute`, `drop_table`, and `drop_attribute` are reachable regardless of the list — plus `create_database`/`drop_database` when `structure_user` is `true`. When it is an array of database names, that carve-out applies only to those databases. Every other operation is still gated normally. | ||
| - **`sql` is not gated by this field.** SQL statements are authorized against table CRUD permissions on their own path, so listing `sql` neither grants nor restricts them, and omitting it does not prevent a role from running SQL. This matters when reading the `read_only` and `standard_user` groups below, both of which include the name. | ||
|
|
||
| The value must be an array of strings. A non-array value is rejected on write, and a role that already holds one (for example a pre-5.0 role that granted a database named `operations`) can prevent the instance from loading its user cache — see [HarperFast/harper#2194](https://github.com/HarperFast/harper/issues/2194). |
Contributor
There was a problem hiding this comment.
According to the repository's guidelines, when documenting behavior changes to an existing surface (such as the change in v5.0.0 where operations became a reserved array of strings instead of allowing a database named operations), we should use the <VersionBadge type="changed" version="vX.Y.0" /> format.
Suggested change
| The value must be an array of strings. A non-array value is rejected on write, and a role that already holds one (for example a pre-5.0 role that granted a database named `operations`) can prevent the instance from loading its user cache — see [HarperFast/harper#2194](https://github.com/HarperFast/harper/issues/2194). | |
| The value must be an array of strings. <VersionBadge type="changed" version="v5.0.0" /> A non-array value is rejected on write, and a role that already holds one (for example a pre-5.0 role that granted a database named <code>operations</code>) can prevent the instance from loading its user cache — see [HarperFast/harper#2194](https://github.com/HarperFast/harper/issues/2194). |
References
- Use the
<VersionBadge type="changed" version="vX.Y.0" />format when documenting behavior changes to existing surface, as prescribed in the repository's guidelines.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-631 This preview will update automatically when you push new commits. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Companion to HarperFast/studio#1628, which surfaces
permission.operationsin the Studio roles UI. Building that UI meant establishing the field's real semantics against the Harper source, and several of them contradict a natural reading of this page — I got each one wrong first and had them caught in review.What changes
The two-gate description reads as universal ("any unlisted operation is denied"), so the obvious next thought is a restricted super user. Three limits are now stated explicitly:
super_user/cluster_user+operationsis a validation error.validateNoSUPermsrejects any multi-key permission setting either flag. Authorization also returns early for asuper_userrole before reaching the allowlist gate, so even a stored one would not narrow it.structure_userbypasses the allowlist for DDL only — the four table/attribute operations, plus create/drop database for the boolean form; the array form scopes that to its listed databases. Everything else is still gated.sqlis authorized on its own path (checkASTPermissions/verifyPermsAST), which never consults the allowlist. Worth stating plainly becausesqlappears in bothread_onlyandstandard_user, so the groups imply the opposite.Plus a pointer to HarperFast/harper#2194 for the non-array case, since a role carrying one can stop the instance loading its user cache.
Verification
Read against harper
v5.2.2(utility/operation_authorization.ts,validation/role_validation.ts,security/user.ts,server/serverHelpers/serverUtilities.ts); thesqlandstructure_userclaims were also exercised against the compileddist/. No behavior change here — this documents what 5.x already does.Review coverage
Written by an LLM (Claude Fable 5). Prose-only change to one file;
prettier --checkclean. The underlying findings were reviewed by @cb1kenobi and @kriszyp on studio#1628.🤖 Generated with Claude Code