Skip to content

Commit 3396db0

Browse files
fqazifqazi
committed
sql/schemachanger: fix RLS policy ordering when swapping elements
Previously, the order of operations when swapping row-level security (RLS) policies was not explicitly enforced, depending instead on their addition order in the builder. New rules for automatically managing `schema_locked` highlighted a risk: without strict ordering, the swap sequence could be inadvertently reversed. This patch addresses this by enforcing that RLS policy drops always occur before the corresponding adds during a swap operation. Fixes: #144767 Fixes: #144762 Release note: None
1 parent 7625d9f commit 3396db0

File tree

3 files changed

+96
-0
lines changed

3 files changed

+96
-0
lines changed

‎pkg/sql/logictest/testdata/logic_test/row_level_security‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3141,3 +3141,42 @@ statement ok
31413141
DROP USER bypassrls_user;
31423142

31433143
subtest end
3144+
3145+
3146+
subtest policy_with_schema_locked
3147+
3148+
statement ok
3149+
CREATE TABLE alter_policy_table_locked (c1 INT8 NOT NULL, c2 STRING NULL, CONSTRAINT alter_policy_table_pkey PRIMARY KEY (c1 ASC), FAMILY fam_0_c1_c2 (c1, c2)) WITH (schema_locked = true);
3150+
3151+
statement ok
3152+
CREATE POLICY p ON alter_policy_table_locked WITH CHECK (TRUE);
3153+
3154+
statement ok
3155+
ALTER POLICY p ON alter_policy_table_locked RENAME TO p_sel;
3156+
3157+
query TT
3158+
SHOW CREATE TABLE alter_policy_table_locked;
3159+
----
3160+
alter_policy_table_locked CREATE TABLE public.alter_policy_table_locked (
3161+
c1 INT8 NOT NULL,
3162+
c2 STRING NULL,
3163+
CONSTRAINT alter_policy_table_pkey PRIMARY KEY (c1 ASC),
3164+
FAMILY fam_0_c1_c2 (c1, c2)
3165+
) WITH (schema_locked = true);
3166+
CREATE POLICY p_sel ON public.alter_policy_table_locked AS PERMISSIVE FOR ALL TO public WITH CHECK (true)
3167+
3168+
statement ok
3169+
ALTER POLICY p_sel ON alter_policy_table_locked WITH CHECK (FALSE);
3170+
3171+
query TT
3172+
SHOW CREATE TABLE alter_policy_table_locked;
3173+
----
3174+
alter_policy_table_locked CREATE TABLE public.alter_policy_table_locked (
3175+
c1 INT8 NOT NULL,
3176+
c2 STRING NULL,
3177+
CONSTRAINT alter_policy_table_pkey PRIMARY KEY (c1 ASC),
3178+
FAMILY fam_0_c1_c2 (c1, c2)
3179+
) WITH (schema_locked = true);
3180+
CREATE POLICY p_sel ON public.alter_policy_table_locked AS PERMISSIVE FOR ALL TO public WITH CHECK (false)
3181+
3182+
subtest end

‎pkg/sql/schemachanger/scplan/internal/rules/current/dep_create_policy.go‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,28 @@ func init() {
2727
},
2828
)
2929
}
30+
31+
func init() {
32+
// This rule ensures that when a policy dependent is being swapped are
33+
// done in the correct order. The dropped element should disappear before
34+
// the replacement element is added.
35+
registerDepRule(
36+
"policy dependents are swapped in order",
37+
scgraph.Precedence,
38+
"drop-policy-dependent", "add-policy-dependent",
39+
func(from, to NodeVars) rel.Clauses {
40+
return rel.Clauses{
41+
from.TypeFilter(rulesVersionKey, isPolicyDependent),
42+
to.TypeFilter(rulesVersionKey, isPolicyDependent),
43+
// Confirm we are joining on the same types.
44+
from.El.AttrEqVar(rel.Type, "sameType"),
45+
to.El.AttrEqVar(rel.Type, "sameType"),
46+
JoinOnPolicyID(from, to, "table-id", "policy-id"),
47+
from.TargetStatus(scpb.ToAbsent),
48+
from.CurrentStatus(scpb.Status_ABSENT),
49+
to.TargetStatus(scpb.ToPublic, scpb.TransientAbsent),
50+
to.CurrentStatus(scpb.Status_PUBLIC),
51+
}
52+
},
53+
)
54+
}

