From a650254c318a234e5a8c4a942b387228f8950a67 Mon Sep 17 00:00:00 2001 From: dilame Date: Sat, 6 Jun 2026 05:20:37 +0800 Subject: [PATCH] Detect routine execute privilege drift Track function and procedure EXECUTE ACLs from pg_proc, including PostgreSQL's implicit PUBLIC EXECUTE default, so plans can revoke leaked routine access and grant the declared role-specific access. Constraint: PostgreSQL stores default routine EXECUTE grants as NULL proacl, so schema fetch must expand ACLDEFAULT('f', owner). Rejected: Treat has_function_privilege() as the diff source | it reports effective inherited access instead of declarative ACL state. Confidence: high Scope-risk: moderate Directive: Keep routine privilege validation aligned with table privilege validation because privilege statements are skipped when roles are absent in temp DB. Tested: go test ./... with PostgreSQL 17 server utilities and PostgreSQL 18 pg_dump for --restrict-key support --- .../privilege_cases_test.go | 33 ++++++++ internal/queries/queries.sql | 14 +++- internal/queries/queries.sql.go | 16 +++- internal/schema/schema.go | 53 +++++++++++-- internal/schema/schema_test.go | 11 ++- pkg/diff/function_sql_vertex_generator.go | 42 ++++++++-- pkg/diff/plan_generator.go | 23 +++++- pkg/diff/privilege_sql_generator.go | 77 ++++++++++++++++--- pkg/diff/procedure_sql_vertex_generator.go | 63 ++++++++++----- pkg/diff/sql_generator.go | 50 +++++++----- pkg/diff/sql_vertex_generator.go | 12 +++ 11 files changed, 326 insertions(+), 68 deletions(-) diff --git a/internal/migration_acceptance_tests/privilege_cases_test.go b/internal/migration_acceptance_tests/privilege_cases_test.go index 27bc3f8..1de2326 100644 --- a/internal/migration_acceptance_tests/privilege_cases_test.go +++ b/internal/migration_acceptance_tests/privilege_cases_test.go @@ -156,6 +156,39 @@ var privilegeAcceptanceTestCases = []acceptanceTestCase{ diff.MigrationHazardTypeAuthzUpdate, }, }, + { + name: "Revoke implicit PUBLIC EXECUTE and grant function EXECUTE to role", + roles: []string{"app_user"}, + oldSchemaDDL: []string{ + ` + CREATE SCHEMA welcome; + CREATE FUNCTION welcome.available_deposit_ordinal() RETURNS integer + LANGUAGE SQL + RETURN 1; + CREATE FUNCTION welcome.on_deposit(deposit_id uuid) RETURNS void + LANGUAGE SQL + AS $$ SELECT $$; + `, + }, + newSchemaDDL: []string{ + ` + CREATE SCHEMA welcome; + CREATE FUNCTION welcome.available_deposit_ordinal() RETURNS integer + LANGUAGE SQL + RETURN 1; + REVOKE EXECUTE ON FUNCTION welcome.available_deposit_ordinal() FROM PUBLIC; + GRANT EXECUTE ON FUNCTION welcome.available_deposit_ordinal() TO app_user; + + CREATE FUNCTION welcome.on_deposit(deposit_id uuid) RETURNS void + LANGUAGE SQL + AS $$ SELECT $$; + REVOKE EXECUTE ON FUNCTION welcome.on_deposit(uuid) FROM PUBLIC; + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeAuthzUpdate, + }, + }, { name: "Grant on partitioned parent table", roles: []string{"app_user"}, diff --git a/internal/queries/queries.sql b/internal/queries/queries.sql index d8397c8..ed37d91 100644 --- a/internal/queries/queries.sql +++ b/internal/queries/queries.sql @@ -266,7 +266,19 @@ SELECT pg_catalog.pg_get_function_identity_arguments( pg_proc.oid ) AS func_identity_arguments, - pg_catalog.pg_get_functiondef(pg_proc.oid) AS func_def + pg_catalog.pg_get_functiondef(pg_proc.oid) AS func_def, + ARRAY( + SELECT json_build_object( + 'grantee', COALESCE(grantee_role.rolname, ''), + 'privilege', acl.privilege_type, + 'is_grantable', acl.is_grantable + )::TEXT + FROM ACLEXPLODE(COALESCE(pg_proc.proacl, ACLDEFAULT('f', pg_proc.proowner))) AS acl + LEFT JOIN pg_catalog.pg_roles AS grantee_role + ON acl.grantee = grantee_role.oid + WHERE acl.grantee != pg_proc.proowner OR acl.grantee = 0 + ORDER BY COALESCE(grantee_role.rolname, ''), acl.privilege_type + ) AS privileges FROM pg_catalog.pg_proc INNER JOIN pg_catalog.pg_namespace AS proc_namespace diff --git a/internal/queries/queries.sql.go b/internal/queries/queries.sql.go index 4315102..06106c1 100644 --- a/internal/queries/queries.sql.go +++ b/internal/queries/queries.sql.go @@ -800,7 +800,19 @@ SELECT pg_catalog.pg_get_function_identity_arguments( pg_proc.oid ) AS func_identity_arguments, - pg_catalog.pg_get_functiondef(pg_proc.oid) AS func_def + pg_catalog.pg_get_functiondef(pg_proc.oid) AS func_def, + ARRAY( + SELECT json_build_object( + 'grantee', COALESCE(grantee_role.rolname, ''), + 'privilege', acl.privilege_type, + 'is_grantable', acl.is_grantable + )::TEXT + FROM ACLEXPLODE(COALESCE(pg_proc.proacl, ACLDEFAULT('f', pg_proc.proowner))) AS acl + LEFT JOIN pg_catalog.pg_roles AS grantee_role + ON acl.grantee = grantee_role.oid + WHERE acl.grantee != pg_proc.proowner OR acl.grantee = 0 + ORDER BY COALESCE(grantee_role.rolname, ''), acl.privilege_type + ) AS privileges FROM pg_catalog.pg_proc INNER JOIN pg_catalog.pg_namespace AS proc_namespace @@ -831,6 +843,7 @@ type GetProcsRow struct { FuncLang string FuncIdentityArguments string FuncDef string + Privileges []string } func (q *Queries) GetProcs(ctx context.Context, prokind interface{}) ([]GetProcsRow, error) { @@ -849,6 +862,7 @@ func (q *Queries) GetProcs(ctx context.Context, prokind interface{}) ([]GetProcs &i.FuncLang, &i.FuncIdentityArguments, &i.FuncDef, + pq.Array(&i.Privileges), ); err != nil { return nil, err } diff --git a/internal/schema/schema.go b/internal/schema/schema.go index b3945d5..27f25a1 100644 --- a/internal/schema/schema.go +++ b/internal/schema/schema.go @@ -86,11 +86,17 @@ func (s Schema) Normalize() Schema { var normFunctions []Function for _, function := range sortSchemaObjectsByName(s.Functions) { function.DependsOnFunctions = sortSchemaObjectsByName(function.DependsOnFunctions) + function.Privileges = sortSchemaObjectsByName(function.Privileges) normFunctions = append(normFunctions, function) } s.Functions = normFunctions - s.Procedures = sortSchemaObjectsByName(s.Procedures) + var normProcedures []Procedure + for _, procedure := range sortSchemaObjectsByName(s.Procedures) { + procedure.Privileges = sortSchemaObjectsByName(procedure.Privileges) + normProcedures = append(normProcedures, procedure) + } + s.Procedures = normProcedures s.Triggers = sortSchemaObjectsByName(s.Triggers) var normViews []View @@ -242,17 +248,17 @@ func (t Table) IsPartition() bool { return t.ParentTable != nil } -// TablePrivilege represents a privilege granted on a table -type TablePrivilege struct { +// Privilege represents a privilege granted on a schema object. +type Privilege struct { // Grantee is the role that has the privilege. Empty string means PUBLIC. Grantee string - // Privilege is the type of privilege (SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, TRIGGER) + // Privilege is the type of privilege (SELECT, EXECUTE, etc.) Privilege string // IsGrantable indicates if the grantee can grant this privilege to others (WITH GRANT OPTION) IsGrantable bool } -func (p TablePrivilege) GetName() string { +func (p Privilege) GetName() string { grantee := p.Grantee if grantee == "" { grantee = "PUBLIC" @@ -260,6 +266,9 @@ func (p TablePrivilege) GetName() string { return fmt.Sprintf("%s:%s", grantee, p.Privilege) } +// TablePrivilege represents a privilege granted on a table. +type TablePrivilege = Privilege + type ColumnIdentityType string const ( @@ -449,6 +458,7 @@ type Function struct { // can track the dependencies of the function (or not) Language string DependsOnFunctions []SchemaQualifiedName + Privileges []Privilege } type Procedure struct { @@ -456,7 +466,8 @@ type Procedure struct { // Def is the statement required to completely (re)create // the procedure, as returned by `pg_get_functiondef`. It is a CREATE OR REPLACE // statement. - Def string + Def string + Privileges []Privilege } var ( @@ -1319,12 +1330,17 @@ func (s *schemaFetcher) buildFunction(ctx context.Context, rawFunction queries.G if err != nil { return Function{}, fmt.Errorf("fetchDependsOnFunctions(%s): %w", rawFunction.Oid, err) } + privileges, err := parseJSONPrivileges(rawFunction.Privileges) + if err != nil { + return Function{}, fmt.Errorf("parseJSONPrivileges(%s): %w", rawFunction.Oid, err) + } return Function{ SchemaQualifiedName: buildProcName(rawFunction.FuncName, rawFunction.FuncIdentityArguments, rawFunction.FuncSchemaName), FunctionDef: rawFunction.FuncDef, Language: rawFunction.FuncLang, DependsOnFunctions: dependsOnFunctions, + Privileges: privileges, }, nil } @@ -1353,9 +1369,14 @@ func (s *schemaFetcher) fetchProcedures(ctx context.Context) ([]Procedure, error var procedures []Procedure for _, rawProcedure := range rawProcedures { + privileges, err := parseJSONPrivileges(rawProcedure.Privileges) + if err != nil { + return nil, fmt.Errorf("parseJSONPrivileges(%s): %w", rawProcedure.Oid, err) + } p := Procedure{ SchemaQualifiedName: buildProcName(rawProcedure.FuncName, rawProcedure.FuncIdentityArguments, rawProcedure.FuncSchemaName), Def: rawProcedure.FuncDef, + Privileges: privileges, } procedures = append(procedures, p) } @@ -1583,6 +1604,26 @@ func parseJSONTableDependencies(vals []string) ([]TableDependency, error) { return out, nil } +func parseJSONPrivileges(vals []string) ([]Privilege, error) { + var out []Privilege + for _, v := range vals { + var p struct { + Grantee string `json:"grantee"` + Privilege string `json:"privilege"` + IsGrantable bool `json:"is_grantable"` + } + if err := json.Unmarshal([]byte(v), &p); err != nil { + return nil, fmt.Errorf("json.Unmarshal(%q, Privilege): %w", string(v), err) + } + out = append(out, Privilege{ + Grantee: p.Grantee, + Privilege: p.Privilege, + IsGrantable: p.IsGrantable, + }) + } + return out, nil +} + // buildProcName is used to build the schema qualified name for a proc (function, procedure), i.e., anything // identified by a name AND its arguments. func buildProcName(name, identityArguments, schemaName string) SchemaQualifiedName { diff --git a/internal/schema/schema_test.go b/internal/schema/schema_test.go index c73bb17..658e298 100644 --- a/internal/schema/schema_test.go +++ b/internal/schema/schema_test.go @@ -34,6 +34,7 @@ var ( EscapedName: `"C"`, SchemaName: "pg_catalog", } + defaultExecutePrivileges = []Privilege{{Privilege: "EXECUTE"}} testCases = []*testCase{ // Exclude materialized views from the test for now because Postgres 14-15 fully qualify column names while Postgres @@ -239,7 +240,7 @@ var ( GRANT SELECT ON schema_2.foo TO some_role_1; GRANT INSERT ON schema_2.foo TO some_role_2 WITH GRANT OPTION; `}, - expectedHash: "4c2174e2cac3956b", + expectedHash: "228b09af8d579433", expectedSchema: Schema{ NamedSchemas: []NamedSchema{ {Name: "public"}, @@ -466,26 +467,31 @@ var ( {EscapedName: "\"add\"(a integer, b integer)", SchemaName: "schema_filtered_1"}, {EscapedName: "\"increment\"(i integer)", SchemaName: "schema_1"}, }, + Privileges: defaultExecutePrivileges, }, { SchemaQualifiedName: SchemaQualifiedName{EscapedName: "\"increment\"(i integer)", SchemaName: "schema_1"}, FunctionDef: "CREATE OR REPLACE FUNCTION schema_1.increment(i integer)\n RETURNS integer\n LANGUAGE plpgsql\nAS $function$\n\t\t\t\t\tBEGIN\n\t\t\t\t\t\t\tRETURN i + 1;\n\t\t\t\t\tEND;\n\t\t\t$function$\n", Language: "plpgsql", + Privileges: defaultExecutePrivileges, }, { SchemaQualifiedName: SchemaQualifiedName{EscapedName: "\"increment_version\"()", SchemaName: "public"}, FunctionDef: "CREATE OR REPLACE FUNCTION public.increment_version()\n RETURNS trigger\n LANGUAGE plpgsql\nAS $function$\n\t\t\t\tBEGIN\n\t\t\t\t\tNEW.version = OLD.version + 1;\n\t\t\t\t\tRETURN NEW;\n\t\t\t\tEND;\n\t\t\t$function$\n", Language: "plpgsql", + Privileges: defaultExecutePrivileges, }, }, Procedures: []Procedure{ { SchemaQualifiedName: SchemaQualifiedName{SchemaName: "public", EscapedName: "\"some_plpgsql_procedure\"(IN foobar numeric)"}, Def: "CREATE OR REPLACE PROCEDURE public.some_plpgsql_procedure(IN foobar numeric)\n LANGUAGE plpgsql\nAS $procedure$\n\t\t\t\tBEGIN\n\t\t\t\t\tRAISE NOTICE 'some notice';\n\t\t\t\tEND\n\t\t\t\t$procedure$\n", + Privileges: defaultExecutePrivileges, }, { SchemaQualifiedName: SchemaQualifiedName{SchemaName: "schema_2", EscapedName: "\"some_insert_procedure\"(IN a integer, IN b integer)"}, Def: "CREATE OR REPLACE PROCEDURE schema_2.some_insert_procedure(IN a integer, IN b integer)\n LANGUAGE sql\nBEGIN ATOMIC\n INSERT INTO schema_2.foo DEFAULT VALUES;\nEND\n", + Privileges: defaultExecutePrivileges, }, }, Triggers: []Trigger{ @@ -591,7 +597,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: "85d661d829e05ae1", expectedSchema: Schema{ NamedSchemas: []NamedSchema{ {Name: "public"}, @@ -881,6 +887,7 @@ var ( SchemaQualifiedName: SchemaQualifiedName{EscapedName: "\"increment_version\"()", SchemaName: "public"}, FunctionDef: "CREATE OR REPLACE FUNCTION public.increment_version()\n RETURNS trigger\n LANGUAGE plpgsql\nAS $function$\n\t\t\t\tBEGIN\n\t\t\t\t\tNEW.version = OLD.version + 1;\n\t\t\t\t\tRETURN NEW;\n\t\t\t\tEND;\n\t\t\t$function$\n", Language: "plpgsql", + Privileges: defaultExecutePrivileges, }, }, Triggers: []Trigger{ diff --git a/pkg/diff/function_sql_vertex_generator.go b/pkg/diff/function_sql_vertex_generator.go index 088ff6a..16fa8e4 100644 --- a/pkg/diff/function_sql_vertex_generator.go +++ b/pkg/diff/function_sql_vertex_generator.go @@ -30,12 +30,21 @@ func (f *functionSQLVertexGenerator) Add(function schema.Function) ([]Statement, "created/altered before this statement.", }) } - return []Statement{{ + stmts := []Statement{{ DDL: function.FunctionDef, Timeout: statementTimeoutDefault, LockTimeout: lockTimeoutDefault, Hazards: hazards, - }}, nil + }} + privilegeStmts, err := exactRoutinePrivilegeStatements( + newFunctionPrivilegeSQLVertexGenerator(function.SchemaQualifiedName), + function.Privileges, + ) + if err != nil { + return nil, fmt.Errorf("generating function privilege statements: %w", err) + } + stmts = append(stmts, privilegeStmts...) + return stmts, nil } func (f *functionSQLVertexGenerator) Delete(function schema.Function) ([]Statement, error) { @@ -60,10 +69,33 @@ func (f *functionSQLVertexGenerator) Delete(function schema.Function) ([]Stateme func (f *functionSQLVertexGenerator) Alter(diff functionDiff) ([]Statement, error) { // We are assuming the function has been normalized, i.e., we don't have to worry DependsOnFunctions ordering // causing a false positive diff detected. - if cmp.Equal(diff.old, diff.new) { - return nil, nil + oldWithoutPrivileges := diff.old + oldWithoutPrivileges.Privileges = nil + newWithoutPrivileges := diff.new + newWithoutPrivileges.Privileges = nil + + var stmts []Statement + if !cmp.Equal(oldWithoutPrivileges, newWithoutPrivileges) { + addStmts, err := f.Add(diff.new) + if err != nil { + return nil, err + } + stmts = append(stmts, addStmts...) + } else { + privilegesPartialGraph, err := generatePartialGraph( + newFunctionPrivilegeSQLVertexGenerator(diff.new.SchemaQualifiedName), + diff.privilegesDiff, + ) + if err != nil { + return nil, fmt.Errorf("resolving function privilege sql: %w", err) + } + privilegeStmts, err := graphStatements(privilegesPartialGraph) + if err != nil { + return nil, fmt.Errorf("ordering function privilege sql: %w", err) + } + stmts = append(stmts, privilegeStmts...) } - return f.Add(diff.new) + return stmts, nil } func canFunctionDependenciesBeTracked(function schema.Function) bool { diff --git a/pkg/diff/plan_generator.go b/pkg/diff/plan_generator.go index 2c2c92b..762cd51 100644 --- a/pkg/diff/plan_generator.go +++ b/pkg/diff/plan_generator.go @@ -279,23 +279,38 @@ 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. +// clearPrivileges returns a copy of the schema with all 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 { +func clearPrivileges(s schema.Schema) schema.Schema { tables := make([]schema.Table, len(s.Tables)) for i, t := range s.Tables { t.Privileges = nil tables[i] = t } s.Tables = tables + + functions := make([]schema.Function, len(s.Functions)) + for i, f := range s.Functions { + f.Privileges = nil + functions[i] = f + } + s.Functions = functions + + procedures := make([]schema.Procedure, len(s.Procedures)) + for i, p := range s.Procedures { + p.Privileges = nil + procedures[i] = p + } + s.Procedures = procedures + return s } func assertMigratedSchemaMatchesTarget(migratedSchema, targetSchema schema.Schema, planOptions *planOptions) error { // Clear privileges from both schemas since privilege statements are skipped during validation // (roles don't exist in temp DB). We make copies to avoid modifying the original schemas. - migratedSchema = clearTablePrivileges(migratedSchema) - targetSchema = clearTablePrivileges(targetSchema) + migratedSchema = clearPrivileges(migratedSchema) + targetSchema = clearPrivileges(targetSchema) toTargetSchemaStmts, err := generateMigrationStatements(migratedSchema, targetSchema, planOptions) if err != nil { diff --git a/pkg/diff/privilege_sql_generator.go b/pkg/diff/privilege_sql_generator.go index d851ca7..b8e7a54 100644 --- a/pkg/diff/privilege_sql_generator.go +++ b/pkg/diff/privilege_sql_generator.go @@ -18,16 +18,71 @@ var ( ) type privilegeSQLVertexGenerator struct { - tableName schema.SchemaQualifiedName + objType string + objName schema.SchemaQualifiedName + sqlTarget string } func newPrivilegeSQLVertexGenerator(tableName schema.SchemaQualifiedName) sqlVertexGenerator[schema.TablePrivilege, privilegeDiff] { - return legacyToNewSqlVertexGenerator[schema.TablePrivilege, privilegeDiff](&privilegeSQLVertexGenerator{ - tableName: tableName, + return newPrivilegeSQLVertexGeneratorForObject("privilege", tableName, tableName.GetFQEscapedName()) +} + +func newPrivilegeSQLVertexGeneratorForObject( + objType string, + objName schema.SchemaQualifiedName, + sqlTarget string, +) sqlVertexGenerator[schema.Privilege, privilegeDiff] { + return legacyToNewSqlVertexGenerator[schema.Privilege, privilegeDiff](&privilegeSQLVertexGenerator{ + objType: objType, + objName: objName, + sqlTarget: sqlTarget, }) } -func (psg *privilegeSQLVertexGenerator) Add(p schema.TablePrivilege) ([]Statement, error) { +func newFunctionPrivilegeSQLVertexGenerator(functionName schema.SchemaQualifiedName) sqlVertexGenerator[schema.Privilege, privilegeDiff] { + return newPrivilegeSQLVertexGeneratorForObject( + "function_privilege", + functionName, + fmt.Sprintf("FUNCTION %s", functionName.GetFQEscapedName()), + ) +} + +func newProcedurePrivilegeSQLVertexGenerator(procedureName schema.SchemaQualifiedName) sqlVertexGenerator[schema.Privilege, privilegeDiff] { + return newPrivilegeSQLVertexGeneratorForObject( + "procedure_privilege", + procedureName, + fmt.Sprintf("PROCEDURE %s", procedureName.GetFQEscapedName()), + ) +} + +func exactRoutinePrivilegeStatements( + generator sqlVertexGenerator[schema.Privilege, privilegeDiff], + privileges []schema.Privilege, +) ([]Statement, error) { + var stmts []Statement + hasDefaultPublicExecute := false + for _, p := range privileges { + if p.Grantee == "" && p.Privilege == "EXECUTE" && !p.IsGrantable { + hasDefaultPublicExecute = true + continue + } + addPartialGraph, err := generator.Add(p) + if err != nil { + return nil, fmt.Errorf("generating grant statement for privilege %s: %w", p.GetName(), err) + } + stmts = append(stmts, stripMigrationHazards(addPartialGraph.statements()...)...) + } + if !hasDefaultPublicExecute { + deletePartialGraph, err := generator.Delete(schema.Privilege{Privilege: "EXECUTE"}) + if err != nil { + return nil, fmt.Errorf("generating revoke statement for default PUBLIC EXECUTE: %w", err) + } + stmts = append(stripMigrationHazards(deletePartialGraph.statements()...), stmts...) + } + return stmts, nil +} + +func (psg *privilegeSQLVertexGenerator) Add(p schema.Privilege) ([]Statement, error) { grantee := p.Grantee if grantee == "" { grantee = "PUBLIC" @@ -35,7 +90,7 @@ func (psg *privilegeSQLVertexGenerator) Add(p schema.TablePrivilege) ([]Statemen 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.sqlTarget, grantee) if p.IsGrantable { ddl += " WITH GRANT OPTION" } @@ -49,7 +104,7 @@ func (psg *privilegeSQLVertexGenerator) Add(p schema.TablePrivilege) ([]Statemen }}, nil } -func (psg *privilegeSQLVertexGenerator) Delete(p schema.TablePrivilege) ([]Statement, error) { +func (psg *privilegeSQLVertexGenerator) Delete(p schema.Privilege) ([]Statement, error) { grantee := p.Grantee if grantee == "" { grantee = "PUBLIC" @@ -57,7 +112,7 @@ func (psg *privilegeSQLVertexGenerator) Delete(p schema.TablePrivilege) ([]State 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.sqlTarget, grantee) return []Statement{{ DDL: ddl, @@ -76,17 +131,17 @@ func (psg *privilegeSQLVertexGenerator) Alter(diff privilegeDiff) ([]Statement, return nil, nil } -func (psg *privilegeSQLVertexGenerator) GetSQLVertexId(p schema.TablePrivilege, diffType diffType) sqlVertexId { - return buildSchemaObjVertexId("privilege", fmt.Sprintf("%s.%s", psg.tableName.GetFQEscapedName(), p.GetName()), diffType) +func (psg *privilegeSQLVertexGenerator) GetSQLVertexId(p schema.Privilege, diffType diffType) sqlVertexId { + return buildSchemaObjVertexId(psg.objType, fmt.Sprintf("%s.%s", psg.objName.GetFQEscapedName(), p.GetName()), diffType) } -func (psg *privilegeSQLVertexGenerator) GetAddAlterDependencies(newPriv, _ schema.TablePrivilege) ([]dependency, error) { +func (psg *privilegeSQLVertexGenerator) GetAddAlterDependencies(newPriv, _ schema.Privilege) ([]dependency, error) { // Ensure delete runs before add/alter (for recreate scenarios) return []dependency{ mustRun(psg.GetSQLVertexId(newPriv, diffTypeDelete)).before(psg.GetSQLVertexId(newPriv, diffTypeAddAlter)), }, nil } -func (psg *privilegeSQLVertexGenerator) GetDeleteDependencies(_ schema.TablePrivilege) ([]dependency, error) { +func (psg *privilegeSQLVertexGenerator) GetDeleteDependencies(_ schema.Privilege) ([]dependency, error) { return nil, nil } diff --git a/pkg/diff/procedure_sql_vertex_generator.go b/pkg/diff/procedure_sql_vertex_generator.go index d9f725b..7db28c7 100644 --- a/pkg/diff/procedure_sql_vertex_generator.go +++ b/pkg/diff/procedure_sql_vertex_generator.go @@ -39,22 +39,32 @@ func (p procedureSQLVertexGenerator) Add(s schema.Procedure) (partialSQLGraph, e deps = append(deps, mustRun(buildProcedureVertexId(s.SchemaQualifiedName, diffTypeAddAlter)).after(buildSequenceVertexId(seq.SchemaQualifiedName, diffTypeAddAlter))) } + stmts := []Statement{{ + DDL: s.Def, + Timeout: statementTimeoutDefault, + LockTimeout: lockTimeoutDefault, + Hazards: []MigrationHazard{{ + Type: MigrationHazardTypeHasUntrackableDependencies, + Message: "Dependencies of procedures are not tracked by Postgres. " + + "As a result, we cannot guarantee that this procedure's dependencies are ordered properly relative to " + + "this statement. For adds, this means you need to ensure that all objects this function depends on " + + "are added before this statement.", + }}, + }} + privilegeStmts, err := exactRoutinePrivilegeStatements( + newProcedurePrivilegeSQLVertexGenerator(s.SchemaQualifiedName), + s.Privileges, + ) + if err != nil { + return partialSQLGraph{}, fmt.Errorf("generating procedure privilege statements: %w", err) + } + stmts = append(stmts, privilegeStmts...) + return partialSQLGraph{ vertices: []sqlVertex{{ - id: buildProcedureVertexId(s.SchemaQualifiedName, diffTypeAddAlter), - priority: sqlPrioritySooner, - statements: []Statement{{ - DDL: s.Def, - Timeout: statementTimeoutDefault, - LockTimeout: lockTimeoutDefault, - Hazards: []MigrationHazard{{ - Type: MigrationHazardTypeHasUntrackableDependencies, - Message: "Dependencies of procedures are not tracked by Postgres. " + - "As a result, we cannot guarantee that this procedure's dependencies are ordered properly relative to " + - "this statement. For adds, this means you need to ensure that all objects this function depends on " + - "are added before this statement.", - }}, - }}, + id: buildProcedureVertexId(s.SchemaQualifiedName, diffTypeAddAlter), + priority: sqlPrioritySooner, + statements: stmts, }}, dependencies: deps, }, nil @@ -105,11 +115,28 @@ func (p procedureSQLVertexGenerator) Delete(s schema.Procedure) (partialSQLGraph } func (p procedureSQLVertexGenerator) Alter(d procedureDiff) (partialSQLGraph, error) { - if cmp.Equal(d.old, d.new) { - return partialSQLGraph{}, nil + oldWithoutPrivileges := d.old + oldWithoutPrivileges.Privileges = nil + newWithoutPrivileges := d.new + newWithoutPrivileges.Privileges = nil + + var partialGraph partialSQLGraph + if !cmp.Equal(oldWithoutPrivileges, newWithoutPrivileges) { + addPartialGraph, err := p.Add(d.new) + if err != nil { + return partialSQLGraph{}, err + } + partialGraph = concatPartialGraphs(partialGraph, addPartialGraph) + } + + privilegesPartialGraph, err := generatePartialGraph( + newProcedurePrivilegeSQLVertexGenerator(d.new.SchemaQualifiedName), + d.privilegesDiff, + ) + if err != nil { + return partialSQLGraph{}, fmt.Errorf("resolving procedure privilege sql: %w", err) } - // New adds or replaces the procedure. - return p.Add(d.new) + return concatPartialGraphs(partialGraph, privilegesPartialGraph), nil } func buildProcedureVertexId(name schema.SchemaQualifiedName, diffType diffType) sqlVertexId { diff --git a/pkg/diff/sql_generator.go b/pkg/diff/sql_generator.go index 6c31e22..af655bc 100644 --- a/pkg/diff/sql_generator.go +++ b/pkg/diff/sql_generator.go @@ -134,10 +134,12 @@ type ( functionDiff struct { oldAndNew[schema.Function] + privilegesDiff listDiff[schema.Privilege, privilegeDiff] } procedureDiff struct { oldAndNew[schema.Procedure] + privilegesDiff listDiff[schema.Privilege, privilegeDiff] } ) @@ -285,11 +287,13 @@ func buildSchemaDiff(old, new schema.Schema) (schemaDiff, bool, error) { } functionDiffs, err := diffLists(old.Functions, new.Functions, func(old, new schema.Function, _, _ int) (functionDiff, bool, error) { + privilegesDiff, err := buildPrivilegeDiffs(old.Privileges, new.Privileges) + if err != nil { + return functionDiff{}, false, fmt.Errorf("diffing privileges: %w", err) + } return functionDiff{ - oldAndNew[schema.Function]{ - old: old, - new: new, - }, + oldAndNew: oldAndNew[schema.Function]{old: old, new: new}, + privilegesDiff: privilegesDiff, }, false, nil }) if err != nil { @@ -297,11 +301,13 @@ func buildSchemaDiff(old, new schema.Schema) (schemaDiff, bool, error) { } procedureDiffs, err := diffLists(old.Procedures, new.Procedures, func(old, new schema.Procedure, _, _ int) (procedureDiff, bool, error) { + privilegesDiff, err := buildPrivilegeDiffs(old.Privileges, new.Privileges) + if err != nil { + return procedureDiff{}, false, fmt.Errorf("diffing privileges: %w", err) + } return procedureDiff{ - oldAndNew[schema.Procedure]{ - old: old, - new: new, - }, + oldAndNew: oldAndNew[schema.Procedure]{old: old, new: new}, + privilegesDiff: privilegesDiff, }, false, nil }) if err != nil { @@ -429,15 +435,7 @@ func buildTableDiff(oldTable, newTable schema.Table, _, _ int) (diff tableDiff, } - privilegesDiff, err := diffLists( - oldTable.Privileges, - newTable.Privileges, - func(old, new schema.TablePrivilege, _, _ int) (privilegeDiff, bool, error) { - // Recreate the privilege if IsGrantable changes - recreate := old.IsGrantable != new.IsGrantable - return privilegeDiff{oldAndNew[schema.TablePrivilege]{old: old, new: new}}, recreate, nil - }, - ) + privilegesDiff, err := buildPrivilegeDiffs(oldTable.Privileges, newTable.Privileges) if err != nil { return tableDiff{}, false, fmt.Errorf("diffing privileges: %w", err) } @@ -454,6 +452,18 @@ func buildTableDiff(oldTable, newTable schema.Table, _, _ int) (diff tableDiff, }, false, nil } +func buildPrivilegeDiffs(oldPrivileges, newPrivileges []schema.Privilege) (listDiff[schema.Privilege, privilegeDiff], error) { + return diffLists( + oldPrivileges, + newPrivileges, + func(old, new schema.Privilege, _, _ int) (privilegeDiff, bool, error) { + // Recreate the privilege if IsGrantable changes. + recreate := old.IsGrantable != new.IsGrantable + return privilegeDiff{oldAndNew[schema.Privilege]{old: old, new: new}}, recreate, nil + }, + ) +} + type indexDiffConfig struct { newSchemaTablesByName map[string]schema.Table addedTablesByName map[string]schema.Table @@ -885,14 +895,14 @@ func (t *tableSQLVertexGenerator) Add(table schema.Table) ([]Statement, error) { stmts = append(stmts, stripMigrationHazards(forceRLSForTable(table))...) } - privilegeGenerator := &privilegeSQLVertexGenerator{tableName: table.SchemaQualifiedName} + privilegeGenerator := newPrivilegeSQLVertexGenerator(table.SchemaQualifiedName) for _, privilege := range table.Privileges { - addPrivilegeStmts, err := privilegeGenerator.Add(privilege) + addPrivilegePartialGraph, err := privilegeGenerator.Add(privilege) if err != nil { return nil, fmt.Errorf("generating add privilege statements for privilege %s: %w", privilege.GetName(), err) } // Remove hazards from statements since the table is brand new - stmts = append(stmts, stripMigrationHazards(addPrivilegeStmts...)...) + stmts = append(stmts, stripMigrationHazards(addPrivilegePartialGraph.statements()...)...) } return stmts, nil diff --git a/pkg/diff/sql_vertex_generator.go b/pkg/diff/sql_vertex_generator.go index 3b54459..db3c83c 100644 --- a/pkg/diff/sql_vertex_generator.go +++ b/pkg/diff/sql_vertex_generator.go @@ -65,6 +65,18 @@ func graphFromPartials(parts partialSQLGraph) (*sqlGraph, error) { return graph, nil } +func graphStatements(parts partialSQLGraph) ([]Statement, error) { + graph, err := graphFromPartials(parts) + if err != nil { + return nil, fmt.Errorf("converting partial SQL graph") + } + stmts, err := graph.toOrderedStatements() + if err != nil { + return nil, fmt.Errorf("getting ordered statements: %w", err) + } + return stmts, nil +} + func mergeVertices(old, new sqlVertex) sqlVertex { priority := old.priority if new.priority != sqlPriorityUnset && (priority == sqlPriorityUnset || new.priority > priority) {