fix(datatables): a second door is refused where it resolves, by the database itself rather than the login

backend/.sqlx/query-e18ae5aab21cda0837933ab1cb401d1d28e19e3658b333e3248c203eb4895b68.json

@@ -1,6 +1,6 @@
 {
   "db_name": "PostgreSQL",
-  "query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"name!\"\n           FROM workspace_settings ws, jsonb_each(ws.datatable->'datatables') dt\n           WHERE COALESCE((dt.value->'permissions'->>'enabled')::boolean, false)\n             AND dt.value->'permissions'->>'database_identity' = $1\n           ORDER BY ws.workspace_id, dt.key",
+  "query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"name!\"\n           FROM workspace_settings ws, jsonb_each(ws.datatable->'datatables') dt\n           WHERE COALESCE((dt.value->'permissions'->>'enabled')::boolean, false)\n             AND dt.value->'permissions'->>'physical_identity' = $1\n           ORDER BY ws.workspace_id, dt.key",
   "describe": {
     "columns": [
       {
@@ -24,5 +24,5 @@
       null
     ]
   },
-  "hash": "c5ca2b9382ece7f23fce8e3327d5b373617be55edd5efdf73fd71ec5847b0359"
+  "hash": "e18ae5aab21cda0837933ab1cb401d1d28e19e3658b333e3248c203eb4895b68"
 }

backend/ee-repo-ref.txt

@@ -1 +1 @@
-1e70ab9734682756988f184e850a3b2c5a0595fe
+0c220600e7ceece78a7623fd4185adfb0054d1c9

backend/windmill-api-integration-tests/tests/datatable_tenants.rs

@@ -733,9 +733,11 @@ async fn a_same_named_resource_counts_only_when_it_reaches_the_same_database(
     let text = preview().await;
     assert!(!text.contains("cannot be enabled"), "{text}");
 
-    // Another path, the same database: a copy, wherever the resource was moved.
+    // Another path and another login, the same database: a copy, wherever the
+    // resource was moved and whoever it connects as.
     sqlx::query(
-        "UPDATE resource SET path = 'f/moved/pg', value = jsonb_set(value, '{dbname}', '\"prod\"')
+        "UPDATE resource SET path = 'f/moved/pg',
+             value = jsonb_set(jsonb_set(value, '{dbname}', '\"prod\"'), '{user}', '\"postgres\"')
          WHERE workspace_id = 'detached' AND path = 'u/test-user/pg'",
     )
     .execute(&db)
@@ -768,13 +770,13 @@ async fn a_same_named_resource_counts_only_when_it_reaches_the_same_database(
         .await?;
         assert_eq!(resp.status(), 200);
         let value: serde_json::Value = resp.json().await?;
-        windmill_common::workspaces::datatable_database_identity(&value)
+        windmill_common::workspaces::physical_database_identity(&value)
     };
     sqlx::query(
         r#"UPDATE workspace_settings
            SET datatable = jsonb_set(datatable, '{datatables,byo,permissions}',
                jsonb_build_object('enabled', true, 'roles', '{"admin": {"tenants": []}}'::jsonb,
-                                  'database_identity', $1::text))
+                                  'physical_identity', $1::text))
            WHERE workspace_id = 'test-workspace'"#,
     )
     .bind(&identity)
