Skip to content

Commit 7ae806f

Browse files
craig[bot]spilchen
andcommitted
Merge #143587
143587: sql: add RLS policy support for UPSERT r=spilchen a=spilchen Previously, row-level security (RLS) was not enforced for UPSERT statements. This change introduces full RLS support for the following UPSERT variants: - UPSERT - INSERT … ON CONFLICT DO UPDATE - INSERT … ON CONFLICT DO NOTHING The specific RLS policies enforced during an UPSERT depend on whether a row conflict occurs: - No Conflict (row is inserted): - SELECT/ALL: used to scan for potential conflicts. - INSERT/ALL: enforced on the newly inserted row. - Conflict (row is updated): - SELECT/ALL: enforced to verify access to the conflicting row. - UPDATE/ALL: enforced on the row being updated. Importantly, SELECT/ALL policies used in conflict detection are not applied as filters. Filtering out unauthorized rows would permit inserting duplicates. Instead, we enforce these policies via explicit checks: if the user is not authorized to see the conflicting row, the UPSERT fails. To support this behaviour, we reused the single synthetic check constraint per table. It now has a combined expression that captures all relevant RLS checks for UPSERTs: 1. Enforce SELECT/ALL on existing rows during conflict detection. 2. Enforce UPDATE/ALL on conflicting rows that are updated. 3. Enforce INSERT/ALL on new rows that are inserted. This implementation differs from PostgreSQL in one key way. In Postgres, the VALUES clause is always subject to constraint and policy checks—even if the row is not written due to a conflict. This is a known limitation discussed in #35370. As a result, we allow statements like the following to succeed if a conflict occurs, even when the VALUES clause violates RLS: ``` INSERT INTO t VALUES (...violates RLS...) ON CONFLICT DO UPDATE SET ...complies with RLS...; INSERT INTO t VALUES (...violates RLS...) ON CONFLICT DO NOTHING; ``` Closes #141998 Closes #144802 Epic: CRDB-11724 Release note: none Co-authored-by: Matt Spilchen <[email protected]>
2 parents 76e7d25 + 3c5c4b5 commit 7ae806f

File tree

10 files changed

+1388
-233
lines changed

10 files changed

+1388
-233
lines changed

pkg/sql/logictest/testdata/logic_test/row_level_security

Lines changed: 786 additions & 0 deletions
Large diffs are not rendered by default.

pkg/sql/opt/cat/policy.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ const (
2727
PolicyScopeUpdate
2828
// PolicyScopeDelete indicates that the policy applies to DELETE operations.
2929
PolicyScopeDelete
30+
// PolicyScopeUpsert is used to indicate it's an INSERT ... ON CONFLICT
31+
// statement.
32+
PolicyScopeUpsert
3033
// PolicyScopeExempt indicates that the operation is exempt from row-level security policies.
3134
PolicyScopeExempt
3235
)

pkg/sql/opt/exec/explain/testdata/row_level_security

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,3 +421,94 @@ select count(*) from t1,t2 where t1.c1 = t2.c1;
421421
table: t2@t2_pkey
422422
spans: FULL SCAN
423423
policies: t2_pol_1
424+
425+
# Show that policies are shown for UPSERT (and its variations).
426+
# ----------------------------------------------------------------------
427+
428+
exec-ddl
429+
CREATE TABLE ups (pk int not null primary key, comment text);
430+
----
431+
432+
exec-ddl
433+
ALTER TABLE ups OWNER TO rls_accessor
434+
----
435+
436+
exec-ddl
437+
ALTER TABLE ups ENABLE ROW LEVEL SECURITY, FORCE ROW LEVEL SECURITY;
438+
----
439+
440+
exec-ddl
441+
CREATE POLICY p_ins ON ups FOR INSERT USING (comment = 'insert');
442+
----
443+
444+
exec-ddl
445+
CREATE POLICY p_sel ON ups FOR SELECT USING (comment = 'select');
446+
----
447+
448+
exec-ddl
449+
CREATE POLICY p_upd ON ups FOR UPDATE USING (comment = 'update');
450+
----
451+
452+
plan
453+
UPSERT INTO ups VALUES (1, 'first value');
454+
----
455+
• upsert
456+
│ into: ups(pk, comment)
457+
│ auto commit
458+
│ arbiter indexes: ups_pkey
459+
│ policies: p_ins, p_sel, p_upd
460+
461+
└── • render
462+
463+
└── • cross join (left outer)
464+
465+
├── • values
466+
│ size: 2 columns, 1 row
467+
468+
└── • scan
469+
table: ups@ups_pkey
470+
spans: 1+ spans
471+
locking strength: for update
472+
473+
plan
474+
INSERT INTO ups VALUES (1, 'first value') ON CONFLICT (pk) DO UPDATE SET comment = 'second value';
475+
----
476+
• upsert
477+
│ into: ups(pk, comment)
478+
│ auto commit
479+
│ arbiter indexes: ups_pkey
480+
│ policies: p_ins, p_sel, p_upd
481+
482+
└── • render
483+
484+
└── • render
485+
486+
└── • cross join (left outer)
487+
488+
├── • values
489+
│ size: 2 columns, 1 row
490+
491+
└── • scan
492+
table: ups@ups_pkey
493+
spans: 1+ spans
494+
locking strength: for update
495+
496+
plan
497+
INSERT INTO ups VALUES (1, 'first value') ON CONFLICT (pk) DO NOTHING;
498+
----
499+
• insert
500+
│ into: ups(pk, comment)
501+
│ auto commit
502+
│ arbiter indexes: ups_pkey
503+
│ policies: p_ins, p_sel
504+
505+
└── • render
506+
507+
└── • cross join (anti)
508+
509+
├── • values
510+
│ size: 2 columns, 1 row
511+
512+
└── • scan
513+
table: ups@ups_pkey
514+
spans: 1+ spans

