fix(datatables): resolve ACL objects against the catalog, and gate on the enterprise edition

A revoke's objects came from the request, argument types included, and those go
into the statement unquoted — so a schema owner could close a routine signature
and append SQL that ran as the data table's administrative login. The request
now only names an object: what reaches the statement is read back from the
catalog, and an object that resolves to nothing is refused.

The planners were behind `private`, which community builds carry, so the
permissions API answered on a CE binary. They take `enterprise` as well, with
a test that pins the refusal in every other edition.

Default privileges are read back scoped to one data table's own roles: two data
tables can share a database, and a new role of one was inheriting the other's
rules.

backend/ee-repo-ref.txt

@@ -1 +1 @@
-ef2f5f487b90ac64d07d39f791592a5445887c47
+67c80ba9088de8cec92a3a528b637648f9b9b3a0

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

@@ -68,6 +68,15 @@ pub enum AclTarget {
 }
 
 impl AclTarget {
+    /// The schema the target is in, absent for the database itself.
+    pub(crate) fn schema(&self) -> Option<&str> {
+        match self {
+            AclTarget::Database => None,
+            AclTarget::Schema { schema } => Some(schema),
+            AclTarget::Table { schema, .. } => Some(schema),
+        }
+    }
+
     /// What it is called in a message.
     pub(crate) fn label(&self, dbname: &str) -> String {
         match self {
@@ -438,6 +447,89 @@ async fn read_owned_objects(
     Ok(objects)
 }
 
+/// The keyword a `REVOKE ... ON` takes for one object, checked rather than
+/// interpolated: it lands in SQL unquoted.
+pub(crate) fn object_keyword(kind: &str) -> Result<&'static str> {
+    match kind.to_uppercase().as_str() {
+        "TABLE" | "VIEW" | "MATERIALIZED VIEW" | "FOREIGN TABLE" => Ok("TABLE"),
+        "SEQUENCE" => Ok("SEQUENCE"),
+        "FUNCTION" => Ok("FUNCTION"),
+        other => Err(Error::BadRequest(format!("Unknown object kind '{other}'"))),
+    }
+}
+
+/// Every object of a schema, named the way the catalog names it.
+async fn read_schema_objects(
+    client: &tokio_postgres::Client,
+    schema: &str,
+) -> Result<Vec<AclObject>> {
+    let rows = client
+        .query(
+            "SELECT CASE c.relkind WHEN 'S' THEN 'SEQUENCE' ELSE 'TABLE' END, c.relname, NULL::text
+             FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace
+             WHERE n.nspname = $1 AND c.relkind = ANY(ARRAY['r','p','v','m','S','f']::\"char\"[])
+             UNION ALL
+             SELECT 'FUNCTION', p.proname, pg_get_function_identity_arguments(p.oid)
+             FROM pg_proc p JOIN pg_namespace n ON n.oid = p.pronamespace
+             WHERE n.nspname = $1",
+            &[&schema],
+        )
+        .await
+        .map_err(|e| {
+            Error::internal_err(format!(
+                "Failed to list the objects of schema '{schema}': {}",
+                pg_error_message(&e)
+            ))
+        })?;
+    Ok(rows
+        .into_iter()
+        .map(|row| AclObject { kind: row.get(0), name: row.get(1), args: row.get(2) })
+        .collect())
+}
+
+/// Replace the objects a revoke names with the catalog's own entry for each.
+///
+/// A routine is identified by its argument types, and those go into the
+/// statement as written — there is no quoting for them — so the request may name
+/// an object but never spell one: what reaches the SQL is read back from
+/// Postgres. An object that resolves to nothing is refused rather than dropped,
+/// since a revoke that silently covers less than it says is worse than an error.
+async fn resolve_acl_objects(
+    client: &tokio_postgres::Client,
+    target: &AclTarget,
+    objects: &[AclObject],
+) -> Result<Vec<AclObject>> {
+    if objects.is_empty() {
+        return Ok(vec![]);
+    }
+    let Some(schema) = target.schema() else {
+        return Err(Error::BadRequest(
+            "A database has no objects of its own to revoke on".to_string(),
+        ));
+    };
+    let known = read_schema_objects(client, schema).await?;
+    objects
+        .iter()
+        .map(|requested| {
+            let keyword = object_keyword(&requested.kind)?;
+            known
+                .iter()
+                .find(|k| {
+                    k.name == requested.name
+                        && k.args == requested.args
+                        && object_keyword(&k.kind).is_ok_and(|k| k == keyword)
+                })
+                .cloned()
+                .ok_or_else(|| {
+                    Error::NotFound(format!(
+                        "'{}' is not an object of schema '{schema}'",
+                        requested.name
+                    ))
+                })
+        })
+        .collect()
+}
+
 async fn get_datatable_acl(
     authed: ApiAuthed,
     Extension(db): Extension<DB>,
@@ -754,9 +846,20 @@ async fn build_acl_plan(
         }
         _ => vec![],
     };
+    // The objects a revoke names come from the request; the ones it plans with
+    // come from the catalog.
+    let change = match &req.change {
+        AclChange::Revoke { role, privileges, scope, objects } => AclChange::Revoke {
+            role: role.clone(),
+            privileges: privileges.clone(),
+            scope: *scope,
+            objects: resolve_acl_objects(&client, &req.target, objects).await?,
+        },
+        change => change.clone(),
+    };
     let plan = crate::datatable_acl_oss::plan_statements(
         &req.target,
-        &req.change,
+        &change,
         &conn.dbname,
         &pg_role,
         &other_pg_roles,

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

@@ -9,20 +9,21 @@
 //! Where the ACL planner comes from: the enterprise one, or a refusal.
 //!
 //! Reading who owns what stays open; every owner change and every grant is the
-//! plan this returns, so an edition without the enterprise module cannot make
-//! one.
+//! plan this returns, so an edition that is not enterprise cannot make one.
+//! `private` alone is not that edition: community builds carry it, so the
+//! planner is behind `enterprise` as well.
 
-#[cfg(feature = "private")]
+#[cfg(all(feature = "private", feature = "enterprise"))]
 #[allow(unused)]
 pub(crate) use crate::datatable_acl_ee::*;
 
-#[cfg(not(feature = "private"))]
+#[cfg(not(all(feature = "private", feature = "enterprise")))]
 use {
     crate::datatable_acl::{AclChange, AclPlan, AclTarget, OwnedObject},
     windmill_common::error::{Error, Result},
 };
 
-#[cfg(not(feature = "private"))]
+#[cfg(not(all(feature = "private", feature = "enterprise")))]
 pub(crate) fn plan_statements(
     _target: &AclTarget,
     _change: &AclChange,

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

@@ -120,7 +120,10 @@ pub(crate) struct PlannedStatement {
 
 impl PlannedStatement {
     /// Only the planner builds these, and that is the enterprise module.
-    #[cfg_attr(not(feature = "private"), allow(dead_code))]
+    #[cfg_attr(
+        not(all(feature = "private", feature = "enterprise")),
+        allow(dead_code)
+    )]
     pub(crate) fn plain(sql: String) -> Self {
         Self { display: sql.clone(), sql }
     }
@@ -134,9 +137,10 @@ pub(crate) struct RolePlan {
     pub(crate) warnings: Vec<String>,
 }
 
-/// Refuse to plan a change on a binary that carries the enterprise modules but
-/// no active plan. Without them there is no planner at all, so this is the whole
-/// gate on an enterprise build.
+/// Refuse to plan a change on an enterprise binary whose plan does not cover it.
+/// A build that is not enterprise has no planner at all — see
+/// [`crate::datatable_permissions_oss`] — so this only has the licensed
+/// editions left to tell apart.
 pub(crate) async fn require_datatable_permissions_license() -> Result<()> {
     #[cfg(feature = "enterprise")]
     if !matches!(
@@ -151,7 +155,10 @@ pub(crate) async fn require_datatable_permissions_license() -> Result<()> {
 }
 
 /// Only the planners quote identifiers, and those are the enterprise modules.
-#[cfg_attr(not(feature = "private"), allow(dead_code))]
+#[cfg_attr(
+    not(all(feature = "private", feature = "enterprise")),
+    allow(dead_code)
+)]
 pub(crate) fn quote_ident(ident: &str) -> String {
     render_db_quoted_identifier(ident, DbType::Postgresql)
 }
@@ -223,29 +230,36 @@ pub(crate) struct DefaultAclRule {
     pub(crate) schema: Option<String>,
     /// `TABLES`, `SEQUENCES`, `FUNCTIONS` or `TYPES`.
     pub(crate) objects: String,
-    /// Postgres role the privileges go to, or `PUBLIC`.
+    /// Postgres role the privileges go to, always one of this data table's own.
     pub(crate) grantee: String,
     pub(crate) privileges: Vec<String>,
 }
 
-async fn read_default_acl_rules(client: &tokio_postgres::Client) -> Result<Vec<DefaultAclRule>> {
-    // Only the rules of this feature's own roles — and of the data table's
-    // connection — are replayed; the rest of the cluster's policy is not ours to
-    // copy onto a new role.
+/// The rules this data table's own roles wrote, which are the only ones a role
+/// of this data table inherits.
+///
+/// Scoped to `own_pg_roles` on both sides. Two data tables can point at one
+/// physical database — and share its administrative login — so a rule is ours
+/// only when both the role that wrote it and the role it grants to are.
+async fn read_default_acl_rules(
+    client: &tokio_postgres::Client,
+    own_pg_roles: &[String],
+) -> Result<Vec<DefaultAclRule>> {
     let rows = client
         .query(
             "SELECT n.nspname,
                     CASE d.defaclobjtype
                         WHEN 'r' THEN 'TABLES' WHEN 'S' THEN 'SEQUENCES'
                         WHEN 'f' THEN 'FUNCTIONS' ELSE 'TYPES' END,
-                    CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END,
+                    pg_get_userbyid(a.grantee),
                     a.privilege_type
              FROM pg_default_acl d
              LEFT JOIN pg_namespace n ON n.oid = d.defaclnamespace,
                   aclexplode(d.defaclacl) a
-             WHERE pg_get_userbyid(d.defaclrole) LIKE 'wm\\_%'
-                OR pg_get_userbyid(d.defaclrole) = current_user",
-            &[],
+             WHERE pg_get_userbyid(d.defaclrole) = ANY($1)
+               AND a.grantee <> 0
+               AND pg_get_userbyid(a.grantee) = ANY($1)",
+            &[&own_pg_roles],
         )
         .await
         .map_err(|e| {
@@ -338,7 +352,19 @@ pub(crate) async fn connect_as_admin(
         })?
         .get(0);
 
-    let default_acl_rules = read_default_acl_rules(&client).await?;
+    // The roles a rule of this data table can be written by or granted to: its
+    // own, plus the connection they were all created from.
+    let mut own_pg_roles = vec![admin_pg_role.clone()];
+    own_pg_roles.extend(
+        read_datatable(db, w_id, datatable_name)
+            .await?
+            .permissions
+            .filter(|p| p.enabled)
+            .into_iter()
+            .flat_map(|p| p.roles.into_values())
+            .filter_map(|role| role.pg_rolename),
+    );
+    let default_acl_rules = read_default_acl_rules(&client, &own_pg_roles).await?;
 
     Ok((
         client,

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

@@ -9,14 +9,15 @@
 //! Where the role planner comes from: the enterprise one, or a refusal.
 //!
 //! Every change to a data table's Postgres roles — creating, renaming, dropping
-//! them — is the plan this returns, so an edition without the enterprise module
-//! cannot make one.
+//! them — is the plan this returns, so an edition that is not enterprise cannot
+//! make one. `private` alone is not that edition: community builds carry it, so
+//! the planner is behind `enterprise` as well.
 
-#[cfg(feature = "private")]
+#[cfg(all(feature = "private", feature = "enterprise"))]
 #[allow(unused)]
 pub(crate) use crate::datatable_permissions_ee::*;
 
-#[cfg(not(feature = "private"))]
+#[cfg(not(all(feature = "private", feature = "enterprise")))]
 use {
     crate::datatable_permissions::{DefaultAclRule, RolePlan, SetDatatablePermissions},
     std::collections::HashSet,
@@ -24,7 +25,7 @@ use {
     windmill_common::workspaces::DataTablePermissions,
 };
 
-#[cfg(not(feature = "private"))]
+#[cfg(not(all(feature = "private", feature = "enterprise")))]
 pub(crate) fn plan_role_changes(
     _w_id: &str,
     _datatable: &str,
@@ -40,3 +41,27 @@ pub(crate) fn plan_role_changes(
         "Data table permissions are a Windmill Enterprise Edition feature".to_string(),
     ))
 }
+
+#[cfg(all(test, not(all(feature = "private", feature = "enterprise"))))]
+mod tests {
+    /// Compiled in every edition that is not enterprise — community builds
+    /// included, which carry the enterprise sources but must not plan with them.
+    #[test]
+    fn a_non_enterprise_build_plans_nothing() {
+        let plan = super::plan_role_changes(
+            "acme",
+            "main",
+            "db",
+            "admin",
+            None,
+            &serde_json::from_value(serde_json::json!({ "enabled": true })).unwrap(),
+            &Default::default(),
+            false,
+            &[],
+        );
+        assert!(plan
+            .unwrap_err()
+            .to_string()
+            .contains("Windmill Enterprise Edition"));
+    }
+}

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

@@ -9,9 +9,11 @@ pub mod workspaces;
 pub mod workspaces_extra;
 pub mod workspaces_oss;
 
-#[cfg(feature = "private")]
+// Community builds carry `private`, so the data table planners take the
+// enterprise feature too: what they plan is enterprise-only.
+#[cfg(all(feature = "private", feature = "enterprise"))]
 pub mod datatable_acl_ee;
-#[cfg(feature = "private")]
+#[cfg(all(feature = "private", feature = "enterprise"))]
 pub mod datatable_permissions_ee;
 #[cfg(feature = "private")]
 pub mod workspaces_ee;

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

@@ -2515,7 +2515,8 @@ async fn get_datatable_schema(
     datatable_name: &str,
 ) -> Result<SchemaMap> {
     // Get the datatable resource (connection credentials)
-    let db_resource = get_datatable_resource_as_default_role(db, authed, w_id, datatable_name).await?;
+    let db_resource =
+        get_datatable_resource_as_default_role(db, authed, w_id, datatable_name).await?;
 
     // Parse the resource as PgDatabase
     let pg_db: PgDatabase = serde_json::from_value(db_resource)
@@ -2764,7 +2765,8 @@ async fn get_datatable_table_columns(
         )));
     }
 
-    let db_resource = get_datatable_resource_as_default_role(db, authed, w_id, datatable_name).await?;
+    let db_resource =
+        get_datatable_resource_as_default_role(db, authed, w_id, datatable_name).await?;
     let pg_db: PgDatabase = serde_json::from_value(db_resource)
         .map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {}", e)))?;
     let (client, connection) = pg_db.connect(Some(db)).await?;