@@ -822,8 +824,54 @@ async fn a_same_named_resource_counts_only_when_it_reaches_the_same_database(
     let status = resp.status().as_u16();
     let text = resp.text().await?;
     assert_eq!(status, 400, "{text}");
+    assert!(text.contains("a data table of another workspace"), "{text}");
+    // Nor deleted and recreated at the path the entry still names.
+    let resp = authed(
+        client().delete(format!(
+            "http://localhost:{port}/api/w/detached/resources/delete/f/moved/pg"
+        )),
+        "SECRET_TOKEN",
+    )
+    .send()
+    .await?;
+    assert_eq!(resp.status(), 200, "{}", resp.text().await?);
+    let resp = authed(
+        client().post(format!("http://localhost:{port}/api/w/detached/resources/create")),
+        "SECRET_TOKEN",
+    )
+    .json(&json!({
+        "path": "f/moved/pg",
+        "resource_type": "postgresql",
+        "value": { "host": "db.example", "port": 5432, "dbname": "prod", "user": "app", "password": "pw", "sslmode": "disable" }
+    }))
+    .send()
+    .await?;
+    let status = resp.status().as_u16();
+    let text = resp.text().await?;
+    assert_eq!(status, 400, "{text}");
+    assert!(text.contains("a data table of another workspace"), "{text}");
+    // A second door that got past every write-time guard — a `$var:` changed
+    // under the resource, say — is refused where it is used.
+    sqlx::query(
+        "INSERT INTO resource (workspace_id, path, value, resource_type, created_by, edited_at)
+         VALUES ('detached', 'f/moved/pg', $1, 'postgresql', 'test-user', now())",
+    )
+    .bind(json!({ "host": "db.example", "port": 5432, "dbname": "prod", "user": "postgres", "password": "pw" }))
+    .execute(&db)
+    .await?;
+    let resp = authed(
+        client().get(format!(
+            "http://localhost:{port}/api/w/detached/workspaces/get_datatable_table_schema?datatable_name=door&schema_name=public&table_name=t"
+        )),
+        "SECRET_TOKEN",
+    )
+    .send()
+    .await?;
+    let status = resp.status().as_u16();
+    let text = resp.text().await?;
+    assert_eq!(status, 401, "{text}");
     assert!(
-        text.contains("data table 'byo' of workspace test-workspace"),
+        text.contains("whose role permissions are enabled"),
         "{text}"
     );
     sqlx::query(

backend/windmill-api-workspaces/src/datatable_permissions.rs

@@ -34,7 +34,8 @@ use windmill_common::utils::require_admin;
 use windmill_common::worker::SqlAnnotations;
 use windmill_common::workspaces::{
     can_use_datatable_role, datatable_database_identity, get_datatable_resource_from_db_unchecked,
-    DataTable, DataTableCatalogResourceType, DataTablePermissions, ADMIN_DATATABLE_ROLE,
+    physical_database_identity, DataTable, DataTableCatalogResourceType, DataTablePermissions,
+    ADMIN_DATATABLE_ROLE,
 };
 use windmill_common::{PgDatabase, DB};
 
@@ -234,6 +235,9 @@ pub(crate) struct AdminConnection {
     /// resolution can tell it has not moved. `None` for an instance database,
     /// which no workspace edit can repoint.
     pub(crate) database_identity: Option<String>,
+    /// The database without the login, instance databases included: what every
+    /// other data table entry is held apart from.
+    pub(crate) physical_identity: String,
     pub(crate) admin_pg_role: String,
     pub(crate) pg_roles: PgRoleInventory,
     /// Whether `PUBLIC` holds CREATE on schema `public`, i.e. every role in this
@@ -352,6 +356,7 @@ pub(crate) async fn connect_as_admin_unchecked(
         .resource_type
         == DataTableCatalogResourceType::Postgresql)
         .then(|| datatable_database_identity(&db_resource));
+    let physical_identity = physical_database_identity(&db_resource);
     let pg_db: PgDatabase = serde_json::from_value(db_resource)
         .map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {e}")))?;
     let dbname = pg_db.dbname.clone();
@@ -451,6 +456,7 @@ pub(crate) async fn connect_as_admin_unchecked(
         AdminConnection {
             dbname,
             database_identity,
+            physical_identity,
             admin_pg_role,
             pg_roles: PgRoleInventory { existing, adoptable },
             public_schema_is_open,
@@ -491,6 +497,7 @@ async fn build_plan(
     // Stamped from the connection the roles are about to be created through, so
     // a resolution that lands anywhere else later can refuse.
     plan.permissions.database_identity = conn.database_identity;
+    plan.permissions.physical_identity = Some(conn.physical_identity);
     if !req.enabled {
         // Opting out is never refused; what it strands is said out loud.
         let roles: HashSet<&str> = datatable
@@ -870,8 +877,9 @@ async fn refuse_enabling_permissions_over_shared_access(
 /// resource is a second door.
 ///
 /// An instance database is matched by name. A resource-backed entry reaches
-/// wherever its resource points, whatever path it names, so it is matched by the
-/// resource's host, port, database and user. Read from the stored resource rows
+/// wherever its resource points, whatever path it names and whichever login it
+/// carries, so it is matched by the resource's host, port and database. Read
+/// from the stored resource rows
 /// in one query: a field that is a `$var:` / `$res:` reference is only resolved
 /// — a per-row read, and a secret backend call where the workspace uses one —
 /// when every plain field already agrees, so the loop that decrypts runs for
@@ -920,7 +928,7 @@ async fn entries_reaching(
         }
         DataTableCatalogResourceType::Postgresql => {
             let ours = get_datatable_resource_from_db_unchecked(db, w_id, datatable_name).await?;
-            let identity = datatable_database_identity(&ours);
+            let identity = physical_database_identity(&ours);
             let is_reference = |v: &serde_json::Value| {
                 v.as_str()
                     .is_some_and(|s| s.starts_with("$var:") || s.starts_with("$res:"))
@@ -933,7 +941,7 @@ async fn entries_reaching(
                 // says what it reaches once resolved.
                 let mut references = !raw.is_object();
                 let mut plain_fields_agree = true;
-                for field in DATABASE_IDENTITY_FIELDS {
+                for field in PHYSICAL_DATABASE_FIELDS {
                     let theirs = raw.get(field).unwrap_or(&serde_json::Value::Null);
                     if is_reference(theirs) {
                         references = true;
@@ -966,7 +974,7 @@ async fn entries_reaching(
                             )))
                         }
                     };
-                if datatable_database_identity(&resolved) == identity {
+                if physical_database_identity(&resolved) == identity {
                     reaching.push(describe(&o.workspace_id, &o.name, o.deleted));
                 }
             }
@@ -975,8 +983,8 @@ async fn entries_reaching(
     Ok(reaching)
 }
 
-/// The fields [`datatable_database_identity`] hashes.
-const DATABASE_IDENTITY_FIELDS: [&str; 4] = ["host", "port", "dbname", "user"];
+/// The fields [`physical_database_identity`] hashes.
+const PHYSICAL_DATABASE_FIELDS: [&str; 3] = ["host", "port", "dbname"];
 
 /// Refuse a data table entry that reaches a database another data table governs.
 ///
@@ -1021,9 +1029,9 @@ pub(crate) async fn refuse_reaching_a_governed_database(
                 db,
             )
             .await?;
-            windmill_common::workspaces::governed_datatable_with_identity(
+            windmill_common::workspaces::governed_datatable_reaching(
                 db,
-                &datatable_database_identity(&resource),
+                &physical_database_identity(&resource),
                 &[(w_id, datatable_name)],
             )
             .await?

backend/windmill-common/src/workspaces.rs

@@ -1247,6 +1247,13 @@ pub struct DataTablePermissions {
     /// repointed by editing a resource.
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub database_identity: Option<String>,
+    /// The database itself, as [`physical_database_identity`] fingerprints it —
+    /// the login left out. Stamped at the opt-in, instance databases included,
+    /// and what every other data table entry is held apart from: one reaching
+    /// this database would hand its users the owning connection, whichever login
+    /// its own resource carries.
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub physical_identity: Option<String>,
 }
 
 impl DataTablePermissions {
@@ -1554,9 +1561,11 @@ fn resource_backs_permissioned_datatable(names: &[String]) -> Error {
 /// table may not be pointed at a database another data table governs — that
 /// entry would be a second door onto it, open to every member as the owning
 /// connection. The settings form refuses such an entry; this refuses the
-/// resource edit that would turn an existing entry into one. The new value is
+/// resource write — an edit, a create at a path an entry still names, a rename
+/// onto one — that would turn an existing entry into one. The new value is
 /// resolved for it, `$var:` references included, since that is what the entry
-/// would reach.
+/// would reach. A resource owner need not be an admin anywhere, so a governed
+/// data table of another workspace is not named to them.
 pub async fn ensure_resource_identity_change_allowed(
     db: &DB,
     w_id: &str,
@@ -1595,12 +1604,12 @@ pub async fn ensure_resource_identity_change_allowed(
         return Ok(());
     };
     let resolved = transform_json_unchecked(new_value, w_id, db).await?;
-    let new_identity = datatable_database_identity(&resolved);
+    let new_identity = physical_database_identity(&resolved);
     let own: Vec<(&str, &str)> = backing
         .iter()
         .map(|(name, _)| (w_id, name.as_str()))
         .collect();
-    if let Some((gw, gname)) = governed_datatable_with_identity(db, &new_identity, &own).await? {
+    if let Some((gw, gname)) = governed_datatable_reaching(db, &new_identity, &own).await? {
         let mut names: Vec<&str> = backing.iter().map(|(n, _)| n.as_str()).collect();
         names.sort();
         return Err(Error::BadRequest(format!(
@@ -1611,30 +1620,31 @@ pub async fn ensure_resource_identity_change_allowed(
             if gw == w_id {
                 format!("data table '{gname}'")
             } else {
-                format!("data table '{gname}' of workspace {gw}")
+                "a data table of another workspace".to_string()
             }
         )));
     }
     Ok(())
 }
 
-/// The permissioned data table, in any workspace, whose opt-in stamped
-/// `identity` — other than the `except` entries — if there is one.
+/// The permissioned data table, in any workspace, whose opt-in stamped this
+/// [`physical_database_identity`] — other than the `except` entries — if there
+/// is one.
 ///
 /// Authorization: performs none, and needs none: it answers with a name, for a
 /// caller that only ever refuses on it.
-pub async fn governed_datatable_with_identity(
+pub async fn governed_datatable_reaching(
     db: &DB,
-    identity: &str,
+    physical_identity: &str,
     except: &[(&str, &str)],
 ) -> Result<Option<(String, String)>> {
     let governed = sqlx::query!(
         r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "name!"
            FROM workspace_settings ws, jsonb_each(ws.datatable->'datatables') dt
            WHERE COALESCE((dt.value->'permissions'->>'enabled')::boolean, false)
-             AND dt.value->'permissions'->>'database_identity' = $1
+             AND dt.value->'permissions'->>'physical_identity' = $1
            ORDER BY ws.workspace_id, dt.key"#,
-        identity,
+        physical_identity,
     )
     .fetch_all(db)
     .await?;
@@ -1671,6 +1681,23 @@ pub fn datatable_database_identity(resolved: &serde_json::Value) -> String {
     hex::encode(hasher.finalize())
 }
 
+/// Fingerprint the database a connection resolves to, without the login: the
+/// same host, port and database reached as another user is the same database,
+/// and for the question of who else reaches it, the login does not matter.
+pub fn physical_database_identity(resolved: &serde_json::Value) -> String {
+    use sha2::{Digest, Sha256};
+    let mut hasher = Sha256::new();
+    for field in ["host", "port", "dbname"] {
+        let value = resolved
+            .get(field)
+            .map(|v| v.to_string())
+            .unwrap_or_default();
+        hasher.update(value.as_bytes());
+        hasher.update([0u8]);
+    }
+    hex::encode(hasher.finalize())
+}
+
 /// Whether a resolution has to prove the data table still points where its roles
 /// live.
 ///
@@ -2137,6 +2164,33 @@ async fn get_datatable_resource_inner(
         ensure_datatable_database_unchanged(name, &datatable, &db_resource)?;
     }
 
+    // An unpermissioned data table must not be a second door onto a database
+    // another data table governs. The writes that could make it one — the
+    // settings form, a resource write — refuse, but none of them is transactional
+    // with the opt-in, and a `$var:` a resource references changes under it
+    // through no write of the resource at all; here, on the resolved connection,
+    // is where the answer is authoritative. Internal callers are the guards
+    // themselves and the data table's own administration.
+    if !internal && !datatable.permissions.as_ref().is_some_and(|p| p.enabled) {
+        if let Some((gw, gname)) = governed_datatable_reaching(
+            db,
+            &physical_database_identity(&db_resource),
+            &[(w_id, name)],
+        )
+        .await?
+        {
+            return Err(Error::NotAuthorized(format!(
+                "Data table '{name}' reaches the database of {}, whose role permissions are \
+                 enabled: its own connection would bypass them. Use that data table instead.",
+                if gw == w_id {
+                    format!("data table '{gname}'")
+                } else {
+                    "a data table of another workspace".to_string()
+                }
+            )));
+        }
+    }
+
     // The role logs in as itself rather than through `SET ROLE`, which a script
     // could `RESET ROLE` its way back out of and regain admin's privileges.
     if let Some((pg_rolename, pg_password)) = role_override {
@@ -3452,6 +3506,7 @@ mod tests {
                 roles: map,
                 default_role: None,
                 database_identity: None,
+                physical_identity: None,
             }),
         }
     }

backend/windmill-store/src/resources.rs

@@ -134,7 +134,10 @@ pub struct EditResourceType {
     /// `Option` conflates: an absent field leaves the extension alone, while an
     /// explicit `null` clears it. A hub pull relies on both — a type that stops
     /// being a file type has to stop being one locally too.
-    #[serde(default, deserialize_with = "windmill_common::more_serde::double_option")]
+    #[serde(
+        default,
+        deserialize_with = "windmill_common::more_serde::double_option"
+    )]
     pub format_extension: Option<Option<String>>,
 }
 
@@ -1231,6 +1234,17 @@ async fn create_resource(
         .await
         .map_err(sanitize_db_error)?;
     } else {
+        // A data table entry may already name this path, its resource deleted since.
+        let nvalue: serde_json::Value = serde_json::from_str(raw_json.0.get())
+            .map_err(|e| Error::BadRequest(format!("Invalid resource value: {e}")))?;
+        windmill_common::workspaces::ensure_resource_identity_change_allowed(
+            &db,
+            &w_id,
+            &resource.path,
+            None,
+            Some(&nvalue),
+        )
+        .await?;
         // Create-only (the default): DO NOTHING + a row-count guard, so a path that appears between
         // check_path_conflict above and this insert is rejected rather than overwritten. A plain
         // DO UPDATE here would clobber a concurrently-created resource, breaking create-only callers
@@ -1842,9 +1856,32 @@ async fn update_resource(
         return Err(Error::PermissionDenied(msg));
     }
 
-    // A rename takes the resource out from under whatever names its path.
-    if ns.path.as_deref().is_some_and(|npath| npath != path) {
+    // A rename takes the resource out from under whatever names its path — and
+    // puts it under whatever names the new one.
+    if let Some(npath) = ns.path.as_deref().filter(|npath| *npath != path) {
         windmill_common::workspaces::ensure_resource_removal_allowed(&db, &w_id, path).await?;
+        let arriving: Option<serde_json::Value> = match ns.value.as_ref() {
+            Some(v) => Some(
+                serde_json::from_str(v.get())
+                    .map_err(|e| Error::BadRequest(format!("Invalid resource value: {e}")))?,
+            ),
+            None => sqlx::query_scalar!(
+                "SELECT value FROM resource WHERE path = $1 AND workspace_id = $2",
+                path,
+                &w_id
+            )
+            .fetch_optional(&db)
+            .await?
+            .flatten(),
+        };
+        windmill_common::workspaces::ensure_resource_identity_change_allowed(
+            &db,
+            &w_id,
+            npath,
+            None,
+            arriving.as_ref(),
+        )
+        .await?;
     }
     // Same as `set_resource_value`: the identity a permissioned data table's
     // roles were created against is not free to move underneath them.