From e6c91aebeb46bf6a7bde93deca59533612e3b545 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Sun, 6 Sep 2026 13:37:19 +0200 Subject: [PATCH] fix(datatables): the same-database scan reads resource rows, the settings form refuses a second door, and a fork keeps no orphan migrations --- ...7bcd5a35bc618ef43d54130be373c7ec8b7da.json | 41 +++ ...baeb468330b51edfb690921c67828c4c0bdf3.json | 14 + ...7100f39a7a7704a8ad31dba82042f6fd0b63.json} | 12 +- backend/ee-repo-ref.txt | 2 +- .../tests/datatable_tenants.rs | 88 +++++++ .../src/datatable_permissions.rs | 242 +++++++++++++----- .../windmill-api-workspaces/src/workspaces.rs | 29 +++ 7 files changed, 364 insertions(+), 64 deletions(-) create mode 100644 backend/.sqlx/query-7918594d0fbbc2dac2397cba0bb7bcd5a35bc618ef43d54130be373c7ec8b7da.json create mode 100644 backend/.sqlx/query-bf7d6e52d23cd0aed238ec1a883baeb468330b51edfb690921c67828c4c0bdf3.json rename backend/.sqlx/{query-41030ce05776505da194798e418e634cade1f712b8a6dd4ab50eb8e2207a4c62.json => query-e53586dfce4275479a353b3bd0ff7100f39a7a7704a8ad31dba82042f6fd0b63.json} (58%) diff --git a/backend/.sqlx/query-7918594d0fbbc2dac2397cba0bb7bcd5a35bc618ef43d54130be373c7ec8b7da.json b/backend/.sqlx/query-7918594d0fbbc2dac2397cba0bb7bcd5a35bc618ef43d54130be373c7ec8b7da.json new file mode 100644 index 0000000000..2d225dcb7f --- /dev/null +++ b/backend/.sqlx/query-7918594d0fbbc2dac2397cba0bb7bcd5a35bc618ef43d54130be373c7ec8b7da.json @@ -0,0 +1,41 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"name!\",\n dt.value->'database' AS \"database!\",\n dt.value->'permissions'->>'database_identity' AS identity\n FROM workspace_settings ws, jsonb_each(ws.datatable->'datatables') dt\n WHERE NOT (ws.workspace_id = $1 AND dt.key = $2)\n AND COALESCE((dt.value->'permissions'->>'enabled')::boolean, false)\n ORDER BY ws.workspace_id, dt.key", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "workspace_id!", + "type_info": "Varchar" + }, + { + "ordinal": 1, + "name": "name!", + "type_info": "Text" + }, + { + "ordinal": 2, + "name": "database!", + "type_info": "Jsonb" + }, + { + "ordinal": 3, + "name": "identity", + "type_info": "Text" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + false, + null, + null, + null + ] + }, + "hash": "7918594d0fbbc2dac2397cba0bb7bcd5a35bc618ef43d54130be373c7ec8b7da" +} diff --git a/backend/.sqlx/query-bf7d6e52d23cd0aed238ec1a883baeb468330b51edfb690921c67828c4c0bdf3.json b/backend/.sqlx/query-bf7d6e52d23cd0aed238ec1a883baeb468330b51edfb690921c67828c4c0bdf3.json new file mode 100644 index 0000000000..5e4cb7ca90 --- /dev/null +++ b/backend/.sqlx/query-bf7d6e52d23cd0aed238ec1a883baeb468330b51edfb690921c67828c4c0bdf3.json @@ -0,0 +1,14 @@ +{ + "db_name": "PostgreSQL", + "query": "DELETE FROM datatable_migrations m\n WHERE m.workspace_id = $1\n AND NOT EXISTS (\n SELECT 1 FROM workspace_settings ws\n WHERE ws.workspace_id = $1 AND ws.datatable->'datatables' ? m.datatable\n )", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [] + }, + "hash": "bf7d6e52d23cd0aed238ec1a883baeb468330b51edfb690921c67828c4c0bdf3" +} diff --git a/backend/.sqlx/query-41030ce05776505da194798e418e634cade1f712b8a6dd4ab50eb8e2207a4c62.json b/backend/.sqlx/query-e53586dfce4275479a353b3bd0ff7100f39a7a7704a8ad31dba82042f6fd0b63.json similarity index 58% rename from backend/.sqlx/query-41030ce05776505da194798e418e634cade1f712b8a6dd4ab50eb8e2207a4c62.json rename to backend/.sqlx/query-e53586dfce4275479a353b3bd0ff7100f39a7a7704a8ad31dba82042f6fd0b63.json index baff76d089..dee4138908 100644 --- a/backend/.sqlx/query-41030ce05776505da194798e418e634cade1f712b8a6dd4ab50eb8e2207a4c62.json +++ b/backend/.sqlx/query-e53586dfce4275479a353b3bd0ff7100f39a7a7704a8ad31dba82042f6fd0b63.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"name!\", w.deleted AS \"deleted!\",\n dt.value->'database' AS \"database!\"\n FROM workspace_settings ws\n JOIN workspace w ON w.id = ws.workspace_id,\n jsonb_each(ws.datatable->'datatables') dt\n WHERE NOT (ws.workspace_id = $1 AND dt.key = $2)\n AND jsonb_typeof(dt.value->'database') = 'object'\n ORDER BY ws.workspace_id, dt.key", + "query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"name!\", w.deleted AS \"deleted!\",\n dt.value->'database' AS \"database!\", r.value AS \"resource?\"\n FROM workspace_settings ws\n JOIN workspace w ON w.id = ws.workspace_id\n CROSS JOIN LATERAL jsonb_each(ws.datatable->'datatables') dt\n LEFT JOIN resource r ON r.workspace_id = ws.workspace_id\n AND dt.value->'database'->>'resource_type' <> 'instance'\n AND r.path = dt.value->'database'->>'resource_path'\n WHERE NOT (ws.workspace_id = $1 AND dt.key = $2)\n AND jsonb_typeof(dt.value->'database') = 'object'\n ORDER BY ws.workspace_id, dt.key", "describe": { "columns": [ { @@ -22,6 +22,11 @@ "ordinal": 3, "name": "database!", "type_info": "Jsonb" + }, + { + "ordinal": 4, + "name": "resource?", + "type_info": "Jsonb" } ], "parameters": { @@ -34,8 +39,9 @@ false, null, false, - null + null, + true ] }, - "hash": "41030ce05776505da194798e418e634cade1f712b8a6dd4ab50eb8e2207a4c62" + "hash": "e53586dfce4275479a353b3bd0ff7100f39a7a7704a8ad31dba82042f6fd0b63" } diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 6935b5a17d..142f90f469 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -f89f0cd16ec24f7beb146ba8373d688e1fe126e8 +1e70ab9734682756988f184e850a3b2c5a0595fe diff --git a/backend/windmill-api-integration-tests/tests/datatable_tenants.rs b/backend/windmill-api-integration-tests/tests/datatable_tenants.rs index d44da8917e..0126116805 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_tenants.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_tenants.rs @@ -428,11 +428,22 @@ async fn a_kept_original_does_not_inherit_the_clone_stamp( "byo": { "database": { "resource_type": "postgresql", "resource_path": "u/test-user/pg" }, "forked_from": { "schema": {} } + }, + "governed": { + "database": { "resource_type": "instance", "resource_path": "dt_governed" }, + "permissions": { "enabled": true, "roles": { "admin": { "tenants": [] } } } } } })) .execute(&db) .await?; + sqlx::query( + "INSERT INTO datatable_migrations (workspace_id, datatable, timestamp, name, code_up) + VALUES ('test-workspace', 'governed', 1, 'init', 'SELECT 1'), + ('test-workspace', 'byo', 1, 'init', 'SELECT 1')", + ) + .execute(&db) + .await?; let resp = authed( client().post(format!("{ws}/workspaces/create_fork")), @@ -450,6 +461,13 @@ async fn a_kept_original_does_not_inherit_the_clone_stamp( .await?; assert_eq!(copy["database"]["resource_path"], json!("u/test-user/pg")); assert!(copy.get("forked_from").is_none(), "{copy}"); + // The permissioned data table stayed out of the fork, its migrations with it. + let migrated: Vec = sqlx::query_scalar( + "SELECT datatable FROM datatable_migrations WHERE workspace_id = 'wm-fork-kept' ORDER BY 1", + ) + .fetch_all(&db) + .await?; + assert_eq!(migrated, vec!["byo".to_string()]); let resp = authed( client().post(format!("{ws}/workspaces/datatable_permissions/byo/preview")), @@ -735,6 +753,76 @@ async fn a_same_named_resource_counts_only_when_it_reaches_the_same_database( "{text}" ); + // The same rule from the other side: once `byo` is governed, the settings + // form cannot add an entry that reaches its database, in this workspace or + // another, and the pointer alone does not decide it. + let identity: String = { + // Stamped by the opt-in in the real flow; here from the same resource. + let resp = authed( + client().get(format!( + "http://localhost:{port}/api/w/test-workspace/resources/get_value_interpolated/u/test-user/pg" + )), + "SECRET_TOKEN", + ) + .send() + .await?; + assert_eq!(resp.status(), 200); + let value: serde_json::Value = resp.json().await?; + use sha2::{Digest, Sha256}; + let mut hasher = Sha256::new(); + for field in ["host", "port", "dbname", "user"] { + hasher.update(value.get(field).map(|v| v.to_string()).unwrap_or_default()); + hasher.update([0u8]); + } + format!("{:x}", hasher.finalize()) + }; + 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)) + WHERE workspace_id = 'test-workspace'"#, + ) + .bind(&identity) + .execute(&db) + .await?; + let save = |w: &'static str, path: &'static str| async move { + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/w/{w}/workspaces/edit_datatable_config" + )), + "SECRET_TOKEN", + ) + .json(&json!({ "settings": { "datatables": { + "door": { "database": { "resource_type": "postgresql", "resource_path": path } } + }}})) + .send() + .await + .unwrap(); + (resp.status().as_u16(), resp.text().await.unwrap()) + }; + let (status, text) = save("detached", "f/moved/pg").await; + assert_eq!(status, 400, "{text}"); + assert!( + text.contains("data table 'byo' of workspace test-workspace"), + "{text}" + ); + sqlx::query( + "UPDATE resource SET value = jsonb_set(value, '{dbname}', '\"other\"') + WHERE workspace_id = 'detached' AND path = 'f/moved/pg'", + ) + .execute(&db) + .await?; + let (status, text) = save("detached", "f/moved/pg").await; + assert_eq!(status, 200, "{text}"); + sqlx::query( + r#"UPDATE workspace_settings + SET datatable = datatable #- '{datatables,byo,permissions}' + WHERE workspace_id = 'test-workspace'"#, + ) + .execute(&db) + .await?; + // A second entry of this workspace on the same resource is a second door. sqlx::query("DELETE FROM workspace_settings WHERE workspace_id = 'detached'") .execute(&db) diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 2a59ce5242..d975b3a4d2 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -499,6 +499,7 @@ async fn build_plan( .map(|p| p.roles.keys().map(String::as_str)) .into_iter() .flatten() + .filter(|name| *name != ADMIN_DATATABLE_ROLE) .collect(); let stranded = migrations_naming(db, w_id, datatable_name, &roles).await?; if !stranded.is_empty() { @@ -849,65 +850,7 @@ async fn refuse_enabling_permissions_over_shared_access( ))); } } - // Every other data table entry on the instance, this workspace's other - // entries included: a second entry on the same resource is a second door. - let entries = sqlx::query!( - r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "name!", w.deleted AS "deleted!", - dt.value->'database' AS "database!" - FROM workspace_settings ws - JOIN workspace w ON w.id = ws.workspace_id, - jsonb_each(ws.datatable->'datatables') dt - WHERE NOT (ws.workspace_id = $1 AND dt.key = $2) - AND jsonb_typeof(dt.value->'database') = 'object' - ORDER BY ws.workspace_id, dt.key"#, - w_id, - datatable_name, - ) - .fetch_all(db) - .await?; - let mut others = Vec::new(); - match datatable.database.resource_type { - DataTableCatalogResourceType::Instance => { - others.extend( - entries - .iter() - .filter(|o| o.database == database) - .map(|o| describe(&o.workspace_id, &o.name, o.deleted)), - ); - } - DataTableCatalogResourceType::Postgresql => { - // A resource-backed entry reaches wherever its resource points, - // whatever path it names: a copy of the resource under another path, - // in another workspace or this one, is the same database. - let identity = datatable_database_identity( - &get_datatable_resource_from_db_unchecked(db, w_id, datatable_name).await?, - ); - for o in entries.iter().filter(|o| { - o.database.get("resource_type").and_then(|t| t.as_str()) != Some("instance") - }) { - let resolved = - match get_datatable_resource_from_db_unchecked(db, &o.workspace_id, &o.name) - .await - { - Ok(resolved) => resolved, - // A resource that is gone reaches nothing. Anything else is - // not proof of not reaching: refused, and named. - Err(Error::NotFound(_)) => continue, - Err(e) => { - return Err(Error::BadRequest(format!( - "Data table permissions cannot be enabled: whether {} reaches the \ - same database could not be checked ({e}). Remove that data \ - table first.", - describe(&o.workspace_id, &o.name, o.deleted) - ))) - } - }; - if datatable_database_identity(&resolved) == identity { - others.push(describe(&o.workspace_id, &o.name, o.deleted)); - } - } - } - } + let others = entries_reaching(db, w_id, datatable_name, &datatable.database).await?; if !others.is_empty() { return Err(Error::BadRequest(format!( "Data table permissions cannot be enabled while another data table reaches the \ @@ -921,6 +864,183 @@ async fn refuse_enabling_permissions_over_shared_access( Ok(()) } +/// `[, archived] (data table '')` for every data table entry on +/// the instance, other than `(w_id, datatable_name)`, that reaches `database` — +/// this workspace's other entries included: a second entry on the same +/// 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 +/// 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 +/// candidates that can match and not for every data table on the instance. A +/// resource that no longer exists reaches nothing. +async fn entries_reaching( + db: &DB, + w_id: &str, + datatable_name: &str, + database: &windmill_common::workspaces::DataTableDatabase, +) -> Result> { + let pointer = serde_json::to_value(database) + .map_err(|e| Error::internal_err(format!("Failed to serialize the database: {e}")))?; + let describe = |workspace_id: &str, name: &str, deleted: bool| { + format!( + "{workspace_id}{} (data table '{name}')", + if deleted { ", archived" } else { "" } + ) + }; + let entries = sqlx::query!( + r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "name!", w.deleted AS "deleted!", + dt.value->'database' AS "database!", r.value AS "resource?" + FROM workspace_settings ws + JOIN workspace w ON w.id = ws.workspace_id + CROSS JOIN LATERAL jsonb_each(ws.datatable->'datatables') dt + LEFT JOIN resource r ON r.workspace_id = ws.workspace_id + AND dt.value->'database'->>'resource_type' <> 'instance' + AND r.path = dt.value->'database'->>'resource_path' + WHERE NOT (ws.workspace_id = $1 AND dt.key = $2) + AND jsonb_typeof(dt.value->'database') = 'object' + ORDER BY ws.workspace_id, dt.key"#, + w_id, + datatable_name, + ) + .fetch_all(db) + .await?; + let mut reaching = Vec::new(); + match database.resource_type { + DataTableCatalogResourceType::Instance => { + reaching.extend( + entries + .iter() + .filter(|o| o.database == pointer) + .map(|o| describe(&o.workspace_id, &o.name, o.deleted)), + ); + } + DataTableCatalogResourceType::Postgresql => { + let ours = get_datatable_resource_from_db_unchecked(db, w_id, datatable_name).await?; + let identity = datatable_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:")) + }; + for o in entries.iter() { + let Some(raw) = o.resource.as_ref() else { + continue; + }; + let Some(fields) = raw.as_object() else { + continue; + }; + let mut references = false; + let mut plain_fields_agree = true; + for field in DATABASE_IDENTITY_FIELDS { + let theirs = fields.get(field).unwrap_or(&serde_json::Value::Null); + if is_reference(theirs) { + references = true; + } else if theirs != ours.get(field).unwrap_or(&serde_json::Value::Null) { + plain_fields_agree = false; + } + } + if !plain_fields_agree { + continue; + } + if !references { + reaching.push(describe(&o.workspace_id, &o.name, o.deleted)); + continue; + } + let resolved = + match get_datatable_resource_from_db_unchecked(db, &o.workspace_id, &o.name) + .await + { + Ok(resolved) => resolved, + Err(Error::NotFound(_)) => continue, + // Not resolving is not proof of not reaching: refused, and + // named. + Err(e) => { + return Err(Error::BadRequest(format!( + "Whether {} reaches the same database could not be checked \ + ({e}). Remove that data table first.", + describe(&o.workspace_id, &o.name, o.deleted) + ))) + } + }; + if datatable_database_identity(&resolved) == identity { + reaching.push(describe(&o.workspace_id, &o.name, o.deleted)); + } + } + } + } + Ok(reaching) +} + +/// The fields [`datatable_database_identity`] hashes. +const DATABASE_IDENTITY_FIELDS: [&str; 4] = ["host", "port", "dbname", "user"]; + +/// Refuse a data table entry that reaches a database another data table governs. +/// +/// The opt-in refuses while any other entry reaches the database; this is the +/// same rule from the other side, for the settings form that adds an entry or +/// points one elsewhere. Without it a second entry on a governed database is a +/// second door, open to every member as the owning connection. Governed entries +/// are matched by the identity their opt-in stamped, so nothing of theirs is +/// resolved; the entry being saved is resolved once, and must resolve. +pub(crate) async fn refuse_reaching_a_governed_database( + db: &DB, + w_id: &str, + datatable_name: &str, + database: &windmill_common::workspaces::DataTableDatabase, +) -> Result<()> { + let pointer = serde_json::to_value(database) + .map_err(|e| Error::internal_err(format!("Failed to serialize the database: {e}")))?; + let governed = sqlx::query!( + r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "name!", + dt.value->'database' AS "database!", + dt.value->'permissions'->>'database_identity' AS identity + FROM workspace_settings ws, jsonb_each(ws.datatable->'datatables') dt + WHERE NOT (ws.workspace_id = $1 AND dt.key = $2) + AND COALESCE((dt.value->'permissions'->>'enabled')::boolean, false) + ORDER BY ws.workspace_id, dt.key"#, + w_id, + datatable_name, + ) + .fetch_all(db) + .await?; + if governed.is_empty() { + return Ok(()); + } + let hit = match database.resource_type { + DataTableCatalogResourceType::Instance => governed.iter().find(|g| g.database == pointer), + DataTableCatalogResourceType::Postgresql => { + // The stored config is what the form is about to replace, so the entry + // is resolved from the database it names rather than from the config. + let resource = windmill_common::workspaces::transform_json_value_unchecked( + &serde_json::Value::String(format!("$res:{}", database.resource_path)), + w_id, + db, + ) + .await?; + let identity = datatable_database_identity(&resource); + governed + .iter() + .find(|g| g.identity.as_deref() == Some(identity.as_str())) + } + }; + if let Some(g) = hit { + return Err(Error::BadRequest(format!( + "Data table '{datatable_name}' would reach the database of {}, whose role \ + permissions are enabled: every member would reach it through this data table's \ + own connection, as every role at once.", + if g.workspace_id == w_id { + format!("data table '{}'", g.name) + } else { + format!("data table '{}' of workspace {}", g.name, g.workspace_id) + } + ))); + } + Ok(()) +} + /// Refuse a save that names something that no longer exists: a tenant whose /// user, group or folder is gone, or a role that stored migrations still name /// and the save no longer defines. @@ -1016,11 +1136,13 @@ async fn ensure_save_names_what_exists( // Renamed away or removed: every old name the save no longer defines. let kept: HashSet<&str> = req.roles.iter().map(|r| r.name.as_str()).collect(); + // `admin` resolves to the data table's own connection whether or not it is + // named, so a migration naming it never strands. let gone: HashSet<&str> = old .map(|old| old.roles.keys().map(String::as_str)) .into_iter() .flatten() - .filter(|name| !kept.contains(name)) + .filter(|name| !kept.contains(name) && *name != ADMIN_DATATABLE_ROLE) .collect(); let blocking = migrations_naming(db, w_id, datatable_name, &gone).await?; if !blocking.is_empty() { diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 86f1221cbf..437a6c9bc1 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3876,6 +3876,21 @@ async fn edit_datatable_config( schema: old.schema.clone(), }), }; + // An entry that is new, or points somewhere new, must not reach a database + // another data table governs. + let points_elsewhere = old.is_none_or(|old| { + old.database.resource_path != dt.database.resource_path + || old.database.resource_type != dt.database.resource_type + }); + if points_elsewhere { + crate::datatable_permissions::refuse_reaching_a_governed_database( + &db, + &w_id, + name, + &dt.database, + ) + .await?; + } // The roles live in the database this data table points at: their logins // were created there and every grant they hold is recorded there. Carried // onto another database they authenticate against a cluster that never @@ -8411,6 +8426,20 @@ async fn create_workspace_fork( .execute(&mut *tx) .await?; + // The migrations were cloned for every data table; the ones left out above + // would otherwise keep a history for a data table the fork does not have. + sqlx::query!( + r#"DELETE FROM datatable_migrations m + WHERE m.workspace_id = $1 + AND NOT EXISTS ( + SELECT 1 FROM workspace_settings ws + WHERE ws.workspace_id = $1 AND ws.datatable->'datatables' ? m.datatable + )"#, + &forked_id, + ) + .execute(&mut *tx) + .await?; + // The settings clone copies the source's ducklake config verbatim — including a parent // fork's own `fork_behavior` stamps. Sharing is a per-fork-creation choice, never // inherited: reset any cloned stamps first, then apply this fork's requested list.