From 2457db4da46a2e7e87f80cce5e726b82174b2ad1 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Mon, 7 Sep 2026 11:22:03 +0200 Subject: [PATCH] fix(datatables): an import carries no login names, a login-less role reaches nothing, tenants are the owner's --- .../tests/datatable_tenants.rs | 21 +++++---- .../src/datatable_permissions.rs | 43 ++++++++++++++----- backend/windmill-api/src/workspaces_export.rs | 7 +-- backend/windmill-common/src/workspaces.rs | 30 +++++++++---- .../DataTablePermissionsButton.svelte | 5 ++- 5 files changed, 75 insertions(+), 31 deletions(-) diff --git a/backend/windmill-api-integration-tests/tests/datatable_tenants.rs b/backend/windmill-api-integration-tests/tests/datatable_tenants.rs index f04027adfe..d02eb64bad 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_tenants.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_tenants.rs @@ -678,9 +678,9 @@ async fn a_deletion_takes_only_the_permissions_it_owns(db: Pool) -> an Ok(()) } -/// An export carries a database's roles, tenants and login names, never its +/// An export carries a database's roles and tenants, never its login names or /// passwords; importing them governs a database nobody governs yet, owned by -/// the importing workspace, with every role refused until a save recreates the +/// the importing workspace, with every role refused until a save creates the /// logins. A database already governed is left alone and reported. #[sqlx::test(migrations = "../migrations", fixtures("base"))] async fn imported_permissions_govern_without_logins(db: Pool) -> anyhow::Result<()> { @@ -740,17 +740,20 @@ async fn imported_permissions_govern_without_logins(db: Pool) -> anyho .fetch_one(&db) .await?; assert_eq!(row.0.as_deref(), Some("test-workspace")); - assert_eq!( - row.1["roles"]["analyst"]["pg_rolename"], - json!("wm_analyst_x") + // A login name is a cluster-wide identifier the next save would rename or + // reset, so it is not taken from an import either. + assert!( + row.1["roles"]["analyst"].get("pg_rolename").is_none(), + "{}", + row.1 ); assert!( row.1["roles"]["analyst"].get("pg_password").is_none(), "{}", row.1 ); - // The tenant is listed as usable; resolving the role, which has no stored - // credential, is refused rather than falling back to the owning connection. + // The tenant is listed as usable; resolving the role, which has no login + // yet, gives the caller nothing rather than the owning connection. let roles = usable_roles(port, "test-workspace", "main", "SECRET_TOKEN_3").await; assert_eq!(roles["roles"], json!(["analyst"]), "{roles}"); let resp = authed( @@ -761,8 +764,10 @@ async fn imported_permissions_govern_without_logins(db: Pool) -> anyho ) .send() .await?; + let status = resp.status().as_u16(); let text = resp.text().await?; - assert!(text.contains("no stored credential"), "{text}"); + assert_eq!(status, 401, "{text}"); + assert!(text.contains("has no login yet"), "{text}"); Ok(()) } diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index dd41004597..8d495d1ece 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -91,8 +91,10 @@ pub struct DatatablePermissionsInfo { /// A database's permissions as a workspace export carries them: named by a data /// table of the workspace that reaches the database — the key is the server's -/// to derive, never the client's to choose — with the roles and their tenants -/// and the login names, never the passwords. +/// to derive, never the client's to choose — with the roles and their tenants. +/// Neither the login names nor the passwords are taken back: a login name is a +/// cluster-wide identifier the next save would rename or reset, so it is the +/// save's to generate. #[derive(Deserialize, Debug)] pub struct ImportedDatabasePermissions { pub datatable: String, @@ -476,10 +478,13 @@ pub(crate) async fn connect_as_admin_unchecked( } /// Plan `req` against the database the data table reaches, whose permissions -/// are `old` (read under the caller's lock, or absent when none are on yet). +/// are `old` (read under the caller's lock, or absent when none are on yet) and +/// belong to `owner_w_id` — the workspace whose principals the tenants name, +/// which is the calling one unless a superadmin manages them from elsewhere. async fn build_plan( db: &DB, w_id: &str, + owner_w_id: &str, datatable_name: &str, old: Option<&DataTablePermissions>, req: &SetDatatablePermissions, @@ -489,7 +494,7 @@ async fn build_plan( // it: a stale tenant or a stranded migration is refused without a round trip, // and the connection is then checked to have reached that same database. let (_, _, key) = resolve_datatable_database_unchecked(db, w_id, datatable_name).await?; - ensure_save_names_what_exists(db, w_id, &key, old, req).await?; + ensure_save_names_what_exists(db, owner_w_id, &key, old, req).await?; let (client, conn) = connect_as_admin_unchecked(db, w_id, datatable_name).await?; let mut plan = crate::datatable_permissions_oss::plan_role_changes( &conn.database_key, @@ -559,8 +564,15 @@ pub(crate) async fn plan_drop_of_datatable_roles( }) else { return Ok::<_, Error>(None); }; - let (client, _, plan) = - build_plan(db, w_id, datatable_name, Some(&record.permissions), &req).await?; + let (client, _, plan) = build_plan( + db, + w_id, + w_id, + datatable_name, + Some(&record.permissions), + &req, + ) + .await?; Ok(Some((client, plan, key))) } .await; @@ -893,7 +905,8 @@ pub(crate) async fn usable_roles( /// that another admin's deletion took off the role in between (deleting the /// principal removes its tenant, see `remove_datatable_tenant`): written back, /// whoever is given that username next would inherit the role. Refusing is what -/// makes the stale save visible; the admin reloads and saves again. +/// makes the stale save visible; the admin reloads and saves again. `w_id` is +/// the workspace whose principals the tenants are — the owning one. /// /// A migration carries its role as a `-- role ` annotation in its own code, /// which neither a rename nor a removal rewrites, so its next run — or the down @@ -1109,9 +1122,14 @@ async fn preview_datatable_permissions( // Refused here too: offering a plan that the save will not run is its own // kind of wrong. ensure_can_manage_permissions(&db, &authed, &w_id, record.as_ref(), req.enabled).await?; + let owner_w_id = record + .as_ref() + .and_then(|r| r.owner_workspace_id.clone()) + .unwrap_or_else(|| w_id.clone()); let (_client, _, plan) = build_plan( &db, &w_id, + &owner_w_id, &datatable_name, record.as_ref().map(|r| &r.permissions), &req, @@ -1161,6 +1179,7 @@ async fn set_datatable_permissions( let (mut client, conn, plan) = build_plan( &db, &w_id, + &owner_workspace_id, &datatable_name, record.as_ref().map(|r| &r.permissions), &req, @@ -1244,10 +1263,11 @@ async fn set_datatable_permissions( } /// Restore exported permissions on the databases this workspace's data tables -/// reach and nobody governs yet, owned by this workspace. The export carries no -/// passwords, so every role is refused until an admin saves the drawer again, -/// which recreates the logins; a database that is already governed is left as -/// it is and reported, by the data table that reaches it. +/// reach and nobody governs yet, owned by this workspace. Roles and tenants +/// only: every role is refused until an admin saves the drawer again, which +/// creates the logins under names the save generates; a database that is +/// already governed is left as it is and reported, by the data table that +/// reaches it. async fn import_datatable_permissions( authed: ApiAuthed, Extension(db): Extension, @@ -1277,6 +1297,7 @@ async fn import_datatable_permissions( let mut permissions = row.permissions; validate_imported_permissions(&permissions)?; for role in permissions.roles.values_mut() { + role.pg_rolename = None; role.pg_password = None; } if !permissions.roles.contains_key(ADMIN_DATATABLE_ROLE) { diff --git a/backend/windmill-api/src/workspaces_export.rs b/backend/windmill-api/src/workspaces_export.rs index 27a201c725..9d01bbe0c9 100644 --- a/backend/windmill-api/src/workspaces_export.rs +++ b/backend/windmill-api/src/workspaces_export.rs @@ -1761,9 +1761,9 @@ pub(crate) async fn tarball_workspace( // The permissions of the databases this workspace governs, as the import // endpoint takes them back: named by a data table of this workspace reaching - // the database, with roles, tenants and login names, never passwords — those - // are direct database logins, and a re-save recreates them. A database no - // entry of the workspace reaches any more cannot be named, and is left out. + // the database, with roles and tenants — no passwords, which are direct + // database logins, and no login names, which a re-save generates. A database + // no entry of the workspace reaches any more cannot be named, and is left out. let permissions = windmill_common::workspaces::database_permissions_owned_by(&mut *tx, &w_id).await?; if !permissions.is_empty() { @@ -1791,6 +1791,7 @@ pub(crate) async fn tarball_workspace( .filter_map(|mut row| { let datatable = reaching.get(&row.database_key)?; for role in row.permissions.roles.values_mut() { + role.pg_rolename = None; role.pg_password = None; } Some(serde_json::json!({ diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index a1cdf9660e..d7af827920 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1284,7 +1284,10 @@ pub struct DatabasePermissions { /// resolves to — the login left out: the same database reached as another user /// is the same database, and whoever can make a resource resolve there either /// holds its credentials, and has that access already, or does not, and gets a -/// connection that fails. +/// connection that fails. The host is read as DNS does, case-insensitively; an +/// alias that names the same server another way (an address for a name, a +/// proxy) is another database to this key, which is where the argument above +/// applies: pointing a resource at one takes its credentials. pub fn datatable_database_key( database: &DataTableDatabase, resolved: &serde_json::Value, @@ -1296,16 +1299,18 @@ pub fn datatable_database_key( DataTableCatalogResourceType::Postgresql => { use sha2::{Digest, Sha256}; // As the connection reads them, not as the JSON spells them: a port - // left out is 5432, a number and its string are one port. + // left out is 5432, a number and its string are one port, a host is + // one host whatever its case. let text = |field: &str| { resolved .get(field) .map(|v| match v.as_str() { - Some(s) => s.to_string(), + Some(s) => s.trim().to_string(), None => v.to_string(), }) .unwrap_or_default() }; + let host = text("host").to_ascii_lowercase(); let port = resolved .get("port") .and_then(|v| { @@ -1316,7 +1321,7 @@ pub fn datatable_database_key( let mut hasher = Sha256::new(); // NUL-joined so a value cannot be replayed by moving characters across // the field boundaries. - for part in [text("host"), port.to_string(), text("dbname")] { + for part in [host, port.to_string(), text("dbname")] { hasher.update(part.as_bytes()); hasher.update([0u8]); } @@ -2042,9 +2047,10 @@ async fn resolve_datatable_role( ))); } - // 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. + // A role without a login, or without its password, 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. Only `admin` + // means that connection. match ( role_entry.pg_rolename.clone(), role_entry.pg_password.clone(), @@ -2053,7 +2059,10 @@ async fn resolve_datatable_role( (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), + (None, _) if role_name == ADMIN_DATATABLE_ROLE => Ok(None), + (None, _) => Err(Error::NotAuthorized(format!( + "Role '{role_name}' of data table '{name}' has no login yet; save its permissions to create it" + ))), } } @@ -3464,6 +3473,11 @@ mod tests { &serde_json::json!({ "host": "db", "port": "5432", "dbname": "prod" }) ) ); + // A host is one host whatever its case, as DNS reads it. + assert_eq!( + datatable_database_key(&pg("u/a/pg"), &resolved("db.example", "prod", "app")), + datatable_database_key(&pg("u/a/pg"), &resolved(" DB.Example ", "prod", "app")) + ); } fn record(roles: &[(&str, &[&str])], default_role: Option<&str>) -> DatabasePermissions { diff --git a/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte b/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte index c9452a5495..c1d94bf569 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte @@ -84,7 +84,10 @@ roles: [] }) - const tenantItems = resource([() => workspace], async ([ws]) => { + // Tenants are principals of the workspace that owns the permissions, which + // is this one unless a superadmin manages them from another reaching the + // same database. + const tenantItems = resource([() => ownerWorkspace ?? workspace], async ([ws]) => { if (!ws) return [] const [users, groups, folders] = await Promise.all([ UserService.listUsernames({ workspace: ws }),