Skip to content

Commit fc06a34

Browse files
craig[bot]spilchen
craig[bot]
and
spilchen
committed
Merge #141722
141722: sql/schemachanger: Block invalid expressions in CREATE POLICY r=rickystewart a=spilchen This change adds a check to prevent CREATE POLICY from using expressions that are not applicable to the specified command. For example: - A policy for INSERT should not use a USING expression, as USING applies only to existing rows. - Policies for DELETE or SELECT should not include a WITH CHECK expression, since they do not add new rows. Postgres enforces these same restrictions, so this update aligns our behaviour for consistency. The error messages are also identical to those in postgres. Epic: CRDB-45203 Release note: none Informs #136696 Co-authored-by: Matt Spilchen <[email protected]>
2 parents 531c0da + d0ba396 commit fc06a34

File tree

6 files changed

+47
-23
lines changed

6 files changed

+47
-23
lines changed

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

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -816,7 +816,7 @@ set role root
816816

817817
# Add policies that apply to other commands. Only SELECT will return rows.
818818
statement ok
819-
CREATE POLICY restrict_insert ON bbteams FOR INSERT TO buck USING (false);
819+
CREATE POLICY restrict_insert ON bbteams FOR INSERT TO buck WITH CHECK (false);
820820

821821
statement ok
822822
CREATE POLICY restrict_delete ON bbteams FOR DELETE TO buck USING (false);
@@ -897,7 +897,7 @@ statement ok
897897
DROP POLICY restrict_delete on bbteams;
898898

899899
statement ok
900-
create policy restrict_delete on bbteams for delete to buck using (is_valid(league) and team = 'tigers' and nextval('seq1') < 1000) with check (true);
900+
create policy restrict_delete on bbteams for delete to buck using (is_valid(league) and team = 'tigers' and nextval('seq1') < 1000);
901901

902902
statement ok
903903
set role buck
@@ -923,9 +923,9 @@ bbteams CREATE TABLE public.bbteams (
923923
);
924924
ALTER TABLE public.bbteams ENABLE ROW LEVEL SECURITY;
925925
CREATE POLICY restrict_select ON public.bbteams AS PERMISSIVE FOR SELECT TO buck, root USING (public.is_valid(league) AND (nextval('public.seq1'::REGCLASS) < 1000:::INT8));
926-
CREATE POLICY restrict_insert ON public.bbteams AS PERMISSIVE FOR INSERT TO buck USING (false);
926+
CREATE POLICY restrict_insert ON public.bbteams AS PERMISSIVE FOR INSERT TO buck WITH CHECK (false);
927927
CREATE POLICY restrict_update ON public.bbteams AS PERMISSIVE FOR UPDATE TO buck USING (public.is_valid(league) AND (nextval('public.seq1'::REGCLASS) < 1000:::INT8));
928-
CREATE POLICY restrict_delete ON public.bbteams AS PERMISSIVE FOR DELETE TO buck USING ((public.is_valid(league) AND (team = 'tigers':::STRING)) AND (nextval('public.seq1'::REGCLASS) < 1000:::INT8)) WITH CHECK (true)
928+
CREATE POLICY restrict_delete ON public.bbteams AS PERMISSIVE FOR DELETE TO buck USING ((public.is_valid(league) AND (team = 'tigers':::STRING)) AND (nextval('public.seq1'::REGCLASS) < 1000:::INT8))
929929

930930
statement ok
931931
set role root
@@ -958,7 +958,7 @@ statement ok
958958
ALTER TABLE animals ENABLE ROW LEVEL SECURITY;
959959

960960
statement ok
961-
create policy p1 on animals for select to current_user using (class in ('mammals','birds')) with check (class in ('reptiles', 'amphibians'));
961+
create policy p1 on animals for select to current_user using (class in ('mammals','birds'));
962962

963963
statement ok
964964
use defaultdb;
@@ -1257,4 +1257,21 @@ SET ROLE root
12571257
statement ok
12581258
DROP ROLE rls_cache_user;
12591259

1260+
subtest block_invalid_expressions
1261+
1262+
statement ok
1263+
CREATE TABLE blocker();
1264+
1265+
statement error pq: only WITH CHECK expression allowed for INSERT
1266+
CREATE POLICY p_insert on blocker FOR INSERT USING (1 = 1);
1267+
1268+
statement error pq: WITH CHECK cannot be applied to SELECT or DELETE
1269+
CREATE POLICY p_select on blocker FOR SELECT WITH CHECK (false);
1270+
1271+
statement error pq: WITH CHECK cannot be applied to SELECT or DELETE
1272+
CREATE POLICY p_delete on blocker FOR DELETE WITH CHECK (1 = 1);
1273+
1274+
statement ok
1275+
DROP TABLE blocker;
1276+
12601277
subtest end

