From 8d85b28bc2c7d0b502188b74055685dded351944 Mon Sep 17 00:00:00 2001 From: dilame Date: Sun, 27 Sep 2026 04:40:47 +0800 Subject: [PATCH] feat: detect column privilege drift Track per-column ACLs from pg_attribute.attacl so plans emit GRANT/REVOKE SELECT/INSERT/UPDATE/REFERENCES (column) statements. Column grants are emitted after CREATE TABLE for new tables and after ADD COLUMN for new columns. A column that is dropped takes its privileges with it, so no REVOKE is emitted for it. Constraint: Column privileges are stored in pg_attribute.attacl, separate from the table ACL in pg_class.relacl, so they need their own introspection query and schema field. Rejected: Folding column ACLs into the table privilege list | table and column privileges live in different catalogs and their SQL statements carry different column lists. Directive: Keep column privilege validation aligned with table privilege validation because privilege statements are skipped when roles are absent in the temp DB. Tested: go test ./... with PostgreSQL 17 server utilities and PostgreSQL 18 pg_dump for --restrict-key support --- .../column_privilege_cases_test.go | 231 ++++++++++++++++++ internal/queries/queries.sql | 47 ++++ internal/queries/queries.sql.go | 87 +++++++ internal/schema/schema.go | 78 +++++- internal/schema/schema_test.go | 13 +- pkg/diff/column_privilege_sql_generator.go | 80 ++++++ pkg/diff/plan_generator.go | 3 +- pkg/diff/privilege_sql_generator.go | 26 +- pkg/diff/sql_generator.go | 81 +++++- 9 files changed, 618 insertions(+), 28 deletions(-) create mode 100644 internal/migration_acceptance_tests/column_privilege_cases_test.go create mode 100644 pkg/diff/column_privilege_sql_generator.go diff --git a/internal/migration_acceptance_tests/column_privilege_cases_test.go b/internal/migration_acceptance_tests/column_privilege_cases_test.go new file mode 100644 index 00000000..18e66c99 --- /dev/null +++ b/internal/migration_acceptance_tests/column_privilege_cases_test.go @@ -0,0 +1,231 @@ +package migration_acceptance_tests + +import ( + "testing" + + "github.com/stripe/pg-schema-diff/pkg/diff" +) + +var columnPrivilegeAcceptanceTestCases = []acceptanceTestCase{ + { + name: "no-op", + roles: []string{ + "app_user", + }, + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar(id INT, data TEXT); + GRANT SELECT (id) ON foobar TO app_user; + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar(id INT, data TEXT); + GRANT SELECT (id) ON foobar TO app_user; + `, + }, + expectEmptyPlan: true, + }, + { + name: "Grant column privileges on new table (no hazards since table is new)", + roles: []string{"app_user"}, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar(id INT, data TEXT); + GRANT SELECT (data), UPDATE (data) ON foobar TO app_user; + `, + }, + // No hazards expected since table is brand new + }, + { + name: "Grant column privilege on existing table", + roles: []string{"app_user"}, + oldSchemaDDL: []string{ + `CREATE TABLE foobar(id INT, data TEXT);`, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar(id INT, data TEXT); + GRANT SELECT (data) ON foobar TO app_user; + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAuthzUpdate, + }, + }, + { + name: "Grant column privilege on multiple columns", + roles: []string{"app_user"}, + oldSchemaDDL: []string{ + `CREATE TABLE foobar(id INT, data TEXT, extra TEXT);`, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar(id INT, data TEXT, extra TEXT); + GRANT SELECT (data, extra) ON foobar TO app_user; + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAuthzUpdate, + }, + }, + { + name: "Revoke column privilege", + roles: []string{"app_user"}, + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar(id INT, data TEXT); + GRANT SELECT (data) ON foobar TO app_user; + `, + }, + newSchemaDDL: []string{ + `CREATE TABLE foobar(id INT, data TEXT);`, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAuthzUpdate, + }, + }, + { + name: "Grant column privilege on new column", + roles: []string{"app_user"}, + oldSchemaDDL: []string{ + `CREATE TABLE foobar(id INT);`, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar(id INT, data TEXT); + GRANT SELECT (data) ON foobar TO app_user; + `, + }, + // The column must be added before its privilege is granted. + expectedPlanDDL: []string{ + `ALTER TABLE "public"."foobar" ADD COLUMN "data" text COLLATE "pg_catalog"."default"`, + `GRANT SELECT ("data") ON "public"."foobar" TO "app_user"`, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAuthzUpdate, + }, + }, + { + name: "Drop column with column privilege (only DeletesData hazard)", + roles: []string{"app_user"}, + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar(id INT, data TEXT); + GRANT SELECT (data) ON foobar TO app_user; + `, + }, + newSchemaDDL: []string{ + `CREATE TABLE foobar(id INT);`, + }, + // The privilege is dropped together with the column, so no revoke is emitted. + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeDeletesData, + }, + }, + { + name: "Change column privilege GRANT OPTION (recreates privilege)", + roles: []string{"app_user"}, + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar(id INT, data TEXT); + GRANT SELECT (data) ON foobar TO app_user; + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar(id INT, data TEXT); + GRANT SELECT (data) ON foobar TO app_user WITH GRANT OPTION; + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAuthzUpdate, + }, + }, + { + name: "Remove column privilege GRANT OPTION (recreates privilege)", + roles: []string{"app_user"}, + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar(id INT, data TEXT); + GRANT SELECT (data) ON foobar TO app_user WITH GRANT OPTION; + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar(id INT, data TEXT); + GRANT SELECT (data) ON foobar TO app_user; + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAuthzUpdate, + }, + }, + { + name: "Column privilege on non-public schema table", + roles: []string{"app_user"}, + oldSchemaDDL: []string{ + ` + CREATE SCHEMA app_schema; + CREATE TABLE app_schema.foobar(id INT, data TEXT); + `, + }, + newSchemaDDL: []string{ + ` + CREATE SCHEMA app_schema; + CREATE TABLE app_schema.foobar(id INT, data TEXT); + GRANT SELECT (data) ON app_schema.foobar TO app_user; + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAuthzUpdate, + }, + }, + { + name: "Column privilege on new partition (not implemented)", + roles: []string{"app_user"}, + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + category TEXT + ) partition by list (category); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + category TEXT + ) partition by list (category); + CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('category'); + GRANT SELECT (category) ON foobar_1 TO app_user; + `, + }, + expectedPlanErrorIs: diff.ErrNotImplemented, + }, + { + name: "Add column privilege on existing partition (not implemented)", + roles: []string{"app_user"}, + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + category TEXT + ) partition by list (category); + CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('category'); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + category TEXT + ) partition by list (category); + CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('category'); + GRANT SELECT (category) ON foobar_1 TO app_user; + `, + }, + expectedPlanErrorIs: diff.ErrNotImplemented, + }, +} + +func TestColumnPrivilegeCases(t *testing.T) { + runTestCases(t, columnPrivilegeAcceptanceTestCases) +} diff --git a/internal/queries/queries.sql b/internal/queries/queries.sql index d8397c88..869445f8 100644 --- a/internal/queries/queries.sql +++ b/internal/queries/queries.sql @@ -666,3 +666,50 @@ LEFT JOIN pg_catalog.pg_roles AS grantee_role -- Exclude privileges granted to the table owner (these are implicit) WHERE pa.grantee_oid != pa.owner_oid OR pa.grantee_oid = 0 ORDER BY pa.table_schema_name, pa.table_name, grantee, pa.privilege_type; + +-- name: GetColumnPrivileges :many +WITH parsed_acl AS ( + SELECT + c.oid AS table_oid, + c.relname AS table_name, + n.nspname AS table_schema_name, + c.relowner AS owner_oid, + a.attname AS column_name, + (ACLEXPLODE(a.attacl)).grantee AS grantee_oid, + (ACLEXPLODE(a.attacl)).privilege_type AS privilege_type, + (ACLEXPLODE(a.attacl)).is_grantable AS is_grantable + FROM pg_catalog.pg_attribute AS a + INNER JOIN pg_catalog.pg_class AS c ON a.attrelid = c.oid + INNER JOIN pg_catalog.pg_namespace AS n ON c.relnamespace = n.oid + WHERE + n.nspname NOT IN ('pg_catalog', 'information_schema') + AND n.nspname !~ '^pg_toast' + AND n.nspname !~ '^pg_temp' + AND (c.relkind = 'r' OR c.relkind = 'p') + AND a.attacl IS NOT NULL + AND a.attnum > 0 + AND NOT a.attisdropped + -- Exclude tables owned by extensions + AND NOT EXISTS ( + SELECT depend.objid + FROM pg_catalog.pg_depend AS depend + WHERE + depend.classid = 'pg_class'::REGCLASS + AND depend.objid = c.oid + AND depend.deptype = 'e' + ) +) + +SELECT + pa.table_name::TEXT, + pa.table_schema_name::TEXT, + pa.column_name::TEXT, + COALESCE(grantee_role.rolname, '')::TEXT AS grantee, + pa.privilege_type::TEXT AS privilege, + pa.is_grantable +FROM parsed_acl AS pa +LEFT JOIN pg_catalog.pg_roles AS grantee_role + ON pa.grantee_oid = grantee_role.oid +-- Exclude privileges granted to the table owner (these are implicit) +WHERE pa.grantee_oid != pa.owner_oid OR pa.grantee_oid = 0 +ORDER BY pa.table_schema_name, pa.table_name, pa.column_name, grantee, pa.privilege_type; diff --git a/internal/queries/queries.sql.go b/internal/queries/queries.sql.go index 43151024..83decb57 100644 --- a/internal/queries/queries.sql.go +++ b/internal/queries/queries.sql.go @@ -88,6 +88,93 @@ func (q *Queries) GetCheckConstraints(ctx context.Context) ([]GetCheckConstraint return items, nil } +const getColumnPrivileges = `-- name: GetColumnPrivileges :many +WITH parsed_acl AS ( + SELECT + c.oid AS table_oid, + c.relname AS table_name, + n.nspname AS table_schema_name, + c.relowner AS owner_oid, + a.attname AS column_name, + (ACLEXPLODE(a.attacl)).grantee AS grantee_oid, + (ACLEXPLODE(a.attacl)).privilege_type AS privilege_type, + (ACLEXPLODE(a.attacl)).is_grantable AS is_grantable + FROM pg_catalog.pg_attribute AS a + INNER JOIN pg_catalog.pg_class AS c ON a.attrelid = c.oid + INNER JOIN pg_catalog.pg_namespace AS n ON c.relnamespace = n.oid + WHERE + n.nspname NOT IN ('pg_catalog', 'information_schema') + AND n.nspname !~ '^pg_toast' + AND n.nspname !~ '^pg_temp' + AND (c.relkind = 'r' OR c.relkind = 'p') + AND a.attacl IS NOT NULL + AND a.attnum > 0 + AND NOT a.attisdropped + -- Exclude tables owned by extensions + AND NOT EXISTS ( + SELECT depend.objid + FROM pg_catalog.pg_depend AS depend + WHERE + depend.classid = 'pg_class'::REGCLASS + AND depend.objid = c.oid + AND depend.deptype = 'e' + ) +) + +SELECT + pa.table_name::TEXT, + pa.table_schema_name::TEXT, + pa.column_name::TEXT, + COALESCE(grantee_role.rolname, '')::TEXT AS grantee, + pa.privilege_type::TEXT AS privilege, + pa.is_grantable +FROM parsed_acl AS pa +LEFT JOIN pg_catalog.pg_roles AS grantee_role + ON pa.grantee_oid = grantee_role.oid +WHERE pa.grantee_oid != pa.owner_oid OR pa.grantee_oid = 0 +ORDER BY pa.table_schema_name, pa.table_name, pa.column_name, grantee, pa.privilege_type +` + +type GetColumnPrivilegesRow struct { + PaTableName string + PaTableSchemaName string + PaColumnName string + Grantee string + Privilege string + IsGrantable interface{} +} + +// Exclude privileges granted to the table owner (these are implicit) +func (q *Queries) GetColumnPrivileges(ctx context.Context) ([]GetColumnPrivilegesRow, error) { + rows, err := q.db.QueryContext(ctx, getColumnPrivileges) + if err != nil { + return nil, err + } + defer rows.Close() + var items []GetColumnPrivilegesRow + for rows.Next() { + var i GetColumnPrivilegesRow + if err := rows.Scan( + &i.PaTableName, + &i.PaTableSchemaName, + &i.PaColumnName, + &i.Grantee, + &i.Privilege, + &i.IsGrantable, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const getColumnsForTable = `-- name: GetColumnsForTable :many WITH identity_col_seq AS ( SELECT diff --git a/internal/schema/schema.go b/internal/schema/schema.go index cc028e70..96ae18fb 100644 --- a/internal/schema/schema.go +++ b/internal/schema/schema.go @@ -134,6 +134,7 @@ func normalizeTable(t Table) Table { t.Policies = normPolicies t.Privileges = sortSchemaObjectsByName(t.Privileges) + t.ColumnPrivileges = sortSchemaObjectsByName(t.ColumnPrivileges) return t } @@ -218,6 +219,7 @@ type Table struct { CheckConstraints []CheckConstraint Policies []Policy Privileges []TablePrivilege + ColumnPrivileges []ColumnPrivilege ReplicaIdentity ReplicaIdentity RLSEnabled bool RLSForced bool @@ -260,6 +262,26 @@ func (p TablePrivilege) GetName() string { return fmt.Sprintf("%s:%s", grantee, p.Privilege) } +// ColumnPrivilege represents a privilege granted on a single column of a table. +type ColumnPrivilege struct { + // ColumnName is the name of the column the privilege is granted on. + ColumnName string + // Grantee is the role that has the privilege. Empty string means PUBLIC. + Grantee string + // Privilege is the type of privilege (SELECT, INSERT, UPDATE, REFERENCES) + Privilege string + // IsGrantable indicates if the grantee can grant this privilege to others (WITH GRANT OPTION) + IsGrantable bool +} + +func (p ColumnPrivilege) GetName() string { + grantee := p.Grantee + if grantee == "" { + grantee = "PUBLIC" + } + return fmt.Sprintf("%s:%s:%s", p.ColumnName, grantee, p.Privilege) +} + type ColumnIdentityType string const ( @@ -957,12 +979,21 @@ func (s *schemaFetcher) fetchTables(ctx context.Context) ([]Table, error) { privilegesByTable[p.table.GetFQEscapedName()] = append(privilegesByTable[p.table.GetFQEscapedName()], p.privilege) } + columnPrivileges, err := s.fetchColumnPrivileges(ctx) + if err != nil { + return nil, fmt.Errorf("fetchColumnPrivileges(): %w", err) + } + columnPrivilegesByTable := make(map[string][]ColumnPrivilege) + for _, p := range columnPrivileges { + columnPrivilegesByTable[p.table.GetFQEscapedName()] = append(columnPrivilegesByTable[p.table.GetFQEscapedName()], p.privilege) + } + goroutineRunner := s.goroutineRunnerFactory() var tableFutures []concurrent.Future[Table] for _, _rawTable := range rawTables { rawTable := _rawTable // Capture loop variables for go routine tableFuture, err := concurrent.SubmitFuture(ctx, goroutineRunner, func() (Table, error) { - return s.buildTable(ctx, rawTable, checkConsByTable, policiesByTable, privilegesByTable) + return s.buildTable(ctx, rawTable, checkConsByTable, policiesByTable, privilegesByTable, columnPrivilegesByTable) }) if err != nil { return nil, fmt.Errorf("starting table future: %w", err) @@ -991,6 +1022,7 @@ func (s *schemaFetcher) buildTable( checkConsByTable map[string][]CheckConstraint, policiesByTable map[string][]Policy, privilegesByTable map[string][]TablePrivilege, + columnPrivilegesByTable map[string][]ColumnPrivilege, ) (Table, error) { rawColumns, err := s.q.GetColumnsForTable(ctx, table.Oid) if err != nil { @@ -1056,6 +1088,7 @@ func (s *schemaFetcher) buildTable( CheckConstraints: checkConsByTable[schemaQualifiedName.GetFQEscapedName()], Policies: policiesByTable[schemaQualifiedName.GetFQEscapedName()], Privileges: privilegesByTable[schemaQualifiedName.GetFQEscapedName()], + ColumnPrivileges: columnPrivilegesByTable[schemaQualifiedName.GetFQEscapedName()], ReplicaIdentity: ReplicaIdentity(table.ReplicaIdentity), RLSEnabled: table.RlsEnabled, RLSForced: table.RlsForced, @@ -1381,6 +1414,11 @@ type privilegeAndTable struct { table SchemaQualifiedName } +type columnPrivilegeAndTable struct { + privilege ColumnPrivilege + table SchemaQualifiedName +} + func (s *schemaFetcher) fetchPolicies(ctx context.Context) ([]policyAndTable, error) { rawPolicies, err := s.q.GetPolicies(ctx) if err != nil { @@ -1454,6 +1492,44 @@ func (s *schemaFetcher) fetchPrivileges(ctx context.Context) ([]privilegeAndTabl return privileges, nil } +func (s *schemaFetcher) fetchColumnPrivileges(ctx context.Context) ([]columnPrivilegeAndTable, error) { + rawPrivileges, err := s.q.GetColumnPrivileges(ctx) + if err != nil { + return nil, fmt.Errorf("GetColumnPrivileges: %w", err) + } + + var privileges []columnPrivilegeAndTable + for _, rp := range rawPrivileges { + // Handle the is_grantable field which may be returned as interface{} + isGrantable := false + if rp.IsGrantable != nil { + if b, ok := rp.IsGrantable.(bool); ok { + isGrantable = b + } + } + + privileges = append(privileges, columnPrivilegeAndTable{ + privilege: ColumnPrivilege{ + ColumnName: rp.PaColumnName, + Grantee: rp.Grantee, + Privilege: rp.Privilege, + IsGrantable: isGrantable, + }, + table: buildNameFromUnescaped(rp.PaTableName, rp.PaTableSchemaName), + }) + } + + privileges = filterSliceByName( + privileges, + func(p columnPrivilegeAndTable) SchemaQualifiedName { + return p.table + }, + s.nameFilter, + ) + + return privileges, nil +} + func (s *schemaFetcher) fetchTriggers(ctx context.Context) ([]Trigger, error) { rawTriggers, err := s.q.GetTriggers(ctx) if err != nil { diff --git a/internal/schema/schema_test.go b/internal/schema/schema_test.go index c73bb177..8686084a 100644 --- a/internal/schema/schema_test.go +++ b/internal/schema/schema_test.go @@ -238,8 +238,12 @@ var ( -- Add table privileges to test they are fetched correctly GRANT SELECT ON schema_2.foo TO some_role_1; GRANT INSERT ON schema_2.foo TO some_role_2 WITH GRANT OPTION; + + -- Add column privileges to test they are fetched correctly + GRANT SELECT (content, author) ON schema_2.foo TO some_role_1; + GRANT UPDATE (content) ON schema_2.foo TO some_role_2 WITH GRANT OPTION; `}, - expectedHash: "4c2174e2cac3956b", + expectedHash: "405b089b1715f8cc", expectedSchema: Schema{ NamedSchemas: []NamedSchema{ {Name: "public"}, @@ -319,6 +323,11 @@ var ( {Grantee: "some_role_2", Privilege: "INSERT", IsGrantable: true}, {Grantee: "some_role_1", Privilege: "SELECT", IsGrantable: false}, }, + ColumnPrivileges: []ColumnPrivilege{ + {ColumnName: "author", Grantee: "some_role_1", Privilege: "SELECT", IsGrantable: false}, + {ColumnName: "content", Grantee: "some_role_1", Privilege: "SELECT", IsGrantable: false}, + {ColumnName: "content", Grantee: "some_role_2", Privilege: "UPDATE", IsGrantable: true}, + }, ReplicaIdentity: ReplicaIdentityIndex, RLSEnabled: true, }, @@ -591,7 +600,7 @@ var ( ALTER TABLE foo_fk_1 ADD CONSTRAINT foo_fk_1_fk FOREIGN KEY (author, content) REFERENCES foo_1 (author, content) NOT VALID; `}, - expectedHash: "32c5a9c52dcfb15e", + expectedHash: "dfaa4acd2e7077d0", expectedSchema: Schema{ NamedSchemas: []NamedSchema{ {Name: "public"}, diff --git a/pkg/diff/column_privilege_sql_generator.go b/pkg/diff/column_privilege_sql_generator.go new file mode 100644 index 00000000..5133b9ea --- /dev/null +++ b/pkg/diff/column_privilege_sql_generator.go @@ -0,0 +1,80 @@ +package diff + +import ( + "fmt" + + "github.com/stripe/pg-schema-diff/internal/schema" +) + +// columnPrivilegeSQLVertexGenerator generates GRANT/REVOKE statements for privileges granted on individual +// columns of a single table. +type columnPrivilegeSQLVertexGenerator struct { + tableName schema.SchemaQualifiedName +} + +func newColumnPrivilegeSQLVertexGenerator(tableName schema.SchemaQualifiedName) sqlVertexGenerator[schema.ColumnPrivilege, columnPrivilegeDiff] { + return legacyToNewSqlVertexGenerator[schema.ColumnPrivilege, columnPrivilegeDiff](&columnPrivilegeSQLVertexGenerator{ + tableName: tableName, + }) +} + +func (cpg *columnPrivilegeSQLVertexGenerator) Add(p schema.ColumnPrivilege) ([]Statement, error) { + ddl := fmt.Sprintf("GRANT %s (%s) ON %s TO %s", + p.Privilege, + schema.EscapeIdentifier(p.ColumnName), + cpg.tableName.GetFQEscapedName(), + privilegeGranteeSQL(p.Grantee), + ) + if p.IsGrantable { + ddl += " WITH GRANT OPTION" + } + + return []Statement{{ + DDL: ddl, + Timeout: statementTimeoutDefault, + LockTimeout: lockTimeoutDefault, + Hazards: []MigrationHazard{migrationHazardPrivilegeGranted}, + SkipValidation: true, + }}, nil +} + +func (cpg *columnPrivilegeSQLVertexGenerator) Delete(p schema.ColumnPrivilege) ([]Statement, error) { + ddl := fmt.Sprintf("REVOKE %s (%s) ON %s FROM %s", + p.Privilege, + schema.EscapeIdentifier(p.ColumnName), + cpg.tableName.GetFQEscapedName(), + privilegeGranteeSQL(p.Grantee), + ) + + return []Statement{{ + DDL: ddl, + Timeout: statementTimeoutDefault, + LockTimeout: lockTimeoutDefault, + Hazards: []MigrationHazard{migrationHazardPrivilegeRevoked}, + SkipValidation: true, + }}, nil +} + +func (cpg *columnPrivilegeSQLVertexGenerator) Alter(diff columnPrivilegeDiff) ([]Statement, error) { + // Column privileges don't support ALTER - if IsGrantable changes, the privilege is recreated + // (handled via requiresRecreation in buildTableDiff). + return nil, nil +} + +func (cpg *columnPrivilegeSQLVertexGenerator) GetSQLVertexId(p schema.ColumnPrivilege, diffType diffType) sqlVertexId { + return buildSchemaObjVertexId("column_privilege", + fmt.Sprintf("%s.%s", cpg.tableName.GetFQEscapedName(), p.GetName()), diffType) +} + +func (cpg *columnPrivilegeSQLVertexGenerator) GetAddAlterDependencies(newPriv, _ schema.ColumnPrivilege) ([]dependency, error) { + return []dependency{ + // Ensure delete runs before add/alter (for recreate scenarios) + mustRun(cpg.GetSQLVertexId(newPriv, diffTypeDelete)).before(cpg.GetSQLVertexId(newPriv, diffTypeAddAlter)), + // The column must exist before a privilege can be granted on it + mustRun(buildColumnVertexId(newPriv.ColumnName, diffTypeAddAlter)).before(cpg.GetSQLVertexId(newPriv, diffTypeAddAlter)), + }, nil +} + +func (cpg *columnPrivilegeSQLVertexGenerator) GetDeleteDependencies(_ schema.ColumnPrivilege) ([]dependency, error) { + return nil, nil +} diff --git a/pkg/diff/plan_generator.go b/pkg/diff/plan_generator.go index 2c2c92b8..6d638469 100644 --- a/pkg/diff/plan_generator.go +++ b/pkg/diff/plan_generator.go @@ -279,12 +279,13 @@ func schemaFromTempDb(ctx context.Context, db *tempdb.Database, plan *planOption return schema.GetSchema(ctx, db.ConnPool, append(plan.getSchemaOpts, db.ExcludeMetadataOptions...)...) } -// clearTablePrivileges returns a copy of the schema with all table privileges cleared. +// clearTablePrivileges returns a copy of the schema with all table and column privileges cleared. // This is used during plan validation because privilege statements are skipped (roles don't exist in temp DB). func clearTablePrivileges(s schema.Schema) schema.Schema { tables := make([]schema.Table, len(s.Tables)) for i, t := range s.Tables { t.Privileges = nil + t.ColumnPrivileges = nil tables[i] = t } s.Tables = tables diff --git a/pkg/diff/privilege_sql_generator.go b/pkg/diff/privilege_sql_generator.go index d851ca79..61b3c9b7 100644 --- a/pkg/diff/privilege_sql_generator.go +++ b/pkg/diff/privilege_sql_generator.go @@ -17,6 +17,14 @@ var ( } ) +// privilegeGranteeSQL returns the SQL representation of a privilege grantee. An empty grantee means PUBLIC. +func privilegeGranteeSQL(grantee string) string { + if grantee == "" { + return "PUBLIC" + } + return schema.EscapeIdentifier(grantee) +} + type privilegeSQLVertexGenerator struct { tableName schema.SchemaQualifiedName } @@ -28,14 +36,7 @@ func newPrivilegeSQLVertexGenerator(tableName schema.SchemaQualifiedName) sqlVer } func (psg *privilegeSQLVertexGenerator) Add(p schema.TablePrivilege) ([]Statement, error) { - grantee := p.Grantee - if grantee == "" { - grantee = "PUBLIC" - } else { - grantee = schema.EscapeIdentifier(grantee) - } - - ddl := fmt.Sprintf("GRANT %s ON %s TO %s", p.Privilege, psg.tableName.GetFQEscapedName(), grantee) + ddl := fmt.Sprintf("GRANT %s ON %s TO %s", p.Privilege, psg.tableName.GetFQEscapedName(), privilegeGranteeSQL(p.Grantee)) if p.IsGrantable { ddl += " WITH GRANT OPTION" } @@ -50,14 +51,7 @@ func (psg *privilegeSQLVertexGenerator) Add(p schema.TablePrivilege) ([]Statemen } func (psg *privilegeSQLVertexGenerator) Delete(p schema.TablePrivilege) ([]Statement, error) { - grantee := p.Grantee - if grantee == "" { - grantee = "PUBLIC" - } else { - grantee = schema.EscapeIdentifier(grantee) - } - - ddl := fmt.Sprintf("REVOKE %s ON %s FROM %s", p.Privilege, psg.tableName.GetFQEscapedName(), grantee) + ddl := fmt.Sprintf("REVOKE %s ON %s FROM %s", p.Privilege, psg.tableName.GetFQEscapedName(), privilegeGranteeSQL(p.Grantee)) return []Statement{{ DDL: ddl, diff --git a/pkg/diff/sql_generator.go b/pkg/diff/sql_generator.go index 0e930c41..d818c5c0 100644 --- a/pkg/diff/sql_generator.go +++ b/pkg/diff/sql_generator.go @@ -112,12 +112,17 @@ type ( oldAndNew[schema.TablePrivilege] } + columnPrivilegeDiff struct { + oldAndNew[schema.ColumnPrivilege] + } + tableDiff struct { oldAndNew[schema.Table] - columnsDiff listDiff[schema.Column, columnDiff] - checkConstraintDiff listDiff[schema.CheckConstraint, checkConstraintDiff] - policiesDiff listDiff[schema.Policy, policyDiff] - privilegesDiff listDiff[schema.TablePrivilege, privilegeDiff] + columnsDiff listDiff[schema.Column, columnDiff] + checkConstraintDiff listDiff[schema.CheckConstraint, checkConstraintDiff] + policiesDiff listDiff[schema.Policy, policyDiff] + privilegesDiff listDiff[schema.TablePrivilege, privilegeDiff] + columnPrivilegesDiff listDiff[schema.ColumnPrivilege, columnPrivilegeDiff] } indexDiff struct { @@ -442,18 +447,53 @@ func buildTableDiff(oldTable, newTable schema.Table, _, _ int) (diff tableDiff, return tableDiff{}, false, fmt.Errorf("diffing privileges: %w", err) } + columnPrivilegesDiff, err := diffLists( + oldTable.ColumnPrivileges, + newTable.ColumnPrivileges, + func(old, new schema.ColumnPrivilege, _, _ int) (columnPrivilegeDiff, bool, error) { + // Recreate the privilege if IsGrantable changes + recreate := old.IsGrantable != new.IsGrantable + return columnPrivilegeDiff{oldAndNew[schema.ColumnPrivilege]{old: old, new: new}}, recreate, nil + }, + ) + if err != nil { + return tableDiff{}, false, fmt.Errorf("diffing column privileges: %w", err) + } + columnPrivilegesDiff.deletes = pruneColumnPrivilegesForDroppedColumns(columnPrivilegesDiff.deletes, newTable) + return tableDiff{ oldAndNew: oldAndNew[schema.Table]{ old: oldTable, new: newTable, }, - columnsDiff: columnsDiff, - checkConstraintDiff: checkConsDiff, - policiesDiff: policiesDiff, - privilegesDiff: privilegesDiff, + columnsDiff: columnsDiff, + checkConstraintDiff: checkConsDiff, + policiesDiff: policiesDiff, + privilegesDiff: privilegesDiff, + columnPrivilegesDiff: columnPrivilegesDiff, }, false, nil } +// pruneColumnPrivilegesForDroppedColumns removes privileges belonging to columns that are absent from the +// new schema. Such privileges are dropped together with their column, so emitting a REVOKE for them would +// be unnecessary (and would fail, since the column no longer exists at that point). +func pruneColumnPrivilegesForDroppedColumns(deletes []schema.ColumnPrivilege, newTable schema.Table) []schema.ColumnPrivilege { + if len(deletes) == 0 { + return deletes + } + columnsInNewTable := make(map[string]bool, len(newTable.Columns)) + for _, column := range newTable.Columns { + columnsInNewTable[column.Name] = true + } + var kept []schema.ColumnPrivilege + for _, p := range deletes { + if columnsInNewTable[p.ColumnName] { + kept = append(kept, p) + } + } + return kept +} + type indexDiffConfig struct { newSchemaTablesByName map[string]schema.Table addedTablesByName map[string]schema.Table @@ -812,6 +852,9 @@ func (t *tableSQLVertexGenerator) Add(table schema.Table) ([]Statement, error) { if len(table.Privileges) > 0 { return nil, fmt.Errorf("privileges on partitions: %w", ErrNotImplemented) } + if len(table.ColumnPrivileges) > 0 { + return nil, fmt.Errorf("column privileges on partitions: %w", ErrNotImplemented) + } // We attach the partitions separately. So the partition must have all the same check constraints // as the original table table.CheckConstraints = append(table.CheckConstraints, t.tablesInNewSchemaByName[table.ParentTable.GetName()].CheckConstraints...) @@ -895,6 +938,16 @@ func (t *tableSQLVertexGenerator) Add(table schema.Table) ([]Statement, error) { stmts = append(stmts, stripMigrationHazards(addPrivilegeStmts...)...) } + columnPrivilegeGenerator := &columnPrivilegeSQLVertexGenerator{tableName: table.SchemaQualifiedName} + for _, privilege := range table.ColumnPrivileges { + addPrivilegeStmts, err := columnPrivilegeGenerator.Add(privilege) + if err != nil { + return nil, fmt.Errorf("generating add column privilege statements for privilege %s: %w", privilege.GetName(), err) + } + // Remove hazards from statements since the table is brand new + stmts = append(stmts, stripMigrationHazards(addPrivilegeStmts...)...) + } + return stmts, nil } @@ -1042,6 +1095,13 @@ func (t *tableSQLVertexGenerator) alterBaseTable(diff tableDiff) ([]Statement, e } partialGraph = concatPartialGraphs(partialGraph, privilegesPartialGraph) + columnPrivilegeGenerator := newColumnPrivilegeSQLVertexGenerator(diff.new.SchemaQualifiedName) + columnPrivilegesPartialGraph, err := generatePartialGraph(columnPrivilegeGenerator, diff.columnPrivilegesDiff) + if err != nil { + return nil, fmt.Errorf("resolving column privilege sql: %w", err) + } + partialGraph = concatPartialGraphs(partialGraph, columnPrivilegesPartialGraph) + graph, err := graphFromPartials(partialGraph) if err != nil { return nil, fmt.Errorf("converting to graph") @@ -1077,6 +1137,11 @@ func (t *tableSQLVertexGenerator) alterPartition(diff tableDiff) ([]Statement, e // _independent_ of how it is ordered. return nil, fmt.Errorf("privileges on partitions: %w", ErrNotImplemented) } + if !diff.columnPrivilegesDiff.isEmpty() { + // Column privilege diffing on individual partitions cannot be supported until where a SQL statement is + // generated is _independent_ of how it is ordered. + return nil, fmt.Errorf("column privileges on partitions: %w", ErrNotImplemented) + } var alteredParentColumnsByName map[string]columnDiff if parentDiff, ok := t.tableDiffsByName[diff.new.ParentTable.GetName()]; ok {