‎pkg/sql/schemachanger/scplan/internal/rules/current/testdata/deprules‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4170,6 +4170,22 @@ deprules
41704170
- $index-Node[CurrentStatus] = TRANSIENT_ABSENT
41714171
- joinTargetNode($partial-predicate, $partial-predicate-Target, $partial-predicate-Node)
41724172
- joinTargetNode($index, $index-Target, $index-Node)
4173+
- name: policy dependents are swapped in order
4174+
from: drop-policy-dependent-Node
4175+
kind: Precedence
4176+
to: add-policy-dependent-Node
4177+
query:
4178+
- $drop-policy-dependent[Type] IN ['*scpb.PolicyDeps', '*scpb.PolicyName', '*scpb.PolicyRole', '*scpb.PolicyUsingExpr', '*scpb.PolicyWithCheckExpr']
4179+
- $add-policy-dependent[Type] IN ['*scpb.PolicyDeps', '*scpb.PolicyName', '*scpb.PolicyRole', '*scpb.PolicyUsingExpr', '*scpb.PolicyWithCheckExpr']
4180+
- $drop-policy-dependent[Type] = $sameType
4181+
- $add-policy-dependent[Type] = $sameType
4182+
- joinOnPolicyID($drop-policy-dependent, $add-policy-dependent, $table-id, $policy-id)
4183+
- $drop-policy-dependent-Target[TargetStatus] = ABSENT
4184+
- $drop-policy-dependent-Node[CurrentStatus] = ABSENT
4185+
- $add-policy-dependent-Target[TargetStatus] IN [PUBLIC, TRANSIENT_ABSENT]
4186+
- $add-policy-dependent-Node[CurrentStatus] = PUBLIC
4187+
- joinTargetNode($drop-policy-dependent, $drop-policy-dependent-Target, $drop-policy-dependent-Node)
4188+
- joinTargetNode($add-policy-dependent, $add-policy-dependent-Target, $add-policy-dependent-Node)
41734189
- name: primary index named right before index becomes public
41744190
from: index-name-Node
41754191
kind: SameStagePrecedence
@@ -9091,6 +9107,22 @@ deprules
90919107
- $index-Node[CurrentStatus] = TRANSIENT_ABSENT
90929108
- joinTargetNode($partial-predicate, $partial-predicate-Target, $partial-predicate-Node)
90939109
- joinTargetNode($index, $index-Target, $index-Node)
9110+
- name: policy dependents are swapped in order
9111+
from: drop-policy-dependent-Node
9112+
kind: Precedence
9113+
to: add-policy-dependent-Node
9114+
query:
9115+
- $drop-policy-dependent[Type] IN ['*scpb.PolicyDeps', '*scpb.PolicyName', '*scpb.PolicyRole', '*scpb.PolicyUsingExpr', '*scpb.PolicyWithCheckExpr']
9116+
- $add-policy-dependent[Type] IN ['*scpb.PolicyDeps', '*scpb.PolicyName', '*scpb.PolicyRole', '*scpb.PolicyUsingExpr', '*scpb.PolicyWithCheckExpr']
9117+
- $drop-policy-dependent[Type] = $sameType
9118+
- $add-policy-dependent[Type] = $sameType
9119+
- joinOnPolicyID($drop-policy-dependent, $add-policy-dependent, $table-id, $policy-id)
9120+
- $drop-policy-dependent-Target[TargetStatus] = ABSENT
9121+
- $drop-policy-dependent-Node[CurrentStatus] = ABSENT
9122+
- $add-policy-dependent-Target[TargetStatus] IN [PUBLIC, TRANSIENT_ABSENT]
9123+
- $add-policy-dependent-Node[CurrentStatus] = PUBLIC
9124+
- joinTargetNode($drop-policy-dependent, $drop-policy-dependent-Target, $drop-policy-dependent-Node)
9125+
- joinTargetNode($add-policy-dependent, $add-policy-dependent-Target, $add-policy-dependent-Node)
90949126
- name: primary index named right before index becomes public
90959127
from: index-name-Node
90969128
kind: SameStagePrecedence

0 commit comments

Comments
 (0)