Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion internal/diff/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -1104,10 +1104,21 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff {
}
}

// 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)
}
}
Comment on lines +1107 to 1122
Copy link

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}

Expand Down
62 changes: 62 additions & 0 deletions internal/diff/privilege.go
Original file line number Diff line number Diff line change
Expand Up @@ -294,3 +294,65 @@ func privilegesEqual(old, new *ir.Privilege) bool {
func (d *privilegeDiff) GetObjectName() string {
return d.New.GetObjectKey()
}

// isPrivilegeCoveredByDefaultPrivileges checks if an explicit privilege is covered
// by default privileges in the desired state. This is used to avoid generating
// spurious REVOKE statements for privileges that are auto-granted via default privileges.
// See https://github.com/pgschema/pgschema/issues/250
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
}
Comment on lines +302 to +325
Copy link

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Add an Owner field to the Privilege struct and check it against dp.OwnerRole
  2. Look up the object owner from the oldIR when checking if a privilege should be dropped
  3. Document this as a known limitation if checking ownership is intentionally omitted for simplification

Copilot uses AI. Check for mistakes.

// privilegeObjectTypeMatchesDefault checks if a privilege object type matches
// a default privilege object type (e.g., TABLE matches TABLES)
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
}
}
Comment on lines +329 to +344
Copy link

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

// privilegesCoveredBy checks if all privileges in 'privs' are covered by 'coveringPrivs'
func privilegesCoveredBy(privs, coveringPrivs []string) bool {
coveringSet := make(map[string]bool)
for _, p := range coveringPrivs {
coveringSet[p] = true
}
for _, p := range privs {
if !coveringSet[p] {
return false
}
}
return true
}
Empty file.
32 changes: 32 additions & 0 deletions testdata/diff/default_privilege/auto_grant_idempotent/new.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
-- https://github.com/pgschema/pgschema/issues/250
--
-- Test: Privileges covered by default privileges should not be revoked.
--
-- This represents the desired state as written in the user's SQL files.
-- The user declares default privileges and creates objects, but does NOT
-- include explicit GRANTs because they expect PostgreSQL to auto-grant them.
--
-- The diff should NOT generate REVOKE statements because the privileges
-- in old.sql are covered by the default privileges defined here.

DO $$
BEGIN
IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'app_role') THEN
CREATE ROLE app_role;
END IF;
IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'owner_role') THEN
CREATE ROLE owner_role;
END IF;
END $$;

-- Default privileges for owner_role (same as old state)
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 (same as old state)
CREATE TABLE users (
id serial PRIMARY KEY,
name text NOT NULL
);
Comment on lines +26 to +30
Copy link

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
-- 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;

Copilot uses AI. Check for mistakes.

-- No explicit GRANTs - covered by default privileges above
36 changes: 36 additions & 0 deletions testdata/diff/default_privilege/auto_grant_idempotent/old.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
-- https://github.com/pgschema/pgschema/issues/250
--
-- Test: Privileges covered by default privileges should not be revoked.
--
-- This test simulates the scenario where:
-- 1. Default privileges are configured for owner_role
-- 2. Objects were created by owner_role, so privileges were auto-granted
-- 3. The explicit GRANTs below represent those auto-granted privileges
--
-- In a real database, these grants would be created automatically by PostgreSQL
-- when owner_role creates objects. Here we simulate this by adding explicit GRANTs.

DO $$
BEGIN
IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'app_role') THEN
CREATE ROLE app_role;
END IF;
IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'owner_role') THEN
CREATE ROLE owner_role;
END IF;
END $$;

-- Default privileges for owner_role
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
);

-- 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;
Comment on lines +27 to +36
Copy link

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"version": "1.0.0",
"pgschema_version": "1.6.1",
"created_at": "1970-01-01T00:00:00Z",
"source_fingerprint": {
"hash": "16119adc01274eb2d4d4fdb177740d5176d82ff4330f7605e3c08a5926b8e734"
},
"groups": null
}
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
No changes detected.
Loading