From 45d5eeda0eb16a4927682847c652acd112e833b3 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Tue, 1 Sep 2026 04:48:44 +0200 Subject: [PATCH] fix(datatables): close the fail-open in role resolution, and gate the ACL endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A role with a stored pg_rolename but no password resolved to the data table's own connection, which owns everything — so a caller authorized as one role got the admin one instead. Exports and git-synced settings redact that password, so a restored config is exactly the shape that produced it. Refuse instead, and name the fix. The ACL endpoints took any workspace member: on a data table without roles every member resolves to that same admin connection, so ownership and grants there are the workspace admins' to change, as the roles themselves are. Also: check a migration batch's roles before applying any of it, keep the role picker for a single non-default role, drop the revoke button from a default privilege on types (which no scope can express), and say what get_datatable_resource_as_default_role actually resolves as. --- backend/ee-repo-ref.txt | 2 +- .../src/datatable_acl.rs | 14 ++++++++ .../src/datatable_migrations.rs | 32 +++++++++++++++---- .../src/datatable_permissions.rs | 6 ++-- .../windmill-api-workspaces/src/workspaces.rs | 15 +++++---- backend/windmill-common/src/worker.rs | 2 +- backend/windmill-common/src/workspaces.rs | 18 ++++++++--- frontend/src/lib/components/DBManager.svelte | 13 ++++++-- .../datatableAcl/PgAclEditor.svelte | 9 +++--- .../lib/components/datatableAcl/aclScopes.ts | 11 +++++++ .../DataTableSettings.svelte | 24 +++++++------- .../NewDataTableMigrationModal.svelte | 4 ++- 12 files changed, 105 insertions(+), 45 deletions(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 4a22116566..4d2618a356 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -e8e323558523176e544927b44acf084c27a1b2aa +ef2f5f487b90ac64d07d39f791592a5445887c47 diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 88931737d9..e07171b34e 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -241,6 +241,20 @@ async fn connect_as_caller( datatable_name: &str, role: Option<&str>, ) -> Result<(tokio_postgres::Client, CallerConnection)> { + // A permissioned data table hands out a connection per role, and the tenant + // lists are what say who reaches which. Without permissions every member + // resolves to the data table's own connection, which owns everything — so + // there it is the workspace admins' to change, as the roles themselves are. + if !authed.is_admin + && !read_datatable(db, w_id, datatable_name) + .await? + .permissions + .is_some_and(|p| p.enabled) + { + return Err(Error::NotAuthorized(format!( + "Only an admin can manage access on data table '{datatable_name}', which has no roles" + ))); + } let resource = get_datatable_resource_from_db( db, w_id, diff --git a/backend/windmill-api-workspaces/src/datatable_migrations.rs b/backend/windmill-api-workspaces/src/datatable_migrations.rs index b2d1f28453..4032f518f5 100644 --- a/backend/windmill-api-workspaces/src/datatable_migrations.rs +++ b/backend/windmill-api-workspaces/src/datatable_migrations.rs @@ -445,7 +445,7 @@ async fn run_datatable_migrations( let applied_versions = read_applied_versions_on_client(&client, &datatable_name).await?; - let mut applied = Vec::new(); + let mut to_run = Vec::new(); for m in migrations { if let Some(only) = query.only { // Run a single specific migration, skipping every other one. @@ -459,11 +459,28 @@ async fn run_datatable_migrations( if applied_versions.contains(&m.timestamp) { continue; } - // Fail the batch rather than skip: a migration the caller may not run is a - // gap in an ordered sequence, and silently leaving it out would apply - // later ones on top of a schema that never got this change. - ensure_migration_role_allowed(&db, &w_id, &datatable_name, &authed, &m.code_up, m.timestamp, &m.name) - .await?; + to_run.push(m); + } + + // Fail the batch rather than skip: a migration the caller may not run is a + // gap in an ordered sequence, and silently leaving it out would apply later + // ones on top of a schema that never got this change. Checked for the whole + // batch first, so the refusal does not land half way through it. + for m in to_run.iter() { + ensure_migration_role_allowed( + &db, + &w_id, + &datatable_name, + &authed, + &m.code_up, + m.timestamp, + &m.name, + ) + .await?; + } + + let mut applied = Vec::new(); + for m in to_run { run_datatable_migration_job(&db, &user_db, &authed, &w_id, &database_arg, &m.code_up) .await .map_err(|e| { @@ -860,7 +877,8 @@ async fn enable_datatable_migrations( // Opting in to migrations is one of the places an admin passes through, and // an instance database provisioned before the grants carried their options // cannot hand privileges to the roles permissions create. - crate::datatable_permissions::ensure_instance_db_can_delegate(&db, &w_id, &datatable_name).await; + crate::datatable_permissions::ensure_instance_db_can_delegate(&db, &w_id, &datatable_name) + .await; audit_log( &db, diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 021c9dcc60..0e20f0705d 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -198,9 +198,6 @@ pub(crate) async fn ensure_instance_db_can_delegate(db: &DB, w_id: &str, datatab } } -/// Connect to the data table's own database as `admin` and report the identity -/// the plan has to be built against: the database name, the role that owns the -/// existing objects, and the roles that actually exist in the cluster. /// What the plan has to be built against, probed from the data table's own /// database rather than assumed from its config. pub(crate) struct AdminConnection { @@ -274,6 +271,9 @@ async fn read_default_acl_rules(client: &tokio_postgres::Client) -> Result Result { // Get the datatable resource (connection credentials) - let db_resource = get_datatable_resource_as_admin(db, authed, w_id, datatable_name).await?; + let db_resource = get_datatable_resource_as_default_role(db, authed, w_id, datatable_name).await?; // Parse the resource as PgDatabase let pg_db: PgDatabase = serde_json::from_value(db_resource) @@ -2763,7 +2764,7 @@ async fn get_datatable_table_columns( ))); } - let db_resource = get_datatable_resource_as_admin(db, authed, w_id, datatable_name).await?; + let db_resource = get_datatable_resource_as_default_role(db, authed, w_id, datatable_name).await?; let pg_db: PgDatabase = serde_json::from_value(db_resource) .map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {}", e)))?; let (client, connection) = pg_db.connect(Some(db)).await?; @@ -2986,7 +2987,7 @@ pub(crate) async fn resolve_pg_source_checked( source: &str, ) -> Result { let db_resource = if let Some(name) = source.strip_prefix("datatable://") { - get_datatable_resource_as_admin(db, authed, w_id, name).await? + get_datatable_resource_as_default_role(db, authed, w_id, name).await? } else if let Some(path) = source.strip_prefix("$res:") { let db_with_authed = windmill_common::db::DbWithOptAuthed::from_authed( authed, diff --git a/backend/windmill-common/src/worker.rs b/backend/windmill-common/src/worker.rs index fa38d0c86e..14ae30bdaa 100644 --- a/backend/windmill-common/src/worker.rs +++ b/backend/windmill-common/src/worker.rs @@ -1084,7 +1084,7 @@ pub struct SqlAnnotations { impl SqlAnnotations { /// If the script declares `-- role `, returns the data table role the /// query runs as. Only meaningful against a permissioned `datatable://` - /// database; absent means the `root` role. + /// database; absent means the data table's default role. /// /// Mirrors `BashAnnotations::ssh_target`: only leading comment lines are /// scanned, and an exact `-- role ` with a valid role name and nothing diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index d12334d6f7..709da03314 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1426,10 +1426,20 @@ async fn resolve_datatable_role( ))); } - Ok(role_entry - .pg_rolename - .clone() - .zip(role_entry.pg_password.clone())) + // A role named without a stored credential is not a reason to fall back to + // the data table's own connection: that one owns everything, so the caller + // would silently get more than the role they asked for. Exports and + // git-synced settings redact the password, so a restored config lands here. + match ( + role_entry.pg_rolename.clone(), + role_entry.pg_password.clone(), + ) { + (Some(rolename), Some(password)) => Ok(Some((rolename, password))), + (Some(_), None) => Err(Error::internal_err(format!( + "Role '{role_name}' of data table '{name}' has no stored credential; save its permissions again to reset it" + ))), + (None, _) => Ok(None), + } } async fn get_datatable_resource_inner( diff --git a/frontend/src/lib/components/DBManager.svelte b/frontend/src/lib/components/DBManager.svelte index 9e08bcff73..965f2e3a99 100644 --- a/frontend/src/lib/components/DBManager.svelte +++ b/frontend/src/lib/components/DBManager.svelte @@ -132,6 +132,9 @@ onImport }: Props = $props() + /** Shown on the entries the server refuses to plan without a license. */ + const EE_PERMISSIONS_TOOLTIP = 'Data table permissions require an Enterprise license' + const sameTable = (a: SelectedTable, b: SelectedTable) => a.datatable === b.datatable && a.schema === b.schema && a.table === b.table @@ -237,8 +240,6 @@ /** Whether the connected role may change access on an object: Postgres asks * for membership of its owner, so for the rest the entry is not offered. */ function canManage(datatable: string | undefined, schemaKey: string, table?: string): boolean { - // The server refuses to plan any of it without a license. - if (!$enterpriseLicense) return false // A workspace admin manages the data table itself, so nothing in it is // hidden from them — `public` is owned by neither Windmill nor its roles. if (canManageDatatable) return true @@ -690,11 +691,13 @@ icon: HistoryIcon, action: () => onDatatableAction?.(dt, 'migrations') }, - ...(canManageDatatable && $enterpriseLicense + ...(canManageDatatable ? [ { displayName: 'Roles', icon: KeyRoundIcon, + disabled: !$enterpriseLicense, + tooltip: EE_PERMISSIONS_TOOLTIP, action: () => onDatatableAction?.(dt, 'roles') } ] @@ -753,6 +756,8 @@ { displayName: 'Permissions', icon: KeyRoundIcon, + disabled: !$enterpriseLicense, + tooltip: EE_PERMISSIONS_TOOLTIP, action: () => (aclDrawer = { datatable: root.datatable, @@ -839,6 +844,8 @@ { displayName: 'Permissions', icon: KeyRoundIcon, + disabled: !$enterpriseLicense, + tooltip: EE_PERMISSIONS_TOOLTIP, action: () => (aclDrawer = { datatable: root.datatable, diff --git a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte index 09d489c7de..3f83346658 100644 --- a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte +++ b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte @@ -13,7 +13,7 @@ import Cell from '../table/Cell.svelte' import { Trash2 } from 'lucide-svelte' import PgGrantBuilder from './PgGrantBuilder.svelte' - import { grantScopeLabel, groupGrants, type AclScope } from './aclScopes' + import { grantScopeLabel, groupGrants, revokeScopeOf } from './aclScopes' let { workspace, @@ -190,7 +190,8 @@ {grant.privileges.join(', ')} {grantScopeLabel(grant)} - {#if info.roles.includes(grant.grantee)} + {@const revokeScope = revokeScopeOf(grant)} + {#if info.roles.includes(grant.grantee) && revokeScope}