diff --git a/backend/.sqlx/query-538dd1779874e4003932d7f17750239c625f85e25b3364bf2edf566f518c8ee2.json b/backend/.sqlx/query-538dd1779874e4003932d7f17750239c625f85e25b3364bf2edf566f518c8ee2.json new file mode 100644 index 0000000000..03613519ad --- /dev/null +++ b/backend/.sqlx/query-538dd1779874e4003932d7f17750239c625f85e25b3364bf2edf566f518c8ee2.json @@ -0,0 +1,29 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"datatable!\"\n FROM workspace_settings ws\n CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt\n WHERE dt.value->'reference'->>'workspace_id' = $1\n AND dt.value->'reference'->>'datatable' = $2", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "workspace_id!", + "type_info": "Varchar" + }, + { + "ordinal": 1, + "name": "datatable!", + "type_info": "Text" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + false, + null + ] + }, + "hash": "538dd1779874e4003932d7f17750239c625f85e25b3364bf2edf566f518c8ee2" +} diff --git a/backend/.sqlx/query-975e404ac3a6258bb8220e122e3de094c7ab23330fdbc74d6e4ad472ddd3c820.json b/backend/.sqlx/query-975e404ac3a6258bb8220e122e3de094c7ab23330fdbc74d6e4ad472ddd3c820.json new file mode 100644 index 0000000000..87666986a3 --- /dev/null +++ b/backend/.sqlx/query-975e404ac3a6258bb8220e122e3de094c7ab23330fdbc74d6e4ad472ddd3c820.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "INSERT INTO global_settings (name, value) VALUES ($1, $2)\n ON CONFLICT (name) DO UPDATE SET value = $2, updated_at = now()", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Varchar", + "Jsonb" + ] + }, + "nullable": [] + }, + "hash": "975e404ac3a6258bb8220e122e3de094c7ab23330fdbc74d6e4ad472ddd3c820" +} diff --git a/backend/.sqlx/query-da5057c86607327bafc2942c218025ca9181a0c396405984d87e422e129521c1.json b/backend/.sqlx/query-da5057c86607327bafc2942c218025ca9181a0c396405984d87e422e129521c1.json new file mode 100644 index 0000000000..4e63a2afa6 --- /dev/null +++ b/backend/.sqlx/query-da5057c86607327bafc2942c218025ca9181a0c396405984d87e422e129521c1.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE workspace_settings ws\n SET datatable = (\n SELECT jsonb_set(ws.datatable, '{datatables}', jsonb_object_agg(\n dt.key,\n CASE WHEN dt.value->'reference'->>'workspace_id' = $1\n AND dt.value->'reference'->>'datatable' = $2\n THEN jsonb_set(dt.value, '{reference,datatable}', to_jsonb($3::text))\n ELSE dt.value END\n ))\n FROM jsonb_each(ws.datatable->'datatables') dt\n )\n WHERE jsonb_typeof(ws.datatable->'datatables') = 'object'\n AND ws.datatable::text LIKE '%\"reference\"%'", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "da5057c86607327bafc2942c218025ca9181a0c396405984d87e422e129521c1" +} diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index 3101739f29..08cc43d0d4 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -358,24 +358,78 @@ async fn concurrent_role_creations_both_survive(db: Pool) -> anyhow::R let body = resp.text().await?; Ok::<_, anyhow::Error>((status, body)) }; - let (a, b) = tokio::join!(create(names[0].clone()), create(names[1].clone())); - let (a, b) = (a?, b?); - assert_eq!(a.0, 200, "{}", a.1); - assert_eq!(b.0, 200, "{}", b.1); + let outcome = async { + let (a, b) = tokio::join!(create(names[0].clone()), create(names[1].clone())); + let (a, b) = (a?, b?); + assert_eq!(a.0, 200, "{}", a.1); + assert_eq!(b.0, 200, "{}", b.1); - let catalog = windmill_common::datatable_roles::read_role_catalog(&db).await?; - let recorded: Vec<&str> = catalog.values().map(|r| r.name.as_str()).collect(); - for name in &names { - assert!( - recorded.contains(&name.as_str()), - "{name} is a live cluster login the catalog forgot: {recorded:?}" - ); + let catalog = windmill_common::datatable_roles::read_role_catalog(&db).await?; + let recorded: Vec<&str> = catalog.values().map(|r| r.name.as_str()).collect(); + for name in &names { + assert!( + recorded.contains(&name.as_str()), + "{name} is a live cluster login the catalog forgot: {recorded:?}" + ); + } + Ok::<_, anyhow::Error>(()) } + .await; + // Roles are cluster-wide, so they outlive this test's throwaway database. Dropped whatever + // happened above — a failing run is exactly the one that created them and did not record them. for name in &names { - sqlx::query(&format!("DROP ROLE IF EXISTS \"{name}\"")) + let _ = sqlx::query(&format!("DROP ROLE IF EXISTS \"{name}\"")) .execute(&db) - .await?; + .await; } + outcome +} + +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn renaming_a_governing_data_table_carries_its_forks(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + // A pointer names the governing data table by name, so a rename that does not follow leaves + // every fork resolving to nothing — the data table vanishes from their pickers and their jobs + // stop, with nothing in the renaming workspace to suggest why. + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/w/test-workspace/workspaces/edit_datatable_config" + )), + "SECRET_TOKEN", + ) + .json(&json!({ + "settings": {"datatables": {"renamed": { + "database": {"resource_type": "instance", "resource_path": "dt_main"} + }}}, + "renames": [{"from": "main", "to": "renamed"}], + "deleted_datatables": [] + })) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + + let entry: Option = sqlx::query_scalar( + "SELECT datatable->'datatables'->'main' FROM workspace_settings WHERE workspace_id = $1", + ) + .bind("wm-fork-dt") + .fetch_one(&db) + .await?; + let entry = entry.unwrap(); + assert_eq!(entry["reference"]["datatable"], "renamed", "{entry}"); + + // And it still resolves, which is the thing the fork actually cares about. + let resp = authed( + client().get(format!( + "http://localhost:{port}/api/w/wm-fork-dt/workspaces/datatable_usable_roles/main" + )), + "SECRET_TOKEN_2", + ) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); Ok(()) } diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index d5d584bd9b..92dad9a4be 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -1356,6 +1356,7 @@ pub async fn get_global_setting( .await? .map(|x| x.value); + let value = value.map(|v| windmill_common::datatable_roles::redact_role_catalog_setting(&key, v)); Ok(Json(value.unwrap_or_else(|| serde_json::Value::Null))) } @@ -1393,7 +1394,13 @@ async fn list_global_settings( require_super_admin(&db, &authed).await?; let settings = sqlx::query_as!(GlobalSetting, "SELECT name, value FROM global_settings") .fetch_all(&db) - .await?; + .await? + .into_iter() + .map(|s| GlobalSetting { + value: windmill_common::datatable_roles::redact_role_catalog_setting(&s.name, s.value), + name: s.name, + }) + .collect(); Ok(Json(settings)) } diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index f8dcc5c579..11ab0c7f9b 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3670,6 +3670,21 @@ async fn edit_datatable_config( dt.permissions = old.and_then(|old| old.permissions.clone()); dt.reference = old.and_then(|old| old.reference.clone()); dt.forked_from = old.and_then(|old| old.forked_from.clone()); + // Carrying the block onto a resource-backed entry would produce a data table the chokepoint + // refuses on every job — a save that succeeds and breaks everything afterwards. Refuse it + // instead: turning roles off first is one step, and it keeps discarding an access decision + // something somebody chose rather than a side effect of moving a database. + if dt.permissions.is_some() + && dt + .database + .as_ref() + .is_some_and(|d| d.resource_type != DataTableCatalogResourceType::Instance) + { + return Err(Error::BadRequest(format!( + "Data table '{name}' is under roles, which only a data table on the instance \ + database can be. Turn its roles off before moving it to a PostgreSQL resource." + ))); + } // A pointer names no database of its own, so the form's empty `database` is correct there. if dt.reference.is_some() { dt.database = None; @@ -3736,6 +3751,53 @@ async fn edit_datatable_config( ) .await?; + // A fork points at a data table by name, so a rename here has to follow or every fork's entry + // resolves to nothing. Inside the transaction: the rename and the pointers that name it are one + // change, and half of it is a fork whose jobs stop. + for r in &new_config.renames { + sqlx::query!( + r#"UPDATE workspace_settings ws + SET datatable = ( + SELECT jsonb_set(ws.datatable, '{datatables}', jsonb_object_agg( + dt.key, + CASE WHEN dt.value->'reference'->>'workspace_id' = $1 + AND dt.value->'reference'->>'datatable' = $2 + THEN jsonb_set(dt.value, '{reference,datatable}', to_jsonb($3::text)) + ELSE dt.value END + )) + FROM jsonb_each(ws.datatable->'datatables') dt + ) + WHERE jsonb_typeof(ws.datatable->'datatables') = 'object' + AND ws.datatable::text LIKE '%"reference"%'"#, + &w_id, + &r.from, + &r.to, + ) + .execute(&mut *tx) + .await?; + } + + // A deletion cannot be followed the same way — there is nothing to point at any more. Read who + // is left stranded so the caller is told, the way deleting a workspace does. + let mut stranded: Vec = Vec::new(); + for name in &new_config.deleted_datatables { + let rows = sqlx::query!( + r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "datatable!" + FROM workspace_settings ws + CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt + WHERE dt.value->'reference'->>'workspace_id' = $1 + AND dt.value->'reference'->>'datatable' = $2"#, + &w_id, + name, + ) + .fetch_all(&mut *tx) + .await?; + stranded.extend( + rows.into_iter() + .map(|r| format!("{}/{}", r.workspace_id, r.datatable)), + ); + } + tx.commit().await?; for substrate in created_substrates { @@ -3750,7 +3812,19 @@ async fn edit_datatable_config( ) .await?; - Ok(format!("Edit datatable config for workspace {}", &w_id)) + if stranded.is_empty() { + Ok(format!("Edit datatable config for workspace {}", &w_id)) + } else { + Ok(format!( + concat!( + "Edit datatable config for workspace {}. These data tables were governed by one ", + "you deleted and no longer resolve: {}. Their databases still exist; a superadmin ", + "can point them at another workspace's data table." + ), + &w_id, + stranded.join(", ") + )) + } } #[derive(Deserialize)] diff --git a/backend/windmill-common/src/datatable_roles.rs b/backend/windmill-common/src/datatable_roles.rs index 3b0b012bb8..4090baee75 100644 --- a/backend/windmill-common/src/datatable_roles.rs +++ b/backend/windmill-common/src/datatable_roles.rs @@ -37,7 +37,6 @@ pub const CUSTOM_INSTANCE_USER: &str = "custom_instance_user"; /// One catalog entry. The password is per role and instance-wide, and lives in the instance's own /// [`DATATABLE_ROLES_SETTING`] row rather than in any workspace's settings. #[derive(Deserialize, Serialize, Clone)] -#[cfg_attr(feature = "instance_config_schema", derive(schemars::JsonSchema))] pub struct InstanceDatatableRole { /// The Postgres role name, verbatim. pub name: String, @@ -115,6 +114,22 @@ fn quote_literal(value: &str) -> String { format!("'{}'", value.replace('\'', "''")) } +/// The catalog as a settings reader may see it: every entry, no password. +/// +/// `GET /settings/global/{key}` and the settings listing hand back whatever is in the row, so the +/// one key whose value is a set of live cluster credentials has to be filtered on the way out. +/// Applied by name, since those endpoints do not know what they are returning. +pub fn redact_role_catalog_setting(name: &str, value: serde_json::Value) -> serde_json::Value { + if name != DATATABLE_ROLES_SETTING { + return value; + } + let mut catalog = parse_role_catalog(Some(value)); + for role in catalog.values_mut() { + role.pwd = None; + } + serde_json::to_value(&catalog).unwrap_or(serde_json::Value::Null) +} + /// Serialize every mutation of the catalog, from the read through the cluster DDL to the write. /// /// The catalog is one JSON document, so create/rename/enable/delete are all read-modify-write. @@ -171,7 +186,7 @@ pub async fn write_role_catalog( .map_err(|e| Error::internal_err(format!("serializing the role catalog: {e}")))?; sqlx::query!( "INSERT INTO global_settings (name, value) VALUES ($1, $2) - ON CONFLICT (name) DO UPDATE SET value = $2", + ON CONFLICT (name) DO UPDATE SET value = $2, updated_at = now()", DATATABLE_ROLES_SETTING, value ) diff --git a/backend/windmill-common/src/lib.rs b/backend/windmill-common/src/lib.rs index 774da9ecdc..a7e84c55d7 100644 --- a/backend/windmill-common/src/lib.rs +++ b/backend/windmill-common/src/lib.rs @@ -1541,7 +1541,8 @@ fn instance_db_grants(dbname: &str) -> String { /// the database and `public` schema owner — since only it can hand out an option it holds. /// /// Authorization: reaches an instance database with the server's own credentials and checks -/// nothing. Callers MUST restrict this to superadmin or internal server paths. +/// nothing. Callers MUST have authorized administration of `dbname` — superadmin, or an admin of +/// the workspace governing a data table on it. pub async fn ensure_instance_db_grant_options_unchecked(db: &DB, dbname: &str) -> error::Result<()> { let dbname = dbname.trim(); validate_dbname(dbname)?;