From a226047dc4aed5283e9464a08afeb2d61e0ccf5f Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 03:49:52 -0400 Subject: [PATCH 1/6] fix(roles): the operations editor knows what Harper 5.3's allowlist grants Harper 5.3.0-beta.3 names every authorization entry (harper feb8e8378, a6844db7a), so deploy_component and 19 other operations that the editor called inert on every 5.x are granted by an allowlist from then on. The editor would have told someone their deploy-only CI role could not deploy. The catalog now records, per entry, where listing a name starts or stops matching the gate, and callers resolve the instance version into a band once: - the 20 newly named operations need 5.3 (badged "needs Harper 5.3" below it, real super_user delegations from it); - catchup matched through the old fallback and is named null from 5.3; - get_backup and read_transaction_log are never granted, so they are no longer offered; - delete_records_before is an alias of delete_files_before (it dispatches to that handler), and drop_custom_function_project is not an alias of drop_component (it has its own handler and grant name); - add_oidc_trust, list_oidc_trust and drop_oidc_trust are catalogued as super_user only; their handlers enforce it. Refs #1788 Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 11 +- .../roles/constants/tableDefinition.tsx | 8 +- .../operations/AllowlistSupportedContext.tsx | 11 +- .../OperationsAllowlistEditor.test.tsx | 47 +++++- .../operations/OperationsAllowlistEditor.tsx | 39 +++-- .../operations/RoleOperationsSummary.test.tsx | 27 ++- .../operations/RoleOperationsSummary.tsx | 10 +- .../operations/operationsCatalog.test.ts | 154 ++++++++++++++++-- .../roles/operations/operationsCatalog.ts | 145 +++++++++-------- .../useOperationsAllowlistSupported.ts | 46 ++++-- 10 files changed, 361 insertions(+), 137 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 07e5d771d..cc9b37b71 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -594,10 +594,13 @@ and each of these cost a review round on #1628 — verify against the Harper sou - **`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. +- **Which names the gate matches depends on the version.** Before 5.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. - **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 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..52dc613cd 100644 --- a/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx +++ b/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx @@ -152,15 +152,58 @@ describe('OperationsAllowlistEditor', () => { expect(screen.getByText(/except the DDL noted above/)).toBeTruthy(); }); - it('marks a grant that the server cannot currently enforce', () => { + 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 Harper 5.3')).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 Harper 5.3')).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..c836d1866 100644 --- a/src/features/instance/config/roles/operations/OperationsAllowlistEditor.tsx +++ b/src/features/instance/config/roles/operations/OperationsAllowlistEditor.tsx @@ -12,17 +12,19 @@ import { Label } from '@/components/ui/label'; import { Switch } from '@/components/ui/switch'; import { expandEffectiveOperations, + gateVerdict, getAvailableGroups, getGrantableOperations, getOperationInfo, GrantableOperation, - isGrantGateInert, + GrantBand, isInertGrant, isOperationGroupName, 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'; @@ -64,9 +66,10 @@ export function OperationsAllowlistEditor({ (group) => !offeredGroups.includes(group) && value?.includes(group.name), ); const grantable = getGrantableOperations(version); + const band = resolveGrantBand(version); 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); @@ -172,6 +175,7 @@ export function OperationsAllowlistEditor({ toggle(name)} disabled={disabled} /> @@ -242,6 +247,9 @@ function GroupCheckbox({ ); } +const NEEDS_5_3_TITLE = 'Before Harper 5.3 the allowlist checks this operation under its internal handler name, so ' + + 'listing it has no effect on this instance (HarperFast/harper#2175). It takes effect on Harper 5.3.0 or later.'; + // 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 +262,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 +318,7 @@ function OperationsPicker({ && !caseVariantOfKnownName && !knownInfo?.nonDelegable && !knownInfo?.aliasOf + && gateVerdict(rawQuery, band) !== 'never' && !grantable.some((operation) => operation.name === rawQuery) && !selectedSet.has(rawQuery) ? rawQuery @@ -373,15 +384,10 @@ function OperationsPicker({ onSelect={(event) => event.preventDefault()} > {operation.name} - {isGrantGateInert(operation.name) + {gateVerdict(operation.name, band) === 'needs-5.3' ? ( - - not yet enforced + + needs Harper 5.3 ) : operation.su && ( @@ -417,22 +423,27 @@ 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.' diff --git a/src/features/instance/config/roles/operations/RoleOperationsSummary.test.tsx b/src/features/instance/config/roles/operations/RoleOperationsSummary.test.tsx index 5f078e1d9..9dfa873ba 100644 --- a/src/features/instance/config/roles/operations/RoleOperationsSummary.test.tsx +++ b/src/features/instance/config/roles/operations/RoleOperationsSummary.test.tsx @@ -2,17 +2,20 @@ * @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' }; + +beforeEach(() => allowlistVersion.mockReturnValue(ON_5_3)); afterEach(() => cleanup()); @@ -82,23 +85,35 @@ describe('RoleOperationsSummary', () => { expect(summary.getAttribute('title')).toContain('user_info'); }); + 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..469a7cb27 100644 --- a/src/features/instance/config/roles/operations/RoleOperationsSummary.tsx +++ b/src/features/instance/config/roles/operations/RoleOperationsSummary.tsx @@ -3,7 +3,7 @@ import { 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 +25,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; @@ -60,7 +60,7 @@ export function RoleOperationsSummary({ role }: { role: LocalRole | undefined }) ? `, 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) ?? []); + const effective = expandEffectiveOperations(getOperationsAllowlist(permission) ?? [], allowlistVersion.band); if (effective.length === 0) { return (

diff --git a/src/features/instance/config/roles/operations/operationsCatalog.test.ts b/src/features/instance/config/roles/operations/operationsCatalog.test.ts index 0ada4e73f..bc8bd86c0 100644 --- a/src/features/instance/config/roles/operations/operationsCatalog.test.ts +++ b/src/features/instance/config/roles/operations/operationsCatalog.test.ts @@ -1,13 +1,17 @@ import { canonicalOperationName, expandEffectiveOperations, + gateVerdict, getAvailableGroups, getGrantableOperations, getOperationInfo, + GrantBand, + isInertGrant, isOperationGroupName, OPERATION_CATALOG, OPERATION_CATEGORIES, OPERATION_GROUPS, + resolveGrantBand, supportsOperationsAllowlist, } from '@/features/instance/config/roles/operations/operationsCatalog'; import { describe, expect, it } from 'vitest'; @@ -52,13 +56,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 +106,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,7 +178,7 @@ 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([ + expect(expandEffectiveOperations(['read_only'], '5.3')).toEqual([ 'describe_all', 'describe_metric', 'describe_schema', @@ -159,36 +197,120 @@ describe('expandEffectiveOperations', () => { 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; 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([]); }); 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('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('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']); + 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) { + expect(expandEffectiveOperations(['get_backup', 'read_transaction_log'], band)).toEqual([]); + } }); it('expands an empty allowlist to nothing', () => { - expect(expandEffectiveOperations([])).toEqual([]); + expect(expandEffectiveOperations([], '5.3')).toEqual([]); + }); +}); + +// 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('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']) { + 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('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 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']); + 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); }); }); @@ -196,6 +318,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..8288111b4 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. @@ -29,7 +29,8 @@ 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. */ @@ -38,49 +39,44 @@ export interface GrantableOperation { 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 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`. + * - `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 side of that change an instance is on; resolve it once per version, not per operation. */ +export type GrantBand = 'pre-5.3' | '5.3'; + +/** 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'; +} + +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'; + } } /** @@ -89,9 +85,9 @@ export function isGrantGateInert(name: string): boolean { * 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. */ -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 || !!info?.outOfGate || gateVerdict(name, band) !== 'grants'; } export interface OperationGroup { @@ -144,9 +140,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 @@ -178,7 +174,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 +194,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 +206,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 +237,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 +379,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 +387,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 +402,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 +412,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 +421,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..3846dc55f 100644 --- a/src/features/instance/config/roles/operations/useOperationsAllowlistSupported.ts +++ b/src/features/instance/config/roles/operations/useOperationsAllowlistSupported.ts @@ -1,21 +1,45 @@ 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 { + /** Whether the instance accepts `permission.operations` at all. */ + supported: boolean; + /** Which names the instance's allowlist gate can match. */ + 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], + ); +} + +/** {@link useAllowlistVersion}'s support verdict alone, for readers that never count grants. */ +export function useOperationsAllowlistSupported(): boolean | undefined { + return useAllowlistVersion()?.supported; } From 4054aba9b063a973f288627a83e184680f203bfc Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 04:04:22 -0400 Subject: [PATCH 2/6] fix(roles): don't tell a 5.3 prerelease it needs Harper 5.3 The "needs Harper 5.3" badge also showed on 5.3.0-alpha.1 through beta.2, which run 5.3 but predate the gate change. The badge now says "needs newer Harper" and its title names the exact first build. Also corrects AGENTS.md's put note, which said registration_info is gate-inert: from 5.3 an allowlist that lists it grants it. Closes #1877 Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 9 +++++---- .../roles/operations/OperationsAllowlistEditor.test.tsx | 4 ++-- .../roles/operations/OperationsAllowlistEditor.tsx | 8 +++++--- .../roles/operations/useOperationsAllowlistSupported.ts | 3 --- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index cc9b37b71..80cf78282 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 diff --git a/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx b/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx index 52dc613cd..e1ceb9b47 100644 --- a/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx +++ b/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx @@ -157,7 +157,7 @@ describe('OperationsAllowlistEditor', () => { openPicker(); filterPicker('deploy_component'); const item = screen.getByRole('menuitemcheckbox', { name: /deploy_component/ }); - expect(within(item).getByText('needs Harper 5.3')).toBeTruthy(); + expect(within(item).getByText('needs newer Harper')).toBeTruthy(); expect(within(item).queryByText('super_user')).toBeNull(); }); @@ -171,7 +171,7 @@ describe('OperationsAllowlistEditor', () => { filterPicker('deploy_component'); const item = screen.getByRole('menuitemcheckbox', { name: /deploy_component/ }); expect(within(item).getByText('super_user')).toBeTruthy(); - expect(within(item).queryByText('needs Harper 5.3')).toBeNull(); + 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', () => { diff --git a/src/features/instance/config/roles/operations/OperationsAllowlistEditor.tsx b/src/features/instance/config/roles/operations/OperationsAllowlistEditor.tsx index c836d1866..a96dde769 100644 --- a/src/features/instance/config/roles/operations/OperationsAllowlistEditor.tsx +++ b/src/features/instance/config/roles/operations/OperationsAllowlistEditor.tsx @@ -12,6 +12,7 @@ import { Label } from '@/components/ui/label'; import { Switch } from '@/components/ui/switch'; import { expandEffectiveOperations, + GATE_NAMES_EVERY_ENTRY_FROM, gateVerdict, getAvailableGroups, getGrantableOperations, @@ -247,8 +248,9 @@ function GroupCheckbox({ ); } -const NEEDS_5_3_TITLE = 'Before Harper 5.3 the allowlist checks this operation under its internal handler name, so ' - + 'listing it has no effect on this instance (HarperFast/harper#2175). It takes effect on Harper 5.3.0 or later.'; +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 @@ -387,7 +389,7 @@ function OperationsPicker({ {gateVerdict(operation.name, band) === 'needs-5.3' ? ( - needs Harper 5.3 + needs newer Harper ) : operation.su && ( diff --git a/src/features/instance/config/roles/operations/useOperationsAllowlistSupported.ts b/src/features/instance/config/roles/operations/useOperationsAllowlistSupported.ts index 3846dc55f..da962a9d1 100644 --- a/src/features/instance/config/roles/operations/useOperationsAllowlistSupported.ts +++ b/src/features/instance/config/roles/operations/useOperationsAllowlistSupported.ts @@ -9,9 +9,7 @@ import { useQuery } from '@tanstack/react-query'; import { useMemo } from 'react'; export interface AllowlistVersion { - /** Whether the instance accepts `permission.operations` at all. */ supported: boolean; - /** Which names the instance's allowlist gate can match. */ band: GrantBand; } @@ -39,7 +37,6 @@ export function useAllowlistVersion(): AllowlistVersion | undefined { ); } -/** {@link useAllowlistVersion}'s support verdict alone, for readers that never count grants. */ export function useOperationsAllowlistSupported(): boolean | undefined { return useAllowlistVersion()?.supported; } From deda5e87a0ad9258289e4667a08a3282fe00a345 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 05:19:57 -0400 Subject: [PATCH 3/6] fix(roles): describe Harper 5.3 gating structure-user DDL and SQL by the allowlist From Harper 5.3.0-alpha.1 (harper f8a5aa90a), verifyPerms runs a role's operations allowlist ahead of its super_user and structure_user returns, and chooseOperation runs it on `sql`. The editor and the assignment summary still said a structure user reaches DDL regardless of the list and that the list does not restrict SQL, and the effective count left `sql` out. GrantBand gains a `5.3-early` band (5.3.0-alpha.1 to beta.2) between `pre-5.3` and `5.3`, so the gate-order change and the beta.3 naming change are both resolved from one band. The structure-user, SQL and empty-list copy, the `sql` chip and the effective count follow it. The permission hooks keep their behavior; their comments and AGENTS.md now say why on each version, and #1894 tracks per-operation checks for 5.3. Closes #1885 Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 71 ++++++++---- .../OperationsAllowlistEditor.test.tsx | 102 +++++++++++++++++- .../operations/OperationsAllowlistEditor.tsx | 94 +++++++++++----- .../operations/RoleOperationsSummary.test.tsx | 42 +++++++- .../operations/RoleOperationsSummary.tsx | 29 +++-- .../operations/operationsCatalog.test.ts | 101 ++++++++++++----- .../roles/operations/operationsCatalog.ts | 63 +++++++---- src/hooks/checkOperationPermission.test.ts | 7 +- src/hooks/checkOperationPermission.ts | 12 +-- src/hooks/checkSchemaTablePermission.test.ts | 2 +- src/hooks/usePermissions.ts | 6 +- src/integrations/api/localRolePermission.ts | 12 ++- 12 files changed, 419 insertions(+), 122 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 80cf78282..ac6c2c8fc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -584,24 +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. -- **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 @@ -617,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 @@ -650,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/operations/OperationsAllowlistEditor.test.tsx b/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx index e1ceb9b47..d127864dd 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 #1884's 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..b4f1f5526 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,25 @@ 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('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 +101,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..5bbf0ca11 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 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..e01d5e44d 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,10 +36,10 @@ 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; } @@ -53,14 +54,38 @@ export interface GrantableOperation { */ 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). */ +export 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'; +} + +/** Whether this band authorizes the operation on its own path, so the list neither grants nor restricts it. */ +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/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 From 737b3ca383fb5bb262f88d1a4fb1920eec987a19 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 05:25:34 -0400 Subject: [PATCH 4/6] refactor(roles): keep the alpha.1 cutoff private and date the naming change Nothing outside the catalog reads ALLOWLIST_GATE_RUNS_FIRST_FROM, and the GateChange doc still said "before 5.3" for a change that landed in 5.3.0-beta.3, which the new 5.3-early band makes ambiguous. Refs #1885 Co-Authored-By: Claude Opus 5.5 --- .../config/roles/operations/operationsCatalog.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/features/instance/config/roles/operations/operationsCatalog.ts b/src/features/instance/config/roles/operations/operationsCatalog.ts index e01d5e44d..78287bfe7 100644 --- a/src/features/instance/config/roles/operations/operationsCatalog.ts +++ b/src/features/instance/config/roles/operations/operationsCatalog.ts @@ -45,9 +45,10 @@ export interface GrantableOperation { } /** - * 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. @@ -66,7 +67,7 @@ export type GateChange = 'from-5.3' | 'until-5.3' | 'never'; 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). */ -export const ALLOWLIST_GATE_RUNS_FIRST_FROM = '5.3.0-alpha.1'; +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'; @@ -83,7 +84,6 @@ export function allowlistGatesEveryPath(band: GrantBand): boolean { return band !== 'pre-5.3'; } -/** Whether this band authorizes the operation on its own path, so the list neither grants nor restricts it. */ export function isOutOfGate(name: string, band: GrantBand): boolean { return catalogByName.get(name)?.outOfGate === band; } From 6676e4444dde8525ad06d58d5e92aec54d497535 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 11:29:56 -0400 Subject: [PATCH 5/6] fix(roles): name a scoped structure user's databases where the allowlist gates its DDL On the 5.3 bands the role summary said only that the structure-user DDL needs listing, dropping the database scope the pre-5.3 copy and the editor's gated note both keep. verifyPerms still applies the structure_user array after gate 1 (harper utility/operation_authorization.ts:810-824), so a listed DDL operation reaches only those databases. Co-Authored-By: Claude Opus 5.5 --- .../roles/operations/RoleOperationsSummary.test.tsx | 11 +++++++++++ .../config/roles/operations/RoleOperationsSummary.tsx | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/features/instance/config/roles/operations/RoleOperationsSummary.test.tsx b/src/features/instance/config/roles/operations/RoleOperationsSummary.test.tsx index b4f1f5526..89e493c8a 100644 --- a/src/features/instance/config/roles/operations/RoleOperationsSummary.test.tsx +++ b/src/features/instance/config/roles/operations/RoleOperationsSummary.test.tsx @@ -87,6 +87,17 @@ describe('RoleOperationsSummary', () => { } }); + 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(); diff --git a/src/features/instance/config/roles/operations/RoleOperationsSummary.tsx b/src/features/instance/config/roles/operations/RoleOperationsSummary.tsx index 5bbf0ca11..9191ea27e 100644 --- a/src/features/instance/config/roles/operations/RoleOperationsSummary.tsx +++ b/src/features/instance/config/roles/operations/RoleOperationsSummary.tsx @@ -58,7 +58,7 @@ export function RoleOperationsSummary({ role }: { role: LocalRole | undefined }) const ddlNote = !ddlScope ? '' : gatesEveryPath - ? ' (its structure-user DDL needs listing too)' + ? ` (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.' From bc31bfe4ed5dac1c079e3ca5eafd7419fb24ddb7 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 11:42:13 -0400 Subject: [PATCH 6/6] docs(roles): stop saying a structure user's DDL always skips the allowlist preparePermissionForSave's doc still described the pre-5.3 gate order as universal; from 5.3.0-alpha.1 Harper gates the structure-user DDL on the list too. Also drops a PR-number reference from a test comment in favour of the boundary it means. Comment-only; raised by the pre-push domain adjudication. Co-Authored-By: Claude Opus 5.5 --- .../roles/operations/OperationsAllowlistEditor.test.tsx | 2 +- .../instance/config/roles/preparePermissionForSave.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx b/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx index d127864dd..db0433bbb 100644 --- a/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx +++ b/src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx @@ -154,7 +154,7 @@ describe('OperationsAllowlistEditor', () => { 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 #1884's naming boundary. + // 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', () => { 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.