diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index ca62cb707a..daf48dc0f0 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -2842,13 +2842,14 @@ lmerge_matched:; * UPDATE/DELETE RLS policies. If those checks fail, we throw an * error. * - * The WITH CHECK quals are applied in ExecUpdate() and hence we need - * not do anything special to handle them. + * The WITH CHECK quals for UPDATE RLS policies are applied in + * ExecUpdateAct() and hence we need not do anything special to handle + * them. * * NOTE: We must do this after WHEN quals are evaluated, so that we * check policies only when they matter. */ - if (resultRelInfo->ri_WithCheckOptions) + if (resultRelInfo->ri_WithCheckOptions && commandType != CMD_NOTHING) { ExecWithCheckOptions(commandType == CMD_UPDATE ? WCO_RLS_MERGE_UPDATE_CHECK : WCO_RLS_MERGE_DELETE_CHECK, diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c index a233dd4758..a0f7b22f13 100644 --- a/src/backend/rewrite/rowsecurity.c +++ b/src/backend/rewrite/rowsecurity.c @@ -387,7 +387,11 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index, * and set them up so that we can enforce the appropriate policy depending * on the final action we take. * - * We already fetched the SELECT policies above. + * We already fetched the SELECT policies above, to check existing rows, + * but we must also check that new rows created by UPDATE actions are + * visible, if SELECT rights are required for this relation. We don't do + * this for INSERT actions, since an INSERT command would only do this + * check if it had a RETURNING list, and MERGE does not support RETURNING. * * We don't push the UPDATE/DELETE USING quals to the RTE because we don't * really want to apply them while scanning the relation since we don't @@ -403,16 +407,20 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index, */ if (commandType == CMD_MERGE) { - List *merge_permissive_policies; - List *merge_restrictive_policies; + List *merge_update_permissive_policies; + List *merge_update_restrictive_policies; + List *merge_delete_permissive_policies; + List *merge_delete_restrictive_policies; + List *merge_insert_permissive_policies; + List *merge_insert_restrictive_policies; /* * Fetch the UPDATE policies and set them up to execute on the * existing target row before doing UPDATE. */ get_policies_for_relation(rel, CMD_UPDATE, user_id, - &merge_permissive_policies, - &merge_restrictive_policies); + &merge_update_permissive_policies, + &merge_update_restrictive_policies); /* * WCO_RLS_MERGE_UPDATE_CHECK is used to check UPDATE USING quals on @@ -420,23 +428,59 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index, */ add_with_check_options(rel, rt_index, WCO_RLS_MERGE_UPDATE_CHECK, - merge_permissive_policies, - merge_restrictive_policies, + merge_update_permissive_policies, + merge_update_restrictive_policies, withCheckOptions, hasSubLinks, true); + /* Enforce the WITH CHECK clauses of the UPDATE policies */ + add_with_check_options(rel, rt_index, + WCO_RLS_UPDATE_CHECK, + merge_update_permissive_policies, + merge_update_restrictive_policies, + withCheckOptions, + hasSubLinks, + false); + /* - * Same with DELETE policies. + * Add ALL/SELECT policies as WCO_RLS_UPDATE_CHECK WCOs, to ensure + * that the updated row is visible when executing an UPDATE action, if + * SELECT rights are required for this relation. + */ + if (rte->requiredPerms & ACL_SELECT) + { + List *merge_select_permissive_policies; + List *merge_select_restrictive_policies; + + get_policies_for_relation(rel, CMD_SELECT, user_id, + &merge_select_permissive_policies, + &merge_select_restrictive_policies); + add_with_check_options(rel, rt_index, + WCO_RLS_UPDATE_CHECK, + merge_select_permissive_policies, + merge_select_restrictive_policies, + withCheckOptions, + hasSubLinks, + true); + } + + /* + * Fetch the DELETE policies and set them up to execute on the + * existing target row before doing DELETE. */ get_policies_for_relation(rel, CMD_DELETE, user_id, - &merge_permissive_policies, - &merge_restrictive_policies); + &merge_delete_permissive_policies, + &merge_delete_restrictive_policies); + /* + * WCO_RLS_MERGE_DELETE_CHECK is used to check DELETE USING quals on + * the existing target row. + */ add_with_check_options(rel, rt_index, WCO_RLS_MERGE_DELETE_CHECK, - merge_permissive_policies, - merge_restrictive_policies, + merge_delete_permissive_policies, + merge_delete_restrictive_policies, withCheckOptions, hasSubLinks, true); @@ -447,22 +491,13 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index, * withCheckOptions. */ get_policies_for_relation(rel, CMD_INSERT, user_id, - &merge_permissive_policies, - &merge_restrictive_policies); + &merge_insert_permissive_policies, + &merge_insert_restrictive_policies); add_with_check_options(rel, rt_index, WCO_RLS_INSERT_CHECK, - merge_permissive_policies, - merge_restrictive_policies, - withCheckOptions, - hasSubLinks, - false); - - /* Enforce the WITH CHECK clauses of the UPDATE policies */ - add_with_check_options(rel, rt_index, - WCO_RLS_UPDATE_CHECK, - merge_permissive_policies, - merge_restrictive_policies, + merge_insert_permissive_policies, + merge_insert_restrictive_policies, withCheckOptions, hasSubLinks, false); diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out index 1fec044c49..2d99b89759 100644 --- a/src/test/regress/expected/rowsecurity.out +++ b/src/test/regress/expected/rowsecurity.out @@ -2127,10 +2127,10 @@ ALTER TABLE document ADD COLUMN dnotes text DEFAULT ''; CREATE POLICY p1 ON document FOR SELECT USING (true); -- one may insert documents only authored by them CREATE POLICY p2 ON document FOR INSERT WITH CHECK (dauthor = current_user); --- one may only update documents in 'novel' category +-- one may only update documents in 'novel' category and new dlevel must be > 0 CREATE POLICY p3 ON document FOR UPDATE USING (cid = (SELECT cid from category WHERE cname = 'novel')) - WITH CHECK (dauthor = current_user); + WITH CHECK (dlevel > 0); -- one may only delete documents in 'manga' category CREATE POLICY p4 ON document FOR DELETE USING (cid = (SELECT cid from category WHERE cname = 'manga')); @@ -2154,12 +2154,12 @@ SELECT * FROM document; (14 rows) SET SESSION AUTHORIZATION regress_rls_bob; --- Fails, since update violates WITH CHECK qual on dauthor +-- Fails, since update violates WITH CHECK qual on dlevel MERGE INTO document d USING (SELECT 1 as sdid) s ON did = s.sdid WHEN MATCHED THEN - UPDATE SET dnotes = dnotes || ' notes added by merge1 ', dauthor = 'regress_rls_alice'; + UPDATE SET dnotes = dnotes || ' notes added by merge1 ', dlevel = 0; ERROR: new row violates row-level security policy for table "document" -- Should be OK since USING and WITH CHECK quals pass MERGE INTO document d @@ -2167,12 +2167,12 @@ USING (SELECT 1 as sdid) s ON did = s.sdid WHEN MATCHED THEN UPDATE SET dnotes = dnotes || ' notes added by merge2 '; --- Even when dauthor is updated explicitly, but to the existing value +-- Even when dlevel is updated explicitly, but to the existing value MERGE INTO document d USING (SELECT 1 as sdid) s ON did = s.sdid WHEN MATCHED THEN - UPDATE SET dnotes = dnotes || ' notes added by merge3 ', dauthor = 'regress_rls_bob'; + UPDATE SET dnotes = dnotes || ' notes added by merge3 ', dlevel = 1; -- There is a MATCH for did = 3, but UPDATE's USING qual does not allow -- updating an item in category 'science fiction' MERGE INTO document d @@ -2211,6 +2211,14 @@ WHEN MATCHED AND dnotes <> '' THEN WHEN MATCHED THEN DELETE; ERROR: target row violates row-level security policy (USING expression) for table "document" +-- OK if DELETE is replaced with DO NOTHING +MERGE INTO document d +USING (SELECT 4 as sdid) s +ON did = s.sdid +WHEN MATCHED AND dnotes <> '' THEN + UPDATE SET dnotes = dnotes || ' notes added by merge ' +WHEN MATCHED THEN + DO NOTHING; SELECT * FROM document WHERE did = 4; did | cid | dlevel | dauthor | dtitle | dnotes -----+-----+--------+-----------------+----------------+-------- @@ -2259,30 +2267,53 @@ WHEN MATCHED THEN WHEN NOT MATCHED THEN INSERT VALUES (12, 11, 1, 'regress_rls_bob', 'another novel'); -- drop and create a new SELECT policy which prevents us from reading --- any document except with category 'magna' +-- any document except with category 'novel' RESET SESSION AUTHORIZATION; DROP POLICY p1 ON document; CREATE POLICY p1 ON document FOR SELECT - USING (cid = (SELECT cid from category WHERE cname = 'manga')); + USING (cid = (SELECT cid from category WHERE cname = 'novel')); SET SESSION AUTHORIZATION regress_rls_bob; -- MERGE can no longer see the matching row and hence attempts the -- NOT MATCHED action, which results in unique key violation MERGE INTO document d -USING (SELECT 1 as sdid) s +USING (SELECT 7 as sdid) s ON did = s.sdid WHEN MATCHED THEN UPDATE SET dnotes = dnotes || ' notes added by merge5 ' WHEN NOT MATCHED THEN INSERT VALUES (12, 11, 1, 'regress_rls_bob', 'another novel'); ERROR: duplicate key value violates unique constraint "document_pkey" +-- UPDATE action fails if new row is not visible +MERGE INTO document d +USING (SELECT 1 as sdid) s +ON did = s.sdid +WHEN MATCHED THEN + UPDATE SET dnotes = dnotes || ' notes added by merge6 ', + cid = (SELECT cid from category WHERE cname = 'technology'); +ERROR: new row violates row-level security policy for table "document" +-- but OK if new row is visible +MERGE INTO document d +USING (SELECT 1 as sdid) s +ON did = s.sdid +WHEN MATCHED THEN + UPDATE SET dnotes = dnotes || ' notes added by merge7 ', + cid = (SELECT cid from category WHERE cname = 'novel'); +-- OK to insert a new row that is not visible +MERGE INTO document d +USING (SELECT 13 as sdid) s +ON did = s.sdid +WHEN MATCHED THEN + UPDATE SET dnotes = dnotes || ' notes added by merge8 ' +WHEN NOT MATCHED THEN + INSERT VALUES (13, 44, 1, 'regress_rls_bob', 'new manga'); RESET SESSION AUTHORIZATION; -- drop the restrictive SELECT policy so that we can look at the -- final state of the table DROP POLICY p1 ON document; -- Just check everything went per plan SELECT * FROM document; - did | cid | dlevel | dauthor | dtitle | dnotes ------+-----+--------+-------------------+----------------------------------+----------------------------------------------------------------------- + did | cid | dlevel | dauthor | dtitle | dnotes +-----+-----+--------+-------------------+----------------------------------+---------------------------------------------------------------------------------------------- 3 | 22 | 2 | regress_rls_bob | my science fiction | 5 | 44 | 2 | regress_rls_bob | my second manga | 6 | 22 | 1 | regress_rls_carol | great science fiction | @@ -2296,8 +2327,9 @@ SELECT * FROM document; 78 | 33 | 1 | regress_rls_bob | some technology novel | 79 | 33 | 1 | regress_rls_bob | technology book, can only insert | 12 | 11 | 1 | regress_rls_bob | another novel | - 1 | 11 | 1 | regress_rls_bob | my first novel | notes added by merge2 notes added by merge3 notes added by merge4 -(14 rows) + 1 | 11 | 1 | regress_rls_bob | my first novel | notes added by merge2 notes added by merge3 notes added by merge4 notes added by merge7 + 13 | 44 | 1 | regress_rls_bob | new manga | +(15 rows) -- -- ROLE/GROUP diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql index faad37ec81..34ea204560 100644 --- a/src/test/regress/sql/rowsecurity.sql +++ b/src/test/regress/sql/rowsecurity.sql @@ -821,10 +821,10 @@ ALTER TABLE document ADD COLUMN dnotes text DEFAULT ''; CREATE POLICY p1 ON document FOR SELECT USING (true); -- one may insert documents only authored by them CREATE POLICY p2 ON document FOR INSERT WITH CHECK (dauthor = current_user); --- one may only update documents in 'novel' category +-- one may only update documents in 'novel' category and new dlevel must be > 0 CREATE POLICY p3 ON document FOR UPDATE USING (cid = (SELECT cid from category WHERE cname = 'novel')) - WITH CHECK (dauthor = current_user); + WITH CHECK (dlevel > 0); -- one may only delete documents in 'manga' category CREATE POLICY p4 ON document FOR DELETE USING (cid = (SELECT cid from category WHERE cname = 'manga')); @@ -833,12 +833,12 @@ SELECT * FROM document; SET SESSION AUTHORIZATION regress_rls_bob; --- Fails, since update violates WITH CHECK qual on dauthor +-- Fails, since update violates WITH CHECK qual on dlevel MERGE INTO document d USING (SELECT 1 as sdid) s ON did = s.sdid WHEN MATCHED THEN - UPDATE SET dnotes = dnotes || ' notes added by merge1 ', dauthor = 'regress_rls_alice'; + UPDATE SET dnotes = dnotes || ' notes added by merge1 ', dlevel = 0; -- Should be OK since USING and WITH CHECK quals pass MERGE INTO document d @@ -847,12 +847,12 @@ ON did = s.sdid WHEN MATCHED THEN UPDATE SET dnotes = dnotes || ' notes added by merge2 '; --- Even when dauthor is updated explicitly, but to the existing value +-- Even when dlevel is updated explicitly, but to the existing value MERGE INTO document d USING (SELECT 1 as sdid) s ON did = s.sdid WHEN MATCHED THEN - UPDATE SET dnotes = dnotes || ' notes added by merge3 ', dauthor = 'regress_rls_bob'; + UPDATE SET dnotes = dnotes || ' notes added by merge3 ', dlevel = 1; -- There is a MATCH for did = 3, but UPDATE's USING qual does not allow -- updating an item in category 'science fiction' @@ -892,6 +892,15 @@ WHEN MATCHED AND dnotes <> '' THEN WHEN MATCHED THEN DELETE; +-- OK if DELETE is replaced with DO NOTHING +MERGE INTO document d +USING (SELECT 4 as sdid) s +ON did = s.sdid +WHEN MATCHED AND dnotes <> '' THEN + UPDATE SET dnotes = dnotes || ' notes added by merge ' +WHEN MATCHED THEN + DO NOTHING; + SELECT * FROM document WHERE did = 4; -- Switch to regress_rls_carol role and try the DELETE again. It should succeed @@ -941,24 +950,49 @@ WHEN NOT MATCHED THEN INSERT VALUES (12, 11, 1, 'regress_rls_bob', 'another novel'); -- drop and create a new SELECT policy which prevents us from reading --- any document except with category 'magna' +-- any document except with category 'novel' RESET SESSION AUTHORIZATION; DROP POLICY p1 ON document; CREATE POLICY p1 ON document FOR SELECT - USING (cid = (SELECT cid from category WHERE cname = 'manga')); + USING (cid = (SELECT cid from category WHERE cname = 'novel')); SET SESSION AUTHORIZATION regress_rls_bob; -- MERGE can no longer see the matching row and hence attempts the -- NOT MATCHED action, which results in unique key violation MERGE INTO document d -USING (SELECT 1 as sdid) s +USING (SELECT 7 as sdid) s ON did = s.sdid WHEN MATCHED THEN UPDATE SET dnotes = dnotes || ' notes added by merge5 ' WHEN NOT MATCHED THEN INSERT VALUES (12, 11, 1, 'regress_rls_bob', 'another novel'); +-- UPDATE action fails if new row is not visible +MERGE INTO document d +USING (SELECT 1 as sdid) s +ON did = s.sdid +WHEN MATCHED THEN + UPDATE SET dnotes = dnotes || ' notes added by merge6 ', + cid = (SELECT cid from category WHERE cname = 'technology'); + +-- but OK if new row is visible +MERGE INTO document d +USING (SELECT 1 as sdid) s +ON did = s.sdid +WHEN MATCHED THEN + UPDATE SET dnotes = dnotes || ' notes added by merge7 ', + cid = (SELECT cid from category WHERE cname = 'novel'); + +-- OK to insert a new row that is not visible +MERGE INTO document d +USING (SELECT 13 as sdid) s +ON did = s.sdid +WHEN MATCHED THEN + UPDATE SET dnotes = dnotes || ' notes added by merge8 ' +WHEN NOT MATCHED THEN + INSERT VALUES (13, 44, 1, 'regress_rls_bob', 'new manga'); + RESET SESSION AUTHORIZATION; -- drop the restrictive SELECT policy so that we can look at the -- final state of the table