Skip to content

Commit a7a8a2d

Browse files
craig[bot]spilchen
craig[bot]
and
spilchen
committed
Merge #140028
140028: sql: Integrate RLS Enabled Field r=spilchen a=spilchen Previously, the new row-level security (RLS) logic for reads was only functional within unit tests. This change utilizes the RLS enabled field in the table descriptor to ensure USING expressions in policies are correctly applied outside of unit tests. Additional logic tests have been added to validate this behavior. Note: There are existing issues with the statement cache that can cause incorrect policy application for users/roles. These will be addressed in a follow-up issue. While creating the test, I encountered two issues with dependency checking: - `DROP CASCADE` did not correctly handle the new policy expression elements, which surfaced when dropping a database. - Sequence back-reference maintenance was incorrect when adding a second reference that only targeted the table (as occurs when a sequence is referenced in a policy expression). Epic: CRDB-11724 Release note: None Informs #136717 Co-authored-by: Matt Spilchen <[email protected]>
2 parents 1d56872 + 2a575b4 commit a7a8a2d

File tree

4 files changed

+292
-6
lines changed

4 files changed

+292
-6
lines changed

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

Lines changed: 272 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -495,4 +495,276 @@ ALTER TABLE roaches NO FORCE ROW LEVEL SECURITY;
495495
statement ok
496496
DROP TABLE roaches;
497497

