diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 4007d6ee0a..f41bb39dd4 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -58,8 +58,13 @@ pub enum AclTarget { /// The data table's own database — where the privilege to create schemas /// lives. Database, - Schema { schema: String }, - Table { schema: String, table: String }, + Schema { + schema: String, + }, + Table { + schema: String, + table: String, + }, } impl AclTarget { @@ -110,9 +115,9 @@ impl TryFrom for AclTarget { ("database", _, _) => Ok(AclTarget::Database), ("schema", Some(schema), _) => Ok(AclTarget::Schema { schema }), ("table", Some(schema), Some(table)) => Ok(AclTarget::Table { schema, table }), - ("schema" | "table", None, _) => Err(Error::BadRequest( - "This target needs a schema".to_string(), - )), + ("schema" | "table", None, _) => { + Err(Error::BadRequest("This target needs a schema".to_string())) + } ("table", _, None) => Err(Error::BadRequest( "A table target needs a table".to_string(), )), @@ -237,6 +242,10 @@ pub struct AclObject { pub name: String, /// `TABLE`, `SEQUENCE`, ... — what the object is, since the keyword differs. pub kind: String, + /// A routine is identified by its argument types, not by its name: two + /// `f` in one schema are two objects. Absent for everything else. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub args: Option, } /// A grant as the database has it, in Windmill's vocabulary where it can be. @@ -461,10 +470,9 @@ fn plan_statements( statements.push(format!("ALTER {} OWNER TO {}", target.object(dbname), role)); for object in existing_objects { statements.push(format!( - "ALTER {} {}.{} OWNER TO {}", + "ALTER {} {} OWNER TO {}", object.keyword, - schema, - quote_ident(&object.name), + object_ref(&schema, &object.name, object.args.as_deref()), role )); } @@ -552,11 +560,10 @@ fn plan_statements( (_, None) if !objects.is_empty() => { for object in objects { statements.push(format!( - "REVOKE {} ON {} {}.{} FROM {}", + "REVOKE {} ON {} {} FROM {}", privileges, object_keyword(&object.kind)?, - schema, - quote_ident(&object.name), + object_ref(&schema, &object.name, object.args.as_deref()), role )); } @@ -609,6 +616,21 @@ struct OwnedObject { name: String, /// The keyword `ALTER ... OWNER TO` takes for this kind of object. keyword: &'static str, + /// Identity arguments of a routine, which is what tells two of the same + /// name apart. `None` for a relation. + args: Option, +} + +/// `"schema"."name"` — with `(args)` for a routine, which is not identified +/// without them. `quoted_schema` comes quoted already, being the same for a +/// whole plan. +fn object_ref(quoted_schema: &str, name: &str, args: Option<&str>) -> String { + format!( + "{}.{}{}", + quoted_schema, + quote_ident(name), + args.map(|a| format!("({a})")).unwrap_or_default() + ) } fn keyword_of_relkind(relkind: i8) -> Option<&'static str> { @@ -643,13 +665,40 @@ async fn read_owned_objects( pg_error_message(&e) )) })?; - Ok(rows + let mut objects: Vec = rows .into_iter() .filter_map(|row| { - keyword_of_relkind(row.get::<_, i8>(1)) - .map(|keyword| OwnedObject { name: row.get(0), keyword }) + keyword_of_relkind(row.get::<_, i8>(1)).map(|keyword| OwnedObject { + name: row.get(0), + keyword, + args: None, + }) }) - .collect()) + .collect(); + // Routines live in `pg_proc`, not `pg_class`, and would keep the previous + // owner while the schema they are in changes hands. `ALTER ROUTINE` covers + // functions, procedures and aggregates alike. + let routines = client + .query( + "SELECT 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 + ORDER BY p.proname", + &[&schema], + ) + .await + .map_err(|e| { + Error::internal_err(format!( + "Failed to list the routines of schema '{schema}': {}", + pg_error_message(&e) + )) + })?; + objects.extend(routines.into_iter().map(|row| OwnedObject { + name: row.get(0), + keyword: "ROUTINE", + args: Some(row.get(1)), + })); + Ok(objects) } async fn get_datatable_acl( @@ -660,8 +709,8 @@ async fn get_datatable_acl( ) -> JsonResult { let role = query.role.clone(); let target: AclTarget = query.try_into()?; - let (client, conn) = connect_as_caller(&db, &authed, &w_id, &datatable_name, role.as_deref()) - .await?; + let (client, conn) = + connect_as_caller(&db, &authed, &w_id, &datatable_name, role.as_deref()).await?; let roles = role_map( &db, &w_id, @@ -716,8 +765,8 @@ async fn get_datatable_acl( )) })?, }; - let owner_row = - owner_row.ok_or_else(|| Error::NotFound(format!("{} not found", target.label(&conn.dbname))))?; + let owner_row = owner_row + .ok_or_else(|| Error::NotFound(format!("{} not found", target.label(&conn.dbname))))?; let owner: String = owner_row.get(0); // Membership in the owning role is what Postgres asks for before an ALTER // ... OWNER or a GRANT on something you do not own; `admin` holds every role @@ -789,7 +838,7 @@ async fn read_grants( AclTarget::Database => client .query( "SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END, - a.privilege_type, NULL::text, NULL::text, NULL::text + a.privilege_type, NULL::text, NULL::text, NULL::text, NULL::text FROM pg_database d, aclexplode(d.datacl) a WHERE d.datname = current_database()", &[], @@ -800,7 +849,7 @@ async fn read_grants( let mut out = client .query( "SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END, - a.privilege_type, NULL::text, NULL::text, NULL::text + a.privilege_type, NULL::text, NULL::text, NULL::text, NULL::text FROM pg_namespace n, aclexplode(n.nspacl) a WHERE n.nspname = $1", &[schema], @@ -812,7 +861,8 @@ async fn read_grants( .query( "SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END, a.privilege_type, c.relname, NULL::text, - CASE c.relkind WHEN 'S' THEN 'SEQUENCE' ELSE 'TABLE' END + CASE c.relkind WHEN 'S' THEN 'SEQUENCE' ELSE 'TABLE' END, + NULL::text FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace, aclexplode(c.relacl) a @@ -822,6 +872,24 @@ async fn read_grants( .await .map_err(grant_read_error)?, ); + out.extend( + client + .query( + // Routines carry their own acl in `pg_proc`; without this + // a grant made here would vanish on the next read and + // could never be revoked back. + "SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END, + a.privilege_type, p.proname, NULL::text, 'FUNCTION', + pg_get_function_identity_arguments(p.oid) + FROM pg_proc p + JOIN pg_namespace n ON n.oid = p.pronamespace, + aclexplode(p.proacl) a + WHERE n.nspname = $1", + &[schema], + ) + .await + .map_err(grant_read_error)?, + ); out.extend( client .query( @@ -829,7 +897,7 @@ async fn read_grants( a.privilege_type, NULL::text, CASE d.defaclobjtype WHEN 'r' THEN 'TABLES' WHEN 'S' THEN 'SEQUENCES' - WHEN 'f' THEN 'FUNCTIONS' ELSE 'TYPES' END, NULL::text + WHEN 'f' THEN 'FUNCTIONS' ELSE 'TYPES' END, NULL::text, NULL::text FROM pg_default_acl d JOIN pg_namespace n ON n.oid = d.defaclnamespace, aclexplode(d.defaclacl) a @@ -844,7 +912,7 @@ async fn read_grants( AclTarget::Table { schema, table } => client .query( "SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END, - a.privilege_type, NULL::text, NULL::text, NULL::text + a.privilege_type, NULL::text, NULL::text, NULL::text, NULL::text FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace, aclexplode(c.relacl) a @@ -857,18 +925,31 @@ async fn read_grants( // One row per privilege — and, for default privileges, one per creating // role. Fold them back into one entry per grantee and object. - let mut folded: BTreeMap<(String, Option<(String, String)>, Option), Vec> = - BTreeMap::new(); + let mut folded: BTreeMap< + ( + String, + Option<(String, String, Option)>, + Option, + ), + Vec, + > = BTreeMap::new(); for row in rows.drain(..) { let grantee: String = row.get(0); let privilege: String = row.get(1); let object: Option = row.get(2); let future: Option = row.get(3); let object_kind: Option = row.get(4); + let object_args: Option = row.get(5); folded .entry(( windmill_role_of(roles, &grantee), - object.map(|name| (name, object_kind.unwrap_or_else(|| "TABLE".to_string()))), + object.map(|name| { + ( + name, + object_kind.unwrap_or_else(|| "TABLE".to_string()), + object_args, + ) + }), future, )) .or_default() @@ -882,7 +963,7 @@ async fn read_grants( AclGrant { grantee, privileges, - object: object.map(|(name, kind)| AclObject { name, kind }), + object: object.map(|(name, kind, args)| AclObject { name, kind, args }), future, } }) @@ -967,7 +1048,8 @@ async fn apply_datatable_acl( ) -> Result { // Granting is passing a privilege on, which this connection cannot do for a // privilege it holds without the grant option. - crate::datatable_permissions::ensure_instance_db_can_delegate(&db, &w_id, &datatable_name).await; + crate::datatable_permissions::ensure_instance_db_can_delegate(&db, &w_id, &datatable_name) + .await; let (mut client, plan, dbname) = build_acl_plan(&db, &authed, &w_id, &datatable_name, &req).await?; @@ -1020,8 +1102,13 @@ mod tests { #[test] fn set_owner_covers_the_schema_and_what_is_in_it() { let objects = vec![ - OwnedObject { name: "orders".to_string(), keyword: "TABLE" }, - OwnedObject { name: "orders_id_seq".to_string(), keyword: "SEQUENCE" }, + OwnedObject { name: "orders".to_string(), keyword: "TABLE", args: None }, + OwnedObject { name: "orders_id_seq".to_string(), keyword: "SEQUENCE", args: None }, + OwnedObject { + name: "total".to_string(), + keyword: "ROUTINE", + args: Some("integer, text".to_string()), + }, ]; let plan = plan_statements( &schema(), @@ -1033,11 +1120,14 @@ mod tests { ) .unwrap(); assert_eq!( - plan.statements[..3], + plan.statements[..4], [ r#"ALTER SCHEMA "analytics" OWNER TO "wm_analyst_1""#.to_string(), r#"ALTER TABLE "analytics"."orders" OWNER TO "wm_analyst_1""#.to_string(), r#"ALTER SEQUENCE "analytics"."orders_id_seq" OWNER TO "wm_analyst_1""#.to_string(), + // A routine is only named by its arguments. + r#"ALTER ROUTINE "analytics"."total"(integer, text) OWNER TO "wm_analyst_1""# + .to_string(), ] ); // What the other roles create later stays within the owner's reach. @@ -1151,6 +1241,7 @@ mod tests { objects: vec![AclObject { name: "orders".to_string(), kind: "TABLE".to_string(), + args: None, }], }, "dt_probe", @@ -1244,8 +1335,8 @@ mod tests { privileges: vec!["SELECT".to_string()], scope: GrantScope::Target, objects: vec![ - AclObject { name: "a".to_string(), kind: "TABLE".to_string() }, - AclObject { name: "b".to_string(), kind: "TABLE".to_string() }, + AclObject { name: "a".to_string(), kind: "TABLE".to_string(), args: None }, + AclObject { name: "b".to_string(), kind: "TABLE".to_string(), args: None }, ], }, "dt_probe", diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index e8fc8b0cde..a82bdac48a 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -27,10 +27,10 @@ use std::collections::{BTreeMap, HashSet}; use windmill_api_auth::ApiAuthed; use windmill_audit::audit_oss::audit_log; use windmill_audit::ActionKind; +use windmill_common::ensure_instance_db_grant_options; use windmill_common::error::{pg_error_message, Error, JsonResult, Result}; use windmill_common::query_builders::{render_db_quoted_identifier, DbType}; use windmill_common::utils::{rd_string, require_admin}; -use windmill_common::ensure_instance_db_grant_options; use windmill_common::workspaces::{ can_use_datatable_role, datatable_pg_role_name, get_datatable_resource_from_db_unchecked, DataTable, DataTableCatalogResourceType, DataTablePermissions, DataTableRole, @@ -182,6 +182,7 @@ fn plan_role_changes( req: &SetDatatablePermissions, existing_pg_roles: &HashSet, public_schema_is_open: bool, + default_acl_rules: &[DefaultAclRule], ) -> Result { // A disabled data table has no Postgres roles, whatever its config says, so // re-enabling always plans every role as a creation. @@ -390,9 +391,14 @@ fn plan_role_changes( "Role '{from}' was expected to exist in the database as '{old_pg}' but does not; it will be created as '{pg_rolename}'." )); creates_sql.push(create_role_statement(&pg_rolename, &password)); - creates_sql.push(grant_role_to_admin_statement(&pg_rolename, admin_pg_role)); + creates_sql + .push(grant_role_to_admin_statement(&pg_rolename, admin_pg_role)); creates_sql.push(revoke_public_create_statement(&pg_rolename)); creates_sql.push(grant_connect_statement(&pg_rolename, dbname)); + creates_sql.extend(replay_default_acl_statements( + &pg_rolename, + default_acl_rules, + )); } } roles.insert( @@ -429,6 +435,10 @@ fn plan_role_changes( creates_sql.push(grant_role_to_admin_statement(&pg_rolename, admin_pg_role)); creates_sql.push(revoke_public_create_statement(&pg_rolename)); creates_sql.push(grant_connect_statement(&pg_rolename, dbname)); + creates_sql.extend(replay_default_acl_statements( + &pg_rolename, + default_acl_rules, + )); roles.insert( name.clone(), DataTableRole { @@ -591,11 +601,7 @@ fn grant_connect_statement(pg_rolename: &str, dbname: &str) -> PlannedStatement )) } -pub(crate) async fn read_datatable( - db: &DB, - w_id: &str, - datatable_name: &str, -) -> Result { +pub(crate) async fn read_datatable(db: &DB, w_id: &str, datatable_name: &str) -> Result { let value = sqlx::query_scalar!( "SELECT ws.datatable->'datatables'->$2 FROM workspace_settings ws WHERE ws.workspace_id = $1", w_id, @@ -628,9 +634,7 @@ pub(crate) async fn ensure_instance_db_can_delegate(db: &DB, w_id: &str, datatab if datatable.database.resource_type != DataTableCatalogResourceType::Instance { return; } - if let Err(e) = - ensure_instance_db_grant_options(db, &datatable.database.resource_path).await - { + if let Err(e) = ensure_instance_db_grant_options(db, &datatable.database.resource_path).await { tracing::warn!( "Could not refresh the grant options of instance database '{}': {}. Continuing.", datatable.database.resource_path, @@ -651,6 +655,98 @@ pub(crate) struct AdminConnection { /// Whether `PUBLIC` holds CREATE on schema `public`, i.e. every role in this /// database — including the ones created here — can make objects in it. pub(crate) public_schema_is_open: bool, + /// The default-privilege rules already in force for this data table's roles. + pub(crate) default_acl_rules: Vec, +} + +/// One `ALTER DEFAULT PRIVILEGES` rule as the catalog has it. +/// +/// Postgres records such a rule per creating role, so a role added later is not +/// covered by any of them: a grant on "future tables" would quietly stop +/// applying to whatever that new role creates. Replaying the existing rules for +/// each new role is what keeps the policy whole. +#[derive(Debug, Clone, PartialEq)] +pub(crate) struct DefaultAclRule { + /// `None` for a rule that is not scoped to a schema. + pub(crate) schema: Option, + /// `TABLES`, `SEQUENCES`, `FUNCTIONS` or `TYPES`. + pub(crate) objects: String, + /// Postgres role the privileges go to, or `PUBLIC`. + 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. + 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, + 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", + &[], + ) + .await + .map_err(|e| { + Error::internal_err(format!( + "Failed to read the default privileges: {}", + pg_error_message(&e) + )) + })?; + let mut folded: BTreeMap<(Option, String, String), Vec> = BTreeMap::new(); + for row in rows { + folded + .entry((row.get(0), row.get(1), row.get(2))) + .or_default() + .push(row.get(3)); + } + Ok(folded + .into_iter() + .map(|((schema, objects, grantee), mut privileges)| { + privileges.sort(); + privileges.dedup(); + DefaultAclRule { schema, objects, grantee, privileges } + }) + .collect()) +} + +/// Put a role that did not exist when a default-privilege rule was written under +/// that same rule, for what it creates from here on. +fn replay_default_acl_statements( + pg_rolename: &str, + rules: &[DefaultAclRule], +) -> Vec { + rules + .iter() + .filter(|rule| rule.grantee != pg_rolename && !rule.privileges.is_empty()) + .map(|rule| { + let grantee = if rule.grantee == "PUBLIC" { + "PUBLIC".to_string() + } else { + quote_ident(&rule.grantee) + }; + PlannedStatement::plain(format!( + "ALTER DEFAULT PRIVILEGES FOR ROLE {}{} GRANT {} ON {} TO {};", + quote_ident(pg_rolename), + rule.schema + .as_ref() + .map(|s| format!(" IN SCHEMA {}", quote_ident(s))) + .unwrap_or_default(), + rule.privileges.join(", "), + rule.objects, + grantee + )) + }) + .collect() } pub(crate) async fn connect_as_admin( @@ -717,9 +813,17 @@ pub(crate) async fn connect_as_admin( })? .get(0); + let default_acl_rules = read_default_acl_rules(&client).await?; + Ok(( client, - AdminConnection { dbname, admin_pg_role, existing_pg_roles, public_schema_is_open }, + AdminConnection { + dbname, + admin_pg_role, + existing_pg_roles, + public_schema_is_open, + default_acl_rules, + }, )) } @@ -740,6 +844,7 @@ async fn build_plan( req, &conn.existing_pg_roles, conn.public_schema_is_open, + &conn.default_acl_rules, )?; Ok((client, plan)) } @@ -1021,6 +1126,16 @@ mod tests { req: &SetDatatablePermissions, existing: &[&str], public_schema_is_open: bool, + ) -> Result { + plan_with_default_acls(old, req, existing, public_schema_is_open, &[]) + } + + fn plan_with_default_acls( + old: Option<&DataTablePermissions>, + req: &SetDatatablePermissions, + existing: &[&str], + public_schema_is_open: bool, + default_acl_rules: &[DefaultAclRule], ) -> Result { plan_role_changes( W_ID, @@ -1031,6 +1146,7 @@ mod tests { req, &existing.iter().map(|r| r.to_string()).collect(), public_schema_is_open, + default_acl_rules, ) } @@ -1073,6 +1189,29 @@ mod tests { .is_none()); } + /// A default-privilege rule binds only the roles it was written for, so a + /// role added later would create tables no one else can read. + #[test] + fn a_new_role_inherits_the_default_privileges_already_in_force() { + let req = SetDatatablePermissions { + enabled: true, + roles: vec![role("admin", &[]), role("writer", &[])], + default_role: None, + renames: vec![], + }; + let rules = vec![DefaultAclRule { + schema: Some("analytics".to_string()), + objects: "TABLES".to_string(), + grantee: "wm_analyst".to_string(), + privileges: vec!["SELECT".to_string()], + }]; + let plan = plan_with_default_acls(None, &req, &[], false, &rules).unwrap(); + let pg_role = datatable_pg_role_name(W_ID, DT, "writer"); + assert!(sql(&plan).contains(&format!( + "ALTER DEFAULT PRIVILEGES FOR ROLE \"{pg_role}\" IN SCHEMA \"analytics\" GRANT SELECT ON TABLES TO \"wm_analyst\";" + ).as_str())); + } + /// A role-scoped REVOKE cannot take back what PUBLIC holds, so the plan has /// to say so instead of pretending the statement closed it. #[test] @@ -1239,7 +1378,11 @@ mod tests { let old_pg = datatable_pg_role_name(W_ID, DT, "analyst"); let req = SetDatatablePermissions { enabled: true, - roles: vec![role("admin", &[]), role("reader", &[]), role("analyst", &[])], + roles: vec![ + role("admin", &[]), + role("reader", &[]), + role("analyst", &[]), + ], default_role: None, renames: vec![DatatableRoleRename { from: "analyst".to_string(), diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 1b8b3ab373..3aee369878 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -34197,6 +34197,9 @@ components: kind: type: string description: TABLE, SEQUENCE, ... — the keyword a REVOKE on it takes + args: + type: string + description: identity arguments of a routine, which is what tells two of the same name apart AclGrant: type: object diff --git a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte index bd16a71b67..09d489c7de 100644 --- a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte +++ b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte @@ -183,7 +183,7 @@ {#each grantRows as grant (grant.grantee + grant.objects - .map((o) => o.name) + .map((o) => `${o.name}(${o.args ?? ''})`) .join() + (grant.future ?? ''))} {grant.grantee} @@ -192,9 +192,8 @@ {#if info.roles.includes(grant.grantee)}