diff --git a/AGENTS.md b/AGENTS.md index a7b071f24..3814f3f93 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -582,24 +582,39 @@ execute relationship joins). Harper 5.0+ roles accept an `operations` allowlist. Almost every intuitive reading of it is wrong, and each of these cost a review round on #1628 — verify against the Harper source before changing -`src/features/instance/config/roles/operations/**` or `src/integrations/api/localRolePermission.ts`. - -- **It does not restrict super users.** `verifyPerms` returns early for `super_user` - ("admins can do (almost) anything") _before_ the allowlist gate. The allowlist is a way to scope - a NON-super role (gate 2 then lets it hold otherwise-SU operations), not a way to narrow an admin. +`src/features/instance/config/roles/operations/**` or `src/integrations/api/localRolePermission.ts`, +**at the tag you mean**. Harper changed gate 1 twice in 5.3, and `resolveGrantBand` turns a version +into one of three bands (`pre-5.3`, `5.3-early`, `5.3`) so callers don't parse SemVer per operation: +from v5.3.0-alpha.1 (harper `f8a5aa90a`) `verifyPerms` runs the gate ahead of every privilege +early-return and `chooseOperation` runs it on `sql`; from v5.3.0-beta.3 every entry names its grant. +`allowlistGatesEveryPath(band)` is the first change. + +- **The list never narrows a stored super user, for a different reason on each side.** Through 5.2.x + `verifyPerms` returns early for `super_user` ("admins can do (almost) anything") _before_ the gate. + From v5.3.0-alpha.1 the gate comes first ("must run before EVERY privilege early-return"), and a + list beside `super_user` would be enforced, but `validateNoSUPerms` (next bullet) refuses to store + one. Either way the allowlist scopes a NON-super role (gate 2 then lets it hold otherwise-SU + operations). - **`super_user`/`cluster_user` + `operations` cannot even be saved.** `validateNoSUPerms` rejects any permission with more than one key that sets either flag, so the combination 400s. Strip the allowlist for those roles rather than sending it. -- **`structure_user` is different and IS gated** — only `create_table`/`create_attribute`/ - `drop_table`/`drop_attribute` (plus create/drop database for the boolean form) short-circuit. The - array form scopes that carve-out to its listed databases and never reaches create/drop database. -- **Which names the gate matches depends on the version.** Before 5.3, 20 entries omit `api_name`, +- **`structure_user` DDL skips the list only before 5.3.** Through 5.2.x `create_table`/ + `create_attribute`/`drop_table`/`drop_attribute` (plus create/drop database for the boolean form) + return before the gate. From v5.3.0-alpha.1 the gate runs first, so an allowlisted structure user + runs that DDL only when it is listed. On both, the array form keeps the DDL to its listed databases + (listing `drop_table` cannot reach another one), and its flag never covers create/drop database. + `structureUserDdlScope` reads the shape only; the editor's copy takes the band. +- **Which names the gate matches depends on the version.** Before v5.3.0-beta.3, 20 entries omit `api_name`, so the gate compares a camelCase handler name against a snake_case grant and never matches (`deploy_component`, `get_status`, `registration_info`, …; HarperFast/harper#2175). From v5.3.0-beta.3 every entry names its grant, and `get_backup`, `read_transaction_log` and `catchup` name `null` on purpose, so no allowlist reaches them. The catalog records this per entry (`gate`), and - callers resolve the instance version into a `GrantBand` once. `sql` is worse: it routes to - `verifyPermsAST`, which never consults the allowlist at all. + callers resolve the instance version into a `GrantBand` once. +- **`sql` ignores the list before 5.3 and needs it from v5.3.0-alpha.1.** Through 5.2.x + `chooseOperation` sends it straight to `checkASTPermissions`, which never consults the allowlist, + so listing or omitting it changes nothing. From alpha.1 it calls `verifyOperationsAllowlist` on + `sql` first, so a list without `sql` (or a group carrying it, like `read_only`) denies every SQL + statement. Table permissions apply on both. The catalog marks it `outOfGate: 'pre-5.3'`. - **Alias spellings are inert too** (`describe_database`, `search_by_id`, `create_schema`, …): both spellings dispatch to one handler whose entry carries only the canonical `api_name`. - **A non-iterable value takes the instance down.** `listUsers` expands every assigned role's @@ -615,22 +630,28 @@ and each of these cost a review round on #1628 — verify against the Harper sou Practical consequence for the UI: never describe a restriction the server does not enforce, and never advise "fixing" an `operations` value in place — both mistakes shipped and were caught in review. `classifyOperationsValue` needs the instance version because the same shape means different -things on either side of the 5.0.0-alpha.8 floor (the feature's first tagged build, not 5.0.0). +things on either side of the 5.0.0-alpha.8 floor (the feature's first tagged build, not 5.0.0). The +editor's structure-user and SQL sentences need the band for the same reason: each is true on one side +of v5.3.0-alpha.1 only. ### Which of those the UI gating turns on The two facts that decide Studio's permission hooks (`src/hooks/checkOperationPermission.ts`): -- **Gate admin chrome on the allowlist and you hide UI that works.** Because super users never reach - the gate, `useInstanceManagePermission` and `useInstanceBrowseManagePermission` are deliberately NOT - gated. Note browse-manage is only half short-circuited: its DDL half is, but the application-editor - operations behind it (`set_component_file`, `drop_component`, `deploy_component`, `set_env_value`, - `restart_service`) are not in `STRUCTURE_USER_OPS`, so for a non-super role they do reach the gate — - where an allowlist listing them _grants_ them via gate 2. Denying that half on the allowlist would - therefore hide UI a scoped role can legitimately use. -- **`isElevatedRole` is the wrong question for a specific operation.** It answers "is the list - unenforceable anywhere for this role", which is what the editor warns about. For DML, - `structure_user` and `cluster_user` are still gated. +- **Gate admin chrome on the allowlist and you hide UI that works.** No stored super user fails the + gate on any version (see above), so `useInstanceManagePermission` is deliberately NOT gated. + `useInstanceBrowseManagePermission` isn't either, but only pre-5.3 makes that exact: there its DDL + half returns before the gate for a structure user. From v5.3.0-alpha.1 nothing does, so an + allowlisted structure user is offered DDL the server refuses unless the list names it. The hooks + can't see the version, and gating them on the list would hide DDL that works on 5.2, so they stay + open and the server's 403 names the operation (HarperFast/studio#1894 tracks per-operation checks). + The application-editor operations behind browse-manage (`set_component_file`, `drop_component`, + `deploy_component`, `set_env_value`, `restart_service`) are not in `STRUCTURE_USER_OPS`, so for a + non-super role they reach the gate on every version — where an allowlist listing them _grants_ them + via gate 2. Denying that half on the allowlist would hide UI a scoped role can legitimately use. +- **Only `super_user` short-circuits a specific operation.** For DML, `structure_user` and + `cluster_user` are gated on every version, so `checkTableOperationsAllowed` exempts the first flag + alone. Asking whether a role carries an elevating flag is the editor's question, not this one. Also two different expansions are correct for two different questions, and sharing one silently broke the second once: `expandEffectiveOperations` folds alias spellings together to describe a role's @@ -648,6 +669,12 @@ denied. `testUtils.preTestPrep()` + `testUtils.setGlobalSchema(...)` provide the `verifyPermsAST` (to show SQL skipping the gate). Run with `npx mocha unitTests/utility/`, read the verdicts, delete the file. Measured this way: `super_user` + `operations: []` still ALLOWS insert, search, restart and get_configuration, while `super_user: false` + `operations: []` DENIES insert. +Those are 5.2 verdicts. A built `dist` answers too, without touching a checkout — a checkout's, or an +installed `harper` package's, which is how to compare versions: `require` its +`utility/operation_authorization.js`, set `global.hdb_schema`, and call `verifyPerms` (or +`verifyOperationsAllowlist(requestJson, 'sql')` for the SQL gate). Run that way on 5.2.3, 5.3.0-beta.2 +and 5.3.1, `structure_user: true` + `operations: ['read_only']` ALLOWS `create_table` on the first and +DENIES it on the other two, and so does `super_user: true` + `operations: []`. ## Datadog RUM — `beforeSend` fails open, so nothing in it may throw diff --git a/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx b/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx index e1ceb9b47..db0433bbb 100644 --- a/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx +++ b/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx @@ -122,7 +122,7 @@ describe('OperationsAllowlistEditor', () => { render( {}} version="5.2.2" structureUserDdl />, ); - // The allowlist IS enforced for a structure user on everything except DDL. + // Before 5.3 the allowlist IS enforced for a structure user on everything except DDL. expect(screen.getByText(/regardless of the list/)).toBeTruthy(); expect(screen.getByText(/on any database/)).toBeTruthy(); expect(screen.queryByText(/has no effect/)).toBeNull(); @@ -152,6 +152,106 @@ describe('OperationsAllowlistEditor', () => { expect(screen.getByText(/except the DDL noted above/)).toBeTruthy(); }); + describe('from 5.3.0-alpha.1, where the list runs ahead of the structure_user and SQL paths', () => { + // 5.3.0-beta.2 is the last build that gates first without naming every entry, so it pins the + // gate-order boundary apart from the beta.3 naming boundary. + const GATE_FIRST_VERSIONS = ['5.3.0-alpha.1', '5.3.0-beta.2', '5.3.0']; + + it('tells a structure user its DDL needs listing, and names what is missing', () => { + for (const version of GATE_FIRST_VERSIONS) { + render( + {}} + version={version} + structureUserDdl + />, + ); + expect(screen.getByText(/gates its DDL too/), version).toBeTruthy(); + expect(screen.queryByText(/regardless of the list/), version).toBeNull(); + const note = screen.getByText(/Not listed, so Harper refuses them/).textContent ?? ''; + expect(note.split('refuses them:')[1], version).toBe( + ' create_attribute, drop_table, drop_attribute, create_database, drop_database.', + ); + cleanup(); + } + }); + + it('drops the missing-DDL sentence once every DDL operation is listed', () => { + const allDdl = [ + 'create_attribute', + 'create_database', + 'create_table', + 'drop_attribute', + 'drop_database', + 'drop_table', + ]; + render( {}} version="5.3.0" structureUserDdl />); + expect(screen.getByText(/gates its DDL too/)).toBeTruthy(); + expect(screen.queryByText(/Not listed/)).toBeNull(); + }); + + it('does not count an alias spelling as listing the database DDL', () => { + // create_schema is registered under create_database's api_name, so the gate never matches it. + render( + {}} version="5.3.0" structureUserDdl />, + ); + expect(screen.getByText(/Not listed, so Harper refuses them/).textContent?.split('refuses them:')[1]) + .toContain('create_database'); + }); + + it('keeps an array-shaped structure user to its databases, without the database DDL', () => { + render( + {}} version="5.3.0" structureUserDdl={['dev']} />, + ); + const note = screen.getByText(/gates its DDL too/).textContent ?? ''; + expect(note).toContain('dev'); + expect(note).toContain('cannot reach another database'); + expect(note).not.toContain('create_database'); + }); + + it('leaves no DDL exception standing on an empty list, and says SQL is denied too', () => { + render( {}} version="5.3.0-alpha.1" structureUserDdl />); + expect(screen.getByText(/not even SQL or its structure-user DDL/)).toBeTruthy(); + expect(screen.queryByText(/except the DDL noted above/)).toBeNull(); + expect(screen.queryByText(/SQL is gated by table permissions, not this list/)).toBeNull(); + }); + + it('says the list gates SQL, where before 5.3 it says the opposite', () => { + render(); + expect(screen.getByText(/anything unlisted is denied, SQL included/)).toBeTruthy(); + expect(screen.queryByText(/this list does not restrict them/)).toBeNull(); + + cleanup(); + render(); + expect(screen.getByText(/this list does not restrict them/)).toBeTruthy(); + expect(screen.queryByText(/SQL included/)).toBeNull(); + }); + + it('counts sql as a grant, in read_only and on its own chip', () => { + render(); + expect(screen.getByText(/Effectively allows 13 operations/)).toBeTruthy(); + + cleanup(); + render(); + expect(screen.getByText(/Effectively allows 1 operation\./)).toBeTruthy(); + const chip = screen.getByRole('button', { name: 'Remove sql' }).parentElement; + expect(chip?.getAttribute('title') ?? '').not.toMatch(/changes nothing/); + }); + + it('still treats the deploy grants as inert on the prereleases before beta.3', () => { + render(); + expect(screen.getByText(/Every entry here is inert/)).toBeTruthy(); + }); + }); + + it('calls a saved sql grant inert before 5.3, and says why on its chip', () => { + render(); + expect(screen.getByText(/Every entry here is inert/)).toBeTruthy(); + const chip = screen.getByRole('button', { name: 'Remove sql' }).parentElement; + expect(chip?.getAttribute('title')).toMatch(/changes nothing/); + }); + it('marks a grant this version cannot enforce, and says which version does', () => { render(); openPicker(); diff --git a/src/features/instance/config/roles/operations/OperationsAllowlistEditor.tsx b/src/features/instance/config/roles/operations/OperationsAllowlistEditor.tsx index a96dde769..a7ecd25ed 100644 --- a/src/features/instance/config/roles/operations/OperationsAllowlistEditor.tsx +++ b/src/features/instance/config/roles/operations/OperationsAllowlistEditor.tsx @@ -11,6 +11,7 @@ import { Input } from '@/components/ui/input'; import { Label } from '@/components/ui/label'; import { Switch } from '@/components/ui/switch'; import { + allowlistGatesEveryPath, expandEffectiveOperations, GATE_NAMES_EVERY_ENTRY_FROM, gateVerdict, @@ -21,6 +22,7 @@ import { GrantBand, isInertGrant, isOperationGroupName, + isOutOfGate, OPERATION_CATALOG, OPERATION_CATEGORIES, OPERATION_GROUPS, @@ -28,7 +30,11 @@ import { resolveGrantBand, summarizeOperations, } from '@/features/instance/config/roles/operations/operationsCatalog'; -import { STRUCTURE_USER_DDL_OPERATIONS } from '@/integrations/api/localRolePermission'; +import { + STRUCTURE_USER_DATABASE_OPERATIONS, + STRUCTURE_USER_DDL_OPERATIONS, +} from '@/integrations/api/localRolePermission'; +import { cn } from '@/lib/cn'; import { groupBy } from '@/lib/groupBy'; import { pluralize } from '@/lib/pluralize'; import { ChevronDown, PlusIcon, XIcon } from 'lucide-react'; @@ -54,7 +60,7 @@ export function OperationsAllowlistEditor({ version: string; /** super_user/cluster_user: Harper refuses to store an allowlist alongside the flag. */ allowlistRejected?: boolean; - /** structure_user DDL carve-out: `true` everywhere, an array for those databases only. */ + /** The role's `structure_user` scope (`structureUserDdlScope`): `true` everywhere, an array for those databases. */ structureUserDdl?: true | string[] | false; disabled?: boolean; }) { @@ -68,6 +74,7 @@ export function OperationsAllowlistEditor({ ); const grantable = getGrantableOperations(version); const band = resolveGrantBand(version); + const gatesEveryPath = allowlistGatesEveryPath(band); const selectedSet = new Set(value ?? []); const selectedOperations = (value ?? []).filter((entry) => !isOperationGroupName(entry)); const effective = value ? expandEffectiveOperations(value, band) : []; @@ -119,12 +126,16 @@ export function OperationsAllowlistEditor({ Restrict operations

