diff --git a/AGENTS.md b/AGENTS.md index 07e5d771d..ac6c2c8fc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -450,10 +450,11 @@ Four things worth knowing before changing the record editor: save that 403s. Check both `attribute_permissions` (v5) and `attribute_restrictions` (translated v4). Only `super_user` short-circuits; `structure_user` covers DDL, not DML. - **Version-gated, but an unreadable version does NOT block the save.** Harper denies - `registration_info` to any role carrying an `operations` allowlist — it is in the gate-inert list in - `config/roles/operations/operationsCatalog.ts` — which is exactly the shape of role - `checkTablePutPermission` exists to accept. Blocking on an unread version therefore made the - feature permanently unavailable for those roles, behind a "reload and try again" that never helps. + `registration_info` to any role whose `operations` allowlist does not list it (and before 5.3 to one + that does — `gate: 'from-5.3'` in `config/roles/operations/operationsCatalog.ts`), which is exactly + the shape of role `checkTablePutPermission` exists to accept. Blocking on an unread version therefore + made the feature permanently unavailable for those roles, behind a "reload and try again" that never + helps. Allowing it is safe because the fallback isn't silent: `put` is a distinct operation name, so a pre-5.3 instance rejects the request outright rather than merging and keeping the attribute. A version that IS readable and predates the operation still blocks, for the clearer message, and @@ -583,21 +584,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. -- **~23 operations are inert when granted.** Their `requiredPermissions` entry omits `api_name`, so - the gate compares a camelCase handler name against a snake_case grant and never matches - (`deploy_component`, `get_status`, `registration_info`, …). Filed as HarperFast/harper#2175. - `sql` is worse: it routes to `verifyPermsAST`, which never consults the allowlist at all. +- **`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` 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 @@ -613,22 +632,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 @@ -646,6 +671,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/constants/tableDefinition.tsx b/src/features/instance/config/roles/constants/tableDefinition.tsx index 1bd5e80cd..0a216afa2 100644 --- a/src/features/instance/config/roles/constants/tableDefinition.tsx +++ b/src/features/instance/config/roles/constants/tableDefinition.tsx @@ -79,12 +79,12 @@ export const dataTableColumns: Array> = [ function OperationsCell({ permission }: { permission: LocalRole['permission'] }) { // One observer for the whole table: the query is shared, but a hook per row would still // allocate an observer and query options for every one of them. - const allowlistSupported = useContext(AllowlistSupportedContext); - if (allowlistSupported === undefined) { + const allowlistVersion = useContext(AllowlistSupportedContext); + if (allowlistVersion === undefined) { // The verdict depends on the version; render nothing rather than guess it for a frame. return null; } - const kind = classifyOperationsValue(permission, allowlistSupported); + const kind = classifyOperationsValue(permission, allowlistVersion.supported); // Below the floor the key is either a real database grant or inert; neither is a restriction. if (kind === 'absent' || kind === 'database' || kind === 'inert') { // aria-label is not exposed on a roleless span, so the text itself has to carry it. @@ -115,7 +115,7 @@ function OperationsCell({ permission }: { permission: LocalRole['permission'] }) ); } - const effective = expandEffectiveOperations(getOperationsAllowlist(permission) ?? []); + const effective = expandEffectiveOperations(getOperationsAllowlist(permission) ?? [], allowlistVersion.band); const summary = summarizeOperations(effective); return ( diff --git a/src/features/instance/config/roles/operations/AllowlistSupportedContext.tsx b/src/features/instance/config/roles/operations/AllowlistSupportedContext.tsx index 9cd7bd56b..aa1a8f7d2 100644 --- a/src/features/instance/config/roles/operations/AllowlistSupportedContext.tsx +++ b/src/features/instance/config/roles/operations/AllowlistSupportedContext.tsx @@ -1,13 +1,16 @@ -import { useOperationsAllowlistSupported } from '@/features/instance/config/roles/operations/useOperationsAllowlistSupported'; +import { + AllowlistVersion, + useAllowlistVersion, +} from '@/features/instance/config/roles/operations/useOperationsAllowlistSupported'; import { createContext, PropsWithChildren } from 'react'; /** - * The instance's allowlist support, resolved once for a list rather than per row. The underlying + * The instance's allowlist verdicts, resolved once for a list rather than per row. The underlying * query is shared, but a hook call per row still allocates an observer and query options for every * row — including the ordinary case where no role carries an `operations` key. */ -export const AllowlistSupportedContext = createContext(undefined); +export const AllowlistSupportedContext = createContext(undefined); export function AllowlistSupportedProvider({ children }: PropsWithChildren) { - return {children}; + return {children}; } diff --git a/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx b/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx index e3eb030c8..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,15 +152,158 @@ describe('OperationsAllowlistEditor', () => { expect(screen.getByText(/except the DDL noted above/)).toBeTruthy(); }); - it('marks a grant that the server cannot currently enforce', () => { + 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(); filterPicker('deploy_component'); const item = screen.getByRole('menuitemcheckbox', { name: /deploy_component/ }); - expect(within(item).getByText('not yet enforced')).toBeTruthy(); + expect(within(item).getByText('needs newer Harper')).toBeTruthy(); expect(within(item).queryByText('super_user')).toBeNull(); }); + it('treats the deploy operations as real super_user delegations on 5.3', () => { + render(); + expect(screen.getByText(/Effectively allows 2 operations, including some that normally require super_user/)) + .toBeTruthy(); + expect(screen.queryByText(/Every entry here is inert/)).toBeNull(); + + openPicker(); + filterPicker('deploy_component'); + const item = screen.getByRole('menuitemcheckbox', { name: /deploy_component/ }); + expect(within(item).getByText('super_user')).toBeTruthy(); + expect(within(item).queryByText('needs newer Harper')).toBeNull(); + }); + + it('stops offering catchup on 5.3, where no allowlist can reach it, and says so on a saved one', () => { + render(); + expect(screen.getByText(/Every entry here is inert/)).toBeTruthy(); + const chip = screen.getByRole('button', { name: 'Remove catchup' }).parentElement; + expect(chip?.getAttribute('title')).toMatch(/never lets an operations allowlist grant this operation/); + + openPicker(); + filterPicker('catchup'); + expect(screen.queryByRole('menuitemcheckbox', { name: /catchup/ })).toBeNull(); + }); + + it('does not offer a never-granted operation through the custom-grant path either', () => { + render(); + openPicker(); + for (const name of ['catchup', 'get_backup', 'read_transaction_log']) { + filterPicker(name); + expect(screen.queryAllByRole('menuitemcheckbox'), name).toHaveLength(0); + } + }); + + it('does not offer the trust-policy operations, whose handlers insist on a super_user', () => { + render(); + openPicker(); + for (const name of ['add_oidc_trust', 'list_oidc_trust', 'drop_oidc_trust']) { + filterPicker(name); + // Not even as a custom grant. + expect(screen.queryAllByRole('menuitemcheckbox'), name).toHaveLength(0); + } + }); + it('does not offer a case variant of a known operation as a custom grant', () => { render(); openPicker(); diff --git a/src/features/instance/config/roles/operations/OperationsAllowlistEditor.tsx b/src/features/instance/config/roles/operations/OperationsAllowlistEditor.tsx index 1ac38c8ee..a7ecd25ed 100644 --- a/src/features/instance/config/roles/operations/OperationsAllowlistEditor.tsx +++ b/src/features/instance/config/roles/operations/OperationsAllowlistEditor.tsx @@ -11,21 +11,30 @@ 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, getAvailableGroups, getGrantableOperations, getOperationInfo, GrantableOperation, - isGrantGateInert, + GrantBand, isInertGrant, isOperationGroupName, + isOutOfGate, OPERATION_CATALOG, OPERATION_CATEGORIES, OPERATION_GROUPS, OperationGroup, + 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'; @@ -51,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; }) { @@ -64,9 +73,11 @@ export function OperationsAllowlistEditor({ (group) => !offeredGroups.includes(group) && value?.includes(group.name), ); 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) : []; + const effective = value ? expandEffectiveOperations(value, band) : []; // expandEffectiveOperations already drops grants the server cannot honor, so anything left that // is su-only is a delegation the role really receives. const delegatesSu = effective.some((name) => getOperationInfo(name)?.su); @@ -115,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 @@ -131,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 && ( <> @@ -172,6 +189,7 @@ export function OperationsAllowlistEditor({ toggle(name)} disabled={disabled} /> @@ -194,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).'}

) : ( @@ -212,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, @@ -242,6 +288,10 @@ function GroupCheckbox({ ); } +const NEEDS_5_3_TITLE = "This instance's Harper checks this operation under its internal handler name, so listing " + + 'it has no effect here (HarperFast/harper#2175). Harper 5.3.0 and later grant it, as do prereleases from ' + + `${GATE_NAMES_EVERY_ENTRY_FROM}.`; + // Component-registered operation names are exact strings, not identifiers — camelCase, hyphens, // dots, and colons are all legal (`deploy-v2`, `acme.deploy`) — so keep the case and only rule // out whitespace/quote garbage that could never be an operation name. @@ -254,11 +304,13 @@ const CUSTOM_OPERATION_PATTERN = /^[A-Za-z][A-Za-z0-9_.:-]*$/; */ function OperationsPicker({ grantable, + band, selectedSet, onToggle, disabled, }: { grantable: GrantableOperation[]; + band: GrantBand; selectedSet: Set; onToggle: (name: string) => void; disabled?: boolean; @@ -308,6 +360,7 @@ function OperationsPicker({ && !caseVariantOfKnownName && !knownInfo?.nonDelegable && !knownInfo?.aliasOf + && gateVerdict(rawQuery, band) !== 'never' && !grantable.some((operation) => operation.name === rawQuery) && !selectedSet.has(rawQuery) ? rawQuery @@ -373,15 +426,10 @@ function OperationsPicker({ onSelect={(event) => event.preventDefault()} > {operation.name} - {isGrantGateInert(operation.name) + {gateVerdict(operation.name, band) === 'needs-5.3' ? ( - - not yet enforced + + needs newer Harper ) : operation.su && ( @@ -417,29 +465,32 @@ function OperationsPicker({ function OperationChip({ name, + band, onRemove, disabled, }: { name: string; + band: GrantBand; onRemove: () => void; disabled?: boolean; }) { const info = getOperationInfo(name); - const inert = isInertGrant(name); + const inert = isInertGrant(name, band); + const verdict = gateVerdict(name, band); const variant = inert ? 'destructive' : info?.su ? 'warning' : info ? 'secondary' : 'outline'; const title = info?.aliasOf ? `Legacy alias: Harper authorizes this operation under '${info.aliasOf}', so this entry grants nothing. ` + `Grant '${info.aliasOf}' instead.` - : isGrantGateInert(name) - ? 'Harper authorizes this operation under its internal handler name, so this grant has no effect on ' - + 'current 5.x releases (HarperFast/harper#2175).' + : verdict === 'needs-5.3' + ? NEEDS_5_3_TITLE + : verdict === 'never' + ? "This instance's Harper version never lets an operations allowlist grant this operation, so this entry " + + 'has no effect; only a super_user role can run it.' : 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 5f078e1d9..89e493c8a 100644 --- a/src/features/instance/config/roles/operations/RoleOperationsSummary.test.tsx +++ b/src/features/instance/config/roles/operations/RoleOperationsSummary.test.tsx @@ -2,17 +2,21 @@ * @vitest-environment jsdom */ import { RoleOperationsSummary } from '@/features/instance/config/roles/operations/RoleOperationsSummary'; +import type { AllowlistVersion } from '@/features/instance/config/roles/operations/useOperationsAllowlistSupported'; import { LocalRole } from '@/integrations/api/api.patch'; import { cleanup, render, screen } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; // The component reads the instance version to tell an allowlist from a pre-5.0 database record. -const { allowlistSupported } = vi.hoisted(() => ({ allowlistSupported: vi.fn<() => boolean | undefined>() })); +const { allowlistVersion } = vi.hoisted(() => ({ allowlistVersion: vi.fn<() => AllowlistVersion | undefined>() })); vi.mock('@/features/instance/config/roles/operations/useOperationsAllowlistSupported', () => ({ - useOperationsAllowlistSupported: () => allowlistSupported(), + useAllowlistVersion: () => allowlistVersion(), })); -beforeEach(() => allowlistSupported.mockReturnValue(true)); +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)); afterEach(() => cleanup()); @@ -50,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. @@ -62,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(); @@ -76,29 +112,59 @@ 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', () => { + const deployRole = role({ operations: ['deploy_component', 'get_job'] }); + render(); + expect(screen.getByText(/restricted to 2 operations/)).toBeTruthy(); + + cleanup(); + allowlistVersion.mockReturnValue({ supported: true, band: 'pre-5.3' }); + render(); + // Before 5.3 the gate cannot match deploy_component, so only get_job is reachable. + expect(screen.getByText(/restricted to 1 operation/)).toBeTruthy(); }); it('says nothing while the instance version is still loading', () => { // Collapsing "unknown" into "unsupported" would briefly describe a restricted role as // unrestricted — and is the shape that let a write path replace a real allowlist. - allowlistSupported.mockReturnValue(undefined); + allowlistVersion.mockReturnValue(undefined); const { container } = render(); expect(container.textContent).toBe(''); }); it('treats a record under operations as a database below the allowlist floor', () => { - allowlistSupported.mockReturnValue(false); + allowlistVersion.mockReturnValue({ supported: false, band: 'pre-5.3' }); const v4 = role({ operations: { tables: {} } } as unknown as LocalRole['permission']); expect(render().container.textContent).toBe(''); // …and as an unmanageable collision once the instance reserves the key — described, never // presented as something to "fix", since replacing it breaks permission translation. cleanup(); - allowlistSupported.mockReturnValue(true); + allowlistVersion.mockReturnValue(ON_5_3); render(); expect(screen.getByText(/breaks authentication for every user/)).toBeTruthy(); // Never describe the grants as live: Harper never gets far enough to honor them. diff --git a/src/features/instance/config/roles/operations/RoleOperationsSummary.tsx b/src/features/instance/config/roles/operations/RoleOperationsSummary.tsx index 808ef435a..9191ea27e 100644 --- a/src/features/instance/config/roles/operations/RoleOperationsSummary.tsx +++ b/src/features/instance/config/roles/operations/RoleOperationsSummary.tsx @@ -1,9 +1,10 @@ import { + allowlistGatesEveryPath, expandEffectiveOperations, summarizeOperations, } from '@/features/instance/config/roles/operations/operationsCatalog'; import { OperationsValueNotice } from '@/features/instance/config/roles/operations/OperationsValueNotice'; -import { useOperationsAllowlistSupported } from '@/features/instance/config/roles/operations/useOperationsAllowlistSupported'; +import { useAllowlistVersion } from '@/features/instance/config/roles/operations/useOperationsAllowlistSupported'; import { LocalRole } from '@/integrations/api/api.patch'; import { classifyOperationsValue, @@ -25,14 +26,14 @@ function isRecordShaped(permission: LocalRole['permission']): boolean { * Renders nothing for unrestricted roles. */ export function RoleOperationsSummary({ role }: { role: LocalRole | undefined }) { - const allowlistSupported = useOperationsAllowlistSupported(); + const allowlistVersion = useAllowlistVersion(); const permission = role?.permission; // While the version is unresolved the verdict is unknowable, so say nothing rather than // briefly describe a restricted role as unrestricted. - if (permission === undefined || allowlistSupported === undefined) { + if (permission === undefined || allowlistVersion === undefined) { return null; } - const kind = classifyOperationsValue(permission, allowlistSupported); + const kind = classifyOperationsValue(permission, allowlistVersion.supported); // Below the floor the key is either a real database grant or inert; neither is a restriction. if (kind === 'absent' || kind === 'database' || kind === 'inert') { return null; @@ -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.' - : ''; - const effective = expandEffectiveOperations(getOperationsAllowlist(permission) ?? []); + : `, 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 0ada4e73f..47661003a 100644 --- a/src/features/instance/config/roles/operations/operationsCatalog.test.ts +++ b/src/features/instance/config/roles/operations/operationsCatalog.test.ts @@ -1,13 +1,19 @@ import { + allowlistGatesEveryPath, canonicalOperationName, expandEffectiveOperations, + gateVerdict, getAvailableGroups, getGrantableOperations, getOperationInfo, + GrantBand, + isInertGrant, isOperationGroupName, + isOutOfGate, OPERATION_CATALOG, OPERATION_CATEGORIES, OPERATION_GROUPS, + resolveGrantBand, supportsOperationsAllowlist, } from '@/features/instance/config/roles/operations/operationsCatalog'; import { describe, expect, it } from 'vitest'; @@ -52,13 +58,16 @@ describe('OPERATION_CATALOG', () => { it('marks exactly the handler-enforced super_user operations as non-delegable', () => { const nonDelegable = OPERATION_CATALOG.filter((operation) => operation.nonDelegable).map((o) => o.name).sort(); expect(nonDelegable).toEqual([ + 'add_oidc_trust', 'create_backup', 'delete_backup', 'delete_secret', + 'drop_oidc_trust', 'get_deployment_payload', 'get_secrets_public_key', 'grant_secret', 'list_backups', + 'list_oidc_trust', 'list_secrets', 'purge_backups', 'restore_backup', @@ -99,6 +108,37 @@ describe('getGrantableOperations', () => { const names = getGrantableOperations('5.2.0-beta.1').map((operation) => operation.name); expect(names).toContain('set_env_value'); }); + + it('never offers what no allowlist can reach, including catchup once 5.3 stops matching it', () => { + const at52 = getGrantableOperations('5.2.15').map((operation) => operation.name); + expect(at52).toContain('catchup'); + expect(at52).not.toContain('get_backup'); + expect(at52).not.toContain('read_transaction_log'); + + const at53 = getGrantableOperations('5.3.0').map((operation) => operation.name); + expect(at53).not.toContain('catchup'); + expect(at53).not.toContain('get_backup'); + expect(at53).not.toContain('read_transaction_log'); + }); + + it('offers the deploy operations on every 5.x, and the trust-policy operations on none', () => { + for (const version of ['5.0.3', '5.2.15', '5.3.0']) { + const names = getGrantableOperations(version).map((operation) => operation.name); + expect(names, version).toContain('deploy_component'); + expect(names, version).not.toContain('add_oidc_trust'); + expect(names, version).not.toContain('list_oidc_trust'); + expect(names, version).not.toContain('drop_oidc_trust'); + } + }); + + it('offers drop_custom_function_project under its own name, and delete_records_before only through its canonical one', () => { + // drop_custom_function_project has its own handler and grant name; delete_records_before + // dispatches to delete_files_before's handler, which is granted by that name alone. + const names = getGrantableOperations('5.3.0').map((operation) => operation.name); + expect(names).toContain('drop_custom_function_project'); + expect(names).not.toContain('delete_records_before'); + expect(names).toContain('delete_files_before'); + }); }); describe('getAvailableGroups', () => { @@ -140,55 +180,190 @@ 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'])).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', () => { // The chip beside the count says an alias grant does nothing, so folding it here would have // the same component claiming access the server denies. - expect(expandEffectiveOperations(['describe_database', 'search_by_id'])).toEqual([]); + expect(expandEffectiveOperations(['describe_database', 'search_by_id'], '5.3')).toEqual([]); + expect(expandEffectiveOperations(['delete_records_before'], '5.3')).toEqual([]); }); it('leaves out entries the allowlist does not govern', () => { - // set_secret self-enforces super_user; sql is authorized on its own path, so listing either - // changes nothing about what the role can do. - expect(expandEffectiveOperations(['set_secret', 'sql'])).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', () => { - const effective = expandEffectiveOperations(['read_only', 'search', 'my_component_op']); + const effective = expandEffectiveOperations(['read_only', 'search', 'my_component_op'], '5.3'); expect(effective.filter((name) => name === 'search')).toHaveLength(1); // A component-registered name is unknown to the catalog, so it cannot be judged inert. expect(effective).toContain('my_component_op'); }); - it('still counts an operation whose registration key already equals its wire name', () => { - // catchup is registered under the bare string 'catchup', so the gate's `?? op` fallback lands - // on the same name the grant carries — it matches, unlike the handler-name registrations. - expect(expandEffectiveOperations(['catchup'])).toEqual(['catchup']); + it('counts catchup only where the gate still matches its wire name', () => { + // Registered under the bare string 'catchup', so the pre-5.3 fallback matched it; 5.3 names it null. + expect(expandEffectiveOperations(['catchup'], 'pre-5.3')).toEqual(['catchup']); + expect(expandEffectiveOperations(['catchup'], '5.3')).toEqual([]); + }); + + 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('does not count a grant the server cannot currently match', () => { - // deploy_component's authorization entry omits api_name, so the chip calls it inert; counting - // it here would have the same component claim access it denies. - expect(expandEffectiveOperations(['deploy_component', 'get_status', 'delete_records_before'])).toEqual([]); + it('never counts the operations Harper keeps out of every allowlist', () => { + for (const band of ['pre-5.3', '5.3-early', '5.3'] as const) { + expect(expandEffectiveOperations(['get_backup', 'read_transaction_log'], band)).toEqual([]); + } }); it('expands an empty allowlist to nothing', () => { - expect(expandEffectiveOperations([])).toEqual([]); + expect(expandEffectiveOperations([], '5.3')).toEqual([]); + }); +}); + +// 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 = [ + 'add_component', + 'cleanup_orphan_blobs', + 'clear_status', + 'delete_audit_logs_before', + 'delete_files_before', + 'delete_transaction_logs_before', + 'deploy_component', + 'drop_component', + 'drop_custom_function', + 'drop_custom_function_project', + 'get_status', + 'install_node_modules', + 'package_component', + 'registration_info', + 'restart_service', + 'search_jobs_by_start_date', + 'set_component_file', + 'set_configuration', + 'set_custom_function', + 'set_status', +]; + +describe('resolveGrantBand', () => { + it('puts the first build that names every entry, and everything after it, on the 5.3 side', () => { + for (const version of ['5.3.0-beta.3', '5.3.0-beta.4', '5.3.0', '5.3.2', '5.4.0', '6.0.0']) { + expect(resolveGrantBand(version), version).toBe('5.3'); + } + }); + + 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'); + } + }); + + it('falls back to the old side for a version it cannot read', () => { + // The conservative side: a grant the UI calls inert can only under-promise. + expect(resolveGrantBand('')).toBe('pre-5.3'); + expect(resolveGrantBand('not-a-version')).toBe('pre-5.3'); + }); +}); + +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', () => { + 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'); + } + }); + + it('marks only get_backup and read_transaction_log as never granted on both sides, and catchup from 5.3', () => { + const never = (band: GrantBand) => + OPERATION_CATALOG.filter((operation) => gateVerdict(operation.name, band) === 'never') + .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']); + }); + + it('does not judge a name the catalog does not know', () => { + expect(gateVerdict('my_component_op', 'pre-5.3')).toBe('grants'); + }); +}); + +describe('isInertGrant', () => { + it('stops calling the deploy-only CI role inert on 5.3', () => { + 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'] : []); + } }); }); @@ -196,6 +371,8 @@ describe('canonicalOperationName', () => { it('collapses aliases onto their authorization entry and leaves everything else alone', () => { expect(canonicalOperationName('create_schema')).toBe('create_database'); expect(canonicalOperationName('search_by_id')).toBe('search_by_hash'); + expect(canonicalOperationName('delete_records_before')).toBe('delete_files_before'); + expect(canonicalOperationName('drop_custom_function_project')).toBe('drop_custom_function_project'); expect(canonicalOperationName('create_database')).toBe('create_database'); expect(canonicalOperationName('my_component_op')).toBe('my_component_op'); }); diff --git a/src/features/instance/config/roles/operations/operationsCatalog.ts b/src/features/instance/config/roles/operations/operationsCatalog.ts index c776aa828..78287bfe7 100644 --- a/src/features/instance/config/roles/operations/operationsCatalog.ts +++ b/src/features/instance/config/roles/operations/operationsCatalog.ts @@ -3,7 +3,7 @@ import { wasAReleasedBeforeB } from '@/lib/string/wasAReleasedBeforeB'; /** * Static catalog of the operation names a role's `permission.operations` allowlist accepts, * extracted from the Harper source (`utility/hdbTerms.ts` OPERATIONS_ENUM + - * `utility/operationPermissions.ts` groups, verified against tags v5.0.0/v5.1.0/v5.2.2). + * `utility/operationPermissions.ts` groups, verified against tags v5.0.0/v5.1.0/v5.2.2/v5.3.0). * Harper has no discovery operation for these names (HarperFast/studio#1627), so the UI ships * the list per supported version and tolerates unknown names (e.g. component-registered * operations on 5.2+) rather than dropping them. @@ -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; @@ -29,69 +30,89 @@ export interface GrantableOperation { addedIn?: string; /** * The handler self-enforces super_user, so the allowlist cannot delegate it - * (secrets store, managed backups, get_deployment_payload). Not offered in the picker. + * (secrets store, managed backups, get_deployment_payload, OIDC trust policies). Not offered in the + * picker. */ 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; } /** - * Operations whose `requiredPermissions` entry omits `api_name` AND is keyed by a handler name that - * differs from the wire name. The gate resolves `requiredPermissions.get(op)?.api_name ?? op`, so - * the fallback lands on the handler name and can never equal a snake_case grant — listing these - * validates and saves but grants nothing on every shipped 5.x (HarperFast/harper#2175). An entry - * registered under a bare string equal to its wire name (`catchup`) matches through that same - * fallback and so is deliberately absent. They stay offered so a role is ready when core - * wires the names, but the UI must not promise a delegation they cannot receive. + * 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. */ -const GATE_INERT_OPERATIONS: ReadonlySet = new Set([ - 'add_component', - 'cleanup_orphan_blobs', - 'clear_status', - 'delete_audit_logs_before', - 'delete_files_before', - 'delete_transaction_logs_before', - 'deploy_component', - 'drop_component', - 'drop_custom_function', - 'drop_custom_function_project', - 'get_backup', - 'get_status', - 'install_node_modules', - 'package_component', - 'read_transaction_log', - 'restart_service', - 'search_jobs_by_start_date', - 'set_component_file', - 'set_configuration', - 'set_custom_function', - 'set_status', - // Shares delete_files_before's handler, whose entry carries no api_name either. - 'delete_records_before', - // Not super_user-only like the rest, but registered without api_name all the same — so a role - // carrying any allowlist is denied registration_info even when it lists it. - 'registration_info', -]); +export type GateChange = 'from-5.3' | 'until-5.3' | 'never'; -/** Whether granting this operation is currently a no-op server-side (HarperFast/harper#2175). */ -export function isGrantGateInert(name: string): boolean { - return GATE_INERT_OPERATIONS.has(name); +/** + * 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 { + 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'; + +/** Whether gate 1 matches a listing of this name. Unknown names are not judged. */ +export function gateVerdict(name: string, band: GrantBand): GateVerdict { + switch (catalogByName.get(name)?.gate) { + case 'from-5.3': + return band === '5.3' ? 'grants' : 'needs-5.3'; + case 'until-5.3': + return band === '5.3' ? 'never' : 'grants'; + case 'never': + return 'never'; + default: + return 'grants'; + } } /** * 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): boolean { +export function isInertGrant(name: string, band: GrantBand): boolean { const info = catalogByName.get(name); - return !!info?.aliasOf || !!info?.nonDelegable || !!info?.outOfGate || isGrantGateInert(name); + return !!info?.aliasOf || !!info?.nonDelegable || isOutOfGate(name, band) || gateVerdict(name, band) !== 'grants'; } export interface OperationGroup { @@ -144,9 +165,9 @@ export const OPERATION_CATEGORIES = [ /** * Every grantable operation name, including non-delegable ones (kept so existing role JSON that - * names them can still be rendered). Deliberately excludes `login`, `logout`, and - * `create_authentication_tokens`: those bypass permission checks entirely, so listing them in an - * allowlist has no effect either way. + * names them can still be rendered). Deliberately excludes `login`, `logout`, + * `create_authentication_tokens`, and `exchange_oidc_token`: those bypass permission checks entirely, + * so listing them in an allowlist has no effect either way. */ export const OPERATION_CATALOG: readonly GrantableOperation[] = [ // Data @@ -159,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 }, @@ -178,7 +199,7 @@ export const OPERATION_CATALOG: readonly GrantableOperation[] = [ // keeps the attribute — the bug being fixed. Each gate takes the safe side of its own failure. { name: 'put', su: false, category: DATA, addedIn: V5_3 }, { name: 'delete', su: false, category: DATA }, - { name: 'delete_records_before', su: true, category: DATA }, + { name: 'delete_records_before', su: true, category: DATA, aliasOf: 'delete_files_before' }, { name: 'csv_data_load', su: false, category: DATA }, { name: 'csv_file_load', su: false, category: DATA }, { name: 'csv_url_load', su: false, category: DATA }, @@ -198,7 +219,7 @@ export const OPERATION_CATALOG: readonly GrantableOperation[] = [ { name: 'drop_table', su: true, category: SCHEMA }, { name: 'create_attribute', su: false, category: SCHEMA }, { name: 'drop_attribute', su: true, category: SCHEMA }, - { name: 'cleanup_orphan_blobs', su: true, category: SCHEMA }, + { name: 'cleanup_orphan_blobs', su: true, category: SCHEMA, gate: 'from-5.3' }, // Users & Roles { name: 'user_info', su: false, category: USERS_ROLES }, { name: 'list_users', su: true, category: USERS_ROLES }, @@ -210,25 +231,28 @@ export const OPERATION_CATALOG: readonly GrantableOperation[] = [ { name: 'alter_role', su: true, category: USERS_ROLES }, { name: 'drop_role', su: true, category: USERS_ROLES }, { name: 'refresh_operation_token', su: false, category: USERS_ROLES }, + { name: 'add_oidc_trust', su: true, category: USERS_ROLES, addedIn: V5_3, nonDelegable: true }, + { name: 'list_oidc_trust', su: true, category: USERS_ROLES, addedIn: V5_3, nonDelegable: true }, + { name: 'drop_oidc_trust', su: true, category: USERS_ROLES, addedIn: V5_3, nonDelegable: true }, // Components & Deployments { name: 'get_components', su: true, category: COMPONENTS }, { name: 'get_component_file', su: true, category: COMPONENTS }, - { name: 'set_component_file', su: true, category: COMPONENTS }, - { name: 'add_component', su: true, category: COMPONENTS }, - { name: 'drop_component', su: true, category: COMPONENTS }, - { name: 'deploy_component', su: true, category: COMPONENTS }, - { name: 'package_component', su: true, category: COMPONENTS }, - { name: 'install_node_modules', su: true, category: COMPONENTS }, + { name: 'set_component_file', su: true, category: COMPONENTS, gate: 'from-5.3' }, + { name: 'add_component', su: true, category: COMPONENTS, gate: 'from-5.3' }, + { name: 'drop_component', su: true, category: COMPONENTS, gate: 'from-5.3' }, + { name: 'deploy_component', su: true, category: COMPONENTS, gate: 'from-5.3' }, + { name: 'package_component', su: true, category: COMPONENTS, gate: 'from-5.3' }, + { name: 'install_node_modules', su: true, category: COMPONENTS, gate: 'from-5.3' }, { name: 'audit_node_modules', su: true, category: COMPONENTS }, { name: 'custom_functions_status', su: true, category: COMPONENTS }, { name: 'get_custom_functions', su: true, category: COMPONENTS }, { name: 'get_custom_function', su: true, category: COMPONENTS }, - { name: 'set_custom_function', su: true, category: COMPONENTS }, - { name: 'drop_custom_function', su: true, category: COMPONENTS }, + { name: 'set_custom_function', su: true, category: COMPONENTS, gate: 'from-5.3' }, + { name: 'drop_custom_function', su: true, category: COMPONENTS, gate: 'from-5.3' }, { name: 'add_custom_function_project', su: true, category: COMPONENTS, aliasOf: 'add_component' }, { name: 'deploy_custom_function_project', su: true, category: COMPONENTS, aliasOf: 'deploy_component' }, { name: 'package_custom_function_project', su: true, category: COMPONENTS, aliasOf: 'package_component' }, - { name: 'drop_custom_function_project', su: true, category: COMPONENTS, aliasOf: 'drop_component' }, + { name: 'drop_custom_function_project', su: true, category: COMPONENTS, gate: 'from-5.3' }, { name: 'list_deployments', su: true, category: COMPONENTS, addedIn: V5_1 }, { name: 'get_deployment', su: true, category: COMPONENTS, addedIn: V5_1 }, { name: 'get_deployment_payload', su: true, category: COMPONENTS, addedIn: V5_1, nonDelegable: true }, @@ -238,33 +262,33 @@ export const OPERATION_CATALOG: readonly GrantableOperation[] = [ { name: 'update_node', su: true, category: CLUSTERING }, { name: 'set_node_replication', su: true, category: CLUSTERING }, { name: 'purge_stream', su: true, category: CLUSTERING }, - { name: 'catchup', su: true, category: CLUSTERING }, + { name: 'catchup', su: true, category: CLUSTERING, gate: 'until-5.3' }, // Jobs { name: 'get_job', su: false, category: JOBS }, - { name: 'search_jobs_by_start_date', su: true, category: JOBS }, + { name: 'search_jobs_by_start_date', su: true, category: JOBS, gate: 'from-5.3' }, { name: 'update_job', su: true, category: JOBS }, { name: 'delete_job', su: true, category: JOBS }, // Logs & Analytics { name: 'read_log', su: true, category: LOGS_ANALYTICS }, { name: 'read_audit_log', su: true, category: LOGS_ANALYTICS }, - { name: 'read_transaction_log', su: true, category: LOGS_ANALYTICS }, - { name: 'delete_files_before', su: true, category: LOGS_ANALYTICS }, - { name: 'delete_audit_logs_before', su: true, category: LOGS_ANALYTICS }, - { name: 'delete_transaction_logs_before', su: true, category: LOGS_ANALYTICS }, + { name: 'read_transaction_log', su: true, category: LOGS_ANALYTICS, gate: 'never' }, + { name: 'delete_files_before', su: true, category: LOGS_ANALYTICS, gate: 'from-5.3' }, + { name: 'delete_audit_logs_before', su: true, category: LOGS_ANALYTICS, gate: 'from-5.3' }, + { name: 'delete_transaction_logs_before', su: true, category: LOGS_ANALYTICS, gate: 'from-5.3' }, { name: 'get_analytics', su: false, category: LOGS_ANALYTICS }, { name: 'list_metrics', su: false, category: LOGS_ANALYTICS }, { name: 'describe_metric', su: false, category: LOGS_ANALYTICS }, // System { name: 'get_configuration', su: true, category: SYSTEM }, - { name: 'set_configuration', su: true, category: SYSTEM }, + { name: 'set_configuration', su: true, category: SYSTEM, gate: 'from-5.3' }, { name: 'restart', su: true, category: SYSTEM }, - { name: 'restart_service', su: true, category: SYSTEM }, + { name: 'restart_service', su: true, category: SYSTEM, gate: 'from-5.3' }, { name: 'system_information', su: true, category: SYSTEM }, - { name: 'get_status', su: true, category: SYSTEM }, - { name: 'set_status', su: true, category: SYSTEM }, - { name: 'clear_status', su: true, category: SYSTEM }, - { name: 'registration_info', su: false, category: SYSTEM }, - { name: 'get_backup', su: true, category: SYSTEM }, + { name: 'get_status', su: true, category: SYSTEM, gate: 'from-5.3' }, + { name: 'set_status', su: true, category: SYSTEM, gate: 'from-5.3' }, + { name: 'clear_status', su: true, category: SYSTEM, gate: 'from-5.3' }, + { name: 'registration_info', su: false, category: SYSTEM, gate: 'from-5.3' }, + { name: 'get_backup', su: true, category: SYSTEM, gate: 'never' }, // Managed Backups (5.2+, handlers self-enforce super_user) { name: 'create_backup', su: true, category: BACKUPS, addedIn: V5_2, nonDelegable: true }, { name: 'verify_backup', su: true, category: BACKUPS, addedIn: V5_2, nonDelegable: true }, @@ -380,6 +404,7 @@ export function supportsOperationsAllowlist(version: string | undefined): boolea * rendered and preserved — just not offered. */ export function getGrantableOperations(version: string): GrantableOperation[] { + const band = resolveGrantBand(version); return OPERATION_CATALOG.filter( (operation) => !operation.nonDelegable @@ -387,6 +412,7 @@ export function getGrantableOperations(version: string): GrantableOperation[] { // carries only the canonical api_name — so granting the alias spelling is inert. Offer // the canonical name only; alias entries stay in the catalog to render existing JSON. && !operation.aliasOf + && gateVerdict(operation.name, band) !== 'never' && (!operation.addedIn || wasAReleasedBeforeB(operation.addedIn, version)), ); } @@ -401,7 +427,7 @@ export function getAvailableGroups(version: string): OperationGroup[] { * operations, everything else (including names we don't recognize) passes through. Sorted and * de-duplicated. */ -export function expandEffectiveOperations(operations: readonly string[]): string[] { +export function expandEffectiveOperations(operations: readonly string[], band: GrantBand): string[] { const effective = new Set(); // A group's members fold to their canonical name: Harper's groups list both spellings of a pair // (search_by_hash/search_by_id, describe_schema/describe_database) but each pair reaches one @@ -411,7 +437,7 @@ export function expandEffectiveOperations(operations: readonly string[]): string if (group) { for (const member of group.members) { const canonical = catalogByName.get(member)?.aliasOf ?? member; - if (!isInertGrant(canonical)) { + if (!isInertGrant(canonical, band)) { effective.add(canonical); } } @@ -420,7 +446,7 @@ export function expandEffectiveOperations(operations: readonly string[]): string // A direct entry is NOT folded: the chip beside this count says an alias grants nothing, and // rewriting it to its canonical name here would credit the role with access it does not have. // Entries the server can never honor are left out for the same reason. - if (!isInertGrant(entry)) { + if (!isInertGrant(entry, band)) { effective.add(entry); } } diff --git a/src/features/instance/config/roles/operations/useOperationsAllowlistSupported.ts b/src/features/instance/config/roles/operations/useOperationsAllowlistSupported.ts index 163d0efd6..da962a9d1 100644 --- a/src/features/instance/config/roles/operations/useOperationsAllowlistSupported.ts +++ b/src/features/instance/config/roles/operations/useOperationsAllowlistSupported.ts @@ -1,21 +1,42 @@ import { useInstanceClientIdParams } from '@/config/useInstanceClient'; -import { supportsOperationsAllowlist } from '@/features/instance/config/roles/operations/operationsCatalog'; +import { + GrantBand, + resolveGrantBand, + supportsOperationsAllowlist, +} from '@/features/instance/config/roles/operations/operationsCatalog'; import { getRegistrationInfoQueryOptions } from '@/integrations/api/instance/status/getRegistrationInfo'; import { useQuery } from '@tanstack/react-query'; +import { useMemo } from 'react'; + +export interface AllowlistVersion { + supported: boolean; + band: GrantBand; +} /** - * Whether this instance accepts `permission.operations`, or `undefined` while the version is still - * loading. Every reader of a role's `operations` value needs it: below the floor an object under - * that key is a database named `operations`, while at or above it the same shape is a broken - * allowlist the UI must not overwrite. + * What this instance's version means for a role's `operations` value, or `undefined` while the + * version is still loading. Every reader of a role's `operations` value needs it: below the floor an + * object under that key is a database named `operations`, while at or above it the same shape is a + * broken allowlist the UI must not overwrite. * - * `undefined` is deliberately distinct from `false` — collapsing them would report a 5.x instance - * as pre-allowlist for the first render, which is how a write path could decide a real allowlist is - * absent and replace it. Callers must resolve it (render nothing, or choose a fail-closed default) - * rather than pass it on. + * `undefined` is deliberately distinct from "unsupported" — collapsing them would report a 5.x + * instance as pre-allowlist for the first render, which is how a write path could decide a real + * allowlist is absent and replace it. Callers must resolve it (render nothing, or choose a fail-closed + * default) rather than pass it on. */ -export function useOperationsAllowlistSupported(): boolean | undefined { +export function useAllowlistVersion(): AllowlistVersion | undefined { const instanceParams = useInstanceClientIdParams(); const { data: registrationInfo } = useQuery(getRegistrationInfoQueryOptions(instanceParams)); - return registrationInfo === undefined ? undefined : supportsOperationsAllowlist(registrationInfo.version); + const version = registrationInfo?.version; + return useMemo( + () => + registrationInfo === undefined + ? undefined + : { supported: supportsOperationsAllowlist(version), band: resolveGrantBand(version ?? '') }, + [registrationInfo, version], + ); +} + +export function useOperationsAllowlistSupported(): boolean | undefined { + return useAllowlistVersion()?.supported; } diff --git a/src/features/instance/config/roles/preparePermissionForSave.ts b/src/features/instance/config/roles/preparePermissionForSave.ts index 78942035b..4a765c1ff 100644 --- a/src/features/instance/config/roles/preparePermissionForSave.ts +++ b/src/features/instance/config/roles/preparePermissionForSave.ts @@ -9,8 +9,8 @@ import { RESERVED_PERMISSION_KEYS, rolePreventsOperationsAllowlist } from '@/int * `validateNoSUPerms` rejects any multi-key permission setting `super_user: true` or * `cluster_user: true`, so such a role is sent with nothing beside its flag: the allowlist and a * `structure_user` value (even `true`) would each fail the save outright. A `structure_user` role - * may carry an allowlist (the gate still runs for everything but DDL), and it survives only as an - * actual array: any other shape under that key is not an allowlist. + * may carry an allowlist (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 * only — such a role still relies on its explicit table permissions elsewhere, so it keeps them. 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 a88d26a29..c787ced21 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