On Wed, 2023-10-25 at 09:45 +0200, Laurenz Albe wrote: > I can accept that the error is intentional, even though it violated the > POLA for me. I can buy into the argument that an UPDATE should not make > a row seem to vanish. > > I cannot buy into the constraint argument. If the table owner wanted to > prevent you from causing a constraint violation error with a row you > cannot see, she wouldn't have given you a FOR UPDATE policy that allows > you to perform such an UPDATE. > > Anyway, it is probably too late to change a behavior that has been like > that for a while and is not manifestly buggy.
I have thought some more about this, and I believe that if FOR SELECT policies are used to check new rows, you should be allowed to specify WITH CHECK on FOR SELECT policies. Why not allow a user to specify different conditions for fetching from a table and for new rows after an UPDATE? The attached patch does that. What so you think? Yours, Laurenz Albe
From c1bf1cb39962690933e4a37af0ab8b00926a0020 Mon Sep 17 00:00:00 2001 From: Laurenz Albe <laurenz.a...@cybertec.at> Date: Thu, 9 Nov 2023 16:09:10 +0100 Subject: [PATCH] Allow WITH CKECK on FOR SELECT policies FOR SELECT or FOR ALL policies are used to check the new row after an UPDATE, but you could not define a WITH CHECK clause on FOR SELECT policies, so the USING clause was used. This patch adds the capability to define WITH CHECK clauses on FOR SELECT policies, which allows the user to specify different criteria for fetching rows from the table and for new rows after an UPDATE. Author: Laurenz Albe Discussion: https://postgr.es/m/aee893f1ec3ca8f62a0da2fc2f9f8b73920f9f9d.camel%40cybertec.at --- doc/src/sgml/ref/create_policy.sgml | 35 +++++++++++++++-------- src/backend/commands/policy.c | 14 ++++----- src/backend/rewrite/rowsecurity.c | 2 +- src/test/regress/expected/rowsecurity.out | 14 +++++++++ src/test/regress/sql/rowsecurity.sql | 11 +++++++ 5 files changed, 55 insertions(+), 21 deletions(-) diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml index e76c342d3d..d473bb1094 100644 --- a/doc/src/sgml/ref/create_policy.sgml +++ b/doc/src/sgml/ref/create_policy.sgml @@ -82,7 +82,7 @@ CREATE POLICY <replaceable class="parameter">name</replaceable> ON <replaceable <para> For policies that can have both <literal>USING</literal> - and <literal>WITH CHECK</literal> expressions (<literal>ALL</literal> + and <literal>WITH CHECK</literal> expressions (<literal>ALL</literal>, <literal>SELECT</literal> and <literal>UPDATE</literal>), if no <literal>WITH CHECK</literal> expression is defined, then the <literal>USING</literal> expression will be used both to determine which rows are visible (normal @@ -270,9 +270,11 @@ CREATE POLICY <replaceable class="parameter">name</replaceable> ON <replaceable that require <literal>SELECT</literal> permissions, such as <literal>UPDATE</literal>, will also only see those records that are allowed by the <literal>SELECT</literal> policy. - A <literal>SELECT</literal> policy cannot have a <literal>WITH - CHECK</literal> expression, as it only applies in cases where - records are being retrieved from the relation. + In addition, rows written by an <command>INSERT</command> or + <command>UPDATE</command> statement are checked against the + <literal>WITH CHECK</literal> expression of <literal>SELECT</literal> + policies on the table (or, if there is no <literal>WITH + CHECK</literal> expression, the <literal>USING</literal> expression). </para> </listitem> </varlistentry> @@ -402,14 +404,17 @@ CREATE POLICY <replaceable class="parameter">name</replaceable> ON <replaceable <table id="sql-createpolicy-summary"> <title>Policies Applied by Command Type</title> - <tgroup cols="6"> - <colspec colnum="4" colname="update-using"/> - <colspec colnum="5" colname="update-check"/> + <tgroup cols="7"> + <colspec colnum="2" colname="insert-using"/> + <colspec colnum="3" colname="insert-check"/> + <colspec colnum="5" colname="update-using"/> + <colspec colnum="6" colname="update-check"/> + <spanspec namest="insert-using" nameend="insert-check" spanname="insert"/> <spanspec namest="update-using" nameend="update-check" spanname="update"/> <thead> <row> <entry morerows="1">Command</entry> - <entry><literal>SELECT/ALL policy</literal></entry> + <entry spanname="insert"><literal>SELECT/ALL policy</literal></entry> <entry><literal>INSERT/ALL policy</literal></entry> <entry spanname="update"><literal>UPDATE/ALL policy</literal></entry> <entry><literal>DELETE/ALL policy</literal></entry> @@ -417,6 +422,7 @@ CREATE POLICY <replaceable class="parameter">name</replaceable> ON <replaceable <row> <entry><literal>USING expression</literal></entry> <entry><literal>WITH CHECK expression</literal></entry> + <entry><literal>WITH CHECK expression</literal></entry> <entry><literal>USING expression</literal></entry> <entry><literal>WITH CHECK expression</literal></entry> <entry><literal>USING expression</literal></entry> @@ -430,11 +436,13 @@ CREATE POLICY <replaceable class="parameter">name</replaceable> ON <replaceable <entry>—</entry> <entry>—</entry> <entry>—</entry> + <entry>—</entry> </row> <row> <entry><command>SELECT FOR UPDATE/SHARE</command></entry> <entry>Existing row</entry> <entry>—</entry> + <entry>—</entry> <entry>Existing row</entry> <entry>—</entry> <entry>—</entry> @@ -442,6 +450,7 @@ CREATE POLICY <replaceable class="parameter">name</replaceable> ON <replaceable <row> <entry><command>INSERT</command> / <command>MERGE ... THEN INSERT</command></entry> <entry>—</entry> + <entry>—</entry> <entry>New row</entry> <entry>—</entry> <entry>—</entry> @@ -449,6 +458,7 @@ CREATE POLICY <replaceable class="parameter">name</replaceable> ON <replaceable </row> <row> <entry><command>INSERT ... RETURNING</command></entry> + <entry>—</entry> <entry> New row <footnote id="rls-select-priv"> <para> @@ -465,9 +475,8 @@ CREATE POLICY <replaceable class="parameter">name</replaceable> ON <replaceable </row> <row> <entry><command>UPDATE</command> / <command>MERGE ... THEN UPDATE</command></entry> - <entry> - Existing & new rows <footnoteref linkend="rls-select-priv"/> - </entry> + <entry>Existing row <footnoteref linkend="rls-select-priv"/></entry> + <entry>New row</entry> <entry>—</entry> <entry>Existing row</entry> <entry>New row</entry> @@ -481,11 +490,13 @@ CREATE POLICY <replaceable class="parameter">name</replaceable> ON <replaceable <entry>—</entry> <entry>—</entry> <entry>—</entry> + <entry>—</entry> <entry>Existing row</entry> </row> <row> <entry><command>ON CONFLICT DO UPDATE</command></entry> - <entry>Existing & new rows</entry> + <entry>Existing row</entry> + <entry>New row</entry> <entry>—</entry> <entry>Existing row</entry> <entry>New row</entry> diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c index 76a45e56bf..e3e19167fe 100644 --- a/src/backend/commands/policy.c +++ b/src/backend/commands/policy.c @@ -597,13 +597,12 @@ CreatePolicy(CreatePolicyStmt *stmt) polcmd = parse_policy_command(stmt->cmd_name); /* - * If the command is SELECT or DELETE then WITH CHECK should be NULL. + * If the command is DELETE then WITH CHECK should be NULL. */ - if ((polcmd == ACL_SELECT_CHR || polcmd == ACL_DELETE_CHR) - && stmt->with_check != NULL) + if (polcmd == ACL_DELETE_CHR && stmt->with_check != NULL) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("WITH CHECK cannot be applied to SELECT or DELETE"))); + errmsg("WITH CHECK cannot be applied to DELETE"))); /* * If the command is INSERT then WITH CHECK should be the only expression @@ -899,13 +898,12 @@ AlterPolicy(AlterPolicyStmt *stmt) polcmd = DatumGetChar(polcmd_datum); /* - * If the command is SELECT or DELETE then WITH CHECK should be NULL. + * If the command is DELETE then WITH CHECK should be NULL. */ - if ((polcmd == ACL_SELECT_CHR || polcmd == ACL_DELETE_CHR) - && stmt->with_check != NULL) + if (polcmd == ACL_DELETE_CHR && stmt->with_check != NULL) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("only USING expression allowed for SELECT, DELETE"))); + errmsg("only USING expression allowed for DELETE"))); /* * If the command is INSERT then WITH CHECK should be the only expression diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c index b1620e4625..8df5422583 100644 --- a/src/backend/rewrite/rowsecurity.c +++ b/src/backend/rewrite/rowsecurity.c @@ -308,7 +308,7 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index, select_restrictive_policies, withCheckOptions, hasSubLinks, - true); + false); } /* diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out index 6988128aa4..a43160cb0d 100644 --- a/src/test/regress/expected/rowsecurity.out +++ b/src/test/regress/expected/rowsecurity.out @@ -4308,6 +4308,20 @@ ERROR: new row violates row-level security policy for table "r1" INSERT INTO r1 VALUES (10) ON CONFLICT ON CONSTRAINT r1_pkey DO UPDATE SET a = 30; ERROR: new row violates row-level security policy for table "r1" +-- Add an explicit WITH CHECK clause to the FOR SELECT policy, so that the +-- UPDATE that failed before can succeed +ALTER POLICY p1 ON r1 WITH CHECK (true); +BEGIN; +UPDATE r1 SET a = 30 WHERE a = 10; +ROLLBACK; +BEGIN; +UPDATE r1 SET a = 30 RETURNING *; + a +---- + 30 +(1 row) + +ROLLBACK; DROP TABLE r1; -- Check dependency handling RESET SESSION AUTHORIZATION; diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql index dec7340538..7ebf89e2ce 100644 --- a/src/test/regress/sql/rowsecurity.sql +++ b/src/test/regress/sql/rowsecurity.sql @@ -2030,6 +2030,17 @@ INSERT INTO r1 VALUES (10) INSERT INTO r1 VALUES (10) ON CONFLICT ON CONSTRAINT r1_pkey DO UPDATE SET a = 30; +-- Add an explicit WITH CHECK clause to the FOR SELECT policy, so that the +-- UPDATE that failed before can succeed +ALTER POLICY p1 ON r1 WITH CHECK (true); + +BEGIN; +UPDATE r1 SET a = 30 WHERE a = 10; +ROLLBACK; +BEGIN; +UPDATE r1 SET a = 30 RETURNING *; +ROLLBACK; + DROP TABLE r1; -- Check dependency handling -- 2.41.0
