From d7d18f00af4e2df8fcb1362c9439d2250d0ce4b6 Mon Sep 17 00:00:00 2001 From: Diego Imbert <70353967+diegoimbert@users.noreply.github.com> Date: Thu, 8 Oct 2026 08:53:23 +0200 Subject: [PATCH] feat: improve data table roles and grants UX (#11562) * feat: improve data table roles and grants UX Co-Authored-By: Claude Opus 5.5 (1M context) * fix: keep selected bordered badges the same height as unselected ones Co-Authored-By: Claude Opus 5.5 (1M context) * fix: refuse taking over roles with members and test grant statements Co-Authored-By: Claude Opus 5.5 (1M context) * fix: keep grant filter chips while a filter is set Co-Authored-By: Claude Opus 5.5 (1M context) * fix: check a taken-over role can log in with the new password Co-Authored-By: Claude Opus 5.5 (1M context) * feat: select several grant filter tags at once Co-Authored-By: Claude Opus 5.5 (1M context) * feat: explain where schema and table permissions are set Co-Authored-By: Claude Opus 5.5 (1M context) * fix: keep the grant create row on one line Co-Authored-By: Claude Opus 5.5 (1M context) * feat: show grants as a role and its privileges Co-Authored-By: Claude Opus 5.5 (1M context) * feat: show grants in a table with role, privileges and scope Co-Authored-By: Claude Opus 5.5 (1M context) * fix: hide the grant scope when it can only be the object itself Co-Authored-By: Claude Opus 5.5 (1M context) * feat: grant schema usage along with grants inside the schema Co-Authored-By: Claude Opus 5.5 (1M context) * feat: confirm before closing the roles drawer with unsaved changes Co-Authored-By: Claude Opus 5.5 (1M context) * chore: update ee-repo-ref to c4002f777f56d90bb0471164291889850c85179d This commit updates the EE repository reference after PR #848 was merged in windmill-ee-private. Previous ee-repo-ref: 7937634a52d04caeb083da94e189705cd00796b3 New ee-repo-ref: c4002f777f56d90bb0471164291889850c85179d Automated by sync-ee-ref workflow. --------- Co-authored-by: Claude Opus 5.5 (1M context) Co-authored-by: windmill-internal-app[bot] --- backend/ee-repo-ref.txt | 2 +- .../tests/datatable_roles.rs | 83 +++++ .../src/datatable_acl.rs | 43 ++- backend/windmill-api/openapi.yaml | 13 + .../windmill-common/src/datatable_roles.rs | 12 +- .../src/datatable_roles_oss.rs | 3 +- frontend/src/lib/components/DBManager.svelte | 23 +- .../src/lib/components/DBManagerDrawer.svelte | 1 + .../src/lib/components/DropdownV2Inner.svelte | 16 +- .../lib/components/common/badge/Badge.svelte | 3 +- .../ConfirmationModal.svelte | 23 +- .../common/drawer/Disposable.svelte | 32 +- .../components/common/drawer/Drawer.svelte | 6 +- .../datatableAcl/PgAclEditor.svelte | 334 +++++++++++++----- .../datatableAcl/PgGrantBuilder.svelte | 96 +++-- .../components/datatableAcl/aclScopes.test.ts | 30 +- .../lib/components/datatableAcl/aclScopes.ts | 50 +-- .../DataTablePermissionsButton.svelte | 226 +++++++----- .../DataTableRolesSection.svelte | 31 +- .../workspaceSettings/datatableRoleModals.ts | 120 +++++++ 20 files changed, 864 insertions(+), 283 deletions(-) create mode 100644 frontend/src/lib/components/workspaceSettings/datatableRoleModals.ts diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index fffcc99883..560bcfb41b 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -1600d49ad5513af8b54ad65688d3d1c7edc977d9 \ No newline at end of file +6f23819e947b18878000e580ddefccfd01a73166 diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index ddb94f8c2b..9800d97841 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -417,6 +417,89 @@ async fn concurrent_role_creations_both_survive(db: Pool) -> anyhow::R outcome } +#[cfg(all(feature = "private", feature = "enterprise"))] +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn an_existing_role_is_taken_over_only_when_asked_and_never_with_members( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + let suffix: String = uuid::Uuid::new_v4().simple().to_string()[..8].to_string(); + let plain = format!("wmtest_plain_{suffix}"); + let group = format!("wmtest_group_{suffix}"); + let member = format!("wmtest_member_{suffix}"); + for sql in [ + // An expired password and no connection slot: kept as is, the login Windmill sets is dead. + format!( + "CREATE ROLE \"{plain}\" LOGIN PASSWORD 'theirs' VALID UNTIL '2000-01-01' \ + CONNECTION LIMIT 0" + ), + format!("CREATE ROLE \"{group}\" NOLOGIN"), + format!("CREATE ROLE \"{member}\" LOGIN"), + format!("GRANT \"{group}\" TO \"{member}\""), + ] { + sqlx::query(&sql).execute(&db).await?; + } + + let create = |name: String, take_over: bool| async move { + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/settings/datatable_roles" + )), + "SECRET_TOKEN", + ) + .json(&json!({ "name": name, "take_over": take_over })) + .send() + .await?; + Ok::<_, anyhow::Error>((resp.status().as_u16(), resp.text().await?)) + }; + let outcome = async { + let (status, body) = create(plain.clone(), false).await?; + assert_eq!( + status, 409, + "an existing role is not taken without asking: {body}" + ); + + // The group's member would inherit every grant the data table role gets. + let (status, body) = create(group.clone(), true).await?; + assert_eq!(status, 400, "{body}"); + assert!( + body.contains(&member), + "the refusal names the member: {body}" + ); + + let (status, body) = create(plain.clone(), true).await?; + assert_eq!(status, 200, "{body}"); + let catalog = windmill_common::datatable_roles::read_role_catalog( + &db, + windmill_common::datatable_roles::DatatableRoleCluster::Instance, + ) + .await?; + let taken = catalog + .values() + .find(|r| r.name == plain) + .ok_or_else(|| anyhow::anyhow!("{plain} is not in the catalog: {catalog:?}"))?; + let options = (*db.connect_options()) + .clone() + .username(&plain) + .password(taken.pwd.as_deref().unwrap_or_default()); + let mut login = ::connect_with(&options).await?; + sqlx::query("SELECT 1").execute(&mut login).await?; + Ok::<_, anyhow::Error>(()) + } + .await; + + // Roles are cluster-wide and outlive this test's database, whatever happened above. + for name in [&member, &group, &plain] { + let _ = sqlx::query(&format!("DROP ROLE IF EXISTS \"{name}\"")) + .execute(&db) + .await; + } + outcome +} + #[cfg(all(feature = "private", feature = "enterprise"))] #[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] async fn a_role_delete_that_fails_part_way_leaves_the_role_disabled( diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index ebab6214ce..efaa712481 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -683,6 +683,9 @@ pub(crate) struct CatalogFacts { pub(crate) former_owner: Option, /// For a revoke: each grant it takes back. pub(crate) revoked_grants: Vec, + /// For a grant on what a schema holds: the role cannot reach the schema yet, so the grant + /// would give it nothing it can use. + pub(crate) lacks_schema_usage: bool, } /// A grant a revoke takes back, as the catalog records it: what `source` gave on `object` (the @@ -1526,7 +1529,24 @@ async fn build_plan( ))); } } - let facts = CatalogFacts { other_pg_roles, existing_objects, former_owner, revoked_grants }; + let lacks_schema_usage = match (&change, target) { + (AclChange::Grant { scope, .. }, AclTarget::Schema { schema }) + if !matches!(scope, GrantScope::Target) => + { + !has_schema_usage(client, schema, &pg_role).await? + } + (AclChange::Grant { .. }, AclTarget::Table { schema, .. }) => { + !has_schema_usage(client, schema, &pg_role).await? + } + _ => false, + }; + let facts = CatalogFacts { + other_pg_roles, + existing_objects, + former_owner, + revoked_grants, + lacks_schema_usage, + }; let mut plan = crate::datatable_acl_oss::plan_statements(target, &change, dbname, &pg_role, &facts)?; if matches!(change, AclChange::SetOwner { .. }) { @@ -1540,6 +1560,27 @@ async fn build_plan( Ok(plan) } +/// Whether `pg_role` may look into `schema`, directly or through a role it inherits from. +async fn has_schema_usage( + client: &tokio_postgres::Client, + schema: &str, + pg_role: &str, +) -> Result { + Ok(client + .query_one( + "SELECT has_schema_privilege($1::name, $2::text, 'USAGE')", + &[&pg_role, &schema], + ) + .await + .map_err(|e| { + Error::internal_err(format!( + "Failed to read the role's privileges on schema {schema}: {}", + pg_error_message(&e) + )) + })? + .get(0)) +} + /// Postgres only hands an object to a role that could have created it: a table to one with /// `CREATE` on its schema, a schema to one with `CREATE` on the database. async fn missing_owner_privilege( diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 1b2aea604e..873fcb7405 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -1765,6 +1765,12 @@ paths: type: string cluster: $ref: "#/components/schemas/DatatableRoleCluster" + take_over: + type: boolean + description: > + manage a Postgres role of that name that already exists on the cluster, + replacing its password, instead of refusing it. A role with cluster-wide + attributes or memberships is refused either way responses: "200": description: the created role @@ -1772,6 +1778,8 @@ paths: application/json: schema: $ref: "#/components/schemas/InstanceDatatableRole" + "409": + description: a Postgres role of that name already exists on the cluster and take_over was not set /settings/datatable_roles/{id}: post: @@ -34525,6 +34533,11 @@ components: type: boolean cluster: $ref: "#/components/schemas/DatatableRoleCluster" + in_use: + type: boolean + description: >- + Whether any data table, in any workspace, lets someone connect as this role. Only set + in a data table's available_roles. DatatableRoleTenants: type: object diff --git a/backend/windmill-common/src/datatable_roles.rs b/backend/windmill-common/src/datatable_roles.rs index f355e733cb..e7e490a6a4 100644 --- a/backend/windmill-common/src/datatable_roles.rs +++ b/backend/windmill-common/src/datatable_roles.rs @@ -331,6 +331,12 @@ pub async fn converge_connect_grants_with( /// `CREATE ROLE LOGIN PASSWORD ...; GRANT TO custom_instance_user` on `cluster`. No /// privileges beyond that — an admin grants them through SQL or the ACL editor. /// +/// A role of that name that already exists is refused with a 409, unless `take_over`: then its +/// password is replaced and it is managed from here on like one created here. A role holding +/// cluster-wide privileges, or inheriting another role's, is refused either way, since every tenant +/// would get them. Returns whether an existing role was taken over: undoing a failed take-over +/// must not drop a role that was there before. +/// /// On Windmill's own cluster the DDL runs on `tx`, so it commits with the catalog row. The external /// cluster is another server: the role is created there before `tx` commits, and callers MUST drop /// it again ([`drop_datatable_role`]) if `tx` then fails to commit. @@ -343,8 +349,10 @@ pub async fn create_datatable_role( cluster: DatatableRoleCluster, name: &str, password: &str, -) -> Result<()> { - crate::datatable_roles_oss::create_datatable_role(db, tx, cluster, name, password).await + take_over: bool, +) -> Result { + crate::datatable_roles_oss::create_datatable_role(db, tx, cluster, name, password, take_over) + .await } /// Authorization: alters a cluster-wide Postgres login. Callers MUST restrict this to superadmin diff --git a/backend/windmill-common/src/datatable_roles_oss.rs b/backend/windmill-common/src/datatable_roles_oss.rs index a97a936827..33d711e7f9 100644 --- a/backend/windmill-common/src/datatable_roles_oss.rs +++ b/backend/windmill-common/src/datatable_roles_oss.rs @@ -125,7 +125,8 @@ mod ce { _cluster: DatatableRoleCluster, _name: &str, _password: &str, - ) -> Result<()> { + _take_over: bool, + ) -> Result { Err(unavailable()) } diff --git a/frontend/src/lib/components/DBManager.svelte b/frontend/src/lib/components/DBManager.svelte index 5204a876b9..837569c575 100644 --- a/frontend/src/lib/components/DBManager.svelte +++ b/frontend/src/lib/components/DBManager.svelte @@ -375,7 +375,8 @@ } if (!isInstanceDatatable(datatable)) return { - reason: 'Only data tables stored in the instance database have roles and grants.', + reason: + "Roles and grants are not supported on data tables backed by a Postgres resource. Store the data table on Windmill's database or on an external Postgres instance (set up by a superadmin in instance settings) to use them.", ee: false } if (needsAdmin && !canManageDatatable) @@ -458,6 +459,11 @@ // default to open — actually be folded; a plain "expanded" set could never // close them, since the default would keep winning. // What the permissions drawer is open on, if anything: a schema, or a table. + let aclEditor = $state(undefined) + /** The open access editor lists the data table's roles, which the roles drawer can change. */ + export function refreshAcl() { + aclEditor?.refresh() + } let aclDrawer = $state<{ datatable: string | undefined; target: AclTarget } | undefined>( undefined ) @@ -1466,7 +1472,7 @@ (aclDrawer = undefined)}> (aclDrawer = undefined)} tooltip="Who owns this, and what each role may do with it." > @@ -1474,7 +1480,18 @@ {@const dt = aclDrawer.datatable ?? currentDatatable} {#if dt} {#key `${dt}~${JSON.stringify(aclDrawer.target)}`} - + onDatatableAction?.(dt, 'roles'), + blocker: accessBlocker(dt, true)?.reason + } + : undefined} + /> {/key} {/if} {/if} diff --git a/frontend/src/lib/components/DBManagerDrawer.svelte b/frontend/src/lib/components/DBManagerDrawer.svelte index bd016544b0..30fc93377d 100644 --- a/frontend/src/lib/components/DBManagerDrawer.svelte +++ b/frontend/src/lib/components/DBManagerDrawer.svelte @@ -311,6 +311,7 @@ function refreshRoles() { datatables.refetch() usableRoles.refetch() + dbManagerContent?.dbManager()?.refreshAcl() } async function handleExportSchema(explicitSource?: string) { diff --git a/frontend/src/lib/components/DropdownV2Inner.svelte b/frontend/src/lib/components/DropdownV2Inner.svelte index 31fd9426f5..70ed83398d 100644 --- a/frontend/src/lib/components/DropdownV2Inner.svelte +++ b/frontend/src/lib/components/DropdownV2Inner.svelte @@ -107,9 +107,19 @@ {})} /> {:else if item.disabled && item.tooltip} -
- {@render menuItem(item)} + lets the hover reach it so the user learns why the item is disabled. The ⓘ sits + outside the disabled button, the only place it can receive a hover. --> +
+
+ {@render menuItem(item)} +
+
+ + {#snippet text()} + {item.tooltip} + {/snippet} + +
{:else} {@render menuItem(item)} diff --git a/frontend/src/lib/components/common/badge/Badge.svelte b/frontend/src/lib/components/common/badge/Badge.svelte index c57d41029c..09a56c427a 100644 --- a/frontend/src/lib/components/common/badge/Badge.svelte +++ b/frontend/src/lib/components/common/badge/Badge.svelte @@ -85,7 +85,8 @@ ['dark-indigo']: 'bg-indigo-700 text-indigo-100 dark:bg-indigo-800 dark:text-indigo-200', ['dark-orange']: 'bg-orange-700 text-orange-100 dark:bg-orange-800 dark:text-orange-200', ['dark-violet']: 'bg-violet-700 text-violet-100 dark:bg-violet-800 dark:text-violet-200', - transparent: 'bg-surface-accent-selected text-accent border-gray-400 dark:border-gray-500' + // Keeps the unselected chip's border, so selecting one does not change its height. + transparent: 'bg-surface-accent-selected text-accent border border-surface-accent-selected' } const hovers: Partial> = { diff --git a/frontend/src/lib/components/common/confirmationModal/ConfirmationModal.svelte b/frontend/src/lib/components/common/confirmationModal/ConfirmationModal.svelte index 524817ba34..4b419536fc 100644 --- a/frontend/src/lib/components/common/confirmationModal/ConfirmationModal.svelte +++ b/frontend/src/lib/components/common/confirmationModal/ConfirmationModal.svelte @@ -29,6 +29,8 @@ * up confines it. Off by default: it moves the dialog out of its DOM position, so opt * in per call site rather than assuming every caller wants it. */ alwaysPortal?: boolean + /** For a notice with nothing to decide: the confirm button is the only one. */ + hideCancel?: boolean children?: Snippet onConfirmed?: () => void | Promise onCanceled?: () => void @@ -47,6 +49,7 @@ trashbin = false, zIndexClass = 'z-[9999]', alwaysPortal = false, + hideCancel = false, children, onConfirmed, onCanceled @@ -228,15 +231,17 @@ {/if} {confirmationText} - + {#if !hideCancel} + + {/if}
diff --git a/frontend/src/lib/components/common/drawer/Disposable.svelte b/frontend/src/lib/components/common/drawer/Disposable.svelte index 886c14042b..4fd472729f 100644 --- a/frontend/src/lib/components/common/drawer/Disposable.svelte +++ b/frontend/src/lib/components/common/drawer/Disposable.svelte @@ -32,6 +32,9 @@ children?: import('svelte').Snippet<[any]> onOpen?: () => void onClose?: () => void + /** Asked before a click away or Escape closes it; resolving false keeps it open, e.g. to + * confirm discarding unsaved changes. */ + confirmClose?: () => boolean | Promise } let { @@ -42,9 +45,25 @@ minZIndex = 0, children, onOpen, - onClose + onClose, + confirmClose }: Props = $props() + // While the question is on screen, the keys and clicks are its own. + let confirming = false + async function requestClose() { + if (confirming) return + if (confirmClose) { + confirming = true + try { + if (!(await confirmClose())) return + } finally { + confirming = false + } + } + closeDrawer() + } + let offset = $state(untrack(() => initialOffset)) // Note: when a Modal with minZIndex is open, all disposables (including // already-open Drawers) are elevated. This is acceptable — relative @@ -115,7 +134,7 @@ const last = stack.val[stack.val.length - 1] if (last === id) { e.stopPropagation() - closeDrawer() + requestClose() } } @@ -125,7 +144,16 @@ if (open) { switch (event.key) { case 'Escape': + // The confirmation on screen answers this Escape itself, as its Cancel. + if (confirming) break if ((id == stack.val[stack.val.length - 1] || stack.val.length == 0) && !preventEscape) { + if (confirmClose) { + event.preventDefault() + event.stopPropagation() + event.stopImmediatePropagation() + requestClose() + break + } stack.val.pop() event.preventDefault() event.stopPropagation() diff --git a/frontend/src/lib/components/common/drawer/Drawer.svelte b/frontend/src/lib/components/common/drawer/Drawer.svelte index 76502735ca..45cf99c7be 100644 --- a/frontend/src/lib/components/common/drawer/Drawer.svelte +++ b/frontend/src/lib/components/common/drawer/Drawer.svelte @@ -28,6 +28,8 @@ positionClass?: string | undefined name?: string children?: import('svelte').Snippet<[any]> + /** Asked before a click away or Escape closes it; resolving false keeps it open. */ + confirmClose?: () => boolean | Promise } let { @@ -43,7 +45,8 @@ class: clazz = '', positionClass = undefined, name = undefined, - children: children_render + children: children_render, + confirmClose }: Props = $props() if (open === undefined) { @@ -146,6 +149,7 @@ onOpen={() => dispatch('open')} onClose={() => dispatch('close')} {preventEscape} + {confirmClose} > {#snippet children({ handleClickAway, zIndex, isTop })}