fix(roles): the role editor says Harper 5.3 gates a structure user's DDL and SQL by the allowlist

AGENTS.md

@@ -582,24 +582,39 @@ execute relationship joins).
 
 Harper 5.0+ roles accept an `operations` allowlist. Almost every intuitive reading of it is wrong,
 and each of these cost a review round on #1628 — verify against the Harper source before changing
-`src/features/instance/config/roles/operations/**` or `src/integrations/api/localRolePermission.ts`.
-
-- **It does not restrict super users.** `verifyPerms` returns early for `super_user`
-  ("admins can do (almost) anything") _before_ the allowlist gate. The allowlist is a way to scope
-  a NON-super role (gate 2 then lets it hold otherwise-SU operations), not a way to narrow an admin.
+`src/features/instance/config/roles/operations/**` or `src/integrations/api/localRolePermission.ts`,
+**at the tag you mean**. Harper changed gate 1 twice in 5.3, and `resolveGrantBand` turns a version
+into one of three bands (`pre-5.3`, `5.3-early`, `5.3`) so callers don't parse SemVer per operation:
+from v5.3.0-alpha.1 (harper `f8a5aa90a`) `verifyPerms` runs the gate ahead of every privilege
+early-return and `chooseOperation` runs it on `sql`; from v5.3.0-beta.3 every entry names its grant.
+`allowlistGatesEveryPath(band)` is the first change.
+
+- **The list never narrows a stored super user, for a different reason on each side.** Through 5.2.x
+  `verifyPerms` returns early for `super_user` ("admins can do (almost) anything") _before_ the gate.
+  From v5.3.0-alpha.1 the gate comes first ("must run before EVERY privilege early-return"), and a
+  list beside `super_user` would be enforced, but `validateNoSUPerms` (next bullet) refuses to store
+  one. Either way the allowlist scopes a NON-super role (gate 2 then lets it hold otherwise-SU
+  operations).
 - **`super_user`/`cluster_user` + `operations` cannot even be saved.** `validateNoSUPerms` rejects
   any permission with more than one key that sets either flag, so the combination 400s. Strip the
   allowlist for those roles rather than sending it.