498+
subtest using_expression_applied
499+
500+
statement ok
501+
CREATE TYPE league AS ENUM('AL','NL');
502+
503+
statement ok
504+
CREATE TABLE bbteams (team text, league league, family (team, league));
505+
506+
statement ok
507+
ALTER TABLE bbteams ENABLE ROW LEVEL SECURITY;
508+
509+
statement ok
510+
INSERT INTO bbteams VALUES ('jays', 'AL'), ('tigers', 'AL'), ('cardinals', 'NL'), ('orioles', 'AL'), ('nationals', 'NL');
511+
512+
# Confirm admin user can see all rows
513+
query TT
514+
select team, league from bbteams order by league, team;
515+
----
516+
jays AL
517+
orioles AL
518+
tigers AL
519+
cardinals NL
520+
nationals NL
521+
522+
statement ok
523+
CREATE USER buck;
524+
525+
statement ok
526+
GRANT ALL ON bbteams TO buck;
527+
528+
statement ok
529+
set role buck
530+
531+
# user buck can't see anything because they aren't admin, rls is enabled and no policies are defined.
532+
query TT
533+
select team, league from bbteams order by league, team;
534+
----
535+
536+
statement ok
537+
set role root
538+
539+
# We will create a function and a sequence to include in the expression, as
540+
# these introduce additional dependencies compared to a plain expression using
541+
# basic types.
542+
statement ok
543+
CREATE FUNCTION is_valid(l league) RETURNS BOOL AS $$
544+
BEGIN
545+
RETURN l = 'AL';
546+
END;
547+
$$ LANGUAGE PLpgSQL;
548+
549+
statement ok
550+
CREATE SEQUENCE seq1;
551+
552+
statement ok
553+
GRANT USAGE ON seq1 TO buck;
554+
555+
statement ok
556+
create policy restrict_select on bbteams for select to buck,current_user,session_user using (is_valid(league) and nextval('seq1') < 1000);
557+
558+
# confirm admin can see all
559+
query TT
560+
select team, league from bbteams where team != 'cardinals' order by league, team;
561+
----
562+
jays AL
563+
orioles AL
564+
tigers AL
565+
nationals NL
566+
567+
statement ok
568+
set role buck
569+
570+
# TODO(136717): Reusing the statement cache prevents recompiling a new plan,
571+
# which leads to incorrect results in this case.
572+
query TT
573+
select team, league from bbteams where team != 'cardinals' order by league, team;
574+
----
575+
jays AL
576+
orioles AL
577+
tigers AL
578+
nationals NL
579+
580+
query TT
581+
select team, league from bbteams where team != 'astros' order by league, team;
582+
----
583+
jays AL
584+
orioles AL
585+
tigers AL
586+
587+
# Verify that if admin is granted to user buck, it sees all rows because RLS is exempt.
588+
statement ok
589+
set role root
590+
591+
statement ok
592+
GRANT admin TO buck;
593+
594+
statement ok
595+
set role buck;
596+
597+
# This is the same query as before, but since admin changed, we will see all of the rows.
598+
# TODO(136717): We are mistakenly reusing the statement plan here. So we see the rows as if
599+
# the policies were applied.
600+
query TT
601+
select team, league from bbteams where team != 'astros' order by league, team;
602+
----
603+
jays AL
604+
orioles AL
605+
tigers AL
606+
607+
# Retry with a query never tried before so we avoid the statement cache.
608+
query TT
609+
select team, league from bbteams where team != 'mariners' order by league, team;
610+
----
611+
jays AL
612+
orioles AL
613+
tigers AL
614+
cardinals NL
615+
nationals NL
616+
617+
statement ok
618+
set role root
619+
620+
statement ok
621+
REVOKE admin FROM buck;
622+
623+
# Add policies that apply to other commands. Only SELECT will return rows.
624+
statement ok
625+
CREATE POLICY restrict_insert ON bbteams FOR INSERT TO buck USING (false);
626+
627+
statement ok
628+
CREATE POLICY restrict_delete ON bbteams FOR DELETE TO buck USING (false);
629+
630+
statement ok
631+
CREATE POLICY restrict_update ON bbteams FOR UPDATE TO buck USING (false);
632+
633+
statement ok
634+
set role buck
635+
636+
# Verify SELECT will use the original policy that restricts rows just to AL teams
637+
query TT
638+
select team, league from bbteams where team != 'jays' order by league, team;
639+
----
640+
orioles AL
641+
tigers AL
642+
643+
# Try updating a row. The policy for update will prevent us from reading the row.
644+
statement ok
645+
UPDATE bbteams SET team = 'blue jays' where team = 'jays';
646+
647+
query TT
648+
select team, league from bbteams order by league, team;
649+
----
650+
jays AL
651+
orioles AL
652+
tigers AL
653+
654+
# Switch the policy to allow an update, but only for rows in the AL league.
655+
statement ok
656+
set role root
657+
658+
statement ok
659+
DROP POLICY restrict_update on bbteams;
660+
661+
statement ok
662+
create policy restrict_update on bbteams for update to buck using (is_valid(league) and nextval('seq1') < 1000);
663+
664+
statement ok
665+
set role buck
666+
667+
# Allowed
668+
statement ok
669+
UPDATE bbteams SET team = 'Jays' where team = 'jays';
670+
671+
# Not allowed
672+
statement ok
673+
UPDATE bbteams SET team = 'Nationals' where team = 'nationals';
674+
675+
statement ok
676+
set role root
677+
678+
query TT
679+
select team, league from bbteams where team in ('jays', 'Jays', 'nationals', 'Nationals') order by league, team;
680+
----
681+
Jays AL
682+
nationals NL
683+
684+
statement ok
685+
set role buck
686+
687+
# Try to delete the row. The delete policy will prevent us from reading the row.
688+
statement ok
689+
DELETE FROM bbteams;
690+
691+
query TT
692+
select team, league from bbteams order by league, team;
693+
----
694+
Jays AL
695+
orioles AL
696+
tigers AL
697+
698+
# Switch the delete policy to allow deletion of only the tigers
699+
statement ok
700+
set role root
701+
702+
statement ok
703+
DROP POLICY restrict_delete on bbteams;
704+
705+
statement ok
706+
create policy restrict_delete on bbteams for delete to buck using (is_valid(league) and team = 'tigers' and nextval('seq1') < 1000) with check (true);
707+
708+
statement ok
709+
set role buck
710+
711+
statement ok
712+
DELETE FROM bbteams WHERE team != 'pirates';
713+
714+
query TT
715+
select team, league from bbteams where team != 'pirates' order by league, team;
716+
----
717+
Jays AL
718+
orioles AL
719+
720+
query TT
721+
SHOW CREATE TABLE bbteams
722+
----
723+
bbteams CREATE TABLE public.bbteams (
724+
team STRING NULL,
725+
league public.league NULL,
726+
rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
727+
CONSTRAINT bbteams_pkey PRIMARY KEY (rowid ASC),
728+
FAMILY fam_0_team_league_rowid (team, league, rowid)
729+
)
730+
731+
statement ok
732+
set role root
733+
734+
statement ok
735+
DROP TABLE bbteams;
736+
737+
statement ok
738+
DROP SEQUENCE seq1;
739+
740+
statement ok
741+
DROP FUNCTION is_valid;
742+
743+
# This subtest verifies proper cleanup of policy elements when dropping with CASCADE.
744+
subtest drop_with_cascade
745+
746+
statement ok
747+
CREATE DATABASE db2;
748+
749+
statement ok
750+
use db2;
751+
752+
statement ok
753+
CREATE TYPE classes AS ENUM('mammals','birds', 'fish', 'reptiles', 'amphibians');
754+
755+
statement ok
756+
CREATE TABLE animals (name text, class classes, family (name, class));
757+
758+
statement ok
759+
ALTER TABLE animals ENABLE ROW LEVEL SECURITY;
760+
761+
statement ok
762+
create policy p1 on animals for select to current_user using (class in ('mammals','birds')) with check (class in ('reptiles', 'amphibians'));
763+
764+
statement ok
765+
use defaultdb;
766+
767+
statement ok
768+
drop database db2 cascade;
769+
498770
subtest end

