diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index bb84188..269640c 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -47,9 +47,157 @@ jobs: cd ${{ github.workspace }}/app ; cargo test --verbose - docker: + integration: needs: build runs-on: ubuntu-latest + + # Round-trip test against every supported PostgreSQL major version. + # For each PG version the job: + # 1. Spins up a fresh postgres container (with wal_level=logical so + # the publication tests in the schemas don't fail at CREATE). + # 2. Creates two databases `a` and `b`. + # 3. Applies `data/test/schema_a.sql` to `a` and `schema_b.sql` to `b`. + # Uses ON_ERROR_STOP=0 because some statements use features specific + # to newer PG versions (NULLS NOT DISTINCT on PG15+, NOT ENFORCED on + # PG18+); they fail to apply on older versions but the round-trip + # property still holds because pgc emits SQL based on dumps, not on + # the source schema files. + # 4. Runs pgc compare to produce `output.sql`. + # 5. Applies `output.sql` to `a`. + # 6. Re-runs pgc compare. The new diff must be empty — that's the + # idempotency contract pgc must satisfy. + strategy: + fail-fast: false + matrix: + pg_version: ["14", "15", "16", "17", "18"] + + steps: + - uses: actions/checkout@v4 + + - name: Install Rust + uses: dtolnay/rust-toolchain@stable + with: + toolchain: "1.95.0" + + - name: Install PostgreSQL client + run: | + sudo apt-get update + sudo apt-get install -y postgresql-client + + - name: Start PostgreSQL ${{ matrix.pg_version }} + run: | + # `services:` does not let us pass `command:` to the container, and + # `wal_level=logical` (required by the publication test cases) needs + # to be set at server start, so we run docker manually. + docker run -d \ + --name pg \ + -e POSTGRES_PASSWORD=postgres \ + -e POSTGRES_USER=postgres \ + -p 5432:5432 \ + postgres:${{ matrix.pg_version }} \ + -c wal_level=logical + # Wait for the server to accept connections. + for i in $(seq 1 60); do + if docker exec pg pg_isready -U postgres -h 127.0.0.1 >/dev/null 2>&1; then + echo "PostgreSQL ${{ matrix.pg_version }} ready after ${i}s" + exit 0 + fi + sleep 1 + done + echo "PostgreSQL did not become ready in time" + docker logs pg + exit 1 + + - name: Create test databases + env: + PGPASSWORD: postgres + run: | + psql -h 127.0.0.1 -U postgres -c "CREATE DATABASE a;" + psql -h 127.0.0.1 -U postgres -c "CREATE DATABASE b;" + + - name: Apply schemas + env: + PGPASSWORD: postgres + run: | + psql -h 127.0.0.1 -U postgres -d a -v ON_ERROR_STOP=0 \ + -f data/test/schema_a.sql + psql -h 127.0.0.1 -U postgres -d b -v ON_ERROR_STOP=0 \ + -f data/test/schema_b.sql + + - name: Build pgc (release) + run: | + cd ${{ github.workspace }}/app + cargo build --release + + - name: Generate pgc config + run: | + cat > /tmp/pgc.conf <<'EOF' + FROM_HOST=127.0.0.1 + FROM_PORT=5432 + FROM_USER=postgres + FROM_PASSWORD=postgres + FROM_DATABASE=a + FROM_SCHEME=test_schema|shared_schema|new_reporting_schema|data + FROM_SSL=false + FROM_DUMP=/tmp/a.dump + TO_HOST=127.0.0.1 + TO_PORT=5432 + TO_USER=postgres + TO_PASSWORD=postgres + TO_DATABASE=b + TO_SCHEME=test_schema|shared_schema|new_reporting_schema|data + TO_SSL=false + TO_DUMP=/tmp/b.dump + OUTPUT=/tmp/output.sql + USE_DROP=true + USE_COMMENTS=false + GRANTS_MODE=full + MAX_CONNECTIONS=10 + USE_SINGLE_TRANSACTION=false + EOF + + - name: Run pgc compare (round 1) — generate diff + run: | + ./app/target/release/pgc --config /tmp/pgc.conf + echo "--- output.sql (round 1) size: $(wc -c < /tmp/output.sql) bytes" + + - name: Apply diff to database `a` + env: + PGPASSWORD: postgres + run: | + # ON_ERROR_STOP=0 mirrors the schema-apply step above: a diff that + # references PG-version-specific syntax may not apply on every + # version, but as long as the round-trip closes (round-2 diff is + # empty) the test still validates idempotency. + psql -h 127.0.0.1 -U postgres -d a -v ON_ERROR_STOP=0 \ + -f /tmp/output.sql + + - name: Run pgc compare (round 2) — must be empty + run: | + ./app/target/release/pgc --config /tmp/pgc.conf + size=$(wc -c < /tmp/output.sql) + echo "--- output.sql (round 2) size: ${size} bytes" + # With USE_COMMENTS=false a fully-idempotent diff produces a 0-byte + # file; a non-empty file (even one containing only whitespace) means + # the comparer emitted SQL that wasn't covered by round 1. + if [ "${size}" -gt 0 ] && [ -n "$(tr -d '[:space:]' < /tmp/output.sql)" ]; then + echo "ERROR: round-2 diff is not empty on PG ${{ matrix.pg_version }}" + echo "--- output.sql contents: ---" + cat /tmp/output.sql + exit 1 + fi + echo "OK: pgc is idempotent on PG ${{ matrix.pg_version }}" + + - name: Stop PostgreSQL + if: always() + run: | + docker logs pg | tail -200 || true + docker stop pg || true + docker rm pg || true + + docker: + needs: [build, integration] + runs-on: ubuntu-latest # Only run automatically on master branch, or when manually triggered if: github.ref == 'refs/heads/master' || github.event_name == 'workflow_dispatch' diff --git a/.gitignore b/.gitignore index 821e5ff..0c7c3ab 100644 --- a/.gitignore +++ b/.gitignore @@ -34,4 +34,6 @@ docker-compose.override.yml *.dump .claude/ -CLAUDE.md \ No newline at end of file +CLAUDE.md + +test.sh \ No newline at end of file diff --git a/CHANGELOG b/CHANGELOG index 70f4ff8..2e670bc 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,131 @@ +2026-04-25 v1.0.18 + + Bug fixes: + - Fixed grants for former owners after an owner change + (two-stage fix). The original v1.0.17 behaviour + filtered both old and new owners as implicit + privilege holders, silently dropping explicit grants + to the previous owner from the diff. The first + attempt switched to filtering only the `to` owner — + which exposed a non-idempotent oscillation: PG + materialises an implicit-owner ACL row in + `pg_class.relacl` (e.g. `pgc_owner_from=arwdDxt/ + pgc_owner_from`) and treating it as a regular + grantee made the comparer emit a long REVOKE on + first run, then GRANT statements on every + subsequent run. The fix splits owner filtering into + asymmetric `from_owners` / `to_owners` lists that + model the effect of `ALTER ... OWNER TO`: the FROM + owner's implicit-owner row is stripped from the + FROM side (because `ALTER OWNER` removes it) and + the TO owner's implicit row is stripped from the + TO side (because `ALTER OWNER` recreates it). When + ownership does not change both lists hold the same + single role, collapsing back to the original + behaviour. Column ACLs (`pg_attribute.attacl`) pass + an empty `from_owners` because `ALTER TABLE OWNER` + does not touch column-level grants — explicit + column grants to a former owner persist and must + still be revoked under `full` mode. + - `FOREIGN TABLE` ACL parsing now uses the same + privilege set as `TABLE` (SELECT, INSERT, UPDATE, + DELETE, TRUNCATE, REFERENCES, TRIGGER, MAINTAIN). + Previously, foreign-table privileges were parsed + against the default minimal set, which dropped + legitimate privileges from the diff. Foreign-table + grant emission now uses `GRANT ... ON TABLE` rather + than the invalid `GRANT ... ON FOREIGN TABLE` + syntax (PostgreSQL has no `ON FOREIGN TABLE` form + for GRANT/REVOKE — applying the previous diff + raised `syntax error at or near "TABLE"`). + - Foreign-table grants are now idempotent for newly + created foreign tables. PostgreSQL applies default + table privileges (`pg_default_acl.defaclobjtype = + 'r'` covers foreign tables too) at CREATE time, so + a TO-only foreign table inherits the FROM database's + default ACL on the freshly created object. Without + adjusting the comparer's effective `from_acl`, a + second compare run after applying the migration + emitted spurious `REVOKE` statements for those + auto-applied privileges. The foreign-table grant + path now mirrors the regular-table path: under + `full` mode, new foreign tables use the FROM + default-table ACL as the effective `from_acl` so + any required revokes are emitted in the same diff + run. + - ACL parser now handles quoted role names with + embedded `=`, `/`, or escaped `""` quotes (e.g. + `"weird=name"=r/owner`). The previous naive + `find('=')` / `find('/')` split misparsed these and + silently corrupted the grantee/grantor strings. + - Dependency-scan needles in the comparer (used to + decide drop/create order for views and routines) + now strip quotes when matching against the + quote-stripped haystack flavour. Dump fields + populated via `quote_ident` literally embed `"` for + mixed-case identifiers, and the previous code could + not match a quoted needle against an unquoted + reference in a definition body — silently losing + real dependency edges. + - `--use-drop=false` is now honoured uniformly for + collations, text-search configurations, text-search + dictionaries, casts, operators, default privileges, + publications, subscriptions, foreign data wrappers, + foreign servers, and user mappings. These eleven + object kinds previously *omitted* the DROP entirely + under `use_drop=false`; they now emit a commented + DROP, matching the behaviour already in place for + tables, views, types, enums, sequences, routines, + rules, event triggers, and foreign tables. + + Reliability: + - `current_setting('server_version_num')` is now + fetched once at the start of `Dump::fill` and + threaded into the per-object-kind futures. Previously + it was queried separately by the type and table + branches, doubling the round-trip cost on every dump. + - Unparseable ACL items are now logged with a + `Warning:` line. Previously a silently-skipped + grantee would simply disappear from the diff if a + future PostgreSQL version introduced an aclitem + syntax pgc did not yet recognise. + - `Config::load` now warns when `FROM_HOST` or + `TO_HOST` is empty. The same-target warning gated + on both hosts being non-empty, so a config with + missing host keys silently fell back to defaults. + - `get_schemas` collapsed an `if let Err` followed by + `result.unwrap()` into a single `?`-chain, removing + a redundant unwrap on a Result that was just + pattern-matched. + + Internals: + - SEQUENCE ACL parsing explicitly documents that + MAINTAIN ('m', PG17+) is filtered out — MAINTAIN is + only valid on tables, views, materialised views, + and foreign tables, never sequences. + - `to_owners` semantics are documented on + `generate_column_grants_script` (extending the + doc already added to `diff_acls`): current owners + are skipped because they have implicit access; + former owners stay in the diff as ordinary grantees. + + Tests: + - New regression tests covering owner-change grant + propagation, explicit grants to former owners under + `full` mode, and the foreign-table privilege set. + - Quoted-grantee ACL parsing tests for embedded `=`, + `/`, escaped `""`, and quoted grantor. + - Owner-unchanged with explicit owner grant in TO + (must be filtered as implicit). + - SEQUENCE with stray MAINTAIN bit (must be silently + dropped, not emitted as an invalid GRANT). + - Column-grant former-owner full-mode test where TO + has *no* explicit column ACL (must still revoke + from former owner without grants to the new owner). + - Dependency-scan needle tests covering both + quoted-against-unquoted and quoted-against-quoted + references. + 2026-04-23 v1.0.17 Bug fixes: diff --git a/app/Cargo.lock b/app/Cargo.lock index e999ca7..f07f20c 100644 --- a/app/Cargo.lock +++ b/app/Cargo.lock @@ -1146,7 +1146,7 @@ checksum = "e3148f5046208a5d56bcfc03053e3ca6334e51da8dfb19b6cdc8b306fae3283e" [[package]] name = "pgc" -version = "1.0.17" +version = "1.0.18" dependencies = [ "chrono", "clap", diff --git a/app/Cargo.toml b/app/Cargo.toml index ad7c2af..964a871 100644 --- a/app/Cargo.toml +++ b/app/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "pgc" -version = "1.0.17" +version = "1.0.18" edition = "2024" license = "MIT" diff --git a/app/src/comparer/core.rs b/app/src/comparer/core.rs index d227d3b..607cbc1 100644 --- a/app/src/comparer/core.rs +++ b/app/src/comparer/core.rs @@ -265,7 +265,26 @@ impl Comparer { } } - // Returns the script generated by the comparer + /// Append a DROP-shaped script to `script`, gated on `use_drop`. When + /// `use_drop` is false, every line is line-prefixed with `-- ` so the + /// SQL is left in the output for review but won't execute. Free + /// associated function (rather than `&mut self` method) so callers + /// holding a borrow into another field of `self` (e.g. a `from_map` + /// keyed by `&self.from.X`) can still emit drops without a borrow + /// conflict on disjoint fields. + fn emit_drop(script: &mut String, use_drop: bool, drop_script: &str) { + if use_drop { + script.push_str(drop_script); + } else { + script.push_str( + &drop_script + .lines() + .map(|l| format!("-- {}\n", l)) + .collect::(), + ); + } + } + fn get_script(&self) -> String { if self.use_comments { return self.script.clone(); @@ -315,6 +334,15 @@ impl Comparer { /// substring of a longer identifier). Call sites in quadratic loops /// should prelower the haystack once via `prelower_pair` and reuse it /// across inner iterations. + /// + /// Dump fields populated via `quote_ident` literally embed `"` for + /// identifiers that need quoting (mixed case, reserved words, etc.), so + /// the needle pair `(schema_lc, name_lc)` may also contain quotes. We + /// match the original (possibly-quoted) needle against `lowered` first + /// so case-sensitive references survive, then fall back to a + /// quote-stripped needle against `unquoted_lowered` so a quoted + /// definition reference is still found in source code that did not + /// quote it (and vice versa). #[inline] fn text_references_qualified_name_pre( lowered: &str, @@ -329,7 +357,12 @@ impl Comparer { if Self::has_whole_qualified_name(lowered, &pattern) { return true; } - Self::has_whole_qualified_name(unquoted_lowered, &pattern) + let unquoted_pattern: String = if pattern.contains('"') { + pattern.chars().filter(|c| *c != '"').collect() + } else { + pattern + }; + Self::has_whole_qualified_name(unquoted_lowered, &unquoted_pattern) } /// Build `(lowercase, lowercase_with_quotes_stripped)` from `text` in a @@ -2363,11 +2396,9 @@ impl Comparer { .map(|c| (format!("{}.{}", c.schema, c.name), c)) .collect(); - if self.use_drop { - for (key, coll) in &from_map { - if !to_map.contains_key(key) { - self.script.push_str(&coll.get_drop_script()); - } + for (key, coll) in &from_map { + if !to_map.contains_key(key) { + Self::emit_drop(&mut self.script, self.use_drop, &coll.get_drop_script()); } } @@ -2399,17 +2430,15 @@ impl Comparer { .map(|c| (format!("{}.{}", c.schema, c.name), c)) .collect(); - if self.use_drop { - for cfg in &self.from.ts_configs { - let key = format!("{}.{}", cfg.schema, cfg.name); - if !self - .to - .ts_configs - .iter() - .any(|c| format!("{}.{}", c.schema, c.name) == key) - { - self.script.push_str(&cfg.get_drop_script()); - } + for cfg in &self.from.ts_configs { + let key = format!("{}.{}", cfg.schema, cfg.name); + if !self + .to + .ts_configs + .iter() + .any(|c| format!("{}.{}", c.schema, c.name) == key) + { + Self::emit_drop(&mut self.script, self.use_drop, &cfg.get_drop_script()); } } @@ -2441,17 +2470,15 @@ impl Comparer { .map(|d| (format!("{}.{}", d.schema, d.name), d)) .collect(); - if self.use_drop { - for d in &self.from.ts_dicts { - let key = format!("{}.{}", d.schema, d.name); - if !self - .to - .ts_dicts - .iter() - .any(|td| format!("{}.{}", td.schema, td.name) == key) - { - self.script.push_str(&d.get_drop_script()); - } + for d in &self.from.ts_dicts { + let key = format!("{}.{}", d.schema, d.name); + if !self + .to + .ts_dicts + .iter() + .any(|td| format!("{}.{}", td.schema, td.name) == key) + { + Self::emit_drop(&mut self.script, self.use_drop, &d.get_drop_script()); } } @@ -2499,27 +2526,25 @@ impl Comparer { .map(|t| format!("{}.{}", t.schema, t.typname)) .collect(); - if self.use_drop { - for cast in &self.from.casts { - let key = format!("{} AS {}", cast.source_type, cast.target_type); - if !self - .to - .casts - .iter() - .any(|c| format!("{} AS {}", c.source_type, c.target_type) == key) - { - // If either the source or target type is a user-defined type being - // removed in this migration, skip the explicit DROP CAST — it will - // be handled automatically by DROP TYPE … CASCADE. - let src_dropped = from_type_names.contains(&cast.source_type) - && !to_type_names.contains(&cast.source_type); - let tgt_dropped = from_type_names.contains(&cast.target_type) - && !to_type_names.contains(&cast.target_type); - if src_dropped || tgt_dropped { - continue; - } - self.script.push_str(&cast.get_drop_script()); + for cast in &self.from.casts { + let key = format!("{} AS {}", cast.source_type, cast.target_type); + if !self + .to + .casts + .iter() + .any(|c| format!("{} AS {}", c.source_type, c.target_type) == key) + { + // If either the source or target type is a user-defined type being + // removed in this migration, skip the explicit DROP CAST — it will + // be handled automatically by DROP TYPE … CASCADE. + let src_dropped = from_type_names.contains(&cast.source_type) + && !to_type_names.contains(&cast.source_type); + let tgt_dropped = from_type_names.contains(&cast.target_type) + && !to_type_names.contains(&cast.target_type); + if src_dropped || tgt_dropped { + continue; } + Self::emit_drop(&mut self.script, self.use_drop, &cast.get_drop_script()); } } @@ -2561,12 +2586,10 @@ impl Comparer { .map(|op| (op_key(op), op)) .collect(); - if self.use_drop { - for op in &self.from.operators { - let key = op_key(op); - if !self.to.operators.iter().any(|o| op_key(o) == key) { - self.script.push_str(&op.get_drop_script()); - } + for op in &self.from.operators { + let key = op_key(op); + if !self.to.operators.iter().any(|o| op_key(o) == key) { + Self::emit_drop(&mut self.script, self.use_drop, &op.get_drop_script()); } } @@ -2602,12 +2625,10 @@ impl Comparer { .map(|dp| (dp_key(dp), dp)) .collect(); - if self.use_drop { - for dp in &self.from.default_privileges { - let key = dp_key(dp); - if !self.to.default_privileges.iter().any(|d| dp_key(d) == key) { - self.script.push_str(&dp.get_revoke_script()); - } + for dp in &self.from.default_privileges { + let key = dp_key(dp); + if !self.to.default_privileges.iter().any(|d| dp_key(d) == key) { + Self::emit_drop(&mut self.script, self.use_drop, &dp.get_revoke_script()); } } @@ -2638,11 +2659,9 @@ impl Comparer { .map(|p| (p.name.as_str(), p)) .collect(); - if self.use_drop { - for pub_ in &self.from.publications { - if !self.to.publications.iter().any(|p| p.name == pub_.name) { - self.script.push_str(&pub_.get_drop_script()); - } + for pub_ in &self.from.publications { + if !self.to.publications.iter().any(|p| p.name == pub_.name) { + Self::emit_drop(&mut self.script, self.use_drop, &pub_.get_drop_script()); } } @@ -2673,11 +2692,9 @@ impl Comparer { .map(|s| (s.name.as_str(), s)) .collect(); - if self.use_drop { - for sub in &self.from.subscriptions { - if !self.to.subscriptions.iter().any(|s| s.name == sub.name) { - self.script.push_str(&sub.get_drop_script()); - } + for sub in &self.from.subscriptions { + if !self.to.subscriptions.iter().any(|s| s.name == sub.name) { + Self::emit_drop(&mut self.script, self.use_drop, &sub.get_drop_script()); } } @@ -2708,16 +2725,14 @@ impl Comparer { .map(|f| (f.name.as_str(), f)) .collect(); - if self.use_drop { - for fdw in &self.from.foreign_data_wrappers { - if !self - .to - .foreign_data_wrappers - .iter() - .any(|f| f.name == fdw.name) - { - self.script.push_str(&fdw.get_drop_script()); - } + for fdw in &self.from.foreign_data_wrappers { + if !self + .to + .foreign_data_wrappers + .iter() + .any(|f| f.name == fdw.name) + { + Self::emit_drop(&mut self.script, self.use_drop, &fdw.get_drop_script()); } } @@ -2748,11 +2763,9 @@ impl Comparer { .map(|s| (s.name.as_str(), s)) .collect(); - if self.use_drop { - for srv in &self.from.foreign_servers { - if !self.to.foreign_servers.iter().any(|s| s.name == srv.name) { - self.script.push_str(&srv.get_drop_script()); - } + for srv in &self.from.foreign_servers { + if !self.to.foreign_servers.iter().any(|s| s.name == srv.name) { + Self::emit_drop(&mut self.script, self.use_drop, &srv.get_drop_script()); } } @@ -2786,12 +2799,10 @@ impl Comparer { .map(|um| (um_key(um), um)) .collect(); - if self.use_drop { - for um in &self.from.user_mappings { - let key = um_key(um); - if !self.to.user_mappings.iter().any(|u| um_key(u) == key) { - self.script.push_str(&um.get_drop_script()); - } + for um in &self.from.user_mappings { + let key = um_key(um); + if !self.to.user_mappings.iter().any(|u| um_key(u) == key) { + Self::emit_drop(&mut self.script, self.use_drop, &um.get_drop_script()); } } @@ -3228,7 +3239,16 @@ impl Comparer { .get(schema.name.as_str()) .copied() .unwrap_or((&[], "")); - let owners: Vec<&str> = [from_owner, schema.owner.as_str()] + // Asymmetric owner filters: ALTER ... OWNER TO removes the FROM + // owner's implicit-owner ACL entry from the FROM relation and + // creates one for the TO owner. Pre-fix the comparer used a + // single symmetric filter and treated the FROM owner's + // implicit privileges as if they would persist post-migration, + // producing non-idempotent REVOKE/GRANT pairs whenever + // ownership changed. + 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 +3258,8 @@ impl Comparer { full, "SCHEMA", &schema.name, - &owners, + &from_owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3317,7 +3338,11 @@ impl Comparer { } else { (&[], "") }; - let owners: Vec<&str> = [from_owner, table.owner.as_str()] + // See the schema branch above for why `from_owners` and + // `to_owners` are kept separate. + 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 +3353,8 @@ impl Comparer { full, "TABLE", &object_name, - &owners, + &from_owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3360,7 +3386,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 +3399,8 @@ impl Comparer { full, "SEQUENCE", &object_name, - &owners, + &from_owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3421,7 +3450,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 +3463,8 @@ impl Comparer { full, "TABLE", &object_name, - &owners, + &from_owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3457,22 +3489,53 @@ impl Comparer { .collect(); for ft in &self.to.foreign_tables { - let (from_acl, from_owner) = 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()] + // Same default-privileges treatment as regular tables: foreign + // tables share `pg_default_acl.defaclobjtype = 'r'`, so a new + // foreign table created during the migration auto-inherits the + // FROM database's default table privileges at CREATE time. In + // full grants mode, swap in the FROM default ACL as the + // effective `from_acl` so any needed REVOKEs are emitted in + // this diff run rather than appearing on the next compare + // (non-idempotent diff). + let is_new_ft = !from_ft_map.contains_key(&(ft.schema.as_str(), ft.name.as_str())); + let ft_default_acl_storage = if is_new_ft && full { + from_default_table_acl + .get(&ft.schema) + .cloned() + .unwrap_or_default() + } else { + Vec::new() + }; + let (from_acl, from_owner): (&[String], &str) = if is_new_ft && full { + (ft_default_acl_storage.as_slice(), "") + } else if let Some(&(acl, owner)) = + from_ft_map.get(&(ft.schema.as_str(), ft.name.as_str())) + { + (acl, owner) + } else { + (&[], "") + }; + 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(); let object_name = format!("{}.{}", ft.schema, ft.name); + // PostgreSQL `GRANT` only accepts `ON TABLE`; there is no + // `ON FOREIGN TABLE` form. Foreign tables share the privilege + // set with regular tables (handled in `valid_privileges` via + // the `"TABLE" | "FOREIGN TABLE"` arm), so passing "TABLE" + // here yields valid SQL while preserving the full privilege + // vocabulary including PG17's MAINTAIN. let grants_script = acl::generate_grants_script( from_acl, &ft.acl, full, - "FOREIGN TABLE", + "TABLE", &object_name, - &owners, + &from_owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3494,7 +3557,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 +3574,8 @@ impl Comparer { full, object_kind, &object_name, - &owners, + &from_owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3539,7 +3605,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 +3618,8 @@ impl Comparer { full, "TYPE", &object_name, - &owners, + &from_owners, + &to_owners, ); if !grants_script.is_empty() { if self.use_comments { @@ -3574,11 +3643,16 @@ 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()] + // Column ACLs are stored in `pg_attribute.attacl`, which — + // unlike `pg_class.relacl` — is NOT touched by `ALTER TABLE + // OWNER TO`. Explicit column grants to the former owner + // persist after the owner change and must be revoked under + // full mode. So we pass an empty `from_owners` to the column + // helper (no implicit-owner entry to strip) but keep the + // `to_owners` filter so a redundant explicit column grant to + // the new owner is not emitted (the new owner already has + // implicit access via table ownership). + let to_table_owners: Vec<&str> = [table.owner.as_str()] .into_iter() .filter(|o| !o.is_empty()) .collect(); @@ -3610,7 +3684,8 @@ 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..84605b3 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,193 @@ 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(); + + // PostgreSQL's GRANT syntax has no `ON FOREIGN TABLE` form — foreign + // tables share the regular `ON TABLE` grant syntax. Pre-fix the + // comparer emitted `ON FOREIGN TABLE`, which produced invalid SQL + // (`syntax error at or near "TABLE"`) when the diff was applied. + assert!( + script.contains("GRANT SELECT, UPDATE ON TABLE public.ft_orders TO writer;"), + "Full must add foreign table grant via ON TABLE syntax, got: {script}" + ); + assert!( + script.contains("REVOKE SELECT ON TABLE public.ft_orders FROM reader;"), + "Full must revoke removed foreign table grant via ON TABLE syntax, got: {script}" + ); + assert!( + !script.contains("ON FOREIGN TABLE"), + "Foreign table grants must not use `ON FOREIGN TABLE` (invalid SQL), got: {script}" + ); +} + +/// User-reported regression: when ownership changes AND TO has an explicit +/// grant to the former owner, the migration must emit exactly one GRANT +/// (for the explicit privilege in TO) and zero REVOKEs (the implicit-owner +/// ACL row is stripped by ALTER OWNER alone). Replays the exact ACL shape +/// you'd see in the schema_a → schema_b owner-change scenario after both +/// FROM and TO have run their explicit GRANTs and PG has materialised the +/// implicit-owner row. +#[tokio::test] +async fn compare_grants_owner_change_with_explicit_grant_to_former_owner_is_idempotent() { + let mut from_dump = Dump::new(DumpConfig::default()); + let mut to_dump = Dump::new(DumpConfig::default()); + + // FROM table: owned by pgc_owner_from. relacl carries the implicit- + // owner row (pg materialises it once any GRANT exists) plus the two + // explicit grants to reader/writer. + let mut from_table = Table::new( + "test_schema".to_string(), + "users".to_string(), + "test_schema".to_string(), + "users".to_string(), + "pgc_owner_from".to_string(), + None, + Vec::new(), + Vec::new(), + Vec::new(), + Vec::new(), + None, + ); + from_table.acl = vec![ + "pgc_owner_from=arwdDxt/pgc_owner_from".to_string(), + "pgc_grant_reader=r/pgc_owner_from".to_string(), + "pgc_grant_writer=arw/pgc_owner_from".to_string(), + ]; + + // TO table: owned by pgc_owner_to. relacl has the new implicit-owner + // row, the same reader grant, the writer with UPDATE removed, and an + // explicit grant to the former owner pgc_owner_from. + let mut to_table = Table::new( + "test_schema".to_string(), + "users".to_string(), + "test_schema".to_string(), + "users".to_string(), + "pgc_owner_to".to_string(), + None, + Vec::new(), + Vec::new(), + Vec::new(), + Vec::new(), + None, + ); + to_table.acl = vec![ + "pgc_owner_to=arwdDxt/pgc_owner_to".to_string(), + "pgc_owner_from=r/pgc_owner_to".to_string(), + "pgc_grant_reader=r/pgc_owner_to".to_string(), + "pgc_grant_writer=ar/pgc_owner_to".to_string(), + ]; + + from_dump.tables.push(from_table); + 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(); + + // Exactly two statements expected: + // - GRANT SELECT TO pgc_owner_from (the new explicit grant in TO) + // - REVOKE UPDATE FROM pgc_grant_writer (UPDATE removed in TO) + assert!( + script.contains("GRANT SELECT ON TABLE test_schema.users TO pgc_owner_from;"), + "Must emit explicit grant to former owner, got: {script}" + ); + assert!( + script.contains("REVOKE UPDATE ON TABLE test_schema.users FROM pgc_grant_writer;"), + "Must revoke writer's UPDATE removed in TO, got: {script}" + ); + // No REVOKE/GRANT for pgc_owner_to (TO owner — implicit privileges). + // No REVOKE for pgc_owner_from's old implicit-owner row — ALTER OWNER + // strips it. Specifically NO REVOKE on pgc_owner_from for the 7 other + // privileges, which is the bug this regression test guards against. + assert!( + !script.contains("FROM pgc_owner_from"), + "Must not REVOKE anything from former owner — ALTER OWNER strips the implicit row, got: {script}" + ); + assert!( + !script.contains("pgc_owner_to"), + "Current owner must not appear in grants output, got: {script}" + ); +} + +/// Regression: a TO-only foreign table must inherit the FROM database's +/// default-table privileges as the effective `from_acl` under `full` mode, +/// because PostgreSQL auto-applies them on CREATE. Without this, the diff +/// is non-idempotent — re-running compare after applying it would emit +/// `REVOKE` statements for the auto-granted privileges that the migration +/// itself is responsible for cleaning up. +#[tokio::test] +async fn compare_grants_new_foreign_table_revokes_default_priv_grants() { + let mut from_dump = Dump::new(DumpConfig::default()); + let mut to_dump = Dump::new(DumpConfig::default()); + + // FROM has a default-privilege rule that grants SELECT to `reader` on + // any new table in `public`. No explicit grants in TO → after CREATE, + // the auto-applied SELECT must be revoked in this same diff. + let dp = DefaultPrivilege { + role_name: String::new(), + schema_name: "public".to_string(), + object_type: "r".to_string(), + acl: vec!["reader=r/owner".to_string()], + hash: Some("dp".to_string()), + }; + from_dump.default_privileges.push(dp); + + // TO-only foreign table (no FROM counterpart, no explicit ACL). + let to_ft = ForeignTable::new( + "public".to_string(), + "ft_new".to_string(), + "fdw_server".to_string(), + "owner".to_string(), + Vec::new(), + Vec::new(), + ); + 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("REVOKE SELECT ON TABLE public.ft_new FROM reader;"), + "New foreign table must revoke auto-applied default-privilege grants under full mode, got: {script}" + ); + assert!( + !script.contains("ON FOREIGN TABLE"), + "Foreign table grants must use `ON TABLE` syntax, got: {script}" + ); +} + #[tokio::test] async fn compare_grants_dropped_view_restores_all_grants() { let mut from_dump = Dump::new(DumpConfig::default()); @@ -4061,21 +4249,177 @@ async fn compare_grants_excludes_owner_acl_entries() { comparer.compare_grants().await.unwrap(); let script = comparer.get_script(); + // The `old_owner=arwdDxt/old_owner` and `new_owner=arwdDxt/new_owner` + // entries are PostgreSQL's implicit-owner ACL rows (materialised once + // any GRANT exists). `ALTER TABLE ... OWNER TO new_owner` removes + // old_owner's implicit row and adds new_owner's automatically — no + // REVOKE/GRANT is needed for those rows. Reader is unchanged on both + // sides. Net diff: empty. Pre-fix the comparer treated the implicit + // FROM-owner row as if it would persist post-migration and emitted a + // long REVOKE, then on the next compare run had nothing to compare + // against and emitted GRANTs — a non-idempotent oscillation. assert!( - !script.contains("old_owner"), - "Must not REVOKE from old owner, got: {script}" + !script.contains("REVOKE"), + "ALTER OWNER alone strips the implicit-owner entry; no REVOKE should be emitted, got: {script}" + ); + assert!( + !script.contains("GRANT"), + "No grants expected — reader is unchanged and new_owner gets implicit privileges via ALTER OWNER, got: {script}" ); assert!( !script.contains("new_owner"), - "Must not GRANT to new owner, got: {script}" + "Must not reference the new owner explicitly, got: {script}" ); assert!( - !script.contains("GRANT"), - "No grants expected (reader unchanged), got: {script}" + !script.contains("old_owner"), + "Must not reference the former owner explicitly when only the implicit-owner ACL row needs to migrate, 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(); + + // Same reasoning as `compare_grants_excludes_owner_acl_entries`: + // `old_owner=UC/old_owner` and `old_owner=ar/old_owner` are + // implicit-owner ACL entries that `ALTER ... OWNER TO new_owner` + // strips automatically. Comparing against TO (which has no entries + // at all) should produce an empty diff, not REVOKE statements. assert!( !script.contains("REVOKE"), - "No revokes expected (reader unchanged), got: {script}" + "Implicit-owner ACL entries are removed by ALTER OWNER alone; no REVOKE should be emitted, got: {script}" + ); + assert!( + !script.contains("new_owner"), + "Current owner must not appear in grant/revoke output, got: {script}" + ); + assert!( + !script.contains("old_owner"), + "Former owner must not appear in grant/revoke output for the implicit-owner ACL row, 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 +6394,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 +6442,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}" ); } @@ -6373,3 +6719,90 @@ async fn buffer_ordering_sequence_drop_before_type_drop() { "sequence_post_script must precede type_post_script, got:\n{script}" ); } + +/// Regression for the dependency-scan needle bug. Dump fields are populated +/// via `quote_ident`, so a mixed-case identifier comes back literally +/// quoted (`"MyView"`). Previously the needle kept the quotes and the +/// quote-stripped haystack flavour could never match an unquoted reference, +/// silently dropping a real dependency. +#[test] +fn text_references_qualified_name_pre_matches_unquoted_reference() { + let (lower, unquoted_lower) = Comparer::prelower_pair("SELECT * FROM public.regular_view;"); + // Needle as built from `quote_ident` for a mixed-case identifier. + assert!(Comparer::text_references_qualified_name_pre( + &lower, + &unquoted_lower, + "\"public\"", + "\"regular_view\"", + )); +} + +#[test] +fn text_references_qualified_name_pre_still_matches_quoted_reference() { + let (lower, unquoted_lower) = Comparer::prelower_pair("SELECT * FROM \"MySchema\".\"MyView\";"); + assert!(Comparer::text_references_qualified_name_pre( + &lower, + &unquoted_lower, + "\"myschema\"", + "\"myview\"", + )); +} + +/// Counterpart to `compare_column_grants_revokes_former_owner_and_excludes_current_owner`: +/// when ownership changes and the new TO has *no* explicit column ACL at all +/// (only the implicit owner privileges), a former owner's column grant in +/// FROM must still be revoked under `full` mode. Without this we would leak +/// the old owner's column-level access into the post-migration database. +#[tokio::test] +async fn compare_column_grants_revokes_former_owner_when_to_has_no_column_acl() { + let mut from_dump = Dump::new(DumpConfig::default()); + let mut to_dump = Dump::new(DumpConfig::default()); + + let mut from_col = int_column("public", "users", "secret", 1); + from_col.acl = vec!["old_owner=r/old_owner".to_string()]; + let from_table = Table::new( + "public".to_string(), + "users".to_string(), + "public".to_string(), + "users".to_string(), + "old_owner".to_string(), + None, + vec![from_col], + Vec::new(), + Vec::new(), + Vec::new(), + None, + ); + + // No column ACL in TO. + let to_col = int_column("public", "users", "secret", 1); + let to_table = Table::new( + "public".to_string(), + "users".to_string(), + "public".to_string(), + "users".to_string(), + "new_owner".to_string(), + None, + vec![to_col], + Vec::new(), + Vec::new(), + Vec::new(), + None, + ); + + from_dump.tables.push(from_table); + 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 SELECT (secret) ON TABLE public.users FROM old_owner;"), + "Former owner column ACL must be revoked even without ACL in TO, got: {script}" + ); + assert!( + !script.contains("new_owner"), + "Current owner must never appear in column grant output, got: {script}" + ); +} diff --git a/app/src/config/core.rs b/app/src/config/core.rs index e996622..7cde685 100644 --- a/app/src/config/core.rs +++ b/app/src/config/core.rs @@ -181,6 +181,24 @@ impl Config { ); } + // An empty FROM/TO host slips past the same-target check above + // (which gates on both hosts being non-empty). A blank host almost + // always means the user forgot a key, mistyped one, or fed in an + // empty config file by mistake; silently falling back to defaults + // produces a baffling connection error later. Surface it now. + if from.host.is_empty() { + eprintln!( + "Warning: FROM_HOST is empty in the configuration file — \ + connection will likely fail. Check that the FROM_* keys are set." + ); + } + if to.host.is_empty() { + eprintln!( + "Warning: TO_HOST is empty in the configuration file — \ + connection will likely fail. Check that the TO_* keys are set." + ); + } + Ok(Config { from, to, diff --git a/app/src/dump/acl.rs b/app/src/dump/acl.rs index 3d5ad1f..8bb23f6 100644 --- a/app/src/dump/acl.rs +++ b/app/src/dump/acl.rs @@ -22,14 +22,45 @@ struct PrivilegeItem { grant_option: bool, } +/// Find the first byte position of `target` in `s` that lies outside any +/// double-quoted region. PostgreSQL aclitem text uses `"..."` to escape role +/// names containing special characters (`=`, `/`, whitespace, etc.) and `""` +/// as a literal quote inside a quoted region. A naive `str::find` on the +/// raw separator misparses entries like `"weird=name"=r/owner`. +fn find_unquoted(s: &str, target: u8) -> Option { + let bytes = s.as_bytes(); + let mut i = 0; + let mut in_quotes = false; + while i < bytes.len() { + let c = bytes[i]; + if in_quotes { + if c == b'"' { + if i + 1 < bytes.len() && bytes[i + 1] == b'"' { + i += 2; + continue; + } + in_quotes = false; + } + } else if c == b'"' { + in_quotes = true; + } else if c == target { + return Some(i); + } + i += 1; + } + None +} + impl AclEntry { - /// Parse a single ACL item string like `"user=arwdDxt/owner"`. + /// Parse a single ACL item string like `"user=arwdDxt/owner"`. Quoted + /// role names with embedded `=` or `/` (e.g. `"weird=name"=r/owner`) are + /// handled correctly via [`find_unquoted`]. pub fn parse(acl_item: &str) -> Option { - let eq_pos = acl_item.find('=')?; - let slash_pos = acl_item.find('/')?; - if slash_pos <= eq_pos { - return None; - } + let eq_pos = find_unquoted(acl_item, b'=')?; + // Search for `/` only after the `=` so a slash inside the grantee + // name does not confuse us. + let slash_rel = find_unquoted(&acl_item[eq_pos + 1..], b'/')?; + let slash_pos = eq_pos + 1 + slash_rel; let grantee = acl_item[..eq_pos].to_string(); let privileges = acl_item[eq_pos + 1..slash_pos].to_string(); let grantor = acl_item[slash_pos + 1..].to_string(); @@ -66,7 +97,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", @@ -76,6 +107,11 @@ impl AclEntry { "TRIGGER", "MAINTAIN", ], + // PG17+ added MAINTAIN as a privilege bit, but it is only valid + // on tables, views, materialised views, and foreign tables — not + // on sequences. Listing only the valid set here causes + // `parse_privileges` to silently drop a stray `m` if it ever + // appears in a sequence ACL, which is the correct behaviour. "SEQUENCE" => &["USAGE", "SELECT", "UPDATE"], "FUNCTION" | "PROCEDURE" => &["EXECUTE"], "SCHEMA" => &["USAGE", "CREATE"], @@ -228,18 +264,28 @@ 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) { - let privs = AclEntry::parse_privileges(&entry.privileges, object_kind); - let grantee_map = map.entry(entry.grantee).or_default(); - for p in privs { - let existing = grantee_map.entry(p.name).or_insert(false); - if p.grant_option { - *existing = true; - } + let Some(entry) = AclEntry::parse(item) else { + // Surfacing unparseable ACL items prevents silent data loss when + // a future PostgreSQL version introduces an aclitem syntax we do + // not yet recognise — without this warning the offending grantee + // would simply disappear from the diff. + eprintln!("Warning: skipping unparseable ACL item ({object_kind}): {item:?}"); + continue; + }; + 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 { + let existing = grantee_map.entry(p.name).or_insert(false); + if p.grant_option { + *existing = true; } } } @@ -252,19 +298,34 @@ 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. +/// Owner filtering is asymmetric to model the effect of `ALTER ... OWNER TO`: +/// +/// * `from_owners` — roles whose implicit-owner ACL entries in `from_acl` +/// will be REMOVED by the migration (typically the FROM-side owner when +/// ownership is changing). +/// * `to_owners` — roles whose implicit-owner ACL entries in `to_acl` +/// are AUTOMATIC after the migration (typically the TO-side owner) and +/// therefore need no GRANT to materialise. +/// +/// When the owner does not change, callers pass the same single role in +/// both lists and the behaviour collapses to "skip the owner everywhere." +/// When the owner changes, the FROM owner is filtered from `from_acl` +/// only — so a former owner that appears in `to_acl` as an explicit +/// grantee shows up correctly in the diff, and the FROM owner's +/// implicit-owner privileges (which `ALTER OWNER` will strip) are not +/// mistakenly compared against the TO ACL. 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 +337,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 +401,22 @@ 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. +/// See [`diff_acls`] for the meaning of `from_owners` / `to_owners`. When +/// the owner does not change between `from` and `to`, callers pass the +/// same single role in both lists; when it does change, `from_owners` +/// holds the FROM owner (whose implicit-owner ACL entry will be removed +/// by `ALTER ... OWNER`) and `to_owners` holds the TO owner (whose +/// implicit-owner ACL entry will be created by it). 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 { @@ -403,29 +465,39 @@ pub fn generate_grants_script( /// Generate GRANT statements for a new object (all ACL entries from "to"). /// -/// `owners` lists role names that own the object; their ACL entries are skipped. +/// `owners` lists role names that own the object; their ACL entries are +/// skipped from `to_acl`. There is no `from_acl`, so the FROM owner list +/// is empty. pub fn generate_new_object_grants( to_acl: &[String], object_kind: &str, 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. /// /// Column privileges use the format: /// `GRANT SELECT (col) ON TABLE schema.table TO grantee;` +/// +/// `from_owners` / `to_owners` follow the same convention as +/// [`generate_grants_script`]. When the parent table's owner does not +/// change, callers pass the same single role in both lists. When it +/// does, `from_owners` is the FROM owner (whose implicit-column-grant +/// entries, if any, vanish under `ALTER TABLE ... OWNER TO`) and +/// `to_owners` is the TO owner. pub fn generate_column_grants_script( from_acl: &[String], to_acl: &[String], 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 +619,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 +633,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 +643,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 +653,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 +666,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 +675,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()); } @@ -630,19 +702,71 @@ 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_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_b"], &["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() { + // Realistic owner-change scenario: FROM's relacl carries owner_a's + // implicit-owner entry (PG materialises it once any GRANT exists). + // After ALTER OWNER TO owner_b, that entry vanishes — so + // `from_owners` filters owner_a out of from_acl, modelling the + // post-migration FROM. TO has an explicit grant to owner_a as a + // regular grantee, which must show up in the diff. let from = vec![ "owner_a=arwdDxt/owner_a".to_string(), "reader=r/owner_a".to_string(), ]; let to = vec![ "owner_b=arwdDxt/owner_b".to_string(), + "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_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()); + } + + /// Regression for the user-reported non-idempotent diff: under the + /// previous symmetric `to_owners` filter, a former owner with no + /// explicit grant in TO would compare its implicit-owner privileges + /// in FROM against an empty set in TO and emit a long REVOKE list; + /// after applying ALTER OWNER + that REVOKE, the former owner has + /// nothing in FROM and the next compare run keeps oscillating. With + /// asymmetric `from_owners` / `to_owners` filters that model + /// `ALTER OWNER`'s effect, the diff is empty when both ACLs already + /// agree on explicit grants. + #[test] + fn test_diff_acls_owner_change_without_explicit_grant_to_former_owner_is_idempotent() { + let from = vec![ + "owner_a=arwdDxt/owner_a".to_string(), + "reader=r/owner_a".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"]); assert!( diffs.is_empty(), - "Owner grantees must be excluded, got: {diffs:?}" + "Owner-change with no explicit grant to former owner must produce an empty diff (ALTER OWNER alone is enough), got: {diffs:?}" ); } @@ -656,7 +780,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"]); @@ -664,9 +788,90 @@ mod tests { assert_eq!(removed.revokes, vec!["SELECT"]); } + #[test] + fn test_parse_quoted_grantee_with_equals() { + // PostgreSQL escapes role names containing `=` by quoting; the parser + // must split on the unquoted `=`, not the one inside the quoted name. + let entry = AclEntry::parse("\"weird=name\"=r/owner").unwrap(); + assert_eq!(entry.grantee, "\"weird=name\""); + assert_eq!(entry.privileges, "r"); + assert_eq!(entry.grantor, "owner"); + } + + #[test] + fn test_parse_quoted_grantee_with_slash() { + // Same idea for `/` — it can appear inside a quoted role name and + // must not be mistaken for the grantor separator. + let entry = AclEntry::parse("\"weird/name\"=r/owner").unwrap(); + assert_eq!(entry.grantee, "\"weird/name\""); + assert_eq!(entry.privileges, "r"); + assert_eq!(entry.grantor, "owner"); + } + + #[test] + fn test_parse_quoted_grantor_with_separators() { + let entry = AclEntry::parse("user=r/\"weird=grantor\"").unwrap(); + assert_eq!(entry.grantee, "user"); + assert_eq!(entry.privileges, "r"); + assert_eq!(entry.grantor, "\"weird=grantor\""); + } + + #[test] + fn test_parse_quoted_grantee_with_escaped_quote() { + // `""` inside a quoted region is an escaped literal `"`; it must not + // close the quoted region prematurely. + let entry = AclEntry::parse("\"qu\"\"ote\"=r/owner").unwrap(); + assert_eq!(entry.grantee, "\"qu\"\"ote\""); + assert_eq!(entry.privileges, "r"); + assert_eq!(entry.grantor, "owner"); + } + + #[test] + fn test_diff_acls_owner_unchanged_skips_explicit_owner_grant() { + // Owner unchanged. TO has an explicit entry for the owner role + // (which PG sometimes emits) — it must be filtered as implicit. + let from = vec!["reader=r/owner_a".to_string()]; + let to = vec![ + "owner_a=arwdDxt/owner_a".to_string(), + "reader=r/owner_a".to_string(), + ]; + let diffs = diff_acls(&from, &to, true, "TABLE", &["owner_a"], &["owner_a"]); + assert!( + diffs.is_empty(), + "Owner-as-grantee entry must be filtered when owner is unchanged, got: {diffs:?}" + ); + } + + #[test] + fn test_foreign_table_uses_full_table_privileges() { + // FOREIGN TABLE shares the same privilege set as TABLE, including + // MAINTAIN (PG17+). Before v1.0.18 it fell through to the empty + // default set and silently dropped privileges from the diff. + let from = vec![]; + let to = vec!["reader=rwm/owner".to_string()]; + let diffs = diff_acls(&from, &to, false, "FOREIGN TABLE", &["owner"], &["owner"]); + assert_eq!(diffs.len(), 1); + let mut grants = diffs[0].grants_plain.clone(); + grants.sort(); + assert_eq!(grants, vec!["MAINTAIN", "SELECT", "UPDATE"]); + } + + #[test] + fn test_sequence_drops_maintain_privilege() { + // MAINTAIN ('m') is not a valid privilege on sequences in PG17+; the + // parser must drop it silently rather than emit an invalid GRANT. + let from = vec![]; + let to = vec!["reader=Urm/owner".to_string()]; + let diffs = diff_acls(&from, &to, false, "SEQUENCE", &["owner"], &["owner"]); + assert_eq!(diffs.len(), 1); + let mut grants = diffs[0].grants_plain.clone(); + grants.sort(); + assert_eq!(grants, vec!["SELECT", "USAGE"]); + } + #[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 +879,16 @@ mod tests { true, "FUNCTION", "public.my_func()", - &["owner_a", "owner_b"], + &["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}" ); } } diff --git a/app/src/dump/core.rs b/app/src/dump/core.rs index 9a0562b..06abb90 100644 --- a/app/src/dump/core.rs +++ b/app/src/dump/core.rs @@ -264,11 +264,20 @@ impl Dump { // high-latency (remote) connections. let schema_filter = self.accessible_schema_filter(); + // Detect PostgreSQL server version once and pass it down to the + // per-object-kind futures. The value is a session constant, so an + // extra round-trip per parallel branch is pure waste. + let pg_version: i32 = + sqlx::query_scalar("select current_setting('server_version_num')::int4;") + .fetch_one(pool) + .await + .map_err(|e| Error::other(format!("Failed to fetch server_version_num: {e}.")))?; + let types_enums_fut = async { let mut types = Vec::new(); let mut enums = Vec::new(); // get_types logic (inlined to avoid &mut self borrow conflicts) - Self::fetch_types_standalone(pool, &schema_filter, &mut types).await?; + Self::fetch_types_standalone(pool, &schema_filter, pg_version, &mut types).await?; Self::fetch_domain_constraints_standalone(pool, &schema_filter, &mut types).await?; Self::fetch_enums_standalone(pool, &mut types, &mut enums).await?; Ok::<(Vec, Vec), Error>((types, enums)) @@ -277,7 +286,7 @@ impl Dump { let extensions_fut = Self::fetch_extensions_standalone(pool, &schema_filter); let sequences_fut = Self::fetch_sequences_standalone(pool, &schema_filter); let routines_fut = Self::fetch_routines_standalone(pool, &schema_filter); - let tables_fut = Self::fetch_tables_standalone(pool, &schema_filter); + let tables_fut = Self::fetch_tables_standalone(pool, &schema_filter, pg_version); let views_fut = Self::fetch_views_standalone(pool, &schema_filter); let foreign_tables_fut = Self::fetch_foreign_tables_standalone(pool, &schema_filter); let statistics_fut = Self::fetch_statistics_standalone(pool, &schema_filter); @@ -379,7 +388,7 @@ impl Dump { } async fn get_schemas(&mut self, pool: &PgPool) -> Result<(), Error> { - let result = sqlx::query( + let rows = sqlx::query( "select quote_ident(n.nspname) as schema_name, n.nspname as raw_schema_name, @@ -397,11 +406,8 @@ impl Dump { ) .bind(&self.configuration.scheme) .fetch_all(pool) - .await; - if let Err(e) = &result { - return Err(Error::other(format!("Failed to fetch schemas: {}.", e))); - } - let rows = result.unwrap(); + .await + .map_err(|e| Error::other(format!("Failed to fetch schemas: {e}.")))?; if rows.is_empty() { println!("No schemas found."); @@ -485,6 +491,7 @@ impl Dump { async fn fetch_types_standalone( pool: &PgPool, schema_filter: &str, + pg_version: i32, types: &mut Vec, ) -> Result<(), Error> { let composite_attributes_rows = sqlx::query( @@ -531,18 +538,10 @@ impl Dump { .push(attribute); } - // Fetch range type metadata from pg_range - let range_pg_version: i32 = - sqlx::query_scalar("select current_setting('server_version_num')::int4;") - .fetch_one(pool) - .await - .map_err(|e| { - Error::other(format!( - "Failed to fetch server_version_num for types: {e}." - )) - })?; - - let has_rngmultitypid: bool = range_pg_version >= 140000 + // Fetch range type metadata from pg_range. `pg_version` is fetched + // once in `Dump::fill` and threaded through to avoid an extra + // round-trip per parallel branch. + let has_rngmultitypid: bool = pg_version >= 140000 && sqlx::query_scalar::<_, bool>( "SELECT EXISTS (SELECT 1 FROM pg_attribute WHERE attrelid = 'pg_range'::regclass AND attname = 'rngmultitypid' AND NOT attisdropped)", ) @@ -1212,6 +1211,7 @@ impl Dump { async fn fetch_tables_standalone( pool: &PgPool, schema_filter: &str, + pg_version: i32, ) -> Result, Error> { // Check once whether the pg_get_tabledef extension function exists. let has_tabledef_fn = @@ -1221,13 +1221,7 @@ impl Dump { .unwrap_or(None) .is_some(); - // Detect PostgreSQL server version for conditional feature support. - let pg_version: i32 = - sqlx::query_scalar("select current_setting('server_version_num')::int4;") - .fetch_one(pool) - .await - .unwrap_or(0); - + // `pg_version` is fetched once in `Dump::fill` and passed in here. // Probe catalog capabilities once for the entire dump run. let caps = PgCatalogCaps::detect(pool, pg_version).await; diff --git a/data/test/README.md b/data/test/README.md index 5c06207..d18dccb 100644 --- a/data/test/README.md +++ b/data/test/README.md @@ -231,6 +231,13 @@ Grant comparison test using roles `pgc_grant_reader` and `pgc_grant_writer`. - `USAGE` on `test_schema.user_id_seq` (sequence) → `pgc_grant_reader` (no grant in TO) - `EXECUTE` on `test_schema.update_timestamp()` (function) → `pgc_grant_reader` (no grant in TO) +#### Former-owner explicit grant after owner change (v1.0.18 regression) +- `SELECT` on `test_schema.users` → `pgc_owner_from` (TO-only): the table's owner changes from `pgc_owner_from` to `pgc_owner_to`, and the TO side retains an explicit grant to the previous owner. Pre-v1.0.18 the comparer filtered both old and new owners as implicit privilege holders and silently dropped this grant from the diff; post-fix only the TO-side owner is filtered, so the grant survives. + +#### Foreign table grants (v1.0.18 regression) +- `SELECT` on `test_schema.foreign_users` → `pgc_grant_reader` (TO-only): pre-v1.0.18 `FOREIGN TABLE` ACL parsing fell through to the empty default privilege set, so any explicit grant on a foreign table was silently filtered out of the diff. Post-fix `FOREIGN TABLE` shares the `TABLE` privilege set (`SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, TRIGGER, MAINTAIN`). +- `MAINTAIN` on `test_schema.foreign_users` → `pgc_grant_writer` (commented out, PG17+): uncomment on PG17+ to additionally exercise the new MAINTAIN privilege bit on a foreign table. + ### 18. Exclusion Constraints (btree_gist) - **Extension**: `btree_gist` added in both schemas - **Modified**: `test_schema.reservations` — exclusion constraint unchanged, but table gains `guest_name` column in Schema B diff --git a/data/test/schema_b.sql b/data/test/schema_b.sql index 27d9a48..004954e 100644 --- a/data/test/schema_b.sql +++ b/data/test/schema_b.sql @@ -1225,6 +1225,15 @@ ALTER FUNCTION test_schema.update_timestamp() OWNER TO pgc_owner_to; ALTER VIEW test_schema.product_inventory OWNER TO pgc_owner_to; ALTER MATERIALIZED VIEW test_schema.active_users_mat OWNER TO pgc_owner_to; +-- v1.0.18 regression: former-owner explicit grant after owner change. +-- After the owner of `test_schema.users` changes from `pgc_owner_from` to +-- `pgc_owner_to`, the TO side retains an explicit grant to the previous +-- owner. Pre-v1.0.18 the comparer filtered both old and new owners as +-- implicit privilege holders, silently dropping this grant from the +-- diff. Post-fix only the TO-side owner is filtered, so the explicit +-- grant to the former owner survives and appears in the migration. +GRANT SELECT ON test_schema.users TO pgc_owner_from; + -- Serial / bigserial / identity column test (TO side) -- These tables are NEW (not present in schema_a / FROM). -- The migration script must use serial/bigserial types instead of @@ -1517,6 +1526,17 @@ GRANT SELECT ON test_schema.product_inventory TO pgc_grant_writer; -- Function grants (update_timestamp grant removed; new function grant added) GRANT EXECUTE ON FUNCTION test_schema.calculate_average_rating(UUID) TO pgc_grant_reader; +-- Foreign table grants +-- v1.0.18 regression: pre-fix, FOREIGN TABLE ACL parsing fell through to +-- the empty default privilege set, so any explicit grant on a foreign +-- table was silently filtered out of the diff. Post-fix, FOREIGN TABLE +-- shares the TABLE privilege set (SELECT, INSERT, UPDATE, DELETE, +-- TRUNCATE, REFERENCES, TRIGGER, MAINTAIN). +GRANT SELECT ON test_schema.foreign_users TO pgc_grant_reader; +-- MAINTAIN is PG17+; uncomment on PG17+ to additionally exercise the +-- new privilege bit on a foreign table: +-- GRANT MAINTAIN ON test_schema.foreign_users TO pgc_grant_writer; + -- ============================================================================= -- Rules comparison test (TO side) -- =============================================================================