-- **`structure_user` is different and IS gated** — only `create_table`/`create_attribute`/
-  `drop_table`/`drop_attribute` (plus create/drop database for the boolean form) short-circuit. The
-  array form scopes that carve-out to its listed databases and never reaches create/drop database.
-- **Which names the gate matches depends on the version.** Before 5.3, 20 entries omit `api_name`,
+- **`structure_user` DDL skips the list only before 5.3.** Through 5.2.x `create_table`/
+  `create_attribute`/`drop_table`/`drop_attribute` (plus create/drop database for the boolean form)
+  return before the gate. From v5.3.0-alpha.1 the gate runs first, so an allowlisted structure user
+  runs that DDL only when it is listed. On both, the array form keeps the DDL to its listed databases
+  (listing `drop_table` cannot reach another one), and its flag never covers create/drop database.
+  `structureUserDdlScope` reads the shape only; the editor's copy takes the band.
+- **Which names the gate matches depends on the version.** Before v5.3.0-beta.3, 20 entries omit `api_name`,
   so the gate compares a camelCase handler name against a snake_case grant and never matches
   (`deploy_component`, `get_status`, `registration_info`, …; HarperFast/harper#2175). From
   v5.3.0-beta.3 every entry names its grant, and `get_backup`, `read_transaction_log` and `catchup`
   name `null` on purpose, so no allowlist reaches them. The catalog records this per entry (`gate`), and
-  callers resolve the instance version into a `GrantBand` once. `sql` is worse: it routes to
-  `verifyPermsAST`, which never consults the allowlist at all.
+  callers resolve the instance version into a `GrantBand` once.
+- **`sql` ignores the list before 5.3 and needs it from v5.3.0-alpha.1.** Through 5.2.x
+  `chooseOperation` sends it straight to `checkASTPermissions`, which never consults the allowlist,
+  so listing or omitting it changes nothing. From alpha.1 it calls `verifyOperationsAllowlist` on
+  `sql` first, so a list without `sql` (or a group carrying it, like `read_only`) denies every SQL
+  statement. Table permissions apply on both. The catalog marks it `outOfGate: 'pre-5.3'`.
 - **Alias spellings are inert too** (`describe_database`, `search_by_id`, `create_schema`, …): both
   spellings dispatch to one handler whose entry carries only the canonical `api_name`.
 - **A non-iterable value takes the instance down.** `listUsers` expands every assigned role's
@@ -615,22 +630,28 @@ and each of these cost a review round on #1628 — verify against the Harper sou
 Practical consequence for the UI: never describe a restriction the server does not enforce, and
 never advise "fixing" an `operations` value in place — both mistakes shipped and were caught in
 review. `classifyOperationsValue` needs the instance version because the same shape means different
-things on either side of the 5.0.0-alpha.8 floor (the feature's first tagged build, not 5.0.0).
+things on either side of the 5.0.0-alpha.8 floor (the feature's first tagged build, not 5.0.0). The
+editor's structure-user and SQL sentences need the band for the same reason: each is true on one side
+of v5.3.0-alpha.1 only.
 
 ### Which of those the UI gating turns on
 
 The two facts that decide Studio's permission hooks (`src/hooks/checkOperationPermission.ts`):
 
-- **Gate admin chrome on the allowlist and you hide UI that works.** Because super users never reach
-  the gate, `useInstanceManagePermission` and `useInstanceBrowseManagePermission` are deliberately NOT
-  gated. Note browse-manage is only half short-circuited: its DDL half is, but the application-editor
-  operations behind it (`set_component_file`, `drop_component`, `deploy_component`, `set_env_value`,
-  `restart_service`) are not in `STRUCTURE_USER_OPS`, so for a non-super role they do reach the gate —
-  where an allowlist listing them _grants_ them via gate 2. Denying that half on the allowlist would
-  therefore hide UI a scoped role can legitimately use.
-- **`isElevatedRole` is the wrong question for a specific operation.** It answers "is the list
-  unenforceable anywhere for this role", which is what the editor warns about. For DML,
-  `structure_user` and `cluster_user` are still gated.
+- **Gate admin chrome on the allowlist and you hide UI that works.** No stored super user fails the
+  gate on any version (see above), so `useInstanceManagePermission` is deliberately NOT gated.
+  `useInstanceBrowseManagePermission` isn't either, but only pre-5.3 makes that exact: there its DDL
+  half returns before the gate for a structure user. From v5.3.0-alpha.1 nothing does, so an
+  allowlisted structure user is offered DDL the server refuses unless the list names it. The hooks
+  can't see the version, and gating them on the list would hide DDL that works on 5.2, so they stay
+  open and the server's 403 names the operation (HarperFast/studio#1894 tracks per-operation checks).
+  The application-editor operations behind browse-manage (`set_component_file`, `drop_component`,
+  `deploy_component`, `set_env_value`, `restart_service`) are not in `STRUCTURE_USER_OPS`, so for a
+  non-super role they reach the gate on every version — where an allowlist listing them _grants_ them
+  via gate 2. Denying that half on the allowlist would hide UI a scoped role can legitimately use.
+- **Only `super_user` short-circuits a specific operation.** For DML, `structure_user` and
+  `cluster_user` are gated on every version, so `checkTableOperationsAllowed` exempts the first flag
+  alone. Asking whether a role carries an elevating flag is the editor's question, not this one.
 
 Also two different expansions are correct for two different questions, and sharing one silently broke
 the second once: `expandEffectiveOperations` folds alias spellings together to describe a role's
@@ -648,6 +669,12 @@ denied. `testUtils.preTestPrep()` + `testUtils.setGlobalSchema(...)` provide the
 `verifyPermsAST` (to show SQL skipping the gate). Run with `npx mocha unitTests/utility/<file>`, 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
 

src/features/instance/config/roles/operations/OperationsAllowlistEditor.test.tsx

@@ -122,7 +122,7 @@ describe('OperationsAllowlistEditor', () => {
 		render(
 			<OperationsAllowlistEditor value={['read_only']} onChange={() => {}} version="5.2.2" structureUserDdl />,
 		);
-		// The allowlist IS enforced for a structure user on everything except DDL.
+		// Before 5.3 the allowlist IS enforced for a structure user on everything except DDL.
 		expect(screen.getByText(/regardless of the list/)).toBeTruthy();
 		expect(screen.getByText(/on any database/)).toBeTruthy();
 		expect(screen.queryByText(/has no effect/)).toBeNull();
@@ -152,6 +152,106 @@ describe('OperationsAllowlistEditor', () => {
 		expect(screen.getByText(/except the DDL noted above/)).toBeTruthy();
 	});
 
+	describe('from 5.3.0-alpha.1, where the list runs ahead of the structure_user and SQL paths', () => {
+		// 5.3.0-beta.2 is the last build that gates first without naming every entry, so it pins the
+		// gate-order boundary apart from the beta.3 naming boundary.
+		const GATE_FIRST_VERSIONS = ['5.3.0-alpha.1', '5.3.0-beta.2', '5.3.0'];
+
+		it('tells a structure user its DDL needs listing, and names what is missing', () => {
+			for (const version of GATE_FIRST_VERSIONS) {
+				render(
+					<OperationsAllowlistEditor
+						value={['read_only', 'create_table']}
+						onChange={() => {}}
+						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(<OperationsAllowlistEditor value={allDdl} onChange={() => {}} 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(
+				<OperationsAllowlistEditor value={['create_schema']} onChange={() => {}} 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(
+				<OperationsAllowlistEditor value={[]} onChange={() => {}} 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(<OperationsAllowlistEditor value={[]} onChange={() => {}} 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(<Harness initial={[]} version="5.3.0-beta.2" />);
+			expect(screen.getByText(/anything unlisted is denied, SQL included/)).toBeTruthy();
+			expect(screen.queryByText(/this list does not restrict them/)).toBeNull();
+
+			cleanup();
+			render(<Harness initial={[]} version="5.2.15" />);
+			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(<Harness initial={['read_only']} version="5.3.0-alpha.1" />);
+			expect(screen.getByText(/Effectively allows 13 operations/)).toBeTruthy();
+
+			cleanup();
+			render(<Harness initial={['sql']} version="5.3.0" />);
+			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(<Harness initial={['deploy_component']} version="5.3.0-beta.2" />);
+			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(<Harness initial={['sql']} version="5.2.15" />);
+		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(<Harness initial={[]} />);
 		openPicker();