-
Notifications
You must be signed in to change notification settings - Fork 37
fix: defer cross-table policy creation until all tables exist (#373) #374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
cba137d
c888b59
3d1af17
bf1d939
ba8c295
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1528,10 +1528,19 @@ func (d *ddlDiff) generateCreateSQL(targetSchema string, collector *diffCollecto | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| key := fmt.Sprintf("%s.%s", tableDiff.Table.Schema, tableDiff.Table.Name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| existingTables[key] = true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Build lookup of all new table names (qualified) for policy deferral (#373). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Policies that reference other new tables must be deferred until all tables exist. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| newTableLookup := make(map[string]struct{}, len(d.addedTables)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for _, table := range d.addedTables { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| newTableLookup[fmt.Sprintf("%s.%s", strings.ToLower(table.Schema), strings.ToLower(table.Name))] = struct{}{} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var shouldDeferPolicy func(*ir.RLSPolicy) bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(newFunctionLookup) > 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(newFunctionLookup) > 0 || len(newTableLookup) > 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| shouldDeferPolicy = func(policy *ir.RLSPolicy) bool { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return policyReferencesNewFunction(policy, newFunctionLookup) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if policyReferencesNewFunction(policy, newFunctionLookup) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return policyReferencesOtherNewTable(policy, newTableLookup) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -2028,6 +2037,38 @@ func policyReferencesNewFunction(policy *ir.RLSPolicy, newFunctions map[string]s | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // policyReferencesOtherNewTable determines if a policy's USING or WITH CHECK expressions | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // reference any newly added table other than the policy's own table (#373). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // newTables keys are fully qualified (schema.table) to avoid cross-schema collisions. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func policyReferencesOtherNewTable(policy *ir.RLSPolicy, newTables map[string]struct{}) bool { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(newTables) == 0 || policy == nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ownQualified := fmt.Sprintf("%s.%s", strings.ToLower(policy.Schema), strings.ToLower(policy.Table)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for _, expr := range []string{policy.Using, policy.WithCheck} { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if expr == "" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| exprLower := strings.ToLower(expr) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for qualifiedName := range newTables { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Skip the policy's own table | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if qualifiedName == ownQualified { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Extract the unqualified table name for substring matching. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Policy expressions may use unqualified or qualified references. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parts := strings.SplitN(qualifiedName, ".", 2) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tableName := parts[len(parts)-1] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if strings.Contains(exprLower, tableName) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+2040
to
+2068
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // policyReferencesOtherNewTable determines if a policy's USING or WITH CHECK expressions | |
| // reference any newly added table other than the policy's own table (#373). | |
| func policyReferencesOtherNewTable(policy *ir.RLSPolicy, newTables map[string]struct{}) bool { | |
| if len(newTables) == 0 || policy == nil { | |
| return false | |
| } | |
| for _, expr := range []string{policy.Using, policy.WithCheck} { | |
| if expr == "" { | |
| continue | |
| } | |
| exprLower := strings.ToLower(expr) | |
| for tableName := range newTables { | |
| // Skip the policy's own table | |
| if strings.EqualFold(tableName, policy.Table) || | |
| strings.EqualFold(tableName, fmt.Sprintf("%s.%s", policy.Schema, policy.Table)) { | |
| continue | |
| } | |
| // Check if the expression contains a reference to this table name | |
| if strings.Contains(exprLower, strings.ToLower(tableName)) { | |
| return true | |
| } | |
| } | |
| } | |
| // policyReferencesOtherNewTable determines if a policy belongs to a newly added table | |
| // that should have its policy creation deferred until all tables exist (#373). | |
| func policyReferencesOtherNewTable(policy *ir.RLSPolicy, newTables map[string]struct{}) bool { | |
| if len(newTables) == 0 || policy == nil { | |
| return false | |
| } | |
| // Normalize table name for lookup. | |
| tableName := strings.ToLower(policy.Table) | |
| // Check unqualified table name. | |
| if _, ok := newTables[tableName]; ok { | |
| return true | |
| } | |
| // Also check schema-qualified form if a schema is present. | |
| if policy.Schema != "" { | |
| qualified := fmt.Sprintf("%s.%s", strings.ToLower(policy.Schema), tableName) | |
| if _, ok := newTables[qualified]; ok { | |
| return true | |
| } | |
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| CREATE TABLE IF NOT EXISTS manager ( | ||
| id SERIAL, | ||
| user_id uuid NOT NULL | ||
| ); | ||
|
|
||
| ALTER TABLE manager ENABLE ROW LEVEL SECURITY; | ||
|
|
||
| CREATE TABLE IF NOT EXISTS project_manager ( | ||
| id SERIAL, | ||
| project_id integer NOT NULL, | ||
| manager_id integer NOT NULL, | ||
| is_deleted boolean DEFAULT false NOT NULL | ||
| ); | ||
|
|
||
| CREATE POLICY employee_manager_select ON manager FOR SELECT TO PUBLIC USING (id IN ( SELECT pam.manager_id FROM project_manager pam WHERE ((pam.project_id IN ( SELECT unnest(ARRAY[1, 2, 3]) AS unnest)) AND (pam.is_deleted = false)))); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| CREATE TABLE project_manager ( | ||
| id SERIAL, | ||
| project_id int NOT NULL, | ||
| manager_id int NOT NULL, | ||
| is_deleted boolean NOT NULL DEFAULT false | ||
| ); | ||
|
|
||
| CREATE TABLE manager ( | ||
| id SERIAL, | ||
| user_id uuid NOT NULL | ||
| ); | ||
|
|
||
| ALTER TABLE manager ENABLE ROW LEVEL SECURITY; | ||
|
|
||
| CREATE POLICY employee_manager_select ON manager | ||
| FOR SELECT | ||
| TO PUBLIC | ||
| USING ( | ||
| id IN ( | ||
| SELECT pam.manager_id | ||
| FROM project_manager pam | ||
| WHERE pam.project_id IN ( | ||
| SELECT unnest(ARRAY[1, 2, 3]) | ||
| ) | ||
| AND pam.is_deleted = false | ||
| ) | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| -- Empty schema (no tables) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| { | ||
| "version": "1.0.0", | ||
| "pgschema_version": "1.7.4", | ||
| "created_at": "1970-01-01T00:00:00Z", | ||
| "source_fingerprint": { | ||
| "hash": "965b1131737c955e24c7f827c55bd78e4cb49a75adfd04229e0ba297376f5085" | ||
| }, | ||
| "groups": [ | ||
| { | ||
| "steps": [ | ||
| { | ||
| "sql": "CREATE TABLE IF NOT EXISTS manager (\n id SERIAL,\n user_id uuid NOT NULL\n);", | ||
| "type": "table", | ||
| "operation": "create", | ||
| "path": "public.manager" | ||
| }, | ||
| { | ||
| "sql": "ALTER TABLE manager ENABLE ROW LEVEL SECURITY;", | ||
| "type": "table.rls", | ||
| "operation": "alter", | ||
| "path": "public.manager" | ||
| }, | ||
| { | ||
| "sql": "CREATE TABLE IF NOT EXISTS project_manager (\n id SERIAL,\n project_id integer NOT NULL,\n manager_id integer NOT NULL,\n is_deleted boolean DEFAULT false NOT NULL\n);", | ||
| "type": "table", | ||
| "operation": "create", | ||
| "path": "public.project_manager" | ||
| }, | ||
| { | ||
| "sql": "CREATE POLICY employee_manager_select ON manager FOR SELECT TO PUBLIC USING (id IN ( SELECT pam.manager_id FROM project_manager pam WHERE ((pam.project_id IN ( SELECT unnest(ARRAY[1, 2, 3]) AS unnest)) AND (pam.is_deleted = false))));", | ||
| "type": "table.policy", | ||
| "operation": "create", | ||
| "path": "public.manager.employee_manager_select" | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| CREATE TABLE IF NOT EXISTS manager ( | ||
| id SERIAL, | ||
| user_id uuid NOT NULL | ||
| ); | ||
|
|
||
| ALTER TABLE manager ENABLE ROW LEVEL SECURITY; | ||
|
|
||
| CREATE TABLE IF NOT EXISTS project_manager ( | ||
| id SERIAL, | ||
| project_id integer NOT NULL, | ||
| manager_id integer NOT NULL, | ||
| is_deleted boolean DEFAULT false NOT NULL | ||
| ); | ||
|
|
||
| CREATE POLICY employee_manager_select ON manager FOR SELECT TO PUBLIC USING (id IN ( SELECT pam.manager_id FROM project_manager pam WHERE ((pam.project_id IN ( SELECT unnest(ARRAY[1, 2, 3]) AS unnest)) AND (pam.is_deleted = false)))); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| Plan: 2 to add. | ||
|
|
||
| Summary by type: | ||
| tables: 2 to add | ||
|
|
||
| Tables: | ||
| + manager | ||
| + employee_manager_select (policy) | ||
| ~ manager (rls) | ||
| + project_manager | ||
|
|
||
| DDL to be executed: | ||
| -------------------------------------------------- | ||
|
|
||
| CREATE TABLE IF NOT EXISTS manager ( | ||
| id SERIAL, | ||
| user_id uuid NOT NULL | ||
| ); | ||
|
|
||
| ALTER TABLE manager ENABLE ROW LEVEL SECURITY; | ||
|
|
||
| CREATE TABLE IF NOT EXISTS project_manager ( | ||
| id SERIAL, | ||
| project_id integer NOT NULL, | ||
| manager_id integer NOT NULL, | ||
| is_deleted boolean DEFAULT false NOT NULL | ||
| ); | ||
|
|
||
| CREATE POLICY employee_manager_select ON manager FOR SELECT TO PUBLIC USING (id IN ( SELECT pam.manager_id FROM project_manager pam WHERE ((pam.project_id IN ( SELECT unnest(ARRAY[1, 2, 3]) AS unnest)) AND (pam.is_deleted = false)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newTableLookup stores an unqualified key (table.Name) for every added table. This collides when the migration adds tables with the same name in different schemas, which can lead to incorrect deferral decisions (e.g., the unqualified key may be treated as the policy’s “own table” and skipped even when the referenced table is actually in another schema). Consider tracking tables by fully-qualified name only, or keeping a mapping from unqualified name → set of qualified names so cross-schema cases can’t be lost.