- {restricted - ? 'Only the operations below are allowed — anything unlisted is denied. Listing an operation that ' + {!restricted + ? 'No operation-level restriction. Add one to limit this role to an explicit list of API operations ' + + '(for example a deploy-only CI role).' + : gatesEveryPath + ? 'Only the operations below are allowed — anything unlisted is denied, SQL included: SQL statements ' + + 'need sql (or a group with it) listed here, and are then checked against table permissions as ' + + 'well. Listing an operation that normally requires super_user deliberately grants it to this role.' + : 'Only the operations below are allowed — anything unlisted is denied. Listing an operation that ' + 'normally requires super_user deliberately grants it to this role. SQL statements are authorized ' - + 'against table permissions instead, so this list does not restrict them.' - : 'No operation-level restriction. Add one to limit this role to an explicit list of API operations ' - + '(for example a deploy-only CI role).'} + + 'against table permissions instead, so this list does not restrict them.'}

{allowlistRejected && ( // validateNoSUPerms rejects any multi-key permission setting super_user/cluster_user, so @@ -135,20 +146,22 @@ export function OperationsAllowlistEditor({ removed when you save.

)} - {structureUserDdl && restricted && ( -

- This role is a structure user, so it reaches{' '} - {STRUCTURE_USER_DDL_OPERATIONS.join(', ')} - {structureUserDdl === true - ? ' (and create/drop database) on any database regardless of the list below.' - : ( - <> - {' '}on {structureUserDdl.join(', ')}{' '} - regardless of the list below — and listing them here cannot reach another database. - - )} Every other operation is still gated by the list. -

- )} + {structureUserDdl && restricted && (gatesEveryPath + ? + : ( +

+ This role is a structure user, so it reaches{' '} + {STRUCTURE_USER_DDL_OPERATIONS.join(', ')} + {structureUserDdl === true + ? ' (and create/drop database) on any database regardless of the list below.' + : ( + <> + {' '}on {structureUserDdl.join(', ')}{' '} + regardless of the list below — and listing them here cannot reach another database. + + )} Every other operation is still gated by the list. +

+ ))} {restricted && ( <> @@ -200,9 +213,10 @@ export function OperationsAllowlistEditor({ ? (

{value?.length ? 'Every entry here is inert' : 'Nothing selected'}{' '} - — this role cannot run any Operations API call{structureUserDdl ? ', except the DDL noted above' : ''} - {' '} - (SQL is gated by table permissions, not this list). + — this role cannot run any Operations API call{gatesEveryPath + ? `, not even SQL${structureUserDdl ? ' or its structure-user DDL' : ''}.` + : `${structureUserDdl ? ', except the DDL noted above' : ''}` + + ' (SQL is gated by table permissions, not this list).'}

) : ( @@ -218,6 +232,32 @@ export function OperationsAllowlistEditor({ ); } +/** The flag still scopes which databases its DDL may touch, but that DDL no longer skips the list. */ +function GatedStructureUserNote({ scope, effective }: { scope: true | string[]; effective: readonly string[] }) { + const covered = scope === true + ? [...STRUCTURE_USER_DDL_OPERATIONS, ...STRUCTURE_USER_DATABASE_OPERATIONS] + : STRUCTURE_USER_DDL_OPERATIONS; + const unlisted = covered.filter((name) => !effective.includes(name)); + return ( +

0 ? 'text-warning' : 'text-muted-foreground')}> + This role is a structure user{scope === true ? '' : ( + <> + {' '}on {scope.join(', ')} + + )}, but on this Harper version the list below gates its DDL too: it can run{' '} + {covered.join(', ')} + {scope === true + ? ' only when they are listed.' + : ' on those databases only when they are listed, and listing them cannot reach another database.'} + {unlisted.length > 0 && ( + <> + {' '}Not listed, so Harper refuses them: {unlisted.join(', ')}. + + )} +

+ ); +} + function GroupCheckbox({ group, note, @@ -449,10 +489,8 @@ function OperationChip({ : info?.nonDelegable ? 'Always requires an actual super_user role; listing it cannot delegate the operation to a ' + 'non-super_user role.' - : info?.outOfGate - ? info.caveat - : info?.caveat - ? info.caveat + : isOutOfGate(name, band) + ? info?.caveat : info?.su ? 'Normally requires super_user; listing it grants it to this role.' : info diff --git a/src/features/instance/config/roles/operations/RoleOperationsSummary.test.tsx b/src/features/instance/config/roles/operations/RoleOperationsSummary.test.tsx index 9dfa873ba..89e493c8a 100644 --- a/src/features/instance/config/roles/operations/RoleOperationsSummary.test.tsx +++ b/src/features/instance/config/roles/operations/RoleOperationsSummary.test.tsx @@ -14,6 +14,7 @@ vi.mock('@/features/instance/config/roles/operations/useOperationsAllowlistSuppo })); const ON_5_3: AllowlistVersion = { supported: true, band: '5.3' }; +const BEFORE_5_3: AllowlistVersion = { supported: true, band: 'pre-5.3' }; beforeEach(() => allowlistVersion.mockReturnValue(ON_5_3)); @@ -53,11 +54,13 @@ describe('RoleOperationsSummary', () => { }); it('notes the DDL carve-out for a structure_user role instead of calling the list inert', () => { + allowlistVersion.mockReturnValue(BEFORE_5_3); render(); expect(screen.getByText(/create\/drop database — apply on any/)).toBeTruthy(); }); it('does not leave the absolute wording standing for a structure_user with an empty list', () => { + allowlistVersion.mockReturnValue(BEFORE_5_3); render(); const text = screen.getByText(/cannot run any Operations API call/).textContent ?? ''; // The DDL carve-out has to qualify the sentence, not contradict it after a full stop. @@ -65,6 +68,36 @@ describe('RoleOperationsSummary', () => { expect(text).toContain('except that it is a structure user'); }); + it('claims no DDL carve-out where the list runs ahead of structure_user', () => { + // 5.3.0-alpha.1 on: Harper checks the allowlist before the structure-user returns. + for (const band of ['5.3-early', '5.3'] as const) { + allowlistVersion.mockReturnValue({ supported: true, band }); + render(); + const text = screen.getByText(/restricted to 13 operations/).textContent ?? ''; + expect(text, band).toContain('its structure-user DDL needs listing too'); + expect(text, band).not.toContain('regardless of the list'); + cleanup(); + + render(); + const empty = screen.getByText(/cannot run any Operations API call/).textContent ?? ''; + expect(empty, band).toContain('not even SQL or its structure-user DDL.'); + expect(empty, band).not.toContain('except that it is a structure user'); + expect(empty, band).not.toContain('SQL is authorized by table permissions'); + cleanup(); + } + }); + + it("keeps a scoped structure user's databases in the note where the list gates its DDL", () => { + // verifyPerms still applies the structure_user array after gate 1, so a listed DDL op reaches only these. + for (const band of ['5.3-early', '5.3'] as const) { + allowlistVersion.mockReturnValue({ supported: true, band }); + render(); + const text = screen.getByText(/restricted to 13 operations/).textContent ?? ''; + expect(text, band).toContain('its structure-user DDL on dev, qa needs listing too'); + cleanup(); + } + }); + it('says the entries are inert rather than claiming the list is empty', () => { // A directly granted alias is listed but grants nothing, so "is empty" would contradict the JSON. render(); @@ -79,10 +112,28 @@ describe('RoleOperationsSummary', () => { it('summarizes the effective expansion with a truncated preview and full title', () => { render(); - const summary = screen.getByText(/restricted to 12 operations/); + // read_only's sql member counts once the list gates SQL; before 5.3 it does not. + const summary = screen.getByText(/restricted to 13 operations/); // Five names shown inline, the full list on the title. expect(summary.textContent).toContain(', …'); expect(summary.getAttribute('title')).toContain('user_info'); + expect(summary.getAttribute('title')).toContain('sql'); + + cleanup(); + allowlistVersion.mockReturnValue(BEFORE_5_3); + render(); + expect(screen.getByText(/restricted to 12 operations/).getAttribute('title')).not.toContain('sql'); + }); + + it('says an empty list leaves SQL to table permissions before 5.3 only', () => { + allowlistVersion.mockReturnValue(BEFORE_5_3); + render(); + expect(screen.getByText(/SQL is authorized by table permissions instead/)).toBeTruthy(); + + cleanup(); + allowlistVersion.mockReturnValue(ON_5_3); + render(); + expect(screen.getByText(/cannot run any Operations API call, not even SQL\./)).toBeTruthy(); }); it('counts a deploy-only CI role by what its instance version grants', () => { diff --git a/src/features/instance/config/roles/operations/RoleOperationsSummary.tsx b/src/features/instance/config/roles/operations/RoleOperationsSummary.tsx index 469a7cb27..9191ea27e 100644 --- a/src/features/instance/config/roles/operations/RoleOperationsSummary.tsx +++ b/src/features/instance/config/roles/operations/RoleOperationsSummary.tsx @@ -1,4 +1,5 @@ import { + allowlistGatesEveryPath, expandEffectiveOperations, summarizeOperations, } from '@/features/instance/config/roles/operations/operationsCatalog'; @@ -50,27 +51,35 @@ export function RoleOperationsSummary({ role }: { role: LocalRole | undefined })

); } + const gatesEveryPath = allowlistGatesEveryPath(allowlistVersion.band); // Phrased to follow either "…cannot run any operation" or "…restricted to N operations", so the // absolute wording is never left standing on a role that still reaches DDL. const ddlScope = structureUserDdlScope(permission); - const ddlNote = ddlScope === true + const ddlNote = !ddlScope + ? '' + : gatesEveryPath + ? ` (its structure-user DDL${ddlScope === true ? '' : ` on ${ddlScope.join(', ')}`} needs listing too)` + : ddlScope === true ? ', except that it is a structure user: table and attribute DDL — and create/drop database — apply on any' + ' database regardless of the list.' - : ddlScope - ? `, except that it is a structure user: table and attribute DDL applies on ${ddlScope.join(', ')}` - + ' regardless of the list, and listing those operations cannot reach another database.' - : ''; + : `, except that it is a structure user: table and attribute DDL applies on ${ddlScope.join(', ')}` + + ' regardless of the list, and listing those operations cannot reach another database.'; const effective = expandEffectiveOperations(getOperationsAllowlist(permission) ?? [], allowlistVersion.band); if (effective.length === 0) { return (

{(getOperationsAllowlist(permission) ?? []).length ? "Every entry in this role's operations allowlist is inert" - : "This role's operations allowlist is empty"}{' '} - — users with it cannot run any Operations API call (SQL is authorized by table permissions instead, so it is - unaffected) - {ddlScope ? '' : '.'} - {ddlNote} + : "This role's operations allowlist is empty"} — users with it cannot run any Operations API call + {gatesEveryPath + ? `, not even SQL${ddlScope ? ' or its structure-user DDL' : ''}.` + : ( + <> + {' '}(SQL is authorized by table permissions instead, so it is unaffected) + {ddlScope ? '' : '.'} + {ddlNote} + + )}

); } diff --git a/src/features/instance/config/roles/operations/operationsCatalog.test.ts b/src/features/instance/config/roles/operations/operationsCatalog.test.ts index bc8bd86c0..47661003a 100644 --- a/src/features/instance/config/roles/operations/operationsCatalog.test.ts +++ b/src/features/instance/config/roles/operations/operationsCatalog.test.ts @@ -1,4 +1,5 @@ import { + allowlistGatesEveryPath, canonicalOperationName, expandEffectiveOperations, gateVerdict, @@ -8,6 +9,7 @@ import { GrantBand, isInertGrant, isOperationGroupName, + isOutOfGate, OPERATION_CATALOG, OPERATION_CATEGORIES, OPERATION_GROUPS, @@ -178,20 +180,14 @@ describe('expandEffectiveOperations', () => { // Harper's group lists both spellings of two alias pairs (describe_schema/describe_database, // search_by_hash/search_by_id), but each pair reaches one handler — counting both would // overstate the role's reach against the chips shown beside the count. - expect(expandEffectiveOperations(['read_only'], '5.3')).toEqual([ - 'describe_all', - 'describe_metric', - 'describe_schema', - 'describe_table', - 'get_analytics', - 'get_job', - 'list_metrics', - 'search', - 'search_by_conditions', - 'search_by_hash', - 'search_by_value', - 'user_info', - ]); + expect(expandEffectiveOperations(['read_only'], 'pre-5.3')).toEqual(READ_ONLY_PRE_5_3); + }); + + it('counts the sql member of read_only once the list gates the SQL path', () => { + // Harper's read_only carries sql, which the gate checks from 5.3.0-alpha.1 on. + for (const band of ['5.3-early', '5.3'] as const) { + expect(expandEffectiveOperations(['read_only'], band), band).toEqual([...READ_ONLY_PRE_5_3, 'sql'].sort()); + } }); it('does not credit a directly granted alias with its canonical operation', () => { @@ -202,9 +198,15 @@ describe('expandEffectiveOperations', () => { }); it('leaves out entries the allowlist does not govern', () => { - // set_secret and the trust-policy operations self-enforce super_user; sql is authorized on its - // own path, so listing any of them changes nothing about what the role can do. - expect(expandEffectiveOperations(['set_secret', 'sql', 'add_oidc_trust', 'list_oidc_trust'], '5.3')).toEqual([]); + // set_secret and the trust-policy operations self-enforce super_user, so listing them changes + // nothing about what the role can do. + expect(expandEffectiveOperations(['set_secret', 'add_oidc_trust', 'list_oidc_trust'], '5.3')).toEqual([]); + }); + + it('counts a direct sql grant only where the gate checks the SQL path', () => { + expect(expandEffectiveOperations(['sql'], 'pre-5.3')).toEqual([]); + expect(expandEffectiveOperations(['sql'], '5.3-early')).toEqual(['sql']); + expect(expandEffectiveOperations(['sql'], '5.3')).toEqual(['sql']); }); it('de-duplicates group members against explicit names and passes unknown names through', () => { @@ -223,11 +225,13 @@ describe('expandEffectiveOperations', () => { it('counts the deploy operations from 5.3 and not before', () => { const deployRole = ['deploy_component', 'get_job']; expect(expandEffectiveOperations(deployRole, 'pre-5.3')).toEqual(['get_job']); + // The early 5.3 prereleases already gate first but have not named the deploy entries yet. + expect(expandEffectiveOperations(deployRole, '5.3-early')).toEqual(['get_job']); expect(expandEffectiveOperations(deployRole, '5.3')).toEqual(['deploy_component', 'get_job']); }); it('never counts the operations Harper keeps out of every allowlist', () => { - for (const band of ['pre-5.3', '5.3'] as const) { + for (const band of ['pre-5.3', '5.3-early', '5.3'] as const) { expect(expandEffectiveOperations(['get_backup', 'read_transaction_log'], band)).toEqual([]); } }); @@ -237,6 +241,22 @@ describe('expandEffectiveOperations', () => { }); }); +// read_only's members less the two folded alias pairs and sql, which pre-5.3 Harper does not gate. +const READ_ONLY_PRE_5_3 = [ + 'describe_all', + 'describe_metric', + 'describe_schema', + 'describe_table', + 'get_analytics', + 'get_job', + 'list_metrics', + 'search', + 'search_by_conditions', + 'search_by_hash', + 'search_by_value', + 'user_info', +]; + // Harper's own record of which names gate 1 grants by name (unitTests/utility/operation_authorization.test.js // at v5.3.0, GRANTED_BY_NAME): the twenty entries that were unnamed before 5.3. const NAMED_FROM_5_3 = [ @@ -269,8 +289,14 @@ describe('resolveGrantBand', () => { } }); - it('keeps the 5.3 prereleases that predate it, and every 5.2, on the old side', () => { - for (const version of ['5.3.0-alpha.1', '5.3.0-beta.1', '5.3.0-beta.2', '5.2.15', '5.0.0']) { + it('puts the 5.3 prereleases that gate first but predate the names in their own band', () => { + for (const version of ['5.3.0-alpha.1', '5.3.0-beta.1', '5.3.0-beta.2']) { + expect(resolveGrantBand(version), version).toBe('5.3-early'); + } + }); + + it('keeps every 5.2 and earlier on the old side', () => { + for (const version of ['5.2.15', '5.2.99', '5.0.0-alpha.8', '5.0.0']) { expect(resolveGrantBand(version), version).toBe('pre-5.3'); } }); @@ -282,12 +308,22 @@ describe('resolveGrantBand', () => { }); }); +describe('allowlistGatesEveryPath', () => { + it('is the gate-order change at 5.3.0-alpha.1, not the naming change at beta.3', () => { + expect(allowlistGatesEveryPath('pre-5.3')).toBe(false); + expect(allowlistGatesEveryPath('5.3-early')).toBe(true); + expect(allowlistGatesEveryPath('5.3')).toBe(true); + }); +}); + describe('gateVerdict', () => { it('marks exactly the twenty newly named entries as needing 5.3', () => { - const needs53 = OPERATION_CATALOG.filter((operation) => gateVerdict(operation.name, 'pre-5.3') === 'needs-5.3') - .map((operation) => operation.name) - .sort(); - expect(needs53).toEqual(NAMED_FROM_5_3); + for (const band of ['pre-5.3', '5.3-early'] as const) { + const needs53 = OPERATION_CATALOG.filter((operation) => gateVerdict(operation.name, band) === 'needs-5.3') + .map((operation) => operation.name) + .sort(); + expect(needs53, band).toEqual(NAMED_FROM_5_3); + } for (const name of NAMED_FROM_5_3) { expect(gateVerdict(name, '5.3'), name).toBe('grants'); } @@ -299,6 +335,8 @@ describe('gateVerdict', () => { .map((operation) => operation.name) .sort(); expect(never('pre-5.3')).toEqual(['get_backup', 'read_transaction_log']); + // catchup still matches through the handler-name fallback until beta.3 names it null. + expect(never('5.3-early')).toEqual(['get_backup', 'read_transaction_log']); expect(never('5.3')).toEqual(['catchup', 'get_backup', 'read_transaction_log']); }); @@ -312,6 +350,21 @@ describe('isInertGrant', () => { expect(isInertGrant('deploy_component', 'pre-5.3')).toBe(true); expect(isInertGrant('deploy_component', '5.3')).toBe(false); }); + + it('calls sql inert only where the SQL path skips the gate', () => { + expect(isInertGrant('sql', 'pre-5.3')).toBe(true); + expect(isInertGrant('sql', '5.3-early')).toBe(false); + expect(isInertGrant('sql', '5.3')).toBe(false); + }); +}); + +describe('isOutOfGate', () => { + it('places sql outside the gate before 5.3 only, and nothing else in any band', () => { + for (const band of ['pre-5.3', '5.3-early', '5.3'] as const) { + const outside = OPERATION_CATALOG.filter((operation) => isOutOfGate(operation.name, band)).map((o) => o.name); + expect(outside, band).toEqual(band === 'pre-5.3' ? ['sql'] : []); + } + }); }); describe('canonicalOperationName', () => { diff --git a/src/features/instance/config/roles/operations/operationsCatalog.ts b/src/features/instance/config/roles/operations/operationsCatalog.ts index 8288111b4..78287bfe7 100644 --- a/src/features/instance/config/roles/operations/operationsCatalog.ts +++ b/src/features/instance/config/roles/operations/operationsCatalog.ts @@ -14,10 +14,11 @@ import { wasAReleasedBeforeB } from '@/lib/string/wasAReleasedBeforeB'; * array denies every operation. * - A listed operation that normally requires super_user is treated as a deliberate admin grant * and allowed without super_user (`su` below marks those). - * - super_user reaches operations without consulting the list, and cannot even hold one: - * validateNoSUPerms rejects a permission that sets super_user/cluster_user alongside other keys. - * structure_user roles DO go through the gate for everything except DDL. - * - `sql` never reaches this gate at all; it is authorized against table permissions. + * - super_user cannot hold a list: validateNoSUPerms rejects a permission that sets + * super_user/cluster_user alongside other keys. + * - Before 5.3 the gate runs after verifyPerms' super_user and structure_user returns, so a + * structure user's DDL skips it, and `sql` never reaches it. From 5.3.0-alpha.1 it runs first and + * on the SQL path too (see `GrantBand`). */ export interface GrantableOperation { name: string; @@ -35,32 +36,56 @@ export interface GrantableOperation { nonDelegable?: boolean; /** Legacy wire alias; Harper's authorization entry is registered under the canonical name. */ aliasOf?: string; - /** Extra qualification shown wherever the operation is offered or listed. */ + /** The band in which it is authorized on a path that never consults the list. */ + outOfGate?: GrantBand; + /** Why the list does not govern it, shown on its chip wherever `outOfGate` applies. */ caveat?: string; - /** Authorized on its own path, so the allowlist neither grants nor restricts it. */ - outOfGate?: boolean; /** Where listing the name stops or starts matching the gate; absent = it matches on every 5.x. */ gate?: GateChange; } /** - * Gate 1 checks a listed name against the authorization entry's `api_name`. Before 5.3 that fell back - * to the handler's own name when the entry had none, which no snake_case grant can equal - * (HarperFast/harper#2175); 5.3 names every entry, deliberately naming a few `null`. + * Gate 1 checks a listed name against the authorization entry's `api_name`. Before 5.3.0-beta.3 that + * fell back to the handler's own name when the entry had none, which no snake_case grant can equal + * (HarperFast/harper#2175); beta.3 names every entry, deliberately naming a few `null`. "5.3" below + * means the `5.3` band: * - `from-5.3`: unnamed before 5.3, so listing it grants nothing there. * - `until-5.3`: registered under its wire name, so the fallback matched it; named `null` from 5.3. * - `never`: no version lets a listing reach it, so it is not offered. */ export type GateChange = 'from-5.3' | 'until-5.3' | 'never'; -/** Which side of that change an instance is on; resolve it once per version, not per operation. */ -export type GrantBand = 'pre-5.3' | '5.3'; +/** + * Which allowlist rules an instance follows; resolve it once per version, not per operation. Harper + * changed gate 1 twice during 5.3, so the bands are named for those changes, not for releases: + * - `pre-5.3`: the gate runs after the super_user and structure_user returns, and the SQL path never + * calls it. + * - `5.3-early` (5.3.0-alpha.1 to beta.2): it runs ahead of both and on the SQL path, but the + * entries `from-5.3` marks are still unnamed. + * - `5.3` (5.3.0-beta.3 on): every entry names its grant as well. + */ +export type GrantBand = 'pre-5.3' | '5.3-early' | '5.3'; + +/** The first tagged build that runs gate 1 first and on `sql` (harper f8a5aa90a). */ +const ALLOWLIST_GATE_RUNS_FIRST_FROM = '5.3.0-alpha.1'; /** The first tagged build that names every authorization entry, not 5.3.0's earliest prerelease. */ export const GATE_NAMES_EVERY_ENTRY_FROM = '5.3.0-beta.3'; export function resolveGrantBand(version: string): GrantBand { - return wasAReleasedBeforeB(GATE_NAMES_EVERY_ENTRY_FROM, version) ? '5.3' : 'pre-5.3'; + if (wasAReleasedBeforeB(GATE_NAMES_EVERY_ENTRY_FROM, version)) { + return '5.3'; + } + return wasAReleasedBeforeB(ALLOWLIST_GATE_RUNS_FIRST_FROM, version) ? '5.3-early' : 'pre-5.3'; +} + +/** Whether a structure user's DDL and `sql` have to be listed too, rather than skipping the list. */ +export function allowlistGatesEveryPath(band: GrantBand): boolean { + return band !== 'pre-5.3'; +} + +export function isOutOfGate(name: string, band: GrantBand): boolean { + return catalogByName.get(name)?.outOfGate === band; } export type GateVerdict = 'grants' | 'needs-5.3' | 'never'; @@ -81,13 +106,13 @@ export function gateVerdict(name: string, band: GrantBand): GateVerdict { /** * Whether listing this name changes nothing: an alias the gate resolves elsewhere, an operation - * whose handler self-enforces super_user, one the gate cannot match, or `sql`, which is authorized - * on its own path and so is neither granted nor restricted by the list. The chip and the effective - * count must agree on this, or the same component credits access it also calls inert. + * whose handler self-enforces super_user, one the gate cannot match, or one this band authorizes + * on its own path. The chip and the effective count must agree on this, or the same component + * credits access it also calls inert. */ export function isInertGrant(name: string, band: GrantBand): boolean { const info = catalogByName.get(name); - return !!info?.aliasOf || !!info?.nonDelegable || !!info?.outOfGate || gateVerdict(name, band) !== 'grants'; + return !!info?.aliasOf || !!info?.nonDelegable || isOutOfGate(name, band) || gateVerdict(name, band) !== 'grants'; } export interface OperationGroup { @@ -155,11 +180,11 @@ export const OPERATION_CATALOG: readonly GrantableOperation[] = [ name: 'sql', su: false, category: DATA, - // serverUtilities routes `sql` to checkASTPermissions/verifyPermsAST, which never consults - // the allowlist — so this entry neither grants nor restricts SQL (HarperFast/harper#2175). - outOfGate: true, - caveat: 'SQL is authorized against table permissions, not this list — listing or omitting it ' - + 'does not restrict SQL statements.', + // Before 5.3 serverUtilities sends `sql` straight to checkASTPermissions, which never consults the + // allowlist. From 5.3.0-alpha.1 it runs gate 1 on `sql` first, so it is a grant like any other. + outOfGate: 'pre-5.3', + caveat: "This instance's Harper authorizes SQL against table permissions only, so listing or omitting " + + 'sql here changes nothing. Harper 5.3 and later gate it by this list as well.', }, { name: 'insert', su: false, category: DATA }, { name: 'update', su: false, category: DATA }, diff --git a/src/features/instance/config/roles/preparePermissionForSave.ts b/src/features/instance/config/roles/preparePermissionForSave.ts index 8b76f40a8..b588432ab 100644 --- a/src/features/instance/config/roles/preparePermissionForSave.ts +++ b/src/features/instance/config/roles/preparePermissionForSave.ts @@ -9,7 +9,7 @@ import { RESERVED_PERMISSION_KEYS, rolePreventsOperationsAllowlist } from '@/int * `operations` survives only where Harper accepts it: `validateNoSUPerms` rejects any multi-key * permission setting `super_user: true` or `cluster_user: true`, so keeping the allowlist there * would fail the save outright — it is dropped alongside the table permissions instead. A - * `structure_user` role may carry one (the gate still runs for everything but DDL), and it + * `structure_user` role may carry one (Harper gates it, and from 5.3.0-alpha.1 its DDL too), and it * survives only as an actual array: any other shape under that key is not an allowlist. * * `structure_user` may also be an array of database names, which grants DDL on *those* databases diff --git a/src/hooks/checkOperationPermission.test.ts b/src/hooks/checkOperationPermission.test.ts index 9c79e3df3..f8da8369a 100644 --- a/src/hooks/checkOperationPermission.test.ts +++ b/src/hooks/checkOperationPermission.test.ts @@ -106,8 +106,8 @@ describe('operation mappings', () => { }); describe('checkTableActionAllowed', () => { - // verifyPerms clears a super_user before reaching the allowlist, so gating one would hide UI that - // works. + // Before 5.3 verifyPerms clears a super_user ahead of the allowlist, and from 5.3 Harper refuses to + // store a list on one, so gating one would hide UI that works. it('ignores the allowlist for a super_user', () => { expect(checkTableActionAllowed(perm({ super_user: true, operations: [] }), 'insert')).toBe(true); expect(checkTableActionAllowed(perm({ super_user: true, operations: ['read_only'] }), 'delete')).toBe( @@ -115,8 +115,7 @@ describe('checkTableActionAllowed', () => { ); }); - // isElevatedRole counts these two as well, because it answers a broader question. DML is where - // they are still gated. + // Elevated, but not past the list for DML on any version. it('still applies the allowlist to structure_user and cluster_user', () => { expect(checkTableActionAllowed(perm({ structure_user: true, operations: ['read_only'] }), 'insert')) .toBe(false); diff --git a/src/hooks/checkOperationPermission.ts b/src/hooks/checkOperationPermission.ts index 131fca745..b87b4e51e 100644 --- a/src/hooks/checkOperationPermission.ts +++ b/src/hooks/checkOperationPermission.ts @@ -11,9 +11,10 @@ import type { ImportSource } from '@/integrations/api/instance/database/importDa import { getOperationsAllowlist } from '@/integrations/api/localRolePermission'; /** - * Harper's `operations` allowlist (5.0+) is reached only by roles verifyPerms has not already cleared: - * `isSuperUser && !isSuSystemOperation` returns first, and `structure_user` returns for create/drop - * database, table, and attribute. Table reads and writes are the only Studio surface it can deny. + * Harper's `operations` allowlist (5.0+) never applies to a super_user: before 5.3 verifyPerms clears + * one first, and from 5.3.0-alpha.1, where the list runs ahead of every privilege return, + * validateNoSUPerms refuses to store a list on one. Table reads and writes are the only Studio + * surface these checks gate. */ export const TABLE_ACTION_OPERATIONS: Record = { @@ -114,9 +115,8 @@ export function checkAnyOperationAllowed( /** * Whether these operations survive gate 1 for a table action. * - * Not `isElevatedRole`: that answers whether the allowlist is unenforceable anywhere for the role, - * which is what the roles editor warns about. DML is where an elevated-but-not-super role is still - * gated — `structure_user` short-circuits for DDL alone, and `cluster_user` appears nowhere in + * Only super_user short-circuits here. DML is gated for every other role on every version: before 5.3 + * `structure_user` skips the list for DDL alone, and `cluster_user` appears nowhere in * operation_authorization.ts. */ export function checkTableOperationsAllowed( diff --git a/src/hooks/checkSchemaTablePermission.test.ts b/src/hooks/checkSchemaTablePermission.test.ts index e8a2402cf..bd939760b 100644 --- a/src/hooks/checkSchemaTablePermission.test.ts +++ b/src/hooks/checkSchemaTablePermission.test.ts @@ -85,7 +85,7 @@ describe('checkSchemaTablePermission', () => { ).toBe(false); }); - // verifyPerms clears a super_user before reaching the allowlist. + // Before 5.3 verifyPerms clears a super_user ahead of the allowlist; from 5.3 Harper refuses to store one. it('leaves super_user alone, allowlist or not', () => { expect(checkSchemaTablePermission(perm({ super_user: true, operations: [] }), 'data', 'dog', 'insert')) .toBe(true); diff --git a/src/hooks/usePermissions.ts b/src/hooks/usePermissions.ts index c45ad2efa..e91586d98 100644 --- a/src/hooks/usePermissions.ts +++ b/src/hooks/usePermissions.ts @@ -225,7 +225,8 @@ export function useInstanceManagePermission(entityId?: EntityIds): boolean { // (We're probably still loading the user.) return false; } - // Not gated on `operations`: verifyPerms clears a super_user before reaching the allowlist. + // Not gated on `operations`: before 5.3 verifyPerms clears a super_user ahead of the allowlist, and + // from 5.3.0-alpha.1, where the list runs first, validateNoSUPerms refuses to store one on a super_user. return permission.super_user === true; } @@ -238,7 +239,8 @@ export function useInstanceBrowseManagePermission(entityId?: EntityIds): boolean // (We're probably still loading the user.) return false; } - // Not gated on `operations` either: structure_user is cleared for every DDL operation this covers. + // Not gated on `operations` either, though only pre-5.3 clears a structure user's DDL ahead of the list: + // this hook cannot see the version, and gating here would hide DDL that works on 5.2 (HarperFast/studio#1894). return permission.super_user === true || permission.structure_user === true; } diff --git a/src/integrations/api/localRolePermission.ts b/src/integrations/api/localRolePermission.ts index 0ee7a89c3..32b9e5107 100644 --- a/src/integrations/api/localRolePermission.ts +++ b/src/integrations/api/localRolePermission.ts @@ -56,12 +56,13 @@ export function rolePreventsOperationsAllowlist(permission: LocalRolePermission } /** - * Which DDL this role reaches without consulting the allowlist, matching verifyPerms: + * Which DDL the role's `structure_user` flag covers, matching verifyPerms: * - `true`: an unscoped structure user — the four table/attribute DDL operations anywhere, plus * create/drop database (that pair requires the boolean form). * - an array: DDL on those databases only, and never create/drop database. - * - `false`: not a structure user; every operation goes through the gate. - * Everything outside that carve-out is still gated for both forms. + * - `false`: not a structure user. + * The shape only: whether that DDL also needs the allowlist depends on the version + * (`allowlistGatesEveryPath`). */ export function structureUserDdlScope( permission: LocalRolePermission | undefined, @@ -73,7 +74,7 @@ export function structureUserDdlScope( return Array.isArray(structureUser) && structureUser.length > 0 ? structureUser : false; } -/** The DDL operations a structure_user role reaches regardless of its allowlist. */ +/** The DDL operations `structure_user` covers in either form (Harper's STRUCTURE_USER_OPS). */ export const STRUCTURE_USER_DDL_OPERATIONS = [ 'create_table', 'create_attribute', @@ -81,6 +82,9 @@ export const STRUCTURE_USER_DDL_OPERATIONS = [ 'drop_attribute', ]; +/** The database DDL only the boolean form covers. */ +export const STRUCTURE_USER_DATABASE_OPERATIONS = ['create_database', 'drop_database']; + /** * The role's operations allowlist, but only when it is well-formed (an array of strings). * Anything else — absent, or a shape the structured UI must not touch — is undefined; use