feat(permissions): apply the operations allowlist to table read/write gating - #1631
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 introduces granular permission checks for database operations, specifically targeting the "Import Data" feature. It adds helper functions and hooks to verify if a user has the necessary permissions (including bulk load operations and job polling) before displaying import options. It also maps standard table actions to their respective operation names and handles operation aliases. The feedback suggests expanding the CSV_LOAD_OPERATIONS array to include other bulk import operations like csv_file_load and import_from_s3 to prevent users with those permissions from being incorrectly blocked.
5689d6b to
93251b6
Compare
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
This will be great, an excellent UI improvement!
Some things to address (sorry for the inline anchor failure):
Proposed inline comments (anchors failed):
src/features/instance/config/roles/operations/operationsCatalog.ts: Blocking correctness issue: this says an empty allowlist denies every operation, but I traced Harper v5.0.0, v5.1.26, and v5.2.2 and found that SQL dispatch is special-cased throughverifyPermsAST; thepermission.operationsgate exists only inverifyPerms. Consequently,operations: []—or any allowlist omittingsql—still permits SQL according to table CRUD, including INSERT/UPDATE/DELETE. The editor would give administrators a false security guarantee. Please enforce the operations gate for SQL in Harper with regression coverage before surfacing this boundary, or restrict Studio support to a server version containing that fix and describe the actual behavior.
— KrAIs (GPT-5)
(src/features/instance/config/roles/operations/operationsCatalog.ts:13 is not part of this PR's diff, so this is a file-level comment)
src/features/instance/config/roles/operations/operationsCatalog.ts: Several enum-valid names offered here are not authorization-grantable in the supported server tags. For example, Harper v5.2.2 registerssetConfigurationwithout anapi_name; the gate therefore compares the handler namesetConfigurationwith the allowlist valueset_configurationand denies it even though role validation accepted the JSON. The same mismatch affects entries such asrestart_service,cleanup_orphan_blobs,read_transaction_log,set_component_file,deploy_component, and the status operations. Please derive the offered set from effective authorization registrations—or fix that server invariant first—and add a contract test proving every picker entry reaches its handler.
— KrAIs (GPT-5)
(src/features/instance/config/roles/operations/operationsCatalog.ts:177 is not part of this PR's diff, so this is a file-level comment)
src/hooks/checkOperationPermission.ts:18: The browse flow needs exact operation capabilities rather than one “any read operation” result.DatabaseTableViewstill unconditionally issuessearch_by_value,search_by_conditions, andsearch_by_hashbased on state, while both Export CSV entry points remain visible. For example, a role allowing onlysearch_by_hashsatisfies this generic read mapping but cannot load the default table view, which callssearch_by_value. Please expose/check the concrete operation for each query and button, including edit-fetch and export prerequisites.
— KrAIs (GPT-5)
src/hooks/checkOperationPermission.ts:89: This OR is only safe if the modal exposes the matching source. Today aninsert-only role passes and opens a table-scoped modal whose default CSV-file path callscsv_data_load; conversely,csv_url_load + get_jobpasses while the modal still offers CSV files and JSON/sample paths that require other operations. Those permitted-looking choices then fail at submit time. Please return per-source capabilities and hide/disable unsupported methods, or require every method displayed by this launcher.
— KrAIs (GPT-5)
src/integrations/api/localRolePermission.ts:38: This collapses an operation-specific bypass into a role-level boolean. Harper bypassesstructure_useronly for its DDL operations; DML and component/environment operations still reach the allowlist, andcluster_useris not a general bypass. The new table tests already encode that DML remains gated, while the editor and user summaries say the list has no effect. Scoped or emptystructure_userarrays are also truthy here. Please model the bypass per operation (and database for scoped structure roles), update the warnings, and audit the many non-DDL consumers ofuseInstanceBrowseManagePermission.
— KrAIs (GPT-5)
src/integrations/api/localRolePermission.ts:55: This conflicts with the pre-5.0 compatibility handled above. On Harper 4.x,{ operations: { tables: ... } }can be a valid database permission, but the roles table and add/edit-user summaries call this helper without a server version and label that role invalid. Please pass the instance version into those presentations or otherwise preserve the object-shaped database interpretation on pre-5.0 servers.
— KrAIs (GPT-5)
src/features/instance/config/roles/operations/OperationsAllowlistEditor.tsx:271: The comment correctly limits component-registered operation grants to 5.2+, butOperationsPickerreceives no version and offers this custom candidate on every supported 5.0/5.1 instance too. Those releases validate only enum and group names, so saving the generated value is rejected. Please pass the version through and enable the custom path only where dynamic grants are supported; newer catalog entries can retain their separately flagged escape hatch.
— KrAIs (GPT-5)
src/integrations/api/api.patch.d.ts:158: Now that scopedstructure_userarrays are modeled explicitly—and the save helper correctly notes that they still need table grants outside their DDL scope—calculateDefaultPermissionsshould not return early for every truthy array. As written, editing such a role never adds newly discovered databases, tables, or attributes to its explicit permission template. Please reserve the early return forstructure_user === trueand add a scoped-role regression test.
— KrAIs (GPT-5)
🤖 Reviewed with Codex
|
Thanks — this was a high-value review. Both server-behavior claims are confirmed by execution, not just reading: I ran 1. SQL bypasses the allowlist — confirmedSame role,
So This is a Harper server issue plus a false promise in #1628's editor copy, not a defect in this PR's gating (which hides controls whose own operations are denied — those really are denied). But #1628 currently tells an admin they've restricted a role when they haven't, which is the failure mode its own commit message calls the worst one a permissions UI has. Filing against Harper and correcting #1628's wording both look necessary; happy to do either. 2. Names offered but not grantable — confirmed, and broader
Of 23 sampled names Studio's picker offers, 9 can never match: Note 3–4. Import gating — fixed in e202ffcYou were right that the any-of launcher gate lets through methods that can't run. Each method is now modeled as the set of paths it can take, because On the read mapping: 5–8
🤖 Addressed by Claude Code |
4943b8d to
eae63ad
Compare
Three rounds of review on #1631 turned on facts that reading the Harper source does not settle on its own: verifyPerms clears super users and structure-user DDL before the gate, SQL never reaches it, ~23 names are inert grants, and alias grants only work in one direction. Each was measured by calling verifyPerms directly, so the recipe for doing that is here too -- it is the cheapest way to answer the next question of this shape. Also records the seam that broke mid-review: describing a role's effective reach folds alias spellings, simulating the gate must not, and one shared helper cannot do both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three rounds of review on #1631 turned on facts that reading the Harper source does not settle on its own: verifyPerms clears super users and structure-user DDL before the gate, SQL never reaches it, ~23 names are inert grants, and alias grants only work in one direction. Each was measured by calling verifyPerms directly, so the recipe for doing that is here too -- it is the cheapest way to answer the next question of this shape. Also records the seam that broke mid-review: describing a role's effective reach folds alias spellings, simulating the gate must not, and one shared helper cannot do both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
697b8b0 to
47e5d06
Compare
DavidCockerill
left a comment
DavidCockerill
left a comment
There was a problem hiding this comment.
Approving at 11db476. The gate order is right and the composition is clean — checkTableActionAllowed(...) && checkTableGrant(...), with the original body extracted verbatim so the pre-existing deny-on-no-permission behaviour is preserved rather than restated.
Endorsing the deviation. Leaving useInstanceManagePermission and useInstanceBrowseManagePermission ungated is correct even though #1627's wording points the other way. verifyPerms returns on isSuperUser && !isSuSystemOperation before the allowlist is consulted, so gating those hooks would have hidden UI the server allows — a fail-closed bug dressed as a security fix. Implementing the obvious version first, reading verifyPerms, and reverting it is the right sequence, and the two one-line comments left behind are the right amount of explanation.
Verified and dropped — four candidates I chased rather than sent:
checkAnyOperationAllowedreturningtruefor an absent or malformed allowlist is fail-open in a permissions path, and correct here: it gates affordances only, Harper denies at gate 1, so the worst case is a button that 403s. Failing closed would hide controls from roles that hold the grant.expandLikeTheGateiterating a non-iterable would throw inside a hook. Not reachable —getOperationsAllowlist(localRolePermission.ts:89-94) returnsoperationsonly whenArray.isArray(...)and every entry is a string, elseundefined.- The database-scoped Import Data launcher consults no grant, and the operation it starts creates a table — but the button sits inside
{canManage && …}, so browse-manage still applies andcanImportis an additional gate. - @gemini-code-assist's
CSV_LOAD_OPERATIONSfinding: @dawsontoth's decline is right and I checked it.csv_file_loadandimport_from_s3appear only inoperationsCatalog.ts, never at a Studio call site, andImportSourcehas exactly three variants. A role granted onlycsv_file_loadshould see Import Data hidden, because Studio cannot issue it.
One thing for the record, not a request on this PR: localRolePermission.ts documents a breaks-auth class where a truthy non-iterable under operations makes listUsers throw behind a truthiness-only guard, failing authentication instance-wide. That is #1628 territory and you have clearly already mapped it — noting so it is not lost.
This is the complete set of my concerns at this head.
— DAIvid (Claude Opus 5)
… gating
Studio's table permission checks ignored `permission.operations`, so a role whose
allowlist excludes a data operation still saw Add Records, Import Data, and the
inline edit/delete affordances -- every one of which the server denies at gate 1,
before the role's table CRUD grants are consulted.
checkSchemaTablePermission and useInstanceSchemaTableAttributePermission now run
the allowlist first, via a new pure checkOperationPermission module that mirrors
the server's matching: groups expand, entries match verbatim, and the checked
operation resolves to its canonical api_name (so a granted legacy alias is the
dead entry it is on the server, while a canonical grant covers both spellings).
That verbatim expansion is deliberately not expandEffectiveOperations, which
folds alias spellings together to describe a role's effective reach -- the gate
does not fold, and simulating it is this module's whole job.
Absent and malformed allowlists restrict nothing -- a denial that can't be proven
must not hide UI -- and an empty array denies everything, as on the server. The
check stays version-blind like the table-grant lookup beside it: only an array of
strings is an allowlist, and what a pre-5.0 instance puts under that key is a
database record, so shape alone separates the two without a version query on a
hook that would not receive this module's entityId.
The manage and browse-manage hooks are deliberately NOT gated. verifyPerms
returns early for super users ("admins can do (almost) anything") before the
allowlist is reached, so those operations succeed however narrow the allowlist
is; hiding the admin chrome would hide UI that works. Only DML is gated, which is
also why this cannot lean on isElevatedRole: structure_user short-circuits for
DDL alone and cluster_user is absent from operation_authorization.ts, so both
still reach gate 1 for insert/update/delete/search.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review found the import launcher's any-of gate still shows methods that cannot run: an insert-only role opened a modal whose default file path bulk-loads, and a csv_url_load role saw sample and file methods it could not use. Each Import Data method is now modeled as the set of paths it can take -- `sample` and `file` are genuinely ambiguous until submit (a bundled dataset bulk-loads while random records insert; a .json upload inserts while any other extension bulk-loads), so a method is offered while either path is open, and the resolved source is checked again at submit where the choice is finally known. The modal renders only allowed methods and defaults to one of them instead of the contextual preference. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three rounds of review on #1631 turned on facts that reading the Harper source does not settle on its own: verifyPerms clears super users and structure-user DDL before the gate, SQL never reaches it, ~23 names are inert grants, and alias grants only work in one direction. Each was measured by calling verifyPerms directly, so the recipe for doing that is here too -- it is the cheapest way to answer the next question of this shape. Also records the seam that broke mid-review: describing a role's effective reach folds alias spellings, simulating the gate must not, and one shared helper cannot do both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sample picker's two groups issue different operations -- bundled datasets bulk-load, the random generator inserts -- so a role holding one and not the other was offered a source that could only fail. Each group is now gated by its own operation, which leaves the submit-time check for `file` alone, where the extension decides the operation and nothing earlier can know it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hose The Import launcher checks the insert grant for the table it was opened from, but the modal's database and table fields are editable -- so a role could launch from a table it can write, retarget an existing table it cannot, and submit. Submit now re-asks for the chosen destination. A table that does not exist yet has no grant to check: creating it is the browse-manage authority the launcher already required. Also corrects the AGENTS.md claim that table read/write is the only surface the allowlist can deny. Browse-manage is only half short-circuited -- its application-editor operations are not in STRUCTURE_USER_OPS, so for a non-super role they do reach the gate, which is where listing them grants them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
11db476 to
1faf64c
Compare
3 participants
Studio's table permission checks ignored
permission.operations, so a role whose allowlist excludes adata operation still saw Add Records, Import Data, and the inline edit/delete affordances — each of
which the server denies at gate 1, before the role's table CRUD grants are consulted.
checkSchemaTablePermissionanduseInstanceSchemaTableAttributePermissionnow consult the allowlistfirst, through a new pure
src/hooks/checkOperationPermission.ts.Stacks on #1628 (base
role-operations-allowlist), which added the catalog and the roles-UI editor.For the human reviewer
Read checkOperationPermission.ts first — its header states the
invariant the whole change rests on — then checkSchemaTablePermission.ts
for the gate order, then the call sites.
The manage and browse-manage hooks are deliberately left alone, which is the opposite of what the
task asked for. #1627 states the allowlist "denies unlisted operations even for super_user roles," so
the obvious change was to gate
useInstanceManagePermission(Users, Roles, Config, Logs, Status) anduseInstanceBrowseManagePermission(DDL, app editors). I implemented exactly that, then readverifyPermsand reverted it:Measured, not inferred (see Verification): a super_user with
operations: []— the strongestpossible test, since a gate that applied at all would deny everything — is still ALLOWED insert,
search, restart and get_configuration. A restricted-super_user's admin actions do not 403, so gating
that chrome would have hidden UI that works. #1628's
7eff23fcreached the same conclusionindependently. That leaves table DML as the only surface gate 1 can deny, which is what this gates.
Decisions worth questioning:
operation it issues. Add Records is strictly
insert. Import Data's methods are modeled as thepaths they can take, because
sampleandfilestay ambiguous until submit — a bundled datasetbulk-loads while random records insert, and a
.jsonupload inserts while any other extensionbulk-loads. A method is offered while either path is open; the resolved source is re-checked at
submit, where the choice is finally known. CSV paths require
get_jobalongside the load,because importData.ts polls the job — a CSV
grant without it leaves a committed load reporting failure, and the obvious retry duplicates rows.
getOperationsAllowlistonly saysyes to an array of strings, and what a pre-5.0 instance puts under that key is a database record
(
permissionsTranslatorwrites one for any upgraded v4 role owning a database namedoperations).Asking the instance version instead would mean a query per check on a hook that does not receive
this module's
entityId— the bug feat(roles): surface operation-level grants (permission.operations) in the roles/users UI #1628'sb7c01cf4just removed.expandEffectiveOperations. That helper folds alias spellingstogether to describe a role's effective reach; the server's gate does not fold, so a lone
search_by_idgrant must stay the dead entry it is. Simulating the gate is this module's whole job,so it expands verbatim and canonicalizes only the operation being checked.
isElevatedRole(from feat(roles): surface operation-level grants (permission.operations) in the roles/users UI #1628; countsstructure_userandcluster_user). It answerswhether the allowlist is unenforceable anywhere for the role. For DML both are still gated:
STRUCTURE_USER_OPSis create/drop table and attribute only, andcluster_userappears nowhere inoperation_authorization.ts. Measured:{structure_user: true, operations: ['read_only']}→ insertDENIED.
hide UI. This differs from feat(roles): surface operation-level grants (permission.operations) in the roles/users UI #1628's editor, which surfaces a malformed value as active — different
jobs: it warns, this hides.
readgating is live but inert today — no production call site passes'read', and theattribute hook has no callers. Mapped for symmetry and covered by tests.
is gated on the operations half alone — otherwise a
structure_userwith an empty allowlist couldstart a load denied after
create_tablesucceeded, leaving an empty table behind.Verification
End-to-end route: executed
verifyPerms/verifyPermsASTdirectly against the Harper checkout(
main, mocha, the same harness asunitTests/utility/operation_authorization.test.js), because thepremise this PR rests on is not covered by any existing test — Harper's allowlist suite sets
super_user: falseeverywhere. Throwaway probes, since deleted (Harper checkout clean):super_user: true,operations: []insert,search_by_conditions,restart,get_configurationsuper_user: true,operations: ['read_only']insert,restartsuper_user: false,operations: []insertstructure_user: true,operations: ['read_only']insertTwo further probes settled @kriszyp's review claims, and both confirmed them — see the PR comment for
detail. They are Harper-side, not defects in this diff: SQL dispatches through
verifyPermsAST, whichhas no allowlist gate (
operations: []still permits SQL writes); and 9 of 23 sampled picker namescan never match the gate because their authorization entry omits
api_name(deploy_componentamongthem, so Surface role operation-level grants (permission.operations) in the roles/users UI #1627's own deploy-only CI user is not buildable today). feat(roles): surface operation-level grants (permission.operations) in the roles/users UI #1628 tracks the second as
role operations allowlist: grants are gate-inert for ops registered without api_name (deploy_component, get_status, …); sql bypasses the allowlist harper#2175 with a
GATE_INERT_OPERATIONSset; the SQL bypass still wants an issue.The Studio-side tests prove an allowlist shows or hides the right control. What remains unproven from
Studio: a full round-trip against a live 5.x instance carrying such a role.
src/hooks/checkOperationPermission.test.ts(new): absent / malformed / empty / exact / unknown /group / both alias directions, a database record under
operationsreading as no allowlist, expansioncaching, per-action mapping,
structure_userandcluster_user, per-method and per-source importcapability, and a mapping-integrity test asserting every mapped name is a canonical, non-group catalog
entry. That last one earned its keep: an earlier draft mapped
list_certificatesandlist_ssh_keys,which harper-pro registers at runtime and the catalog therefore lacks, so they would never have matched.
checkSchemaTablePermission.test.tsextended: allowlist over a CRUD grant, overstructure_user, thesuper_user pass-through, and the Add-Records-vs-Import divergence.
TableContextMenuItems.test.tsxandImportDataModal.permissions.test.tsx(both new) mock only theauth store and the router, so they run the real allowlist → hook → rendered-control chain: entries
hidden for a read-only allowlist, Import Data surviving a CSV grant while Add Records drops, Import
Data hidden when a CSV load has no
get_job, the URL method dropped for an insert-only role, and thedefault selection falling back to an allowed method.
Full gate, matching
verify-pr.yamlexactly —tsc -b✓,pnpm test:coverage✓ (301 files /2369 passed, 11 skipped),
pnpm lint✓,pnpm build✓;dprintno-op. The one uncovered branch inthe new module is the
?? []guard TypeScript makes unreachable.test:e2e:dockeris not part of CI's PR gate and its specs cover auth/signup/org-users only —nothing in the instance browse UI — so it cannot exercise this change.
Review coverage
Authored by Claude Opus 5. Outside coverage via the bundled pre-push CLI (
--author claude), plus ahuman review; every round changed the design:
gpt-5.6-sol(codex)gpt-5.6-sol(codex)CHANGES— the shared insert flag conflated Add Records with Import Data--fullgpt-5.6-sol(codex)CHANGES— CSV imports missingget_job; the ungated third Import launcher--fullagy(default model)csv_file_load/import_from_s3added — declined with rationale,ImportSourceissues neithercursor-grok✗ (pruned by policy),cursor-composer✗ (cursor-agentnot installed), Harper domainadjudication ✗ (exit 1) — so the decision list above is mine, not an adjudicated ledger.
Rebased twice onto a moving #1628 (
b7c01cf4,61d068be), both times with semantic merges rather thantextual ones: the version-blind reversal and the
expandEffectiveOperationssplit above both came out ofthose. No cross-model round has run against the current head; the changes since the last reviewed SHA are
the import-method gating, the version-blind revert, and tests.
Docs
No companion PR to HarperFast/documentation: this makes Studio's UI agree with server behavior that
already ships, and adds no API, config, CLI, or default. The allowlist feature's own documentation
belongs with #1628.
Human-Review-Need: 4 @ eae63ad