fix: revoke default privilege grants on new tables#257
Conversation
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 <noreply@anthropic.com>
tianzhou
force-pushed
the
default_privilege
branch
from
034d0e0 to
b543284
Compare
There was a problem hiding this comment.
Pull request overview
Fixes migration planning so that when PostgreSQL default privileges would auto-grant privileges on a newly created table, but the desired end state does not include those privileges (because they were explicitly revoked in the desired schema), the plan includes the needed REVOKE in the same apply cycle (Issue #253).
Changes:
- Add diff logic to synthesize “revoke default-granted privileges on newly created tables” steps after table creation.
- Wire the new revoke steps into the create-phase SQL generation ordering (after tables, before explicit GRANTs).
- Add a new file-based diff test case covering the one-cycle convergence scenario.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
internal/diff/privilege.go |
Adds computeRevokedDefaultGrants to generate synthetic privilege revokes for default-granted privileges on new tables. |
internal/diff/diff.go |
Stores and emits the new revoke steps during create-phase generation. |
testdata/diff/privilege/revoke_default_privilege/old.sql |
Test setup: default privileges granting table access to a role. |
testdata/diff/privilege/revoke_default_privilege/new.sql |
Desired state: create table + explicit revoke for the default-granted role + grant to admin role. |
testdata/diff/privilege/revoke_default_privilege/diff.sql |
Expected generated SQL now includes the missing REVOKE. |
testdata/diff/privilege/revoke_default_privilege/plan.txt |
Human-readable plan output updated to include the REVOKE step. |
testdata/diff/privilege/revoke_default_privilege/plan.sql |
Raw plan SQL output updated to include the REVOKE step. |
testdata/diff/privilege/revoke_default_privilege/plan.json |
JSON plan output updated to include the REVOKE step. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
computeRevokedDefaultGrants currently scans all keys in newPrivs for every (new table × default privilege) pair. For schemas with many privileges this becomes unnecessarily expensive and makes the logic harder to follow. Consider precomputing an index like map[objectKey][]*ir.Privilege (or at least a map[objectKey]bool) once, and doing O(1) lookups inside this loop.
- 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Create default privileges BEFORE tables so auto-grants apply to new tables | ||
| generateCreateDefaultPrivilegesSQL(d.addedDefaultPrivileges, targetSchema, collector) | ||
|
|
There was a problem hiding this comment.
generateCreateSQL creates added default privileges before tables, but default privilege modifications still run later in generateModifySQL (after CREATE TABLE). Since ALTER DEFAULT PRIVILEGES is not retroactive, any tables created in the same migration will inherit the old defaults if a default privilege is modified, which can leave newly created tables with broader or narrower access than the desired state until a follow-up apply (security window). Consider ensuring the effective default privileges (including modifications relevant to TABLES) are applied before creating new tables, or avoiding relying on default privileges for new-table privilege convergence when defaults are being modified in the same migration.
- 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: #257 (review) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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 | ||
| // 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 existing, ok := privSetByObjectKey[key]; ok { | ||
| // Merge privileges from both entries | ||
| for _, priv := range p.Privileges { | ||
| existing[priv] = true |
There was a problem hiding this comment.
computeRevokedDefaultGrants merges desired privileges by GetObjectKey() (which intentionally ignores WithGrantOption). If a default privilege would auto-grant e.g. SELECT WITH GRANT OPTION on a new table, but the desired state only has SELECT (no grant option), this function will treat SELECT as “present” and won’t generate the required REVOKE GRANT OPTION FOR SELECT ... after table creation. That leaves the new table with an unintended grant option and likely reintroduces the “two apply cycles to converge” behavior for grant-option changes. Consider tracking grantable vs non-grantable privileges separately (e.g., keyed by GetFullKey() or splitting sets by WithGrantOption) and emitting a post-create REVOKE GRANT OPTION FOR when the only mismatch is the grant option.
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Compute which default privileges need to be revoked | ||
| // (privileges in dp.Privileges but not in existingPrivSet) | ||
| var privsToRevoke []string | ||
| if existingPrivSet == nil { | ||
| // No explicit privilege exists - revoke all default privileges | ||
| privsToRevoke = dp.Privileges | ||
| } else { | ||
| // Compute set difference: dp.Privileges - existingPrivSet | ||
| for _, p := range dp.Privileges { | ||
| if !existingPrivSet[p] { | ||
| privsToRevoke = append(privsToRevoke, p) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 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: privsToRevoke, | ||
| WithGrantOption: dp.WithGrantOption, | ||
| }) | ||
| } |
There was a problem hiding this comment.
computeRevokedDefaultGrants merges desired privileges across grant-option variants (keyed by GetObjectKey), and only computes missing privilege types. This means a new table that inherits default privileges WITH GRANT OPTION but whose desired state removes only the grant option (via REVOKE GRANT OPTION FOR ...) will not get a corresponding revoke step, and may still require a second apply (and leave the grant option enabled). Consider extending this logic to detect grant-option-only differences for auto-granted privileges on new tables and emit a REVOKE GRANT OPTION FOR statement (e.g., by tracking desired privileges separately by WithGrantOption and generating an alter-style privilege diff).
| // Compute which default privileges need to be revoked | |
| // (privileges in dp.Privileges but not in existingPrivSet) | |
| var privsToRevoke []string | |
| if existingPrivSet == nil { | |
| // No explicit privilege exists - revoke all default privileges | |
| privsToRevoke = dp.Privileges | |
| } else { | |
| // Compute set difference: dp.Privileges - existingPrivSet | |
| for _, p := range dp.Privileges { | |
| if !existingPrivSet[p] { | |
| privsToRevoke = append(privsToRevoke, p) | |
| } | |
| } | |
| } | |
| 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: privsToRevoke, | |
| WithGrantOption: dp.WithGrantOption, | |
| }) | |
| } | |
| // Compute which default privileges need to be revoked. | |
| // We distinguish between: | |
| // - full revokes (privilege type should not exist at all), and | |
| // - grant-option-only revokes (privilege type should exist, but | |
| // the auto-granted WITH GRANT OPTION from default privileges | |
| // should be removed). | |
| var fullRevokes []string | |
| var grantOptionOnlyRevokes []string | |
| if existingPrivSet == nil { | |
| // No explicit privilege exists - revoke all default privileges fully. | |
| fullRevokes = dp.Privileges | |
| } else { | |
| for _, p := range dp.Privileges { | |
| if !existingPrivSet[p] { | |
| // Privilege type is not desired at all: full revoke. | |
| fullRevokes = append(fullRevokes, p) | |
| } else if dp.WithGrantOption { | |
| // Privilege type is desired, but the default privilege | |
| // would auto-grant WITH GRANT OPTION. Since the desired | |
| // state only guarantees the presence of the privilege type | |
| // (tracked via existingPrivSet) and not the grant option, | |
| // emit a separate revoke of the grant option. | |
| grantOptionOnlyRevokes = append(grantOptionOnlyRevokes, p) | |
| } | |
| } | |
| } | |
| if len(fullRevokes) > 0 { | |
| // Create a synthetic privilege to fully revoke the missing default grants. | |
| revokedPrivs = append(revokedPrivs, &ir.Privilege{ | |
| ObjectType: ir.PrivilegeObjectTypeTable, | |
| ObjectName: table.Name, | |
| Grantee: dp.Grantee, | |
| Privileges: fullRevokes, | |
| WithGrantOption: false, | |
| }) | |
| } | |
| if len(grantOptionOnlyRevokes) > 0 { | |
| // Create a synthetic privilege to revoke only the grant option | |
| // for default privileges that would otherwise auto-grant it. | |
| revokedPrivs = append(revokedPrivs, &ir.Privilege{ | |
| ObjectType: ir.PrivilegeObjectTypeTable, | |
| ObjectName: table.Name, | |
| Grantee: dp.Grantee, | |
| Privileges: grantOptionOnlyRevokes, | |
| WithGrantOption: true, | |
| }) | |
| } |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 participant
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.
Fix #253