diff --git a/app/src/comparer/core.rs b/app/src/comparer/core.rs index d227d3b..00dc729 100644 --- a/app/src/comparer/core.rs +++ b/app/src/comparer/core.rs @@ -3224,11 +3224,11 @@ 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 owners: Vec<&str> = [from_owner, schema.owner.as_str()] + let to_owners: Vec<&str> = [schema.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3238,7 +3238,7 @@ impl Comparer { full, "SCHEMA", &schema.name, - &owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3308,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())) @@ -3317,7 +3317,7 @@ impl Comparer { } else { (&[], "") }; - let owners: Vec<&str> = [from_owner, table.owner.as_str()] + let to_owners: Vec<&str> = [table.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3328,7 +3328,7 @@ impl Comparer { full, "TABLE", &object_name, - &owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3351,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())) @@ -3360,7 +3360,7 @@ impl Comparer { } else { (&[], "") }; - let owners: Vec<&str> = [from_owner, seq.owner.as_str()] + let to_owners: Vec<&str> = [seq.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3371,7 +3371,7 @@ impl Comparer { full, "SEQUENCE", &object_name, - &owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3409,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 @@ -3421,7 +3421,7 @@ impl Comparer { } else { (&[], "") }; - let owners: Vec<&str> = [from_owner, view.owner.as_str()] + let to_owners: Vec<&str> = [view.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3432,7 +3432,7 @@ impl Comparer { full, "TABLE", &object_name, - &owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3457,11 +3457,11 @@ 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 owners: Vec<&str> = [from_owner, ft.owner.as_str()] + let to_owners: Vec<&str> = [ft.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3472,7 +3472,7 @@ impl Comparer { full, "FOREIGN TABLE", &object_name, - &owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3486,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(), @@ -3494,7 +3494,7 @@ impl Comparer { )) .copied() .unwrap_or((&[], "")); - let owners: Vec<&str> = [from_owner, routine.owner.as_str()] + let to_owners: Vec<&str> = [routine.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3509,7 +3509,7 @@ impl Comparer { full, object_kind, &object_name, - &owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3535,11 +3535,11 @@ 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 owners: Vec<&str> = [from_owner, pg_type.owner.as_str()] + let to_owners: Vec<&str> = [pg_type.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3550,7 +3550,7 @@ impl Comparer { full, "TYPE", &object_name, - &owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3574,11 +3574,7 @@ 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 table_owners: Vec<&str> = [from_owner, table.owner.as_str()] + let to_table_owners: Vec<&str> = [table.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3610,7 +3606,7 @@ impl Comparer { full, &object_table_name, &col.name, - &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..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,147 @@ 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}" + ); +} + +#[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}" ); } @@ -6050,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()); @@ -6098,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 3d5ad1f..58b28f7 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", @@ -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,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 `owners` are skipped entirely — PostgreSQL object owners have -/// implicit full privileges, so GRANT/REVOKE targeting them is meaningless. +/// 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, - 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, to_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 +281,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 +345,17 @@ 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. +/// `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], full: bool, object_kind: &str, object_name: &str, - 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, to_owners); let mut script = String::new(); for entry in &diffs { @@ -423,9 +424,9 @@ pub fn generate_column_grants_script( full: bool, table_name: &str, column_name: &str, - 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", to_owners); let mut script = String::new(); for entry in &diffs { @@ -630,22 +631,38 @@ 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(), "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:?}" ); } + #[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_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![ @@ -666,7 +683,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, @@ -674,19 +691,15 @@ 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}" ); } }