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
81 changes: 67 additions & 14 deletions internal/diff/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1097,20 +1098,60 @@ 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
for _, dbSchema := range newIR.Schemas {
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,
// 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 {
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...)

// 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 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 active default privileges, skip it
// The default privileges will auto-grant when the table is created
if newTableNames[p.ObjectName] && isPrivilegeCoveredByDefaultPrivileges(p, activeDefaultPrivileges) {
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 {
Expand All @@ -1122,6 +1163,13 @@ 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, activeDefaultPrivileges)

// Sort privileges for deterministic output
sort.Slice(diff.addedPrivileges, func(i, j int) bool {
return diff.addedPrivileges[i].GetObjectKey() < diff.addedPrivileges[j].GetObjectKey()
Expand Down Expand Up @@ -1432,6 +1480,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)

Comment on lines +1483 to +1485
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
// 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{}
Expand Down Expand Up @@ -1475,8 +1526,10 @@ 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
generateDropPrivilegesSQL(d.revokedDefaultGrantsOnNewTables, targetSchema, collector)

// Create explicit object privileges
generateCreatePrivilegesSQL(d.addedPrivileges, targetSchema, collector)
Expand Down
74 changes: 74 additions & 0 deletions internal/diff/privilege.go
Original file line number Diff line number Diff line change
Expand Up @@ -356,3 +356,77 @@ func privilegesCoveredBy(privs, coveringPrivs []string) bool {
}
return true
}

// computeRevokedDefaultGrants finds privileges that would be auto-granted by default privileges
// 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

// 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
Comment on lines +366 to +375
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}
} else {
privSet := make(map[string]bool)
for _, priv := range p.Privileges {
privSet[priv] = true
}
privSetByObjectKey[key] = privSet
}
}

// 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
}

// Look up explicit privilege for this exact (table, grantee) pair
objectKey := string(ir.PrivilegeObjectTypeTable) + ":" + table.Name + ":" + dp.Grantee
existingPrivSet := privSetByObjectKey[objectKey]

// 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)
}
}
}
Comment on lines 386 to 411
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

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,
})
}
Comment on lines +398 to +422
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

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).

Suggested change
// 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,
})
}

Copilot uses AI. Check for mistakes.
}
}

// Sort for deterministic output
sort.Slice(revokedPrivs, func(i, j int) bool {
return revokedPrivs[i].GetObjectKey() < revokedPrivs[j].GetObjectKey()
})

return revokedPrivs
}
6 changes: 6 additions & 0 deletions testdata/diff/default_privilege/add_table_privilege/diff.sql
Original file line number Diff line number Diff line change
@@ -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)
);
6 changes: 6 additions & 0 deletions testdata/diff/default_privilege/add_table_privilege/new.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
);
6 changes: 6 additions & 0 deletions testdata/diff/default_privilege/add_table_privilege/plan.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
]
}
Expand Down
6 changes: 6 additions & 0 deletions testdata/diff/default_privilege/add_table_privilege/plan.sql
Original file line number Diff line number Diff line change
@@ -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)
);
12 changes: 11 additions & 1 deletion testdata/diff/default_privilege/add_table_privilege/plan.txt
Original file line number Diff line number Diff line change
@@ -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)
);
15 changes: 15 additions & 0 deletions testdata/diff/privilege/revoke_default_privilege/diff.sql
Original file line number Diff line number Diff line change
@@ -0,0 +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;
34 changes: 34 additions & 0 deletions testdata/diff/privilege/revoke_default_privilege/new.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
-- 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;

-- 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 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;
16 changes: 16 additions & 0 deletions testdata/diff/privilege/revoke_default_privilege/old.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
-- 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;
38 changes: 38 additions & 0 deletions testdata/diff/privilege/revoke_default_privilege/plan.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
{
"version": "1.0.0",
"pgschema_version": "1.6.1",
"created_at": "1970-01-01T00:00:00Z",
"source_fingerprint": {
"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",
"operation": "drop",
"path": "privileges.TABLE.secrets.reader"
}
]
}
]
}
15 changes: 15 additions & 0 deletions testdata/diff/privilege/revoke_default_privilege/plan.sql
Original file line number Diff line number Diff line change
@@ -0,0 +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;
Loading