pkg/sql/opt/optbuilder/insert.go

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,7 @@ func (b *Builder) buildInsert(ins *tree.Insert, inScope *scope) (outScope *scope
310310
mb.buildRowLevelBeforeTriggers(tree.TriggerEventInsert, false /* cascade */)
311311

312312
// Build the final insert statement, including any returned expressions.
313-
mb.buildInsert(returning, ins.VectorInsert())
313+
mb.buildInsert(returning, ins.VectorInsert(), false /* hasOnConflict */)
314314

315315
// Case 2: INSERT..ON CONFLICT DO NOTHING.
316316
case ins.OnConflict.DoNothing:
@@ -325,7 +325,7 @@ func (b *Builder) buildInsert(ins *tree.Insert, inScope *scope) (outScope *scope
325325

326326
// Since buildInputForDoNothing filters out rows with conflicts, always
327327
// insert rows that are not filtered.
328-
mb.buildInsert(returning, false /* vectorInsert */)
328+
mb.buildInsert(returning, false /* vectorInsert */, true /* hasOnConflict */)
329329

330330
// Case 3: UPSERT statement.
331331
case ins.OnConflict.IsUpsertAlias():
@@ -425,6 +425,8 @@ func (b *Builder) buildInsert(ins *tree.Insert, inScope *scope) (outScope *scope
425425
// 4. Each update value is the same as the corresponding insert value.
426426
// 5. There are no inbound foreign keys containing non-key columns.
427427
// 6. There are no UPDATE triggers on the target table.
428+
// 7. Row-level security is disabled for the table. RLS may need to check for
429+
// policy violations on the old rows, so we always need to fetch them.
428430
//
429431
// TODO(andyk): The fast path is currently only enabled when the UPSERT alias
430432
// is explicitly selected by the user. It's possible to fast path some queries
@@ -490,7 +492,9 @@ func (mb *mutationBuilder) needExistingRows() bool {
490492
}
491493
}
492494

493-
return false
495+
// If RLS is enabled, we need to fetch the old rows to check for policy
496+
// violations.
497+
return mb.tab.IsRowLevelSecurityEnabled()
494498
}
495499

496500
// addTargetNamedColsForInsert adds a list of user-specified column names to the
@@ -749,13 +753,16 @@ func (mb *mutationBuilder) addSynthesizedColsForInsert() {
749753

750754
// buildInsert constructs an Insert operator, possibly wrapped by a Project
751755
// operator that corresponds to the given RETURNING clause.
752-
func (mb *mutationBuilder) buildInsert(returning *tree.ReturningExprs, vectorInsert bool) {
756+
func (mb *mutationBuilder) buildInsert(
757+
returning *tree.ReturningExprs, vectorInsert bool, hasOnConflict bool,
758+
) {
753759
// Disambiguate names so that references in any expressions, such as a
754760
// check constraint, refer to the correct columns.
755761
mb.disambiguateColumns()
756762

757763
// Add any check constraint boolean columns to the input.
758-
mb.addCheckConstraintCols(false /* isUpdate */)
764+
mb.addCheckConstraintCols(false, /* isUpdate */
765+
cat.PolicyScopeInsert, returning != nil || hasOnConflict /* includeSelectOnInsert */)
759766

760767
// Project partial index PUT boolean columns.
761768
mb.projectPartialIndexPutCols()
@@ -960,7 +967,8 @@ func (mb *mutationBuilder) buildUpsert(returning *tree.ReturningExprs) {
960967
mb.disambiguateColumns()
961968

962969
// Add any check constraint boolean columns to the input.
963-
mb.addCheckConstraintCols(false /* isUpdate */)
970+
mb.addCheckConstraintCols(false, /* isUpdate */
971+
cat.PolicyScopeUpsert, false /* includeSelectOnInsert */)
964972

965973
// Add the partial index predicate expressions to the table metadata.
966974
// These expressions are used to prune fetch columns during

0 commit comments

Comments
 (0)