‎pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_policy.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,13 @@ func CreatePolicy(b BuildCtx, n *tree.CreatePolicy) {
4848
}
4949
})
5050

51+
// Depending on the command for the policy, some expressions are blocked.
52+
if n.Cmd == tree.PolicyCommandInsert && n.Exprs.Using != nil {
53+
panic(pgerror.Newf(pgcode.Syntax, "only WITH CHECK expression allowed for INSERT"))
54+
} else if (n.Cmd == tree.PolicyCommandDelete || n.Cmd == tree.PolicyCommandSelect) && n.Exprs.WithCheck != nil {
55+
panic(pgerror.Newf(pgcode.Syntax, "WITH CHECK cannot be applied to SELECT or DELETE"))
56+
}
57+
5158
policyID := b.NextTablePolicyID(tbl.TableID)
5259
policy := &scpb.Policy{
5360
TableID: tbl.TableID,

‎pkg/sql/schemachanger/scbuild/testdata/create_policy

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,14 @@ CREATE FUNCTION is_valid(n INT) returns bool as $$ begin return n < 10; end; $$
55
----
66

77
build
8-
CREATE POLICY "first policy" on defaultdb.foo AS PERMISSIVE FOR SELECT TO fred USING (i > 0) WITH CHECK (i % 2 = 0);
8+
CREATE POLICY "first policy" on defaultdb.foo AS PERMISSIVE FOR UPDATE TO fred USING (i > 0) WITH CHECK (i % 2 = 0);
99
----
1010
- [[IndexData:{DescID: 104, IndexID: 1}, PUBLIC], PUBLIC]
1111
{indexId: 1, tableId: 104}
1212
- [[TableData:{DescID: 104, ReferencedDescID: 100}, PUBLIC], PUBLIC]
1313
{databaseId: 100, tableId: 104}
1414
- [[Policy:{DescID: 104, PolicyID: 1}, PUBLIC], ABSENT]
15-
{command: 2, policyId: 1, tableId: 104, type: 1}
15+
{command: 4, policyId: 1, tableId: 104, type: 1}
1616
- [[PolicyName:{DescID: 104, Name: first policy, PolicyID: 1}, PUBLIC], ABSENT]
1717
{name: first policy, policyId: 1, tableId: 104}
1818
- [[PolicyRole:{DescID: 104, Name: fred, PolicyID: 1}, PUBLIC], ABSENT]
@@ -25,7 +25,7 @@ CREATE POLICY "first policy" on defaultdb.foo AS PERMISSIVE FOR SELECT TO fred U
2525
{policyId: 1, tableId: 104}
2626

2727
build
28-
CREATE POLICY "second policy" on defaultdb.foo AS RESTRICTIVE FOR INSERT USING (false);
28+
CREATE POLICY "second policy" on defaultdb.foo AS RESTRICTIVE FOR INSERT WITH CHECK (false);
2929
----
3030
- [[IndexData:{DescID: 104, IndexID: 1}, PUBLIC], PUBLIC]
3131
{indexId: 1, tableId: 104}
@@ -37,13 +37,13 @@ CREATE POLICY "second policy" on defaultdb.foo AS RESTRICTIVE FOR INSERT USING (
3737
{name: second policy, policyId: 1, tableId: 104}
3838
- [[PolicyRole:{DescID: 104, Name: public, PolicyID: 1}, PUBLIC], ABSENT]
3939
{policyId: 1, roleName: public, tableId: 104}
40-
- [[PolicyUsingExpr:{DescID: 104, Expr: false, PolicyID: 1}, PUBLIC], ABSENT]
40+
- [[PolicyWithCheckExpr:{DescID: 104, Expr: false, PolicyID: 1}, PUBLIC], ABSENT]
4141
{expr: "false", policyId: 1, tableId: 104}
4242
- [[PolicyDeps:{DescID: 104, PolicyID: 1}, PUBLIC], ABSENT]
4343
{policyId: 1, tableId: 104}
4444

4545
build
46-
CREATE POLICY "third policy" on defaultdb.foo FOR DELETE TO CURRENT_USER,fred WITH CHECK (i < 0);
46+
CREATE POLICY "third policy" on defaultdb.foo FOR DELETE TO CURRENT_USER,fred USING (i < 0);
4747
----
4848
- [[IndexData:{DescID: 104, IndexID: 1}, PUBLIC], PUBLIC]
4949
{indexId: 1, tableId: 104}
@@ -57,7 +57,7 @@ CREATE POLICY "third policy" on defaultdb.foo FOR DELETE TO CURRENT_USER,fred WI
5757
{policyId: 1, roleName: root, tableId: 104}
5858
- [[PolicyRole:{DescID: 104, Name: fred, PolicyID: 1}, PUBLIC], ABSENT]
5959
{policyId: 1, roleName: fred, tableId: 104}
60-
- [[PolicyWithCheckExpr:{DescID: 104, Expr: i < 0:::INT8, PolicyID: 1}, PUBLIC], ABSENT]
60+
- [[PolicyUsingExpr:{DescID: 104, Expr: i < 0:::INT8, PolicyID: 1}, PUBLIC], ABSENT]
6161
{expr: 'i < 0:::INT8', policyId: 1, referencedColumnIds: [1], tableId: 104}
6262
- [[PolicyDeps:{DescID: 104, PolicyID: 1}, PUBLIC], ABSENT]
6363
{policyId: 1, tableId: 104}

‎pkg/sql/schemachanger/scbuild/testdata/drop_policy

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,9 @@ SET enable_row_level_security = true;
33
CREATE FUNCTION is_valid(n INT) returns bool as $$ begin return n < 10; end; $$ language plpgsql;
44
CREATE TABLE defaultdb.foo (i INT PRIMARY KEY);
55
CREATE USER fred;
6-
CREATE POLICY "first policy" on defaultdb.foo AS PERMISSIVE FOR SELECT TO fred USING (i > 0) WITH CHECK (i % 2 = 0);
7-
CREATE POLICY "second policy" on defaultdb.foo AS RESTRICTIVE FOR INSERT USING (false);
8-
CREATE POLICY "third policy" on defaultdb.foo FOR DELETE TO CURRENT_USER,fred WITH CHECK (i < 0);
6+
CREATE POLICY "first policy" on defaultdb.foo AS PERMISSIVE FOR UPDATE TO fred USING (i > 0) WITH CHECK (i % 2 = 0);
7+
CREATE POLICY "second policy" on defaultdb.foo AS RESTRICTIVE FOR INSERT WITH CHECK (false);
8+
CREATE POLICY "third policy" on defaultdb.foo FOR DELETE TO CURRENT_USER,fred USING (i < 0);
99
CREATE POLICY "fourth policy" on defaultdb.foo AS PERMISSIVE TO PUBLIC,SESSION_USER;
1010
CREATE POLICY "fifth policy" on defaultdb.foo USING (is_valid(i));
1111
CREATE POLICY "sixth policy" on defaultdb.foo WITH CHECK (is_valid(i));
@@ -17,7 +17,7 @@ DROP POLICY "first policy" on defaultdb.foo;
1717
- [[IndexData:{DescID: 105, IndexID: 1}, PUBLIC], PUBLIC]
1818
{indexId: 1, tableId: 105}
1919
- [[Policy:{DescID: 105, PolicyID: 1}, ABSENT], PUBLIC]
20-
{command: 2, policyId: 1, tableId: 105, type: 1}
20+
{command: 4, policyId: 1, tableId: 105, type: 1}
2121
- [[PolicyName:{DescID: 105, Name: first policy, PolicyID: 1}, ABSENT], PUBLIC]
2222
{name: first policy, policyId: 1, tableId: 105}
2323
- [[PolicyRole:{DescID: 105, Name: fred, PolicyID: 1}, ABSENT], PUBLIC]
@@ -42,7 +42,7 @@ DROP POLICY "second policy" on defaultdb.foo CASCADE;
4242
{name: second policy, policyId: 2, tableId: 105}
4343
- [[PolicyRole:{DescID: 105, Name: public, PolicyID: 2}, ABSENT], PUBLIC]
4444
{policyId: 2, roleName: public, tableId: 105}
45-
- [[PolicyUsingExpr:{DescID: 105, Expr: false, PolicyID: 2}, ABSENT], PUBLIC]
45+
- [[PolicyWithCheckExpr:{DescID: 105, Expr: false, PolicyID: 2}, ABSENT], PUBLIC]
4646
{expr: "false", policyId: 2, tableId: 105}
4747
- [[PolicyDeps:{DescID: 105, PolicyID: 2}, ABSENT], PUBLIC]
4848
{policyId: 2, tableId: 105}
@@ -62,7 +62,7 @@ DROP POLICY "third policy" on defaultdb.foo RESTRICT;
6262
{policyId: 3, roleName: root, tableId: 105}
6363
- [[PolicyRole:{DescID: 105, Name: fred, PolicyID: 3}, ABSENT], PUBLIC]
6464
{policyId: 3, roleName: fred, tableId: 105}
65-
- [[PolicyWithCheckExpr:{DescID: 105, Expr: i < 0:::INT8, PolicyID: 3}, ABSENT], PUBLIC]
65+
- [[PolicyUsingExpr:{DescID: 105, Expr: i < 0:::INT8, PolicyID: 3}, ABSENT], PUBLIC]
6666
{expr: 'i < 0:::INT8', policyId: 3, referencedColumnIds: [1], tableId: 105}
6767
- [[PolicyDeps:{DescID: 105, PolicyID: 3}, ABSENT], PUBLIC]
6868
{policyId: 3, tableId: 105}

‎pkg/sql/schemachanger/scplan/testdata/create_policy

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ CREATE FUNCTION is_even(n INT) returns bool as $$ begin return n % 2 = 0; end; $
1010
----
1111

1212
ops
13-
CREATE POLICY "policy 1" on t1 AS PERMISSIVE FOR SELECT TO user1,user2,public USING (tenant_id = '01538898-f55c-44db-a306-89078e2c430e' AND (c1).x > 0) WITH CHECK (nextval('seq1') < 10);
13+
CREATE POLICY "policy 1" on t1 AS PERMISSIVE FOR UPDATE TO user1,user2,public USING (tenant_id = '01538898-f55c-44db-a306-89078e2c430e' AND (c1).x > 0) WITH CHECK (nextval('seq1') < 10);
1414
----
1515
StatementPhase stage 1 of 1 with 9 MutationType ops
1616
transitions:
@@ -25,7 +25,7 @@ StatementPhase stage 1 of 1 with 9 MutationType ops
2525
ops:
2626
*scop.AddPolicy
2727
Policy:
28-
Command: 2
28+
Command: 4
2929
PolicyID: 1
3030
TableID: 106
3131
Type: 1
@@ -95,7 +95,7 @@ PreCommitPhase stage 2 of 2 with 9 MutationType ops
9595
ops:
9696
*scop.AddPolicy
9797
Policy:
98-
Command: 2
98+
Command: 4
9999
PolicyID: 1
100100
TableID: 106
101101
Type: 1

‎pkg/sql/schemachanger/scplan/testdata/drop_policy

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ SET enable_row_level_security=on;
88
CREATE TYPE greeting AS ENUM('hi', 'howdy', 'hello');
99
CREATE FUNCTION is_valid(n INT) returns bool as $$ begin return n < 10; end; $$ language plpgsql;
1010
CREATE FUNCTION is_even(n INT) returns bool as $$ begin return n % 2 = 0; end; $$ language plpgsql;
11-
CREATE POLICY "policy 1" on t1 AS PERMISSIVE FOR SELECT TO user1,user2,public USING (tenant_id = '01538898-f55c-44db-a306-89078e2c430e' AND (c1).x > 0) WITH CHECK (nextval('seq1') < 10);
11+
CREATE POLICY "policy 1" on t1 AS PERMISSIVE FOR UPDATE TO user1,user2,public USING (tenant_id = '01538898-f55c-44db-a306-89078e2c430e' AND (c1).x > 0) WITH CHECK (nextval('seq1') < 10);
1212
CREATE POLICY "policy 2" on t1 USING (c2::greeting = 'hello'::greeting) WITH CHECK (c2::greeting = 'hi'::greeting);
1313
CREATE POLICY "policy 3" on t1 USING (is_valid((c1).x)) WITH CHECK (is_even((c1).y));
1414
----
@@ -58,7 +58,7 @@ StatementPhase stage 1 of 1 with 8 MutationType ops
5858
- 107
5959
*scop.RemovePolicy
6060
Policy:
61-
Command: 2
61+
Command: 4
6262
PolicyID: 1
6363
TableID: 106
6464
Type: 1
@@ -117,7 +117,7 @@ PreCommitPhase stage 2 of 2 with 8 MutationType ops
117117
- 107
118118
*scop.RemovePolicy
119119
Policy:
120-
Command: 2
120+
Command: 4
121121
PolicyID: 1
122122
TableID: 106
123123
Type: 1

0 commit comments

Comments
 (0)