From fa4c06658f20df813d9d06d92548a9747df9243f Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Tue, 1 Sep 2026 11:58:58 +0200 Subject: [PATCH] fix(datatables): resolve ACL objects against the catalog, and gate on the enterprise edition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 2 +- .../src/datatable_acl.rs | 105 +++++++++++++++++- .../src/datatable_acl_oss.rs | 11 +- .../src/datatable_permissions.rs | 56 +++++++--- .../src/datatable_permissions_oss.rs | 35 +++++- backend/windmill-api-workspaces/src/lib.rs | 6 +- .../windmill-api-workspaces/src/workspaces.rs | 6 +- 7 files changed, 190 insertions(+), 31 deletions(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 4d2618a356..4c7dc9a5ac 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -ef2f5f487b90ac64d07d39f791592a5445887c47 +67c80ba9088de8cec92a3a528b637648f9b9b3a0 diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index e07171b34e..81d8a0ad67 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/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> { + 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> { + 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, @@ -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, diff --git a/backend/windmill-api-workspaces/src/datatable_acl_oss.rs b/backend/windmill-api-workspaces/src/datatable_acl_oss.rs index c790507e3d..40b9ccecd9 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl_oss.rs +++ b/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, diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 0e20f0705d..1df699c833 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/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, } -/// 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, /// `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, } -async fn read_default_acl_rules(client: &tokio_postgres::Client) -> Result> { - // 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> { 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, diff --git a/backend/windmill-api-workspaces/src/datatable_permissions_oss.rs b/backend/windmill-api-workspaces/src/datatable_permissions_oss.rs index d62783c955..90da8051ae 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions_oss.rs +++ b/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")); + } +} diff --git a/backend/windmill-api-workspaces/src/lib.rs b/backend/windmill-api-workspaces/src/lib.rs index 3fb81da3d2..dcd88df8ae 100644 --- a/backend/windmill-api-workspaces/src/lib.rs +++ b/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; diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 18997300d9..4fa4d32c84 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -2515,7 +2515,8 @@ async fn get_datatable_schema( datatable_name: &str, ) -> Result { // 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?;