Merge remote-tracking branch 'origin/datatable-roles-redesign-part-2' into datatable-roles-redesign-part-4

# Conflicts:
#	backend/ee-repo-ref.txt
#	backend/windmill-api-integration-tests/tests/datatable_roles.rs

backend/ee-repo-ref.txt

@@ -1 +1 @@
-6ced9da74422ae8feee10da70419ccedf313506c
+4759cf46829760038a8a07ec7c6a8682fc9a81ef

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<Postgres>,
+) -> 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",

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<serde_json::Value>) -> bool {
+    let Some(Ok(now)) = entry_now.map(serde_json::from_value::<DataTable>) 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<serde_json::Value>>(
+        "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| {

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

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<String> = 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<String> = 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."
             )));
         }
     }

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<Item = &'a str>,
+) -> 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

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())

backend/windmill-common/src/workspaces.rs

@@ -1664,9 +1664,10 @@ pub async fn get_datatable_resource_from_db(
 ) -> Result<serde_json::Value> {
     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(