From b543284d73dd770adf48fd9086b8478b0d8b4fc2 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Mon, 26 Jan 2026 06:41:07 -0800 Subject: [PATCH 1/7] fix: revoke default privilege grants on new tables (#253) When default privileges grant permissions to a role on new tables, and a new table is created with explicit REVOKE for that role, the REVOKE statement was missing from the initial migration plan. This caused the schema to require two apply cycles to converge. The fix detects when privileges would be auto-granted by default privileges on new objects, but the user didn't include those explicit privileges in the new state (meaning they intentionally revoked them). These REVOKE statements are now generated after the table is created. Co-Authored-By: Claude Opus 4.5 --- internal/diff/diff.go | 22 +++++++--- internal/diff/privilege.go | 42 +++++++++++++++++++ .../revoke_default_privilege/diff.sql | 7 ++++ .../revoke_default_privilege/new.sql | 19 +++++++++ .../revoke_default_privilege/old.sql | 10 +++++ .../revoke_default_privilege/plan.json | 26 ++++++++++++ .../revoke_default_privilege/plan.sql | 7 ++++ .../revoke_default_privilege/plan.txt | 22 ++++++++++ 8 files changed, 150 insertions(+), 5 deletions(-) create mode 100644 testdata/diff/privilege/revoke_default_privilege/diff.sql create mode 100644 testdata/diff/privilege/revoke_default_privilege/new.sql create mode 100644 testdata/diff/privilege/revoke_default_privilege/old.sql create mode 100644 testdata/diff/privilege/revoke_default_privilege/plan.json create mode 100644 testdata/diff/privilege/revoke_default_privilege/plan.sql create mode 100644 testdata/diff/privilege/revoke_default_privilege/plan.txt diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 7b69943c..6d8d1ec5 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -275,11 +275,12 @@ type ddlDiff struct { droppedDefaultPrivileges []*ir.DefaultPrivilege modifiedDefaultPrivileges []*defaultPrivilegeDiff // Explicit object privileges - addedPrivileges []*ir.Privilege - droppedPrivileges []*ir.Privilege - modifiedPrivileges []*privilegeDiff - addedRevokedDefaultPrivs []*ir.RevokedDefaultPrivilege - droppedRevokedDefaultPrivs []*ir.RevokedDefaultPrivilege + addedPrivileges []*ir.Privilege + droppedPrivileges []*ir.Privilege + modifiedPrivileges []*privilegeDiff + revokedDefaultGrantsOnNewTables []*ir.Privilege // Privileges to revoke on newly created tables (issue #253) + addedRevokedDefaultPrivs []*ir.RevokedDefaultPrivilege + droppedRevokedDefaultPrivs []*ir.RevokedDefaultPrivilege // Column-level privileges addedColumnPrivileges []*ir.ColumnPrivilege droppedColumnPrivileges []*ir.ColumnPrivilege @@ -1122,6 +1123,12 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { } } + // Handle privileges that would be auto-granted by default privileges on new objects + // but should be explicitly revoked because the user didn't include them in the new state. + // These must be processed AFTER the tables are created, not in the drop phase. + // See https://github.com/pgschema/pgschema/issues/253 + diff.revokedDefaultGrantsOnNewTables = computeRevokedDefaultGrants(diff.addedTables, newPrivs, newDefaultPrivileges) + // Sort privileges for deterministic output sort.Slice(diff.addedPrivileges, func(i, j int) bool { return diff.addedPrivileges[i].GetObjectKey() < diff.addedPrivileges[j].GetObjectKey() @@ -1478,6 +1485,11 @@ func (d *ddlDiff) generateCreateSQL(targetSchema string, collector *diffCollecto // Create default privileges generateCreateDefaultPrivilegesSQL(d.addedDefaultPrivileges, targetSchema, collector) + // Revoke default grants on new tables that the user explicitly didn't include + // This must happen AFTER tables are created but BEFORE explicit grants + // See https://github.com/pgschema/pgschema/issues/253 + generateDropPrivilegesSQL(d.revokedDefaultGrantsOnNewTables, targetSchema, collector) + // Create explicit object privileges generateCreatePrivilegesSQL(d.addedPrivileges, targetSchema, collector) diff --git a/internal/diff/privilege.go b/internal/diff/privilege.go index e763a364..54d0d829 100644 --- a/internal/diff/privilege.go +++ b/internal/diff/privilege.go @@ -356,3 +356,45 @@ func privilegesCoveredBy(privs, coveringPrivs []string) bool { } return true } + +// computeRevokedDefaultGrants finds privileges that would be auto-granted by default privileges +// on new objects, but should be explicitly revoked because the user didn't include them in the new state. +// See https://github.com/pgschema/pgschema/issues/253 +func computeRevokedDefaultGrants(addedTables []*ir.Table, newPrivs map[string]*ir.Privilege, defaultPrivileges []*ir.DefaultPrivilege) []*ir.Privilege { + var revokedPrivs []*ir.Privilege + + // For each new table, check which default privileges would auto-grant + for _, table := range addedTables { + for _, dp := range defaultPrivileges { + // Only process default privileges for TABLES + if dp.ObjectType != ir.DefaultPrivilegeObjectTypeTables { + continue + } + + // Check if the new state has an explicit privilege for this table+grantee + // If not, the user intentionally revoked the default grant + privilegeKey := string(ir.PrivilegeObjectTypeTable) + ":" + table.Name + ":" + dp.Grantee + hasExplicitGrant := false + for key := range newPrivs { + // Match by object key prefix (without grant option suffix) + if len(key) >= len(privilegeKey) && key[:len(privilegeKey)] == privilegeKey { + hasExplicitGrant = true + break + } + } + + if !hasExplicitGrant { + // Create a synthetic privilege to revoke the default grant + revokedPrivs = append(revokedPrivs, &ir.Privilege{ + ObjectType: ir.PrivilegeObjectTypeTable, + ObjectName: table.Name, + Grantee: dp.Grantee, + Privileges: dp.Privileges, + WithGrantOption: dp.WithGrantOption, + }) + } + } + } + + return revokedPrivs +} diff --git a/testdata/diff/privilege/revoke_default_privilege/diff.sql b/testdata/diff/privilege/revoke_default_privilege/diff.sql new file mode 100644 index 00000000..54f569f7 --- /dev/null +++ b/testdata/diff/privilege/revoke_default_privilege/diff.sql @@ -0,0 +1,7 @@ +CREATE TABLE IF NOT EXISTS secrets ( + id integer, + data text, + CONSTRAINT secrets_pkey PRIMARY KEY (id) +); + +REVOKE SELECT ON TABLE secrets FROM reader; diff --git a/testdata/diff/privilege/revoke_default_privilege/new.sql b/testdata/diff/privilege/revoke_default_privilege/new.sql new file mode 100644 index 00000000..d5e4969d --- /dev/null +++ b/testdata/diff/privilege/revoke_default_privilege/new.sql @@ -0,0 +1,19 @@ +-- Create role for testing +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'reader') THEN + CREATE ROLE reader; + END IF; +END $$; + +-- Default privileges grant SELECT on all new tables to reader +ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT ON TABLES TO reader; + +-- Create a table that should NOT inherit the default SELECT privilege +CREATE TABLE secrets ( + id integer PRIMARY KEY, + data text +); + +-- Explicitly revoke the auto-granted default privilege +REVOKE SELECT ON TABLE secrets FROM reader; diff --git a/testdata/diff/privilege/revoke_default_privilege/old.sql b/testdata/diff/privilege/revoke_default_privilege/old.sql new file mode 100644 index 00000000..23ad6245 --- /dev/null +++ b/testdata/diff/privilege/revoke_default_privilege/old.sql @@ -0,0 +1,10 @@ +-- Create role for testing +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'reader') THEN + CREATE ROLE reader; + END IF; +END $$; + +-- Default privileges grant SELECT on all new tables to reader +ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT ON TABLES TO reader; diff --git a/testdata/diff/privilege/revoke_default_privilege/plan.json b/testdata/diff/privilege/revoke_default_privilege/plan.json new file mode 100644 index 00000000..7066c70e --- /dev/null +++ b/testdata/diff/privilege/revoke_default_privilege/plan.json @@ -0,0 +1,26 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.6.1", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "f2bd42139d9b75b298bed35098f5d5a9205c335f6b02afff27c71913ec5210ec" + }, + "groups": [ + { + "steps": [ + { + "sql": "CREATE TABLE IF NOT EXISTS secrets (\n id integer,\n data text,\n CONSTRAINT secrets_pkey PRIMARY KEY (id)\n);", + "type": "table", + "operation": "create", + "path": "public.secrets" + }, + { + "sql": "REVOKE SELECT ON TABLE secrets FROM reader;", + "type": "privilege", + "operation": "drop", + "path": "privileges.TABLE.secrets.reader" + } + ] + } + ] +} diff --git a/testdata/diff/privilege/revoke_default_privilege/plan.sql b/testdata/diff/privilege/revoke_default_privilege/plan.sql new file mode 100644 index 00000000..54f569f7 --- /dev/null +++ b/testdata/diff/privilege/revoke_default_privilege/plan.sql @@ -0,0 +1,7 @@ +CREATE TABLE IF NOT EXISTS secrets ( + id integer, + data text, + CONSTRAINT secrets_pkey PRIMARY KEY (id) +); + +REVOKE SELECT ON TABLE secrets FROM reader; diff --git a/testdata/diff/privilege/revoke_default_privilege/plan.txt b/testdata/diff/privilege/revoke_default_privilege/plan.txt new file mode 100644 index 00000000..5290ef84 --- /dev/null +++ b/testdata/diff/privilege/revoke_default_privilege/plan.txt @@ -0,0 +1,22 @@ +Plan: 1 to add, 1 to drop. + +Summary by type: + tables: 1 to add + privileges: 1 to drop + +Tables: + + secrets + +Privileges: + - reader + +DDL to be executed: +-------------------------------------------------- + +CREATE TABLE IF NOT EXISTS secrets ( + id integer, + data text, + CONSTRAINT secrets_pkey PRIMARY KEY (id) +); + +REVOKE SELECT ON TABLE secrets FROM reader; From 0da97ffbaa8810a408663e2853af9a6472844cf3 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Mon, 26 Jan 2026 07:12:39 -0800 Subject: [PATCH 2/7] fix: improve default privilege handling for new tables - Filter out redundant GRANTs on new tables when covered by default privileges - Move default privileges creation before table creation so auto-grants apply - Enhanced add_table_privilege test case with a new table to verify behavior When creating a new table along with default privileges, the system now: 1. Creates default privileges first 2. Creates tables (so PostgreSQL auto-grants based on default privileges) 3. Skips explicit GRANTs that are covered by default privileges Co-Authored-By: Claude Opus 4.5 --- internal/diff/diff.go | 32 +++++++++++++------ .../add_table_privilege/diff.sql | 6 ++++ .../add_table_privilege/new.sql | 6 ++++ .../add_table_privilege/plan.json | 6 ++++ .../add_table_privilege/plan.sql | 6 ++++ .../add_table_privilege/plan.txt | 12 ++++++- 6 files changed, 57 insertions(+), 11 deletions(-) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 6d8d1ec5..9fb87a77 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -1098,13 +1098,6 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { } } - // Find added privileges (in new but not matched) - for fullKey, p := range newPrivs { - if !matchedNew[fullKey] { - diff.addedPrivileges = append(diff.addedPrivileges, p) - } - } - // Collect default privileges from the desired state (new IR) // These are used to filter out privileges that are covered by default privileges var newDefaultPrivileges []*ir.DefaultPrivilege @@ -1112,6 +1105,25 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { newDefaultPrivileges = append(newDefaultPrivileges, dbSchema.DefaultPrivileges...) } + // Build a set of new table names for quick lookup + newTableNames := make(map[string]bool) + for _, t := range diff.addedTables { + newTableNames[t.Name] = true + } + + // Find added privileges (in new but not matched) + // Skip privileges on new tables that are covered by default privileges + for fullKey, p := range newPrivs { + if !matchedNew[fullKey] { + // If this privilege is on a new table and covered by default privileges, skip it + // The default privileges will auto-grant when the table is created + if newTableNames[p.ObjectName] && isPrivilegeCoveredByDefaultPrivileges(p, newDefaultPrivileges) { + continue + } + diff.addedPrivileges = append(diff.addedPrivileges, p) + } + } + // Find dropped privileges (in old but not matched) // Skip privileges that are covered by default privileges in the desired state for fullKey, p := range oldPrivs { @@ -1439,6 +1451,9 @@ func (d *ddlDiff) generateCreateSQL(targetSchema string, collector *diffCollecto } } + // Create default privileges BEFORE tables so auto-grants apply to new tables + generateCreateDefaultPrivilegesSQL(d.addedDefaultPrivileges, targetSchema, collector) + // Separate tables into those that depend on new functions and those that don't // This ensures we create functions before tables that use them in defaults/checks tablesWithoutFunctionDeps := []*ir.Table{} @@ -1482,9 +1497,6 @@ func (d *ddlDiff) generateCreateSQL(targetSchema string, collector *diffCollecto // Create views generateCreateViewsSQL(d.addedViews, targetSchema, collector) - // Create default privileges - generateCreateDefaultPrivilegesSQL(d.addedDefaultPrivileges, targetSchema, collector) - // Revoke default grants on new tables that the user explicitly didn't include // This must happen AFTER tables are created but BEFORE explicit grants // See https://github.com/pgschema/pgschema/issues/253 diff --git a/testdata/diff/default_privilege/add_table_privilege/diff.sql b/testdata/diff/default_privilege/add_table_privilege/diff.sql index edc6c324..31e729dc 100644 --- a/testdata/diff/default_privilege/add_table_privilege/diff.sql +++ b/testdata/diff/default_privilege/add_table_privilege/diff.sql @@ -1,3 +1,9 @@ ALTER DEFAULT PRIVILEGES FOR ROLE testuser IN SCHEMA public GRANT SELECT ON TABLES TO PUBLIC; ALTER DEFAULT PRIVILEGES FOR ROLE testuser IN SCHEMA public GRANT INSERT, UPDATE ON TABLES TO app_user; + +CREATE TABLE IF NOT EXISTS users ( + id integer, + name text NOT NULL, + CONSTRAINT users_pkey PRIMARY KEY (id) +); diff --git a/testdata/diff/default_privilege/add_table_privilege/new.sql b/testdata/diff/default_privilege/add_table_privilege/new.sql index 025d4e72..37c79d09 100644 --- a/testdata/diff/default_privilege/add_table_privilege/new.sql +++ b/testdata/diff/default_privilege/add_table_privilege/new.sql @@ -11,3 +11,9 @@ ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT ON TABLES TO PUBLIC; -- Grant INSERT, UPDATE to app_user role ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT INSERT, UPDATE ON TABLES TO app_user; + +-- Create a new table - default privileges should apply automatically +CREATE TABLE users ( + id integer PRIMARY KEY, + name text NOT NULL +); diff --git a/testdata/diff/default_privilege/add_table_privilege/plan.json b/testdata/diff/default_privilege/add_table_privilege/plan.json index 670f893a..e39c0dc5 100644 --- a/testdata/diff/default_privilege/add_table_privilege/plan.json +++ b/testdata/diff/default_privilege/add_table_privilege/plan.json @@ -19,6 +19,12 @@ "type": "default_privilege", "operation": "create", "path": "default_privileges.testuser.TABLES.app_user" + }, + { + "sql": "CREATE TABLE IF NOT EXISTS users (\n id integer,\n name text NOT NULL,\n CONSTRAINT users_pkey PRIMARY KEY (id)\n);", + "type": "table", + "operation": "create", + "path": "public.users" } ] } diff --git a/testdata/diff/default_privilege/add_table_privilege/plan.sql b/testdata/diff/default_privilege/add_table_privilege/plan.sql index edc6c324..31e729dc 100644 --- a/testdata/diff/default_privilege/add_table_privilege/plan.sql +++ b/testdata/diff/default_privilege/add_table_privilege/plan.sql @@ -1,3 +1,9 @@ ALTER DEFAULT PRIVILEGES FOR ROLE testuser IN SCHEMA public GRANT SELECT ON TABLES TO PUBLIC; ALTER DEFAULT PRIVILEGES FOR ROLE testuser IN SCHEMA public GRANT INSERT, UPDATE ON TABLES TO app_user; + +CREATE TABLE IF NOT EXISTS users ( + id integer, + name text NOT NULL, + CONSTRAINT users_pkey PRIMARY KEY (id) +); diff --git a/testdata/diff/default_privilege/add_table_privilege/plan.txt b/testdata/diff/default_privilege/add_table_privilege/plan.txt index 37c672b1..763d6b69 100644 --- a/testdata/diff/default_privilege/add_table_privilege/plan.txt +++ b/testdata/diff/default_privilege/add_table_privilege/plan.txt @@ -1,15 +1,25 @@ -Plan: 2 to add. +Plan: 3 to add. Summary by type: default privileges: 2 to add + tables: 1 to add Default privileges: + PUBLIC + app_user +Tables: + + users + DDL to be executed: -------------------------------------------------- ALTER DEFAULT PRIVILEGES FOR ROLE testuser IN SCHEMA public GRANT SELECT ON TABLES TO PUBLIC; ALTER DEFAULT PRIVILEGES FOR ROLE testuser IN SCHEMA public GRANT INSERT, UPDATE ON TABLES TO app_user; + +CREATE TABLE IF NOT EXISTS users ( + id integer, + name text NOT NULL, + CONSTRAINT users_pkey PRIMARY KEY (id) +); From f5a13be084a35b2188d323c136f46a4f9a0acf93 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Mon, 26 Jan 2026 07:35:31 -0800 Subject: [PATCH 3/7] fix: address review feedback for default privilege handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Fix prefix matching bug: Use exact object key lookup instead of prefix matching to avoid false positives (e.g., app_role matching app_role2) 2. Handle partial revokes: Compute set difference between default privileges and actual privileges, generating REVOKE only for the missing privileges instead of all-or-nothing 3. Ensure deterministic ordering: Sort revokedPrivs by GetObjectKey() for consistent output across runs 4. Improve performance: Build an index of privileges by object key for O(1) lookups instead of O(n²) scanning Added test case: partial_revoke_default_privilege to verify partial revoke behavior works correctly. Co-Authored-By: Claude Opus 4.5 --- internal/diff/privilege.go | 65 +++++++++++++++---- .../partial_revoke_default_privilege/diff.sql | 7 ++ .../partial_revoke_default_privilege/new.sql | 19 ++++++ .../partial_revoke_default_privilege/old.sql | 10 +++ .../plan.json | 26 ++++++++ .../partial_revoke_default_privilege/plan.sql | 7 ++ .../partial_revoke_default_privilege/plan.txt | 22 +++++++ 7 files changed, 144 insertions(+), 12 deletions(-) create mode 100644 testdata/diff/privilege/partial_revoke_default_privilege/diff.sql create mode 100644 testdata/diff/privilege/partial_revoke_default_privilege/new.sql create mode 100644 testdata/diff/privilege/partial_revoke_default_privilege/old.sql create mode 100644 testdata/diff/privilege/partial_revoke_default_privilege/plan.json create mode 100644 testdata/diff/privilege/partial_revoke_default_privilege/plan.sql create mode 100644 testdata/diff/privilege/partial_revoke_default_privilege/plan.txt diff --git a/internal/diff/privilege.go b/internal/diff/privilege.go index 54d0d829..9271c83a 100644 --- a/internal/diff/privilege.go +++ b/internal/diff/privilege.go @@ -363,6 +363,31 @@ func privilegesCoveredBy(privs, coveringPrivs []string) bool { func computeRevokedDefaultGrants(addedTables []*ir.Table, newPrivs map[string]*ir.Privilege, defaultPrivileges []*ir.DefaultPrivilege) []*ir.Privilege { var revokedPrivs []*ir.Privilege + // Build an index of privileges by (ObjectType:ObjectName:Grantee) for O(1) lookups + // This avoids O(n²) complexity when scanning newPrivs for each (table, default privilege) pair + privsByObjectKey := make(map[string]*ir.Privilege) + for _, p := range newPrivs { + key := p.GetObjectKey() + // If multiple entries exist (different grant options), keep any one - we just need the privileges + if existing, ok := privsByObjectKey[key]; ok { + // Merge privileges from both entries + privSet := make(map[string]bool) + for _, priv := range existing.Privileges { + privSet[priv] = true + } + for _, priv := range p.Privileges { + privSet[priv] = true + } + merged := make([]string, 0, len(privSet)) + for priv := range privSet { + merged = append(merged, priv) + } + existing.Privileges = merged + } else { + privsByObjectKey[key] = p + } + } + // For each new table, check which default privileges would auto-grant for _, table := range addedTables { for _, dp := range defaultPrivileges { @@ -371,30 +396,46 @@ func computeRevokedDefaultGrants(addedTables []*ir.Table, newPrivs map[string]*i continue } - // Check if the new state has an explicit privilege for this table+grantee - // If not, the user intentionally revoked the default grant - privilegeKey := string(ir.PrivilegeObjectTypeTable) + ":" + table.Name + ":" + dp.Grantee - hasExplicitGrant := false - for key := range newPrivs { - // Match by object key prefix (without grant option suffix) - if len(key) >= len(privilegeKey) && key[:len(privilegeKey)] == privilegeKey { - hasExplicitGrant = true - break + // Look up explicit privilege for this exact (table, grantee) pair + objectKey := string(ir.PrivilegeObjectTypeTable) + ":" + table.Name + ":" + dp.Grantee + existingPriv := privsByObjectKey[objectKey] + + // Compute which default privileges need to be revoked + // (privileges in dp.Privileges but not in existingPriv.Privileges) + var privsToRevoke []string + if existingPriv == nil { + // No explicit privilege exists - revoke all default privileges + privsToRevoke = dp.Privileges + } else { + // Compute set difference: dp.Privileges - existingPriv.Privileges + existingSet := make(map[string]bool) + for _, p := range existingPriv.Privileges { + existingSet[p] = true + } + for _, p := range dp.Privileges { + if !existingSet[p] { + privsToRevoke = append(privsToRevoke, p) + } } } - if !hasExplicitGrant { - // Create a synthetic privilege to revoke the default grant + if len(privsToRevoke) > 0 { + // Create a synthetic privilege to revoke the missing default grants revokedPrivs = append(revokedPrivs, &ir.Privilege{ ObjectType: ir.PrivilegeObjectTypeTable, ObjectName: table.Name, Grantee: dp.Grantee, - Privileges: dp.Privileges, + Privileges: privsToRevoke, WithGrantOption: dp.WithGrantOption, }) } } } + // Sort for deterministic output + sort.Slice(revokedPrivs, func(i, j int) bool { + return revokedPrivs[i].GetObjectKey() < revokedPrivs[j].GetObjectKey() + }) + return revokedPrivs } diff --git a/testdata/diff/privilege/partial_revoke_default_privilege/diff.sql b/testdata/diff/privilege/partial_revoke_default_privilege/diff.sql new file mode 100644 index 00000000..1f79dd5a --- /dev/null +++ b/testdata/diff/privilege/partial_revoke_default_privilege/diff.sql @@ -0,0 +1,7 @@ +CREATE TABLE IF NOT EXISTS readonly_data ( + id integer, + value text, + CONSTRAINT readonly_data_pkey PRIMARY KEY (id) +); + +REVOKE DELETE, INSERT, UPDATE ON TABLE readonly_data FROM app_user; diff --git a/testdata/diff/privilege/partial_revoke_default_privilege/new.sql b/testdata/diff/privilege/partial_revoke_default_privilege/new.sql new file mode 100644 index 00000000..751e53a7 --- /dev/null +++ b/testdata/diff/privilege/partial_revoke_default_privilege/new.sql @@ -0,0 +1,19 @@ +-- Create role for testing +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'app_user') THEN + CREATE ROLE app_user; + END IF; +END $$; + +-- Default privileges grant SELECT, INSERT, UPDATE, DELETE on all new tables +ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO app_user; + +-- Create a read-only table - user should only have SELECT, not INSERT/UPDATE/DELETE +CREATE TABLE readonly_data ( + id integer PRIMARY KEY, + value text +); + +-- Revoke write privileges - keep only SELECT from the default grants +REVOKE INSERT, UPDATE, DELETE ON TABLE readonly_data FROM app_user; diff --git a/testdata/diff/privilege/partial_revoke_default_privilege/old.sql b/testdata/diff/privilege/partial_revoke_default_privilege/old.sql new file mode 100644 index 00000000..a3f953c1 --- /dev/null +++ b/testdata/diff/privilege/partial_revoke_default_privilege/old.sql @@ -0,0 +1,10 @@ +-- Create role for testing +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'app_user') THEN + CREATE ROLE app_user; + END IF; +END $$; + +-- Default privileges grant SELECT, INSERT, UPDATE, DELETE on all new tables +ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO app_user; diff --git a/testdata/diff/privilege/partial_revoke_default_privilege/plan.json b/testdata/diff/privilege/partial_revoke_default_privilege/plan.json new file mode 100644 index 00000000..945f5059 --- /dev/null +++ b/testdata/diff/privilege/partial_revoke_default_privilege/plan.json @@ -0,0 +1,26 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.6.1", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "4bac506d0b54d7b2808b01f4cd402d232bbe48221fd1acde17ecc0a4e13e8fe8" + }, + "groups": [ + { + "steps": [ + { + "sql": "CREATE TABLE IF NOT EXISTS readonly_data (\n id integer,\n value text,\n CONSTRAINT readonly_data_pkey PRIMARY KEY (id)\n);", + "type": "table", + "operation": "create", + "path": "public.readonly_data" + }, + { + "sql": "REVOKE DELETE, INSERT, UPDATE ON TABLE readonly_data FROM app_user;", + "type": "privilege", + "operation": "drop", + "path": "privileges.TABLE.readonly_data.app_user" + } + ] + } + ] +} diff --git a/testdata/diff/privilege/partial_revoke_default_privilege/plan.sql b/testdata/diff/privilege/partial_revoke_default_privilege/plan.sql new file mode 100644 index 00000000..1f79dd5a --- /dev/null +++ b/testdata/diff/privilege/partial_revoke_default_privilege/plan.sql @@ -0,0 +1,7 @@ +CREATE TABLE IF NOT EXISTS readonly_data ( + id integer, + value text, + CONSTRAINT readonly_data_pkey PRIMARY KEY (id) +); + +REVOKE DELETE, INSERT, UPDATE ON TABLE readonly_data FROM app_user; diff --git a/testdata/diff/privilege/partial_revoke_default_privilege/plan.txt b/testdata/diff/privilege/partial_revoke_default_privilege/plan.txt new file mode 100644 index 00000000..69b927cd --- /dev/null +++ b/testdata/diff/privilege/partial_revoke_default_privilege/plan.txt @@ -0,0 +1,22 @@ +Plan: 1 to add, 1 to drop. + +Summary by type: + tables: 1 to add + privileges: 1 to drop + +Tables: + + readonly_data + +Privileges: + - app_user + +DDL to be executed: +-------------------------------------------------- + +CREATE TABLE IF NOT EXISTS readonly_data ( + id integer, + value text, + CONSTRAINT readonly_data_pkey PRIMARY KEY (id) +); + +REVOKE DELETE, INSERT, UPDATE ON TABLE readonly_data FROM app_user; From 34f7e6c1a6b0461363eac4c6c773c11e235a5937 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Mon, 26 Jan 2026 08:01:11 -0800 Subject: [PATCH 4/7] fix: address review feedback for IR mutation and timing - Fix IR object mutation bug: use local map[string]map[string]bool instead of mutating shared *ir.Privilege objects when merging privileges from multiple entries - Fix modified default privilege timing: use "active" defaults (old + added) instead of all new defaults when filtering GRANTs and computing REVOKEs. Modified defaults run AFTER table creation, so their old version is what's active when the table is created. See: https://github.com/pgschema/pgschema/pull/257#pullrequestreview-3706696119 Co-Authored-By: Claude Opus 4.5 --- internal/diff/diff.go | 23 +++++++++++++++++++---- internal/diff/privilege.go | 35 +++++++++++++---------------------- 2 files changed, 32 insertions(+), 26 deletions(-) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 9fb87a77..36150b34 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -1105,6 +1105,19 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { newDefaultPrivileges = append(newDefaultPrivileges, dbSchema.DefaultPrivileges...) } + // Build "active" default privileges - defaults that will be active when new tables are created. + // This includes: + // 1. Old defaults (already on target database) - these apply immediately when table is created + // 2. Added defaults (will be created BEFORE tables in our migration order) + // NOT included: Modified defaults - the modification runs AFTER table creation, so the OLD + // version is what's active when the table is created. The old defaults are already included. + // See https://github.com/pgschema/pgschema/pull/257#pullrequestreview-3706696119 + var activeDefaultPrivileges []*ir.DefaultPrivilege + for _, dbSchema := range oldIR.Schemas { + activeDefaultPrivileges = append(activeDefaultPrivileges, dbSchema.DefaultPrivileges...) + } + activeDefaultPrivileges = append(activeDefaultPrivileges, diff.addedDefaultPrivileges...) + // Build a set of new table names for quick lookup newTableNames := make(map[string]bool) for _, t := range diff.addedTables { @@ -1112,12 +1125,13 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { } // Find added privileges (in new but not matched) - // Skip privileges on new tables that are covered by default privileges + // Skip privileges on new tables that are covered by ACTIVE default privileges + // (defaults that will be in effect when the table is created) for fullKey, p := range newPrivs { if !matchedNew[fullKey] { - // If this privilege is on a new table and covered by default privileges, skip it + // If this privilege is on a new table and covered by active default privileges, skip it // The default privileges will auto-grant when the table is created - if newTableNames[p.ObjectName] && isPrivilegeCoveredByDefaultPrivileges(p, newDefaultPrivileges) { + if newTableNames[p.ObjectName] && isPrivilegeCoveredByDefaultPrivileges(p, activeDefaultPrivileges) { continue } diff.addedPrivileges = append(diff.addedPrivileges, p) @@ -1138,8 +1152,9 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { // Handle privileges that would be auto-granted by default privileges on new objects // but should be explicitly revoked because the user didn't include them in the new state. // These must be processed AFTER the tables are created, not in the drop phase. + // Use activeDefaultPrivileges because that's what will be granted when the table is created. // See https://github.com/pgschema/pgschema/issues/253 - diff.revokedDefaultGrantsOnNewTables = computeRevokedDefaultGrants(diff.addedTables, newPrivs, newDefaultPrivileges) + diff.revokedDefaultGrantsOnNewTables = computeRevokedDefaultGrants(diff.addedTables, newPrivs, activeDefaultPrivileges) // Sort privileges for deterministic output sort.Slice(diff.addedPrivileges, func(i, j int) bool { diff --git a/internal/diff/privilege.go b/internal/diff/privilege.go index 9271c83a..4ccf6cb4 100644 --- a/internal/diff/privilege.go +++ b/internal/diff/privilege.go @@ -365,26 +365,21 @@ func computeRevokedDefaultGrants(addedTables []*ir.Table, newPrivs map[string]*i // Build an index of privileges by (ObjectType:ObjectName:Grantee) for O(1) lookups // This avoids O(n²) complexity when scanning newPrivs for each (table, default privilege) pair - privsByObjectKey := make(map[string]*ir.Privilege) + // Use a separate map to track merged privilege sets to avoid mutating shared IR objects + privSetByObjectKey := make(map[string]map[string]bool) for _, p := range newPrivs { key := p.GetObjectKey() - // If multiple entries exist (different grant options), keep any one - we just need the privileges - if existing, ok := privsByObjectKey[key]; ok { + if existing, ok := privSetByObjectKey[key]; ok { // Merge privileges from both entries - privSet := make(map[string]bool) - for _, priv := range existing.Privileges { - privSet[priv] = true + for _, priv := range p.Privileges { + existing[priv] = true } + } else { + privSet := make(map[string]bool) for _, priv := range p.Privileges { privSet[priv] = true } - merged := make([]string, 0, len(privSet)) - for priv := range privSet { - merged = append(merged, priv) - } - existing.Privileges = merged - } else { - privsByObjectKey[key] = p + privSetByObjectKey[key] = privSet } } @@ -398,22 +393,18 @@ func computeRevokedDefaultGrants(addedTables []*ir.Table, newPrivs map[string]*i // Look up explicit privilege for this exact (table, grantee) pair objectKey := string(ir.PrivilegeObjectTypeTable) + ":" + table.Name + ":" + dp.Grantee - existingPriv := privsByObjectKey[objectKey] + existingPrivSet := privSetByObjectKey[objectKey] // Compute which default privileges need to be revoked - // (privileges in dp.Privileges but not in existingPriv.Privileges) + // (privileges in dp.Privileges but not in existingPrivSet) var privsToRevoke []string - if existingPriv == nil { + if existingPrivSet == nil { // No explicit privilege exists - revoke all default privileges privsToRevoke = dp.Privileges } else { - // Compute set difference: dp.Privileges - existingPriv.Privileges - existingSet := make(map[string]bool) - for _, p := range existingPriv.Privileges { - existingSet[p] = true - } + // Compute set difference: dp.Privileges - existingPrivSet for _, p := range dp.Privileges { - if !existingSet[p] { + if !existingPrivSet[p] { privsToRevoke = append(privsToRevoke, p) } } From 9bd8a9cf13d09743896929ea5414d8bba5665734 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Mon, 26 Jan 2026 08:28:58 -0800 Subject: [PATCH 5/7] test: combine partial_revoke into revoke_default_privilege test Merge partial_revoke_default_privilege test case into revoke_default_privilege to test both scenarios in a single comprehensive test: 1. Full revoke: secrets table where reader role has no privileges (revokes auto-granted SELECT from default privileges) 2. Partial revoke: readonly_data table where app_user has only SELECT (revokes INSERT, UPDATE, DELETE but keeps SELECT from default privileges) Co-Authored-By: Claude Opus 4.5 --- .../partial_revoke_default_privilege/diff.sql | 7 ----- .../partial_revoke_default_privilege/new.sql | 19 -------------- .../partial_revoke_default_privilege/old.sql | 10 ------- .../plan.json | 26 ------------------- .../partial_revoke_default_privilege/plan.sql | 7 ----- .../partial_revoke_default_privilege/plan.txt | 22 ---------------- .../revoke_default_privilege/diff.sql | 8 ++++++ .../revoke_default_privilege/new.sql | 21 ++++++++++++--- .../revoke_default_privilege/old.sql | 8 +++++- .../revoke_default_privilege/plan.json | 14 +++++++++- .../revoke_default_privilege/plan.sql | 8 ++++++ .../revoke_default_privilege/plan.txt | 16 +++++++++--- 12 files changed, 67 insertions(+), 99 deletions(-) delete mode 100644 testdata/diff/privilege/partial_revoke_default_privilege/diff.sql delete mode 100644 testdata/diff/privilege/partial_revoke_default_privilege/new.sql delete mode 100644 testdata/diff/privilege/partial_revoke_default_privilege/old.sql delete mode 100644 testdata/diff/privilege/partial_revoke_default_privilege/plan.json delete mode 100644 testdata/diff/privilege/partial_revoke_default_privilege/plan.sql delete mode 100644 testdata/diff/privilege/partial_revoke_default_privilege/plan.txt diff --git a/testdata/diff/privilege/partial_revoke_default_privilege/diff.sql b/testdata/diff/privilege/partial_revoke_default_privilege/diff.sql deleted file mode 100644 index 1f79dd5a..00000000 --- a/testdata/diff/privilege/partial_revoke_default_privilege/diff.sql +++ /dev/null @@ -1,7 +0,0 @@ -CREATE TABLE IF NOT EXISTS readonly_data ( - id integer, - value text, - CONSTRAINT readonly_data_pkey PRIMARY KEY (id) -); - -REVOKE DELETE, INSERT, UPDATE ON TABLE readonly_data FROM app_user; diff --git a/testdata/diff/privilege/partial_revoke_default_privilege/new.sql b/testdata/diff/privilege/partial_revoke_default_privilege/new.sql deleted file mode 100644 index 751e53a7..00000000 --- a/testdata/diff/privilege/partial_revoke_default_privilege/new.sql +++ /dev/null @@ -1,19 +0,0 @@ --- Create role for testing -DO $$ -BEGIN - IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'app_user') THEN - CREATE ROLE app_user; - END IF; -END $$; - --- Default privileges grant SELECT, INSERT, UPDATE, DELETE on all new tables -ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO app_user; - --- Create a read-only table - user should only have SELECT, not INSERT/UPDATE/DELETE -CREATE TABLE readonly_data ( - id integer PRIMARY KEY, - value text -); - --- Revoke write privileges - keep only SELECT from the default grants -REVOKE INSERT, UPDATE, DELETE ON TABLE readonly_data FROM app_user; diff --git a/testdata/diff/privilege/partial_revoke_default_privilege/old.sql b/testdata/diff/privilege/partial_revoke_default_privilege/old.sql deleted file mode 100644 index a3f953c1..00000000 --- a/testdata/diff/privilege/partial_revoke_default_privilege/old.sql +++ /dev/null @@ -1,10 +0,0 @@ --- Create role for testing -DO $$ -BEGIN - IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'app_user') THEN - CREATE ROLE app_user; - END IF; -END $$; - --- Default privileges grant SELECT, INSERT, UPDATE, DELETE on all new tables -ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO app_user; diff --git a/testdata/diff/privilege/partial_revoke_default_privilege/plan.json b/testdata/diff/privilege/partial_revoke_default_privilege/plan.json deleted file mode 100644 index 945f5059..00000000 --- a/testdata/diff/privilege/partial_revoke_default_privilege/plan.json +++ /dev/null @@ -1,26 +0,0 @@ -{ - "version": "1.0.0", - "pgschema_version": "1.6.1", - "created_at": "1970-01-01T00:00:00Z", - "source_fingerprint": { - "hash": "4bac506d0b54d7b2808b01f4cd402d232bbe48221fd1acde17ecc0a4e13e8fe8" - }, - "groups": [ - { - "steps": [ - { - "sql": "CREATE TABLE IF NOT EXISTS readonly_data (\n id integer,\n value text,\n CONSTRAINT readonly_data_pkey PRIMARY KEY (id)\n);", - "type": "table", - "operation": "create", - "path": "public.readonly_data" - }, - { - "sql": "REVOKE DELETE, INSERT, UPDATE ON TABLE readonly_data FROM app_user;", - "type": "privilege", - "operation": "drop", - "path": "privileges.TABLE.readonly_data.app_user" - } - ] - } - ] -} diff --git a/testdata/diff/privilege/partial_revoke_default_privilege/plan.sql b/testdata/diff/privilege/partial_revoke_default_privilege/plan.sql deleted file mode 100644 index 1f79dd5a..00000000 --- a/testdata/diff/privilege/partial_revoke_default_privilege/plan.sql +++ /dev/null @@ -1,7 +0,0 @@ -CREATE TABLE IF NOT EXISTS readonly_data ( - id integer, - value text, - CONSTRAINT readonly_data_pkey PRIMARY KEY (id) -); - -REVOKE DELETE, INSERT, UPDATE ON TABLE readonly_data FROM app_user; diff --git a/testdata/diff/privilege/partial_revoke_default_privilege/plan.txt b/testdata/diff/privilege/partial_revoke_default_privilege/plan.txt deleted file mode 100644 index 69b927cd..00000000 --- a/testdata/diff/privilege/partial_revoke_default_privilege/plan.txt +++ /dev/null @@ -1,22 +0,0 @@ -Plan: 1 to add, 1 to drop. - -Summary by type: - tables: 1 to add - privileges: 1 to drop - -Tables: - + readonly_data - -Privileges: - - app_user - -DDL to be executed: --------------------------------------------------- - -CREATE TABLE IF NOT EXISTS readonly_data ( - id integer, - value text, - CONSTRAINT readonly_data_pkey PRIMARY KEY (id) -); - -REVOKE DELETE, INSERT, UPDATE ON TABLE readonly_data FROM app_user; diff --git a/testdata/diff/privilege/revoke_default_privilege/diff.sql b/testdata/diff/privilege/revoke_default_privilege/diff.sql index 54f569f7..31552756 100644 --- a/testdata/diff/privilege/revoke_default_privilege/diff.sql +++ b/testdata/diff/privilege/revoke_default_privilege/diff.sql @@ -1,7 +1,15 @@ +CREATE TABLE IF NOT EXISTS readonly_data ( + id integer, + value text, + CONSTRAINT readonly_data_pkey PRIMARY KEY (id) +); + CREATE TABLE IF NOT EXISTS secrets ( id integer, data text, CONSTRAINT secrets_pkey PRIMARY KEY (id) ); +REVOKE DELETE, INSERT, UPDATE ON TABLE readonly_data FROM app_user; + REVOKE SELECT ON TABLE secrets FROM reader; diff --git a/testdata/diff/privilege/revoke_default_privilege/new.sql b/testdata/diff/privilege/revoke_default_privilege/new.sql index d5e4969d..00d5b700 100644 --- a/testdata/diff/privilege/revoke_default_privilege/new.sql +++ b/testdata/diff/privilege/revoke_default_privilege/new.sql @@ -1,19 +1,34 @@ --- Create role for testing +-- Create roles for testing DO $$ BEGIN IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'reader') THEN CREATE ROLE reader; END IF; + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'app_user') THEN + CREATE ROLE app_user; + END IF; END $$; -- Default privileges grant SELECT on all new tables to reader ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT ON TABLES TO reader; --- Create a table that should NOT inherit the default SELECT privilege +-- Default privileges grant SELECT, INSERT, UPDATE, DELETE on all new tables to app_user +ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO app_user; + +-- Create a table that should NOT inherit any default privileges for reader (full revoke) CREATE TABLE secrets ( id integer PRIMARY KEY, data text ); --- Explicitly revoke the auto-granted default privilege +-- Explicitly revoke the auto-granted SELECT from reader REVOKE SELECT ON TABLE secrets FROM reader; + +-- Create a read-only table - app_user should only have SELECT, not INSERT/UPDATE/DELETE (partial revoke) +CREATE TABLE readonly_data ( + id integer PRIMARY KEY, + value text +); + +-- Revoke write privileges from app_user - keep only SELECT from the default grants +REVOKE INSERT, UPDATE, DELETE ON TABLE readonly_data FROM app_user; diff --git a/testdata/diff/privilege/revoke_default_privilege/old.sql b/testdata/diff/privilege/revoke_default_privilege/old.sql index 23ad6245..8b7f8e15 100644 --- a/testdata/diff/privilege/revoke_default_privilege/old.sql +++ b/testdata/diff/privilege/revoke_default_privilege/old.sql @@ -1,10 +1,16 @@ --- Create role for testing +-- Create roles for testing DO $$ BEGIN IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'reader') THEN CREATE ROLE reader; END IF; + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'app_user') THEN + CREATE ROLE app_user; + END IF; END $$; -- Default privileges grant SELECT on all new tables to reader ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT ON TABLES TO reader; + +-- Default privileges grant SELECT, INSERT, UPDATE, DELETE on all new tables to app_user +ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO app_user; diff --git a/testdata/diff/privilege/revoke_default_privilege/plan.json b/testdata/diff/privilege/revoke_default_privilege/plan.json index 7066c70e..898e7d19 100644 --- a/testdata/diff/privilege/revoke_default_privilege/plan.json +++ b/testdata/diff/privilege/revoke_default_privilege/plan.json @@ -3,17 +3,29 @@ "pgschema_version": "1.6.1", "created_at": "1970-01-01T00:00:00Z", "source_fingerprint": { - "hash": "f2bd42139d9b75b298bed35098f5d5a9205c335f6b02afff27c71913ec5210ec" + "hash": "60a01e6c2c18215bcce70b3d9d1153e7ca0371ef8e12252c152547462d1d92bd" }, "groups": [ { "steps": [ + { + "sql": "CREATE TABLE IF NOT EXISTS readonly_data (\n id integer,\n value text,\n CONSTRAINT readonly_data_pkey PRIMARY KEY (id)\n);", + "type": "table", + "operation": "create", + "path": "public.readonly_data" + }, { "sql": "CREATE TABLE IF NOT EXISTS secrets (\n id integer,\n data text,\n CONSTRAINT secrets_pkey PRIMARY KEY (id)\n);", "type": "table", "operation": "create", "path": "public.secrets" }, + { + "sql": "REVOKE DELETE, INSERT, UPDATE ON TABLE readonly_data FROM app_user;", + "type": "privilege", + "operation": "drop", + "path": "privileges.TABLE.readonly_data.app_user" + }, { "sql": "REVOKE SELECT ON TABLE secrets FROM reader;", "type": "privilege", diff --git a/testdata/diff/privilege/revoke_default_privilege/plan.sql b/testdata/diff/privilege/revoke_default_privilege/plan.sql index 54f569f7..31552756 100644 --- a/testdata/diff/privilege/revoke_default_privilege/plan.sql +++ b/testdata/diff/privilege/revoke_default_privilege/plan.sql @@ -1,7 +1,15 @@ +CREATE TABLE IF NOT EXISTS readonly_data ( + id integer, + value text, + CONSTRAINT readonly_data_pkey PRIMARY KEY (id) +); + CREATE TABLE IF NOT EXISTS secrets ( id integer, data text, CONSTRAINT secrets_pkey PRIMARY KEY (id) ); +REVOKE DELETE, INSERT, UPDATE ON TABLE readonly_data FROM app_user; + REVOKE SELECT ON TABLE secrets FROM reader; diff --git a/testdata/diff/privilege/revoke_default_privilege/plan.txt b/testdata/diff/privilege/revoke_default_privilege/plan.txt index 5290ef84..268e8b6a 100644 --- a/testdata/diff/privilege/revoke_default_privilege/plan.txt +++ b/testdata/diff/privilege/revoke_default_privilege/plan.txt @@ -1,22 +1,32 @@ -Plan: 1 to add, 1 to drop. +Plan: 2 to add, 2 to drop. Summary by type: - tables: 1 to add - privileges: 1 to drop + tables: 2 to add + privileges: 2 to drop Tables: + + readonly_data + secrets Privileges: + - app_user - reader DDL to be executed: -------------------------------------------------- +CREATE TABLE IF NOT EXISTS readonly_data ( + id integer, + value text, + CONSTRAINT readonly_data_pkey PRIMARY KEY (id) +); + CREATE TABLE IF NOT EXISTS secrets ( id integer, data text, CONSTRAINT secrets_pkey PRIMARY KEY (id) ); +REVOKE DELETE, INSERT, UPDATE ON TABLE readonly_data FROM app_user; + REVOKE SELECT ON TABLE secrets FROM reader; From 4da5f88362a4629816a3fbc6c9d030b8c1b6951a Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Mon, 26 Jan 2026 08:33:38 -0800 Subject: [PATCH 6/7] fix: exclude dropped default privileges from active defaults When computing activeDefaultPrivileges for GRANT/REVOKE filtering on new tables, exclude default privileges that are scheduled to be dropped. Since drops run before creates in the migration order, dropped defaults won't be in effect when new tables are created. This fixes a case where: 1. Old state has default privilege granting SELECT to role 2. New state drops that default privilege and creates a new table 3. Previously: would skip explicit GRANT (thinking default covers it) 4. Now: correctly generates explicit GRANT since default is dropped Co-Authored-By: Claude Opus 4.5 --- internal/diff/diff.go | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 36150b34..f54f49de 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -1107,14 +1107,28 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { // Build "active" default privileges - defaults that will be active when new tables are created. // This includes: - // 1. Old defaults (already on target database) - these apply immediately when table is created + // 1. Old defaults (already on target database) - these apply immediately when table is created, + // EXCEPT those scheduled to be dropped (drops run before creates) // 2. Added defaults (will be created BEFORE tables in our migration order) // NOT included: Modified defaults - the modification runs AFTER table creation, so the OLD // version is what's active when the table is created. The old defaults are already included. // See https://github.com/pgschema/pgschema/pull/257#pullrequestreview-3706696119 + + // Build a set of dropped default privilege keys for exclusion + droppedDefaultPrivKeys := make(map[string]bool) + for _, dp := range diff.droppedDefaultPrivileges { + key := dp.OwnerRole + ":" + string(dp.ObjectType) + ":" + dp.Grantee + droppedDefaultPrivKeys[key] = true + } + var activeDefaultPrivileges []*ir.DefaultPrivilege for _, dbSchema := range oldIR.Schemas { - activeDefaultPrivileges = append(activeDefaultPrivileges, dbSchema.DefaultPrivileges...) + for _, dp := range dbSchema.DefaultPrivileges { + key := dp.OwnerRole + ":" + string(dp.ObjectType) + ":" + dp.Grantee + if !droppedDefaultPrivKeys[key] { + activeDefaultPrivileges = append(activeDefaultPrivileges, dp) + } + } } activeDefaultPrivileges = append(activeDefaultPrivileges, diff.addedDefaultPrivileges...) From 6cdc180e3d71b2b84e9fca0dc35c233d9ae9c856 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Mon, 26 Jan 2026 08:51:18 -0800 Subject: [PATCH 7/7] Update internal/diff/privilege.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/diff/privilege.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/diff/privilege.go b/internal/diff/privilege.go index 4ccf6cb4..f53a5fb7 100644 --- a/internal/diff/privilege.go +++ b/internal/diff/privilege.go @@ -358,7 +358,7 @@ func privilegesCoveredBy(privs, coveringPrivs []string) bool { } // computeRevokedDefaultGrants finds privileges that would be auto-granted by default privileges -// on new objects, but should be explicitly revoked because the user didn't include them in the new state. +// on new tables, but should be explicitly revoked because the user didn't include them in the new state. // See https://github.com/pgschema/pgschema/issues/253 func computeRevokedDefaultGrants(addedTables []*ir.Table, newPrivs map[string]*ir.Privilege, defaultPrivileges []*ir.DefaultPrivilege) []*ir.Privilege { var revokedPrivs []*ir.Privilege