fix(datatables): an import carries no login names, a login-less role reaches nothing, tenants are the owner's

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<Postgres>) -> 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<Postgres>) -> anyhow::Result<()> {
@@ -740,17 +740,20 @@ async fn imported_permissions_govern_without_logins(db: Pool<Postgres>) -> 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<Postgres>) -> 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(())
 }

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 <name>` 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<DB>,
@@ -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) {

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!({

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 {

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 }),