‎pkg/sql/opt_catalog.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1152,9 +1152,7 @@ func newOptTable(
11521152
ot.triggers = getOptTriggers(desc.GetTriggers())
11531153

11541154
// Store row-level security information
1155-
// TODO(136717): Update this to utilize the tableDescriptor's field for
1156-
// indicating if RLS is enabled once the field is added.
1157-
ot.rlsEnabled = false
1155+
ot.rlsEnabled = desc.IsRowLevelSecurityEnabled()
11581156
ot.policies = getOptPolicies(desc.GetPolicies())
11591157

11601158
// Add stats last, now that other metadata is initialized.

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,8 @@ func dropCascadeDescriptor(b BuildCtx, id catid.DescID) {
234234
dropCascadeDescriptor(next, t.FunctionID)
235235
case *scpb.TriggerDeps:
236236
dropCascadeDescriptor(next, t.TableID)
237+
case *scpb.PolicyDeps:
238+
dropCascadeDescriptor(next, t.TableID)
237239
case *scpb.Column, *scpb.ColumnType, *scpb.SecondaryIndexPartial:
238240
// These only have type references.
239241
break
@@ -245,6 +247,8 @@ func dropCascadeDescriptor(b BuildCtx, id catid.DescID) {
245247
*scpb.ColumnDefaultExpression,
246248
*scpb.ColumnOnUpdateExpression,
247249
*scpb.ColumnComputeExpression,
250+
*scpb.PolicyUsingExpr,
251+
*scpb.PolicyWithCheckExpr,
248252
*scpb.CheckConstraint,
249253
*scpb.CheckConstraintUnvalidated,
250254
*scpb.ForeignKeyConstraint,
@@ -253,7 +257,7 @@ func dropCascadeDescriptor(b BuildCtx, id catid.DescID) {
253257
*scpb.DatabaseRegionConfig:
254258
b.Drop(e)
255259
default:
256-
panic(errors.AssertionFailedf("un-dropped backref %T (%v) should be either be"+
260+
panic(errors.AssertionFailedf("un-dropped backref %T (%v) should either be "+
257261
"dropped or skipped", e, target))
258262
}
259263
})

‎pkg/sql/schemachanger/scexec/scmutationexec/references.go

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -456,6 +456,9 @@ func (i *immediateVisitor) RemovePolicyBackReferenceInFunctions(
456456
return nil
457457
}
458458

459+
// updateBackReferencesInSequences will maintain the back-references in the
460+
// sequence to a given table (and optional column).
461+
//
459462
// Look through `seqID`'s dependedOnBy slice, find the back-reference to `tblID`,
460463
// and update it to either
461464
// - upsert `colID` to ColumnIDs field of that back-reference, if `forwardRefs` contains `seqID`; or
@@ -473,23 +476,32 @@ func updateBackReferencesInSequences(
473476
return err
474477
}
475478
var current, updated catalog.TableColSet
479+
var hasExistingFwdRef bool
476480
_ = seq.ForeachDependedOnBy(func(dep *descpb.TableDescriptor_Reference) error {
477481
if dep.ID == tblID {
482+
hasExistingFwdRef = true
478483
current = catalog.MakeTableColSet(dep.ColumnIDs...)
479484
return iterutil.StopIteration()
480485
}
481486
return nil
482487
})
483488
if forwardRefs.Contains(seqID) {
484-
if current.Contains(colID) {
489+
// The sequence should maintain a back reference to the table. Check if the
490+
// current reference is sufficient. We can skip updating if the forward reference
491+
// already points to the correct column (if specified) or, if no column is given,
492+
// the table itself has an existing reference.
493+
if current.Contains(colID) || (colID == 0 && hasExistingFwdRef) {
485494
return nil
486495
}
487496
updated.UnionWith(current)
488497
if colID != 0 {
489498
updated.Add(colID)
490499
}
491500
} else {
492-
if !current.Contains(colID) && colID != 0 {
501+
// The sequence should no longer reference the table—either for the specified
502+
// column (if provided) or for the entire table if no column is given. Check
503+
// if an update is needed.
504+
if (colID != 0 && !current.Contains(colID)) || (colID == 0 && !hasExistingFwdRef) {
493505
return nil
494506
}
495507
current.ForEach(func(id descpb.ColumnID) {

0 commit comments

Comments
 (0)