diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index bbb7ee4259..c1ac38cda2 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -6ced9da74422ae8feee10da70419ccedf313506c +4759cf46829760038a8a07ec7c6a8682fc9a81ef diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index 5543947ce5..10b273cbc7 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -1085,6 +1085,68 @@ async fn browsing_as_a_role_the_caller_may_not_use_is_refused( Ok(()) } +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn an_alias_saved_elsewhere_waits_for_roles_going_on_for_its_database( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + sqlx::query( + r#"UPDATE workspace_settings SET datatable = jsonb_set(datatable, '{datatables,other}', + '{"database": {"resource_type": "instance", "resource_path": "dt_other"}}') + WHERE workspace_id = 'test-workspace'"#, + ) + .execute(&db) + .await?; + + // Roles going on for `dt_other`, not committed yet: it holds only its own workspace's settings + // row, so an alias saved from another workspace that looked for roles now would miss them. + let enabling = { + let mut tx = db.begin().await?; + windmill_common::datatable_roles::lock_instance_databases_governance( + &mut *tx, + ["dt_other"], + ) + .await?; + sqlx::query( + r#"UPDATE workspace_settings SET datatable = jsonb_set(datatable, + '{datatables,other,permissions}', + '{"default_role": "admin", "roles": {"admin": {"tenants": ["*"]}}}') + WHERE workspace_id = 'test-workspace'"#, + ) + .execute(&mut *tx) + .await?; + tx + }; + + let server = ApiServer::start(db.clone()).await?; + let url = format!( + "http://localhost:{}/api/w/wm-fork-dt/workspaces/edit_datatable_config", + server.addr.port() + ); + let save = tokio::spawn( + authed(client().post(&url), "SECRET_TOKEN") + .json(&json!({ "settings": { "datatables": { + "direct": { "database": { "resource_type": "instance", "resource_path": "dt_other" } } + } } })) + .send(), + ); + tokio::time::sleep(std::time::Duration::from_millis(500)).await; + assert!( + !save.is_finished(), + "an alias was saved while roles were going on for its database" + ); + enabling.commit().await?; + + let resp = save.await??; + let status = resp.status(); + let body = resp.text().await?; + assert!( + status == 400 && body.contains("which a data table under roles uses"), + "the alias reached the database whose roles went on while it waited ({status}): {body}" + ); + Ok(()) +} + #[cfg(not(all(feature = "private", feature = "enterprise")))] const ENTERPRISE_REFUSAL: &str = "Data table roles are a Windmill Enterprise Edition feature"; @@ -1174,22 +1236,25 @@ async fn without_the_enterprise_edition_a_data_table_under_roles_is_refused_a_co assert!(err.to_string().contains(ENTERPRISE_REFUSAL), "{err}"); } - // Not under roles, it resolves as it always has; naming a role on it is refused. + // Not under roles, it resolves as it always has, including when `admin` is named — which every + // migration does; naming any other role on it is refused. sqlx::query( "UPDATE workspace_settings SET datatable = datatable #- '{datatables,main,permissions}' WHERE workspace_id = 'test-workspace'", ) .execute(&db) .await?; - let resolved = get_datatable_resource_from_db( - &db, - "test-workspace", - "main", - None, - DatatableAccess::NoIdentity, - ) - .await?; - assert_eq!(resolved["dbname"], "dt_main", "{resolved}"); + for role in [None, Some("admin")] { + let resolved = get_datatable_resource_from_db( + &db, + "test-workspace", + "main", + role, + DatatableAccess::NoIdentity, + ) + .await?; + assert_eq!(resolved["dbname"], "dt_main", "{resolved}"); + } let err = get_datatable_resource_from_db( &db, "test-workspace", diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 39fa8fac5d..8ec966d475 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -37,7 +37,8 @@ use windmill_common::datatable_roles::{ }; use windmill_common::error::{pg_error_message, Error, JsonResult, Result}; use windmill_common::workspaces::{ - get_datatable_resource_from_db_unchecked, resolve_governing_datatable, GoverningDatatable, + get_datatable_resource_from_db_unchecked, resolve_governing_datatable, DataTable, + GoverningDatatable, }; use windmill_common::{PgDatabase, DB}; @@ -1307,6 +1308,76 @@ async fn authorize_acl_change( Ok(governing) } +static APPLY_SLOT: tokio::sync::Semaphore = tokio::sync::Semaphore::const_new(1); + +/// A role passes on only privileges it holds with grant option, and an instance database +/// provisioned before data table roles gave `custom_instance_user` none. Adds that option to its +/// database and `public` privileges, and nothing else: default privileges are left alone, since a +/// schema's change of owner is planned against them. Best-effort, as a grant it fails to enable is +/// refused when it runs. +async fn ensure_grant_options(client: &tokio_postgres::Client, db: &DB, dbname: &str) { + let held = client + .query_one( + "SELECT has_database_privilege(current_database(), 'CONNECT WITH GRANT OPTION') + AND has_database_privilege(current_database(), 'CREATE WITH GRANT OPTION') + AND (to_regnamespace('public') IS NULL + OR (has_schema_privilege('public', 'USAGE WITH GRANT OPTION') + AND has_schema_privilege('public', 'CREATE WITH GRANT OPTION')))", + &[], + ) + .await + .is_ok_and(|row| row.get::<_, bool>(0)); + if held { + return; + } + if let Err(e) = grant_options_as_server(db, dbname).await { + tracing::warn!("Could not enable grant options on '{dbname}': {e}"); + } +} + +/// Only the database's owner, the server's own Postgres user, can hand out an option it holds. +async fn grant_options_as_server(db: &DB, dbname: &str) -> Result<()> { + let server = PgDatabase::parse_uri(&windmill_common::get_database_url().await?.as_str().await)?; + let creds = PgDatabase { dbname: dbname.to_string(), ..server }; + let (client, connection) = creds.connect(Some(db)).await?; + let join_handle = tokio::spawn(async move { connection.await }); + let role = quote_ident(CUSTOM_INSTANCE_USER); + let result = client + .batch_execute(&format!( + "GRANT CONNECT, CREATE ON DATABASE {} TO {role} WITH GRANT OPTION; + DO $$ BEGIN + IF to_regnamespace('public') IS NOT NULL THEN + GRANT USAGE, CREATE ON SCHEMA public TO {role} WITH GRANT OPTION; + END IF; + END $$;", + quote_ident(dbname) + )) + .await; + drop(client); + windmill_common::shutdown_pg_connection(join_handle).await?; + result.map_err(|e| { + Error::internal_err(format!( + "Failed to grant options on '{dbname}': {}", + pg_error_message(&e) + )) + }) +} + +/// Whether the governing entry an apply was authorized on is still the one in the settings, read +/// under the lock: a save in between could have pointed it at another database or changed its roles. +fn entry_unchanged(governing: &GoverningDatatable, entry_now: Option) -> bool { + let Some(Ok(now)) = entry_now.map(serde_json::from_value::) else { + return false; + }; + match ( + serde_json::to_value(&now), + serde_json::to_value(&governing.datatable), + ) { + (Ok(now), Ok(authorized)) => now == authorized, + _ => false, + } +} + /// Plan one change against the catalog and the database as they are now. async fn build_plan( client: &tokio_postgres::Client, @@ -1502,26 +1573,37 @@ async fn apply_datatable_acl( .to_string(), ) })?; - // Refuses without taking a lock; everything is checked again once they are held. + // Everything that needs the pool happens before the locks: once `tx` holds them, a second pool + // connection could wait forever on a pool that concurrent applies, queued on the same locks, + // have exhausted. let governing = authorize_acl_change(&db, &authed, &w_id, &datatable_name).await?; + // Applies queue on an instance-wide lock while each holds a direct connection to the instance's + // Postgres; unbounded, the queue alone could exhaust its connection limit. One at a time per + // server, and the ones waiting hold no connection at all. + let _slot = APPLY_SLOT + .acquire() + .await + .map_err(|e| Error::internal_err(format!("ACL apply slot closed: {e}")))?; + let (mut client, mut notices, dbname) = connect_as_admin_unchecked(&db, &governing).await?; + ensure_grant_options(&client, &db, &dbname).await; // Held until the change is committed: a role renamed or dropped meanwhile would change what // the plan names, and a settings save could move the entry onto another database. Taken in the // same order as the permissions save, so the two cannot deadlock. let mut tx = db.begin().await?; lock_role_catalog(&mut tx).await?; - sqlx::query!( - "SELECT 1 AS one FROM workspace_settings WHERE workspace_id = $1 FOR UPDATE", - &governing.workspace_id + let entry_now = sqlx::query_scalar::<_, Option>( + "SELECT datatable->'datatables'->$2 FROM workspace_settings WHERE workspace_id = $1 FOR UPDATE", ) + .bind(&governing.workspace_id) + .bind(&governing.name) .fetch_optional(&mut *tx) - .await?; - let governing = authorize_acl_change(&db, &authed, &w_id, &datatable_name).await?; + .await? + .flatten(); let catalog = read_role_catalog_tx(&mut tx).await?; - let (mut client, mut notices, dbname) = connect_as_admin_unchecked(&db, &governing).await?; let plan = build_plan(&client, &dbname, &catalog, &req.target, &req.change).await?; - if &plan.statements != confirmed { + if !entry_unchanged(&governing, entry_now) || &plan.statements != confirmed { return Err(Error::BadRequest( "The data table or its roles changed since this was planned, so it would no longer \ run what was confirmed. Plan it again." @@ -1529,14 +1611,6 @@ async fn apply_datatable_acl( )); } - // Postgres only lets a role pass on a privilege it holds with grant option, and an instance - // database provisioned before data table roles holds none. Best-effort: a grant this fails to - // enable is refused below rather than skipped. - if let Err(e) = windmill_common::ensure_instance_db_grant_options_unchecked(&db, &dbname).await - { - tracing::warn!("Could not refresh grant options on '{dbname}': {e}"); - } - // One transaction: a half-applied ownership transfer leaves one schema's objects owned by two // different roles. let pg_tx = client.transaction().await.map_err(|e| { diff --git a/backend/windmill-api-workspaces/src/datatable_migrations.rs b/backend/windmill-api-workspaces/src/datatable_migrations.rs index 8900e708e4..fa433b2cbc 100644 --- a/backend/windmill-api-workspaces/src/datatable_migrations.rs +++ b/backend/windmill-api-workspaces/src/datatable_migrations.rs @@ -174,6 +174,10 @@ async fn datatable_database_arg( // default role — which is what `ensure_migration_role_allowed` gated it as, and which is the // only role a DDL statement can be expected to succeed under. A migration that does declare a // role overrides this: the annotation wins over the reference. + // + // A legacy name containing `?` cannot be migrated through this reference: the appended query + // makes it neither an exact name nor a parseable one. Accepted on purpose, since such names can + // no longer be created and none are expected to carry migrations. Ok(to_raw_value(&format!( "datatable://{datatable_name}?role={ADMIN_DATATABLE_ROLE}" ))) diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 0a2e343065..b85ea992a9 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -4002,50 +4002,64 @@ async fn edit_datatable_config( // entry through a declared rename alone, and a settings sync never declares one, so an entry // without roles that newly points at such a database — a name added, or an existing one // repointed — would answer everyone there as `admin`. That holds whichever workspace governs it. - let governed_elsewhere: Vec = sqlx::query_scalar( - "SELECT DISTINCT dt.value->'database'->>'resource_path' FROM workspace_settings ws - CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt - WHERE ws.workspace_id <> $1 AND dt.value ? 'permissions' - AND dt.value->'database'->>'resource_type' = 'instance'", + let newly_pointed: Vec<(&String, &str)> = new_config + .settings + .datatables + .iter() + .filter(|(_, dt)| dt.permissions.is_none()) + .filter_map(|(name, dt)| { + let db = dt + .database + .as_ref() + .filter(|d| d.resource_type == DataTableCatalogResourceType::Instance)?; + let lookup = rename_src + .get(name.as_str()) + .copied() + .unwrap_or(name.as_str()); + let repointed = old_datatables + .get(lookup) + .and_then(|old| old.database.as_ref()) + .is_none_or(|old_db| { + old_db.resource_type != db.resource_type + || old_db.resource_path != db.resource_path + }); + repointed.then_some((name, db.resource_path.as_str())) + }) + .collect(); + // Another workspace turning roles on for the same database holds only its own settings row, so + // without this the scan below could read past its uncommitted write. + windmill_common::datatable_roles::lock_instance_databases_governance( + &mut *tx, + newly_pointed.iter().map(|(_, dbname)| *dbname), ) - .bind(&w_id) - .fetch_all(&mut *tx) .await?; - for (name, dt) in new_config.settings.datatables.iter() { - if dt.permissions.is_some() { - continue; - } - let Some(db) = dt - .database - .as_ref() - .filter(|d| d.resource_type == DataTableCatalogResourceType::Instance) - else { - continue; - }; - let lookup = rename_src - .get(name.as_str()) - .copied() - .unwrap_or(name.as_str()); - let repointed = old_datatables - .get(lookup) - .and_then(|old| old.database.as_ref()) - .is_none_or(|old_db| { - old_db.resource_type != db.resource_type || old_db.resource_path != db.resource_path - }); + let governed_elsewhere: Vec = if newly_pointed.is_empty() { + vec![] + } else { + sqlx::query_scalar( + "SELECT DISTINCT dt.value->'database'->>'resource_path' FROM workspace_settings ws + CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt + WHERE ws.workspace_id <> $1 AND dt.value ? 'permissions' + AND dt.value->'database'->>'resource_type' = 'instance'", + ) + .bind(&w_id) + .fetch_all(&mut *tx) + .await? + }; + for (name, dbname) in newly_pointed { let governed_here = old_datatables.values().any(|old| { old.permissions.is_some() && old.database.as_ref().is_some_and(|d| { d.resource_type == DataTableCatalogResourceType::Instance - && d.resource_path == db.resource_path + && d.resource_path == dbname }) }); - if repointed && (governed_here || governed_elsewhere.contains(&db.resource_path)) { + if governed_here || governed_elsewhere.iter().any(|g| g == dbname) { return Err(Error::BadRequest(format!( - "Data table '{name}' would point at database '{}', which a data table under roles \ - uses, without carrying those roles: everyone reaching '{name}' would connect there \ - as `admin`. Rename the data table under roles from the data table settings, which \ - carries its roles, or turn its roles off first.", - db.resource_path + "Data table '{name}' would point at database '{dbname}', which a data table under \ + roles uses, without carrying those roles: everyone reaching '{name}' would connect \ + there as `admin`. Rename the data table under roles from the data table settings, \ + which carries its roles, or turn its roles off first." ))); } } diff --git a/backend/windmill-common/src/datatable_roles.rs b/backend/windmill-common/src/datatable_roles.rs index 3c269d1ada..4dd9b06fde 100644 --- a/backend/windmill-common/src/datatable_roles.rs +++ b/backend/windmill-common/src/datatable_roles.rs @@ -140,6 +140,25 @@ pub async fn lock_datatable_streams(conn: &mut sqlx::PgConnection, exclusive: bo Ok(()) } +/// Whether an instance database is reached only through entries under roles is decided by two +/// writes that lock different workspaces' settings rows: turning roles on for one entry, and a +/// settings save pointing an entry without roles at the database. Each holds this for every +/// database it decides on, so neither reads past the other's uncommitted write. Held for the +/// transaction; the names are locked in sorted order so two holders cannot deadlock. +pub async fn lock_instance_databases_governance<'a>( + conn: &mut sqlx::PgConnection, + dbnames: impl IntoIterator, +) -> Result<()> { + let dbnames: std::collections::BTreeSet<&str> = dbnames.into_iter().collect(); + for dbname in dbnames { + sqlx::query("SELECT pg_advisory_xact_lock(hashtext('datatable_instance_database:' || $1))") + .bind(dbname) + .execute(&mut *conn) + .await?; + } + Ok(()) +} + /// Disclosure: returns every role's stored Postgres password in plaintext. Any server path that /// has to resolve or name a role may call it — including handlers open to a workspace member, who /// need the names — but callers MUST NOT let `pwd` reach a response, a log line, an audit record diff --git a/backend/windmill-common/src/datatable_roles_oss.rs b/backend/windmill-common/src/datatable_roles_oss.rs index 569aa7ff1f..2a3f9dda48 100644 --- a/backend/windmill-common/src/datatable_roles_oss.rs +++ b/backend/windmill-common/src/datatable_roles_oss.rs @@ -164,8 +164,8 @@ mod ce { Err(unavailable()) } - /// A data table not under roles, asked for no role, is not a role decision and passes, as it - /// did before roles existed. Anything else is refused. + /// A data table not under roles, asked for no role or for `admin`, is not a role decision and + /// passes, as it did before roles existed. Anything else is refused. pub(crate) async fn ensure_can_use_datatable_role( db: &DB, w_id: &str, @@ -175,7 +175,9 @@ mod ce { _context: &str, ) -> Result<()> { let governing = resolve_governing_datatable(db, w_id, name).await?; - if governing.datatable.permissions.is_none() && role.is_none() { + if governing.datatable.permissions.is_none() + && role.is_none_or(|r| r == crate::datatable_roles::ADMIN_DATATABLE_ROLE) + { Ok(()) } else { Err(unavailable()) diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 7ce41e78d3..878f78f4f0 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1664,9 +1664,10 @@ pub async fn get_datatable_resource_from_db( ) -> Result { let governing = resolve_governing_datatable(db, w_id, name).await?; let db_resource = resolve_datatable_connection_unchecked(db, &governing, false).await?; - // Not under roles and asked for none: the `admin` connection, as before roles existed, in - // every edition. Anything else is a role decision. - if governing.datatable.permissions.is_none() && role.is_none() { + // Not under roles and asked for none, or for `admin` by name: the `admin` connection, as before + // roles existed, in every edition. Anything else is a role decision. Every migration names + // `admin` explicitly, so an edition without roles must not treat that as one. + if governing.datatable.permissions.is_none() && role.is_none_or(|r| r == ADMIN_DATATABLE_ROLE) { return Ok(db_resource); } crate::datatable_roles_oss::resolve_datatable_role_connection(