From 20f9c8bf65eb0274145636a60149f7438ad22b9e Mon Sep 17 00:00:00 2001 From: iBarBuba <350579+iBarBuba@users.noreply.github.com> Date: Sat, 25 Apr 2026 10:51:52 +0200 Subject: [PATCH 1/3] Fix grants for former owners after owner change --- app/src/comparer/core.rs | 56 +++++++++++++++++------- app/src/comparer/core_tests.rs | 78 ++++++++++++++++++++++++++++++++++ app/src/dump/acl.rs | 71 ++++++++++++++++++++----------- 3 files changed, 164 insertions(+), 41 deletions(-) diff --git a/app/src/comparer/core.rs b/app/src/comparer/core.rs index d227d3b..586fecf 100644 --- a/app/src/comparer/core.rs +++ b/app/src/comparer/core.rs @@ -3228,7 +3228,9 @@ impl Comparer { .get(schema.name.as_str()) .copied() .unwrap_or((&[], "")); - let owners: Vec<&str> = [from_owner, schema.owner.as_str()] + let from_owners: Vec<&str> = + [from_owner].into_iter().filter(|o| !o.is_empty()).collect(); + let to_owners: Vec<&str> = [schema.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3238,7 +3240,8 @@ impl Comparer { full, "SCHEMA", &schema.name, - &owners, + &from_owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3317,7 +3320,9 @@ impl Comparer { } else { (&[], "") }; - let owners: Vec<&str> = [from_owner, table.owner.as_str()] + let from_owners: Vec<&str> = + [from_owner].into_iter().filter(|o| !o.is_empty()).collect(); + let to_owners: Vec<&str> = [table.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3328,7 +3333,8 @@ impl Comparer { full, "TABLE", &object_name, - &owners, + &from_owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3360,7 +3366,9 @@ impl Comparer { } else { (&[], "") }; - let owners: Vec<&str> = [from_owner, seq.owner.as_str()] + let from_owners: Vec<&str> = + [from_owner].into_iter().filter(|o| !o.is_empty()).collect(); + let to_owners: Vec<&str> = [seq.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3371,7 +3379,8 @@ impl Comparer { full, "SEQUENCE", &object_name, - &owners, + &from_owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3421,7 +3430,9 @@ impl Comparer { } else { (&[], "") }; - let owners: Vec<&str> = [from_owner, view.owner.as_str()] + let from_owners: Vec<&str> = + [from_owner].into_iter().filter(|o| !o.is_empty()).collect(); + let to_owners: Vec<&str> = [view.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3432,7 +3443,8 @@ impl Comparer { full, "TABLE", &object_name, - &owners, + &from_owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3461,7 +3473,9 @@ impl Comparer { .get(&(ft.schema.as_str(), ft.name.as_str())) .copied() .unwrap_or((&[], "")); - let owners: Vec<&str> = [from_owner, ft.owner.as_str()] + let from_owners: Vec<&str> = + [from_owner].into_iter().filter(|o| !o.is_empty()).collect(); + let to_owners: Vec<&str> = [ft.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3472,7 +3486,8 @@ impl Comparer { full, "FOREIGN TABLE", &object_name, - &owners, + &from_owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3494,7 +3509,9 @@ impl Comparer { )) .copied() .unwrap_or((&[], "")); - let owners: Vec<&str> = [from_owner, routine.owner.as_str()] + let from_owners: Vec<&str> = + [from_owner].into_iter().filter(|o| !o.is_empty()).collect(); + let to_owners: Vec<&str> = [routine.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3509,7 +3526,8 @@ impl Comparer { full, object_kind, &object_name, - &owners, + &from_owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3539,7 +3557,9 @@ impl Comparer { .get(&(pg_type.schema.as_str(), pg_type.typname.as_str())) .copied() .unwrap_or((&[], "")); - let owners: Vec<&str> = [from_owner, pg_type.owner.as_str()] + let from_owners: Vec<&str> = + [from_owner].into_iter().filter(|o| !o.is_empty()).collect(); + let to_owners: Vec<&str> = [pg_type.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3550,7 +3570,8 @@ impl Comparer { full, "TYPE", &object_name, - &owners, + &from_owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3578,7 +3599,9 @@ impl Comparer { .get(&(table.schema.as_str(), table.name.as_str())) .map(|&(_, o)| o) .unwrap_or(""); - let table_owners: Vec<&str> = [from_owner, table.owner.as_str()] + let from_table_owners: Vec<&str> = + [from_owner].into_iter().filter(|o| !o.is_empty()).collect(); + let to_table_owners: Vec<&str> = [table.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3610,7 +3633,8 @@ impl Comparer { full, &object_table_name, &col.name, - &table_owners, + &from_table_owners, + &to_table_owners, ); if !col_grants.is_empty() { if self.use_comments { diff --git a/app/src/comparer/core_tests.rs b/app/src/comparer/core_tests.rs index df0fa99..d6bce70 100644 --- a/app/src/comparer/core_tests.rs +++ b/app/src/comparer/core_tests.rs @@ -4079,6 +4079,84 @@ async fn compare_grants_excludes_owner_acl_entries() { ); } +#[tokio::test] +async fn compare_grants_emits_explicit_grants_to_former_owner() { + let mut from_dump = Dump::new(DumpConfig::default()); + let mut to_dump = Dump::new(DumpConfig::default()); + + let mut from_schema = Schema::new("billing".to_string(), "billing".to_string(), None); + from_schema.owner = "old_owner".to_string(); + + let mut to_schema = Schema::new("billing".to_string(), "billing".to_string(), None); + to_schema.owner = "new_owner".to_string(); + to_schema.acl = vec![ + "old_owner=UC/new_owner".to_string(), + "app_user=U/new_owner".to_string(), + ]; + + let from_table = Table::new( + "billing".to_string(), + "invoice".to_string(), + "billing".to_string(), + "invoice".to_string(), + "old_owner".to_string(), + None, + Vec::new(), + Vec::new(), + Vec::new(), + Vec::new(), + None, + ); + + let mut to_table = Table::new( + "billing".to_string(), + "invoice".to_string(), + "billing".to_string(), + "invoice".to_string(), + "new_owner".to_string(), + None, + Vec::new(), + Vec::new(), + Vec::new(), + Vec::new(), + None, + ); + to_table.acl = vec![ + "old_owner=ar/new_owner".to_string(), + "app_user=r/new_owner".to_string(), + ]; + + from_dump.schemas.push(from_schema); + from_dump.tables.push(from_table); + to_dump.schemas.push(to_schema); + to_dump.tables.push(to_table); + + let mut comparer = Comparer::new(from_dump, to_dump, false, false, true, GrantsMode::AddOnly); + comparer.compare_grants().await.unwrap(); + let script = comparer.get_script(); + + assert!( + script.contains("GRANT CREATE, USAGE ON SCHEMA billing TO old_owner;"), + "Former schema owner must receive explicit TO grant, got: {script}" + ); + assert!( + script.contains("GRANT USAGE ON SCHEMA billing TO app_user;"), + "Non-owner schema grant must still be emitted, got: {script}" + ); + assert!( + script.contains("GRANT INSERT, SELECT ON TABLE billing.invoice TO old_owner;"), + "Former table owner must receive explicit TO grant, got: {script}" + ); + assert!( + script.contains("GRANT SELECT ON TABLE billing.invoice TO app_user;"), + "Non-owner table grant must still be emitted, got: {script}" + ); + assert!( + !script.contains("TO new_owner"), + "Current owner must not receive explicit grants, got: {script}" + ); +} + #[tokio::test] async fn compare_grants_owner_excluded_nonowner_still_diffed() { let mut from_dump = Dump::new(DumpConfig::default()); diff --git a/app/src/dump/acl.rs b/app/src/dump/acl.rs index 3d5ad1f..f279fc7 100644 --- a/app/src/dump/acl.rs +++ b/app/src/dump/acl.rs @@ -228,11 +228,15 @@ pub struct AclDiffEntry { fn build_privilege_map( acl: &[String], object_kind: &str, + owners: &[&str], ) -> std::collections::HashMap> { use std::collections::HashMap; let mut map: HashMap> = HashMap::new(); for item in acl { if let Some(entry) = AclEntry::parse(item) { + if owners.contains(&entry.grantee.as_str()) { + continue; + } let privs = AclEntry::parse_privileges(&entry.privileges, object_kind); let grantee_map = map.entry(entry.grantee).or_default(); for p in privs { @@ -252,19 +256,21 @@ fn build_privilege_map( /// **ignoring the grantor** field entirely. Returns one [`AclDiffEntry`] per grantee /// that has at least one action. Revoke entries are only produced when `full` is `true`. /// -/// Grantees listed in `owners` are skipped entirely — PostgreSQL object owners have -/// implicit full privileges, so GRANT/REVOKE targeting them is meaningless. +/// Grantees listed in `from_owners` are skipped only in the FROM ACL; grantees +/// listed in `to_owners` are skipped only in the TO ACL. PostgreSQL object owners +/// have implicit full privileges only on the side where they own the object. pub fn diff_acls( from_acl: &[String], to_acl: &[String], full: bool, object_kind: &str, - owners: &[&str], + from_owners: &[&str], + to_owners: &[&str], ) -> Vec { use std::collections::BTreeSet; - let from_map = build_privilege_map(from_acl, object_kind); - let to_map = build_privilege_map(to_acl, object_kind); + let from_map = build_privilege_map(from_acl, object_kind, from_owners); + let to_map = build_privilege_map(to_acl, object_kind, to_owners); let empty_privs: std::collections::HashMap = std::collections::HashMap::new(); @@ -276,11 +282,6 @@ pub fn diff_acls( all_grantees.insert(g.as_str()); } - // Remove object owners — they have implicit full privileges - for owner in owners { - all_grantees.remove(owner); - } - let mut result = Vec::new(); for grantee in &all_grantees { @@ -345,16 +346,18 @@ pub fn diff_acls( /// Generate the combined GRANT/REVOKE script for an object. /// -/// `owners` lists role names that own the object (from/to); their ACL entries are skipped. +/// `from_owners` and `to_owners` list role names that own the object on each +/// side; owner ACL entries are skipped only for the side where they are owners. pub fn generate_grants_script( from_acl: &[String], to_acl: &[String], full: bool, object_kind: &str, object_name: &str, - owners: &[&str], + from_owners: &[&str], + to_owners: &[&str], ) -> String { - let diffs = diff_acls(from_acl, to_acl, full, object_kind, owners); + let diffs = diff_acls(from_acl, to_acl, full, object_kind, from_owners, to_owners); let mut script = String::new(); for entry in &diffs { @@ -410,7 +413,7 @@ pub fn generate_new_object_grants( object_name: &str, owners: &[&str], ) -> String { - generate_grants_script(&[], to_acl, false, object_kind, object_name, owners) + generate_grants_script(&[], to_acl, false, object_kind, object_name, &[], owners) } /// Generate column-level GRANT/REVOKE statements. @@ -423,9 +426,10 @@ pub fn generate_column_grants_script( full: bool, table_name: &str, column_name: &str, - owners: &[&str], + from_owners: &[&str], + to_owners: &[&str], ) -> String { - let diffs = diff_acls(from_acl, to_acl, full, "COLUMN", owners); + let diffs = diff_acls(from_acl, to_acl, full, "COLUMN", from_owners, to_owners); let mut script = String::new(); for entry in &diffs { @@ -547,7 +551,7 @@ mod tests { // FROM: plain SELECT; TO: SELECT with grant option → upgrade, no revoke let from = vec!["user1=r/owner".to_string()]; let to = vec!["user1=r*/owner".to_string()]; - let diffs = diff_acls(&from, &to, true, "TABLE", &[]); + let diffs = diff_acls(&from, &to, true, "TABLE", &[], &[]); assert_eq!(diffs.len(), 1); assert_eq!(diffs[0].grantee, "user1"); assert_eq!(diffs[0].grants_with_option, vec!["SELECT"]); @@ -561,7 +565,7 @@ mod tests { // FROM: SELECT with grant option; TO: plain SELECT → revoke grant option let from = vec!["user1=r*/owner".to_string()]; let to = vec!["user1=r/owner".to_string()]; - let diffs = diff_acls(&from, &to, true, "TABLE", &[]); + let diffs = diff_acls(&from, &to, true, "TABLE", &[], &[]); assert_eq!(diffs.len(), 1); assert_eq!(diffs[0].revoke_option_for, vec!["SELECT"]); assert!(diffs[0].revokes.is_empty()); @@ -571,7 +575,7 @@ mod tests { fn test_diff_acls_addonly() { let from = vec!["user1=r/owner".to_string()]; let to = vec!["user1=r/owner".to_string(), "user2=rw/owner".to_string()]; - let diffs = diff_acls(&from, &to, false, "TABLE", &[]); + let diffs = diff_acls(&from, &to, false, "TABLE", &[], &[]); assert_eq!(diffs.len(), 1); assert_eq!(diffs[0].grantee, "user2"); assert_eq!(diffs[0].grants_plain, vec!["SELECT", "UPDATE"]); @@ -581,7 +585,7 @@ mod tests { fn test_diff_acls_full() { let from = vec!["user1=r/owner".to_string(), "user3=d/owner".to_string()]; let to = vec!["user1=r/owner".to_string(), "user2=rw/owner".to_string()]; - let diffs = diff_acls(&from, &to, true, "TABLE", &[]); + let diffs = diff_acls(&from, &to, true, "TABLE", &[], &[]); assert_eq!(diffs.len(), 2); let user2 = diffs.iter().find(|d| d.grantee == "user2").unwrap(); assert_eq!(user2.grants_plain, vec!["SELECT", "UPDATE"]); @@ -594,7 +598,7 @@ mod tests { // Same grantee and privileges, different grantor → no diff let from = vec!["user1=rw/owner1".to_string()]; let to = vec!["user1=rw/owner2".to_string()]; - let diffs = diff_acls(&from, &to, true, "TABLE", &[]); + let diffs = diff_acls(&from, &to, true, "TABLE", &[], &[]); assert!(diffs.is_empty()); } @@ -603,7 +607,7 @@ mod tests { // Same grantee, privileges split across grantors → merged, no diff let from = vec!["user1=r/owner1".to_string(), "user1=w/owner2".to_string()]; let to = vec!["user1=rw/owner3".to_string()]; - let diffs = diff_acls(&from, &to, true, "TABLE", &[]); + let diffs = diff_acls(&from, &to, true, "TABLE", &[], &[]); assert!(diffs.is_empty()); } @@ -639,13 +643,29 @@ mod tests { "owner_b=arwdDxt/owner_b".to_string(), "reader=r/owner_b".to_string(), ]; - let diffs = diff_acls(&from, &to, true, "TABLE", &["owner_a", "owner_b"]); + let diffs = diff_acls(&from, &to, true, "TABLE", &["owner_a"], &["owner_b"]); assert!( diffs.is_empty(), "Owner grantees must be excluded, got: {diffs:?}" ); } + #[test] + fn test_diff_acls_grants_to_former_owner_when_explicit_in_to() { + let from = vec!["reader=r/owner_a".to_string()]; + let to = vec![ + "owner_a=ar/owner_b".to_string(), + "reader=r/owner_b".to_string(), + ]; + let diffs = diff_acls(&from, &to, true, "TABLE", &["owner_a"], &["owner_b"]); + assert_eq!(diffs.len(), 1); + assert_eq!(diffs[0].grantee, "owner_a"); + assert_eq!(diffs[0].grants_plain, vec!["INSERT", "SELECT"]); + assert!(diffs[0].grants_with_option.is_empty()); + assert!(diffs[0].revoke_option_for.is_empty()); + assert!(diffs[0].revokes.is_empty()); + } + #[test] fn test_diff_acls_excludes_owner_keeps_others() { let from = vec![ @@ -656,7 +676,7 @@ mod tests { "theowner=arwdDxt/theowner".to_string(), "new_reader=r/theowner".to_string(), ]; - let diffs = diff_acls(&from, &to, true, "TABLE", &["theowner"]); + let diffs = diff_acls(&from, &to, true, "TABLE", &["theowner"], &["theowner"]); assert_eq!(diffs.len(), 2); let added = diffs.iter().find(|d| d.grantee == "new_reader").unwrap(); assert_eq!(added.grants_plain, vec!["SELECT"]); @@ -674,7 +694,8 @@ mod tests { true, "FUNCTION", "public.my_func()", - &["owner_a", "owner_b"], + &["owner_a"], + &["owner_b"], ); assert!( script.contains("GRANT EXECUTE ON FUNCTION public.my_func() TO app;"), From d96ed88da8685c8e34d03e14c7b0abab8e5e07a5 Mon Sep 17 00:00:00 2001 From: iBarBuba <350579+iBarBuba@users.noreply.github.com> Date: Sat, 25 Apr 2026 10:51:52 +0200 Subject: [PATCH 2/3] Address ACL review comments --- app/src/comparer/core_tests.rs | 141 +++++++++++++++++++++++++++++---- app/src/dump/acl.rs | 32 ++++---- 2 files changed, 138 insertions(+), 35 deletions(-) diff --git a/app/src/comparer/core_tests.rs b/app/src/comparer/core_tests.rs index d6bce70..a7fbdc9 100644 --- a/app/src/comparer/core_tests.rs +++ b/app/src/comparer/core_tests.rs @@ -3,6 +3,7 @@ use crate::config::dump_config::DumpConfig; use crate::config::grants_mode::GrantsMode; use crate::dump::default_privilege::DefaultPrivilege; use crate::dump::extension::Extension; +use crate::dump::foreign_table::ForeignTable; use crate::dump::pg_type::{CompositeAttribute, PgType}; use crate::dump::routine::Routine; use crate::dump::schema::Schema; @@ -3227,6 +3228,48 @@ async fn compare_grants_view() { ); } +#[tokio::test] +async fn compare_grants_foreign_table_add_and_revoke() { + let mut from_dump = Dump::new(DumpConfig::default()); + let mut to_dump = Dump::new(DumpConfig::default()); + + let mut from_ft = ForeignTable::new( + "public".to_string(), + "ft_orders".to_string(), + "fdw_server".to_string(), + "owner".to_string(), + Vec::new(), + Vec::new(), + ); + from_ft.acl = vec!["reader=r/owner".to_string()]; + + let mut to_ft = ForeignTable::new( + "public".to_string(), + "ft_orders".to_string(), + "fdw_server".to_string(), + "owner".to_string(), + Vec::new(), + Vec::new(), + ); + to_ft.acl = vec!["writer=rw/owner".to_string()]; + + from_dump.foreign_tables.push(from_ft); + to_dump.foreign_tables.push(to_ft); + + let mut comparer = Comparer::new(from_dump, to_dump, false, false, true, GrantsMode::Full); + comparer.compare_grants().await.unwrap(); + let script = comparer.get_script(); + + assert!( + script.contains("GRANT SELECT, UPDATE ON FOREIGN TABLE public.ft_orders TO writer;"), + "Full must add foreign table grant, got: {script}" + ); + assert!( + script.contains("REVOKE SELECT ON FOREIGN TABLE public.ft_orders FROM reader;"), + "Full must revoke removed foreign table grant, got: {script}" + ); +} + #[tokio::test] async fn compare_grants_dropped_view_restores_all_grants() { let mut from_dump = Dump::new(DumpConfig::default()); @@ -4062,8 +4105,10 @@ async fn compare_grants_excludes_owner_acl_entries() { let script = comparer.get_script(); assert!( - !script.contains("old_owner"), - "Must not REVOKE from old owner, got: {script}" + script.contains( + "REVOKE DELETE, INSERT, REFERENCES, SELECT, TRIGGER, TRUNCATE, UPDATE ON TABLE public.data FROM old_owner;" + ), + "Full mode must revoke former-owner explicit table privileges, got: {script}" ); assert!( !script.contains("new_owner"), @@ -4073,9 +4118,69 @@ async fn compare_grants_excludes_owner_acl_entries() { !script.contains("GRANT"), "No grants expected (reader unchanged), got: {script}" ); +} + +#[tokio::test] +async fn compare_grants_full_revokes_explicit_grants_from_former_owner() { + let mut from_dump = Dump::new(DumpConfig::default()); + let mut to_dump = Dump::new(DumpConfig::default()); + + let mut from_schema = Schema::new("billing".to_string(), "billing".to_string(), None); + from_schema.owner = "old_owner".to_string(); + from_schema.acl = vec!["old_owner=UC/old_owner".to_string()]; + + let mut to_schema = Schema::new("billing".to_string(), "billing".to_string(), None); + to_schema.owner = "new_owner".to_string(); + + let mut from_table = Table::new( + "billing".to_string(), + "invoice".to_string(), + "billing".to_string(), + "invoice".to_string(), + "old_owner".to_string(), + None, + Vec::new(), + Vec::new(), + Vec::new(), + Vec::new(), + None, + ); + from_table.acl = vec!["old_owner=ar/old_owner".to_string()]; + + let to_table = Table::new( + "billing".to_string(), + "invoice".to_string(), + "billing".to_string(), + "invoice".to_string(), + "new_owner".to_string(), + None, + Vec::new(), + Vec::new(), + Vec::new(), + Vec::new(), + None, + ); + + from_dump.schemas.push(from_schema); + from_dump.tables.push(from_table); + to_dump.schemas.push(to_schema); + to_dump.tables.push(to_table); + + let mut comparer = Comparer::new(from_dump, to_dump, false, false, true, GrantsMode::Full); + comparer.compare_grants().await.unwrap(); + let script = comparer.get_script(); + assert!( - !script.contains("REVOKE"), - "No revokes expected (reader unchanged), got: {script}" + script.contains("REVOKE CREATE, USAGE ON SCHEMA billing FROM old_owner;"), + "Former schema owner grant must be revoked in full mode, got: {script}" + ); + assert!( + script.contains("REVOKE INSERT, SELECT ON TABLE billing.invoice FROM old_owner;"), + "Former table owner grant must be revoked in full mode, got: {script}" + ); + assert!( + !script.contains("new_owner"), + "Current owner must not receive explicit grant/revoke output, got: {script}" ); } @@ -6128,10 +6233,10 @@ async fn compare_grants_dropped_view_grants_extra_over_default() { } /// When table ownership changes between FROM and TO, column-level ACL -/// diffing must exclude both old and new owners from the diff so that -/// implicit-privilege entries do not produce spurious GRANT/REVOKE. +/// diffing must keep former-owner entries diffable while suppressing +/// current-owner implicit privilege entries. #[tokio::test] -async fn compare_column_grants_excludes_both_old_and_new_owner() { +async fn compare_column_grants_revokes_former_owner_and_excludes_current_owner() { let mut from_dump = Dump::new(DumpConfig::default()); let mut to_dump = Dump::new(DumpConfig::default()); @@ -6176,17 +6281,19 @@ async fn compare_column_grants_excludes_both_old_and_new_owner() { comparer.compare_grants().await.unwrap(); let script = comparer.get_script(); - // Both old_owner and new_owner have implicit privileges as owners, - // so no GRANT or REVOKE should appear for them on the column. - let has_col_grant = script - .lines() - .any(|l| l.contains("secret") && l.trim_start().to_lowercase().starts_with("grant ")); - let has_col_revoke = script - .lines() - .any(|l| l.contains("secret") && l.trim_start().to_lowercase().starts_with("revoke ")); assert!( - !has_col_grant && !has_col_revoke, - "Owner ACL entries must be excluded for both old and new owner, got: {script}" + script.contains("REVOKE SELECT (secret) ON TABLE public.users FROM old_owner;"), + "Former owner column ACL must remain diffable in full mode, got: {script}" + ); + assert!( + !script.contains("new_owner"), + "Current owner column ACL entries must be suppressed, got: {script}" + ); + assert!( + !script + .lines() + .any(|l| l.contains("secret") && l.trim_start().to_lowercase().starts_with("grant ")), + "Unexpected column GRANT for owner ACL entries, got: {script}" ); } diff --git a/app/src/dump/acl.rs b/app/src/dump/acl.rs index f279fc7..281f382 100644 --- a/app/src/dump/acl.rs +++ b/app/src/dump/acl.rs @@ -66,7 +66,7 @@ impl AclEntry { /// Valid privilege names for a given object kind. fn valid_privileges(object_kind: &str) -> &'static [&'static str] { match object_kind { - "TABLE" => &[ + "TABLE" | "FOREIGN TABLE" => &[ "SELECT", "INSERT", "UPDATE", @@ -256,20 +256,20 @@ fn build_privilege_map( /// **ignoring the grantor** field entirely. Returns one [`AclDiffEntry`] per grantee /// that has at least one action. Revoke entries are only produced when `full` is `true`. /// -/// Grantees listed in `from_owners` are skipped only in the FROM ACL; grantees -/// listed in `to_owners` are skipped only in the TO ACL. PostgreSQL object owners -/// have implicit full privileges only on the side where they own the object. +/// Grantees listed in `to_owners` are skipped in both ACLs. Current owners have +/// implicit privileges after the migration, while former owners must remain in +/// the diff so full mode can revoke or preserve their explicit grants. pub fn diff_acls( from_acl: &[String], to_acl: &[String], full: bool, object_kind: &str, - from_owners: &[&str], + _from_owners: &[&str], to_owners: &[&str], ) -> Vec { use std::collections::BTreeSet; - let from_map = build_privilege_map(from_acl, object_kind, from_owners); + let from_map = build_privilege_map(from_acl, object_kind, to_owners); let to_map = build_privilege_map(to_acl, object_kind, to_owners); let empty_privs: std::collections::HashMap = std::collections::HashMap::new(); @@ -346,8 +346,8 @@ pub fn diff_acls( /// Generate the combined GRANT/REVOKE script for an object. /// -/// `from_owners` and `to_owners` list role names that own the object on each -/// side; owner ACL entries are skipped only for the side where they are owners. +/// `to_owners` lists role names that own the object after the migration; those +/// owners are skipped. Former owners stay in the diff as normal grantees. pub fn generate_grants_script( from_acl: &[String], to_acl: &[String], @@ -634,10 +634,10 @@ mod tests { #[test] fn test_diff_acls_excludes_owners() { - // Owner in FROM has explicit entry, absent in TO → should NOT produce a REVOKE + // Current owner entries are skipped as implicit owner privileges. let from = vec![ - "owner_a=arwdDxt/owner_a".to_string(), - "reader=r/owner_a".to_string(), + "owner_b=arwdDxt/owner_b".to_string(), + "reader=r/owner_b".to_string(), ]; let to = vec![ "owner_b=arwdDxt/owner_b".to_string(), @@ -686,7 +686,7 @@ mod tests { #[test] fn test_generate_grants_script_excludes_owner() { - let from = vec!["owner_a=X/owner_a".to_string()]; + let from = vec!["owner_b=X/owner_b".to_string()]; let to = vec!["owner_b=X/owner_b".to_string(), "app=X/owner_b".to_string()]; let script = generate_grants_script( &from, @@ -694,20 +694,16 @@ mod tests { true, "FUNCTION", "public.my_func()", - &["owner_a"], + &["owner_b"], &["owner_b"], ); assert!( script.contains("GRANT EXECUTE ON FUNCTION public.my_func() TO app;"), "Must grant to non-owner, got: {script}" ); - assert!( - !script.contains("owner_a"), - "Must not reference old owner, got: {script}" - ); assert!( !script.contains("owner_b"), - "Must not reference new owner, got: {script}" + "Must not reference owner, got: {script}" ); } } From f6564af435b8610cef16d1ee29a65e6ccf478f6e Mon Sep 17 00:00:00 2001 From: iBarBuba <350579+iBarBuba@users.noreply.github.com> Date: Sat, 25 Apr 2026 11:02:24 +0200 Subject: [PATCH 3/3] Simplify ACL owner filtering API --- app/src/comparer/core.rs | 42 +++++++--------------------------------- app/src/dump/acl.rs | 28 ++++++++++++--------------- 2 files changed, 19 insertions(+), 51 deletions(-) diff --git a/app/src/comparer/core.rs b/app/src/comparer/core.rs index 586fecf..00dc729 100644 --- a/app/src/comparer/core.rs +++ b/app/src/comparer/core.rs @@ -3224,12 +3224,10 @@ impl Comparer { // --- Schemas --- for schema in &self.to.schemas { - let (from_acl, from_owner) = from_schema_map + let (from_acl, _) = from_schema_map .get(schema.name.as_str()) .copied() .unwrap_or((&[], "")); - let from_owners: Vec<&str> = - [from_owner].into_iter().filter(|o| !o.is_empty()).collect(); let to_owners: Vec<&str> = [schema.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) @@ -3240,7 +3238,6 @@ impl Comparer { full, "SCHEMA", &schema.name, - &from_owners, &to_owners, ); if !grants_script.is_empty() { @@ -3311,7 +3308,7 @@ impl Comparer { } else { Vec::new() }; - let (from_acl, from_owner): (&[String], &str) = if use_default { + let (from_acl, _): (&[String], &str) = if use_default { (default_acl_storage.as_slice(), "") } else if let Some(&(acl, owner)) = from_table_map.get(&(table.schema.as_str(), table.name.as_str())) @@ -3320,8 +3317,6 @@ impl Comparer { } else { (&[], "") }; - let from_owners: Vec<&str> = - [from_owner].into_iter().filter(|o| !o.is_empty()).collect(); let to_owners: Vec<&str> = [table.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) @@ -3333,7 +3328,6 @@ impl Comparer { full, "TABLE", &object_name, - &from_owners, &to_owners, ); if !grants_script.is_empty() { @@ -3357,7 +3351,7 @@ impl Comparer { } else { Vec::new() }; - let (from_acl, from_owner): (&[String], &str) = if is_new_seq && full { + let (from_acl, _): (&[String], &str) = if is_new_seq && full { (default_seq_acl_storage.as_slice(), "") } else if let Some(&(acl, owner)) = from_seq_map.get(&(seq.schema.as_str(), seq.name.as_str())) @@ -3366,8 +3360,6 @@ impl Comparer { } else { (&[], "") }; - let from_owners: Vec<&str> = - [from_owner].into_iter().filter(|o| !o.is_empty()).collect(); let to_owners: Vec<&str> = [seq.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) @@ -3379,7 +3371,6 @@ impl Comparer { full, "SEQUENCE", &object_name, - &from_owners, &to_owners, ); if !grants_script.is_empty() { @@ -3418,7 +3409,7 @@ impl Comparer { } else { Vec::new() }; - let (from_acl, from_owner): (&[String], &str) = if use_view_default { + let (from_acl, _): (&[String], &str) = if use_view_default { (view_default_acl_storage.as_slice(), "") } else if is_dropped { // use_drop=false: view wasn't actually dropped @@ -3430,8 +3421,6 @@ impl Comparer { } else { (&[], "") }; - let from_owners: Vec<&str> = - [from_owner].into_iter().filter(|o| !o.is_empty()).collect(); let to_owners: Vec<&str> = [view.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) @@ -3443,7 +3432,6 @@ impl Comparer { full, "TABLE", &object_name, - &from_owners, &to_owners, ); if !grants_script.is_empty() { @@ -3469,12 +3457,10 @@ impl Comparer { .collect(); for ft in &self.to.foreign_tables { - let (from_acl, from_owner) = from_ft_map + let (from_acl, _) = from_ft_map .get(&(ft.schema.as_str(), ft.name.as_str())) .copied() .unwrap_or((&[], "")); - let from_owners: Vec<&str> = - [from_owner].into_iter().filter(|o| !o.is_empty()).collect(); let to_owners: Vec<&str> = [ft.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) @@ -3486,7 +3472,6 @@ impl Comparer { full, "FOREIGN TABLE", &object_name, - &from_owners, &to_owners, ); if !grants_script.is_empty() { @@ -3501,7 +3486,7 @@ impl Comparer { // --- Routines --- for routine in &self.to.routines { - let (from_acl, from_owner) = from_routine_map + let (from_acl, _) = from_routine_map .get(&( routine.schema.as_str(), routine.name.as_str(), @@ -3509,8 +3494,6 @@ impl Comparer { )) .copied() .unwrap_or((&[], "")); - let from_owners: Vec<&str> = - [from_owner].into_iter().filter(|o| !o.is_empty()).collect(); let to_owners: Vec<&str> = [routine.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) @@ -3526,7 +3509,6 @@ impl Comparer { full, object_kind, &object_name, - &from_owners, &to_owners, ); if !grants_script.is_empty() { @@ -3553,12 +3535,10 @@ impl Comparer { .collect(); for pg_type in &self.to.types { - let (from_acl, from_owner) = from_type_map + let (from_acl, _) = from_type_map .get(&(pg_type.schema.as_str(), pg_type.typname.as_str())) .copied() .unwrap_or((&[], "")); - let from_owners: Vec<&str> = - [from_owner].into_iter().filter(|o| !o.is_empty()).collect(); let to_owners: Vec<&str> = [pg_type.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) @@ -3570,7 +3550,6 @@ impl Comparer { full, "TYPE", &object_name, - &from_owners, &to_owners, ); if !grants_script.is_empty() { @@ -3595,12 +3574,6 @@ impl Comparer { .collect(); for table in &self.to.tables { - let from_owner = from_table_map - .get(&(table.schema.as_str(), table.name.as_str())) - .map(|&(_, o)| o) - .unwrap_or(""); - let from_table_owners: Vec<&str> = - [from_owner].into_iter().filter(|o| !o.is_empty()).collect(); let to_table_owners: Vec<&str> = [table.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) @@ -3633,7 +3606,6 @@ impl Comparer { full, &object_table_name, &col.name, - &from_table_owners, &to_table_owners, ); if !col_grants.is_empty() { diff --git a/app/src/dump/acl.rs b/app/src/dump/acl.rs index 281f382..58b28f7 100644 --- a/app/src/dump/acl.rs +++ b/app/src/dump/acl.rs @@ -264,7 +264,6 @@ pub fn diff_acls( to_acl: &[String], full: bool, object_kind: &str, - _from_owners: &[&str], to_owners: &[&str], ) -> Vec { use std::collections::BTreeSet; @@ -354,10 +353,9 @@ pub fn generate_grants_script( full: bool, object_kind: &str, object_name: &str, - from_owners: &[&str], to_owners: &[&str], ) -> String { - let diffs = diff_acls(from_acl, to_acl, full, object_kind, from_owners, to_owners); + let diffs = diff_acls(from_acl, to_acl, full, object_kind, to_owners); let mut script = String::new(); for entry in &diffs { @@ -413,7 +411,7 @@ pub fn generate_new_object_grants( object_name: &str, owners: &[&str], ) -> String { - generate_grants_script(&[], to_acl, false, object_kind, object_name, &[], owners) + generate_grants_script(&[], to_acl, false, object_kind, object_name, owners) } /// Generate column-level GRANT/REVOKE statements. @@ -426,10 +424,9 @@ pub fn generate_column_grants_script( full: bool, table_name: &str, column_name: &str, - from_owners: &[&str], to_owners: &[&str], ) -> String { - let diffs = diff_acls(from_acl, to_acl, full, "COLUMN", from_owners, to_owners); + let diffs = diff_acls(from_acl, to_acl, full, "COLUMN", to_owners); let mut script = String::new(); for entry in &diffs { @@ -551,7 +548,7 @@ mod tests { // FROM: plain SELECT; TO: SELECT with grant option → upgrade, no revoke let from = vec!["user1=r/owner".to_string()]; let to = vec!["user1=r*/owner".to_string()]; - let diffs = diff_acls(&from, &to, true, "TABLE", &[], &[]); + let diffs = diff_acls(&from, &to, true, "TABLE", &[]); assert_eq!(diffs.len(), 1); assert_eq!(diffs[0].grantee, "user1"); assert_eq!(diffs[0].grants_with_option, vec!["SELECT"]); @@ -565,7 +562,7 @@ mod tests { // FROM: SELECT with grant option; TO: plain SELECT → revoke grant option let from = vec!["user1=r*/owner".to_string()]; let to = vec!["user1=r/owner".to_string()]; - let diffs = diff_acls(&from, &to, true, "TABLE", &[], &[]); + let diffs = diff_acls(&from, &to, true, "TABLE", &[]); assert_eq!(diffs.len(), 1); assert_eq!(diffs[0].revoke_option_for, vec!["SELECT"]); assert!(diffs[0].revokes.is_empty()); @@ -575,7 +572,7 @@ mod tests { fn test_diff_acls_addonly() { let from = vec!["user1=r/owner".to_string()]; let to = vec!["user1=r/owner".to_string(), "user2=rw/owner".to_string()]; - let diffs = diff_acls(&from, &to, false, "TABLE", &[], &[]); + let diffs = diff_acls(&from, &to, false, "TABLE", &[]); assert_eq!(diffs.len(), 1); assert_eq!(diffs[0].grantee, "user2"); assert_eq!(diffs[0].grants_plain, vec!["SELECT", "UPDATE"]); @@ -585,7 +582,7 @@ mod tests { fn test_diff_acls_full() { let from = vec!["user1=r/owner".to_string(), "user3=d/owner".to_string()]; let to = vec!["user1=r/owner".to_string(), "user2=rw/owner".to_string()]; - let diffs = diff_acls(&from, &to, true, "TABLE", &[], &[]); + let diffs = diff_acls(&from, &to, true, "TABLE", &[]); assert_eq!(diffs.len(), 2); let user2 = diffs.iter().find(|d| d.grantee == "user2").unwrap(); assert_eq!(user2.grants_plain, vec!["SELECT", "UPDATE"]); @@ -598,7 +595,7 @@ mod tests { // Same grantee and privileges, different grantor → no diff let from = vec!["user1=rw/owner1".to_string()]; let to = vec!["user1=rw/owner2".to_string()]; - let diffs = diff_acls(&from, &to, true, "TABLE", &[], &[]); + let diffs = diff_acls(&from, &to, true, "TABLE", &[]); assert!(diffs.is_empty()); } @@ -607,7 +604,7 @@ mod tests { // Same grantee, privileges split across grantors → merged, no diff let from = vec!["user1=r/owner1".to_string(), "user1=w/owner2".to_string()]; let to = vec!["user1=rw/owner3".to_string()]; - let diffs = diff_acls(&from, &to, true, "TABLE", &[], &[]); + let diffs = diff_acls(&from, &to, true, "TABLE", &[]); assert!(diffs.is_empty()); } @@ -643,7 +640,7 @@ mod tests { "owner_b=arwdDxt/owner_b".to_string(), "reader=r/owner_b".to_string(), ]; - let diffs = diff_acls(&from, &to, true, "TABLE", &["owner_a"], &["owner_b"]); + let diffs = diff_acls(&from, &to, true, "TABLE", &["owner_b"]); assert!( diffs.is_empty(), "Owner grantees must be excluded, got: {diffs:?}" @@ -657,7 +654,7 @@ mod tests { "owner_a=ar/owner_b".to_string(), "reader=r/owner_b".to_string(), ]; - let diffs = diff_acls(&from, &to, true, "TABLE", &["owner_a"], &["owner_b"]); + let diffs = diff_acls(&from, &to, true, "TABLE", &["owner_b"]); assert_eq!(diffs.len(), 1); assert_eq!(diffs[0].grantee, "owner_a"); assert_eq!(diffs[0].grants_plain, vec!["INSERT", "SELECT"]); @@ -676,7 +673,7 @@ mod tests { "theowner=arwdDxt/theowner".to_string(), "new_reader=r/theowner".to_string(), ]; - let diffs = diff_acls(&from, &to, true, "TABLE", &["theowner"], &["theowner"]); + let diffs = diff_acls(&from, &to, true, "TABLE", &["theowner"]); assert_eq!(diffs.len(), 2); let added = diffs.iter().find(|d| d.grantee == "new_reader").unwrap(); assert_eq!(added.grants_plain, vec!["SELECT"]); @@ -695,7 +692,6 @@ mod tests { "FUNCTION", "public.my_func()", &["owner_b"], - &["owner_b"], ); assert!( script.contains("GRANT EXECUTE ON FUNCTION public.my_func() TO app;"),