fix: skip revoking privileges covered by default privileges (#250) #251
Conversation
When default privileges are set for a role, objects created by that role automatically inherit those privileges. Previously, pgschema would incorrectly generate REVOKE statements for these auto-granted privileges because they appeared in the current state but not explicitly in the desired state SQL files. This fix adds logic to check if a privilege being dropped is covered by a matching default privilege in the desired state. If the privilege's object type, grantee, grant option, and privilege types are all covered by a default privilege, the REVOKE is skipped. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This pull request fixes an issue where pgschema incorrectly generated REVOKE statements for privileges that are automatically granted through default privileges. The fix adds logic to check if a privilege being dropped is covered by a matching default privilege in the desired state, and skips generating the REVOKE statement if it is.
Changes:
- Added three helper functions in privilege.go to check if explicit privileges are covered by default privileges
- Modified the privilege comparison logic in diff.go to skip REVOKE statements for privileges covered by default privileges
- Added a test case to verify that privileges matching default privileges don't generate spurious REVOKE statements
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/diff/privilege.go | Added isPrivilegeCoveredByDefaultPrivileges, privilegeObjectTypeMatchesDefault, and privilegesCoveredBy helper functions to detect when explicit privileges are covered by default privileges |
| internal/diff/diff.go | Modified GenerateMigration to collect default privileges from new IR and skip dropped privileges that are covered by default privileges |
| testdata/diff/default_privilege/auto_grant_idempotent/old.sql | Test case with default privileges and matching explicit grants |
| testdata/diff/default_privilege/auto_grant_idempotent/new.sql | Test case with default privileges but no explicit grants |
| testdata/diff/default_privilege/auto_grant_idempotent/plan.txt | Expected output showing no changes detected |
| testdata/diff/default_privilege/auto_grant_idempotent/plan.json | Test metadata |
| testdata/diff/default_privilege/auto_grant_idempotent/plan.sql | Empty expected diff SQL file |
| testdata/diff/default_privilege/auto_grant_idempotent/diff.sql | Empty actual diff SQL file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -- Table and sequence (same as old state) | ||
| CREATE TABLE users ( | ||
| id serial PRIMARY KEY, | ||
| name text NOT NULL | ||
| ); |
There was a problem hiding this comment.
The test case doesn't accurately reflect real PostgreSQL behavior. The table and sequence are created without using SET ROLE owner_role, which means they will be owned by the current database user (typically postgres or testuser), not owner_role. Default privileges defined with "FOR ROLE owner_role" only apply to objects created BY owner_role.
The test should include:
- SET ROLE owner_role; before creating the table and sequence
- RESET ROLE; after creating them
Or alternatively, the objects should be created with explicit ownership:
- ALTER TABLE users OWNER TO owner_role;
- ALTER SEQUENCE users_id_seq OWNER TO owner_role;
Without this, the privileges on the users table in a real PostgreSQL database would NOT be auto-granted by the default privileges, making this test inaccurate.
| -- Table and sequence (same as old state) | |
| CREATE TABLE users ( | |
| id serial PRIMARY KEY, | |
| name text NOT NULL | |
| ); | |
| -- Table and sequence (same as old state) | |
| SET ROLE owner_role; | |
| CREATE TABLE users ( | |
| id serial PRIMARY KEY, | |
| name text NOT NULL | |
| ); | |
| RESET ROLE; |
| // 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 | ||
| for _, dbSchema := range newIR.Schemas { | ||
| newDefaultPrivileges = append(newDefaultPrivileges, dbSchema.DefaultPrivileges...) | ||
| } | ||
|
|
||
| // 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 { | ||
| if !matchedOld[fullKey] { | ||
| diff.droppedPrivileges = append(diff.droppedPrivileges, p) | ||
| // Check if this privilege is covered by a default privilege | ||
| if !isPrivilegeCoveredByDefaultPrivileges(p, newDefaultPrivileges) { | ||
| diff.droppedPrivileges = append(diff.droppedPrivileges, p) | ||
| } | ||
| } |
There was a problem hiding this comment.
The code collects default privileges from all schemas in the new IR (lines 1110-1112) but doesn't check if the privilege's schema matches the default privilege's schema when calling isPrivilegeCoveredByDefaultPrivileges. This could lead to incorrect matches where a privilege in schema A is incorrectly considered covered by a default privilege in schema B.
The Privilege struct doesn't have a schema field, and the DefaultPrivilege struct also doesn't have a schema field. While privileges are queried per schema and stored in each schema's Privileges slice, when checking coverage, we need to ensure we're only matching default privileges from the same schema as the privilege being checked.
A possible fix would be to pass the schema context to isPrivilegeCoveredByDefaultPrivileges or filter the default privileges list to only include those from the same schema before calling the function.
| func isPrivilegeCoveredByDefaultPrivileges(p *ir.Privilege, defaultPrivileges []*ir.DefaultPrivilege) bool { | ||
| for _, dp := range defaultPrivileges { | ||
| // Match object types (TABLE -> TABLES, SEQUENCE -> SEQUENCES, etc.) | ||
| if !privilegeObjectTypeMatchesDefault(p.ObjectType, dp.ObjectType) { | ||
| continue | ||
| } | ||
|
|
||
| // Match grantee | ||
| if p.Grantee != dp.Grantee { | ||
| continue | ||
| } | ||
|
|
||
| // Match grant option | ||
| if p.WithGrantOption != dp.WithGrantOption { | ||
| continue | ||
| } | ||
|
|
||
| // Check if all privilege types are covered by the default privilege | ||
| if privilegesCoveredBy(p.Privileges, dp.Privileges) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
The isPrivilegeCoveredByDefaultPrivileges function is missing a critical check for object ownership. In PostgreSQL, default privileges only apply to objects created by the specific owner role specified in the ALTER DEFAULT PRIVILEGES statement. Without checking whether the object is actually owned by the OwnerRole from the DefaultPrivilege, this function may incorrectly skip REVOKE statements for privileges that are NOT actually covered by default privileges.
For example, if owner_role has a default privilege to grant SELECT to app_role, but a table is owned by a different role (e.g., postgres), the default privilege does not apply to that table. The current implementation would incorrectly consider the privilege as covered and skip the REVOKE statement.
To fix this, you need to either:
- Add an Owner field to the Privilege struct and check it against dp.OwnerRole
- Look up the object owner from the oldIR when checking if a privilege should be dropped
- Document this as a known limitation if checking ownership is intentionally omitted for simplification
| func privilegeObjectTypeMatchesDefault(privType ir.PrivilegeObjectType, defaultType ir.DefaultPrivilegeObjectType) bool { | ||
| switch privType { | ||
| case ir.PrivilegeObjectTypeTable: | ||
| return defaultType == ir.DefaultPrivilegeObjectTypeTables | ||
| case ir.PrivilegeObjectTypeSequence: | ||
| return defaultType == ir.DefaultPrivilegeObjectTypeSequences | ||
| case ir.PrivilegeObjectTypeFunction: | ||
| return defaultType == ir.DefaultPrivilegeObjectTypeFunctions | ||
| case ir.PrivilegeObjectTypeProcedure: | ||
| return defaultType == ir.DefaultPrivilegeObjectTypeFunctions // Procedures use FUNCTIONS default | ||
| case ir.PrivilegeObjectTypeType: | ||
| return defaultType == ir.DefaultPrivilegeObjectTypeTypes | ||
| default: | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
The privilegeObjectTypeMatchesDefault function doesn't handle all object types that can have privileges. Specifically, PrivilegeObjectTypeView is not handled in the switch statement. While views may not have corresponding default privilege types in PostgreSQL (default privileges apply to TABLES, which includes base tables and views), this should be documented or the case should be explicitly handled to avoid confusion. Consider adding a comment explaining why VIEW is not matched or adding an explicit case that returns false with a comment.
| -- Default privileges | ||
| ALTER DEFAULT PRIVILEGES FOR ROLE owner_role IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO app_role; | ||
| ALTER DEFAULT PRIVILEGES FOR ROLE owner_role IN SCHEMA public GRANT USAGE ON SEQUENCES TO app_role; | ||
|
|
||
| -- Table and sequence | ||
| CREATE TABLE users ( | ||
| id serial PRIMARY KEY, | ||
| name text NOT NULL | ||
| ); | ||
|
|
||
| -- Explicit grants that match the default privileges above | ||
| GRANT SELECT, INSERT, UPDATE, DELETE ON TABLE users TO app_role; | ||
| GRANT USAGE ON SEQUENCE users_id_seq TO app_role; |
There was a problem hiding this comment.
The test case doesn't accurately reflect real PostgreSQL behavior. The table and sequence are created without using SET ROLE owner_role, which means they will be owned by the current database user (typically postgres or testuser), not owner_role. Default privileges defined with "FOR ROLE owner_role" only apply to objects created BY owner_role.
The test should include:
- SET ROLE owner_role; before creating the table and sequence
- RESET ROLE; after creating them
Or alternatively, the objects should be created with explicit ownership:
- ALTER TABLE users OWNER TO owner_role;
- ALTER SEQUENCE users_id_seq OWNER TO owner_role;
Without this, the privileges on the users table in a real PostgreSQL database would NOT be auto-granted by the default privileges, making this test inaccurate.
Address PR review feedback by adding detailed comments explaining: - The test simulates auto-granted privileges using explicit GRANTs - Why this approach is used (testing diff logic, not PostgreSQL behavior) - What scenario the test represents (re-applying same schema) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -- Table and sequence | ||
| CREATE TABLE users ( | ||
| id serial PRIMARY KEY, | ||
| name text NOT NULL | ||
| ); | ||
|
|
||
| -- Simulate auto-granted privileges (what PostgreSQL would grant automatically | ||
| -- when owner_role creates objects with the above default privileges) | ||
| GRANT SELECT, INSERT, UPDATE, DELETE ON TABLE users TO app_role; | ||
| GRANT USAGE ON SEQUENCE users_id_seq TO app_role; |
There was a problem hiding this comment.
The test case may not properly validate the owner matching requirement for default privileges. The table 'users' is created without an explicit owner (line 28-31), so it will be owned by the database user executing the SQL (typically 'postgres' in tests), not 'owner_role'. However, the default privileges are set FOR ROLE owner_role (line 24).
In a real PostgreSQL database, default privileges FOR ROLE owner_role only apply to objects created by owner_role. Since the test doesn't set the table owner to owner_role (e.g., via ALTER TABLE users OWNER TO owner_role), this test may not accurately represent the scenario described in the comments.
Consider adding ALTER TABLE users OWNER TO owner_role and ALTER SEQUENCE users_id_seq OWNER TO owner_role after creating the objects to ensure the test properly validates the owner matching behavior.
…) (pgplex#251) * fix: skip revoking privileges covered by default privileges (pgplex#250) When default privileges are set for a role, objects created by that role automatically inherit those privileges. Previously, pgschema would incorrectly generate REVOKE statements for these auto-granted privileges because they appeared in the current state but not explicitly in the desired state SQL files. This fix adds logic to check if a privilege being dropped is covered by a matching default privilege in the desired state. If the privilege's object type, grantee, grant option, and privilege types are all covered by a default privilege, the REVOKE is skipped. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * docs: improve test case documentation for default privilege coverage Address PR review feedback by adding detailed comments explaining: - The test simulates auto-granted privileges using explicit GRANTs - Why this approach is used (testing diff logic, not PostgreSQL behavior) - What scenario the test represents (re-applying same schema) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
When default privileges are set for a role, objects created by that role automatically inherit those privileges. Previously, pgschema would incorrectly generate REVOKE statements for these auto-granted privileges because they appeared in the current state but not explicitly in the desired state SQL files.
This fix adds logic to check if a privilege being dropped is covered by a matching default privilege in the desired state. If the privilege's object type, grantee, grant option, and privilege types are all covered by a default privilege, the REVOKE is skipped.