Fix misbehavior of DROP OWNED BY with duplicate polroles entries.

Ordinarily, a pg_policy.polroles array wouldn't list the same role
more than once; but CREATE POLICY does not prevent that.  If we
perform DROP OWNED BY on a role that is listed more than once,
RemoveRoleFromObjectPolicy either suffered an assertion failure
or encountered a tuple-updated-by-self error.  Rewrite it to cope
correctly with duplicate entries, and add a CommandCounterIncrement
call to prevent the other problem.

Per discussion, there's other cleanup that ought to happen here,
but this seems like the minimum essential fix.

Per bug #17062 from Alexander Lakhin.  It's been broken all along,
so back-patch to all supported branches.

Discussion: https://postgr.es/m/17062-11f471ae3199ca23@postgresql.org

diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index fbbbd4914b1a7655f8768b17cdb37199cd19d73f..e5e4e15e5f8b82741ad3483aac634b80c6ce5230 100644 (file)
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -17,6 +17,7 @@
 #include "access/htup.h"
 #include "access/htup_details.h"
 #include "access/sysattr.h"
+#include "access/xact.h"
 #include "catalog/catalog.h"
 #include "catalog/dependency.h"
 #include "catalog/indexing.h"
@@ -424,10 +425,9 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
    Oid         relid;
    Relation    rel;
    ArrayType  *policy_roles;
-   int         num_roles;
    Datum       roles_datum;
    bool        attr_isnull;
-   bool        noperm = true;
+   bool        keep_policy = true;
 
    Assert(classid == PolicyRelationId);
 
@@ -480,31 +480,20 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
 
    policy_roles = DatumGetArrayTypePCopy(roles_datum);
 
-   /* We should be removing exactly one entry from the roles array */
-   num_roles = ARR_DIMS(policy_roles)[0] - 1;
-
-   Assert(num_roles >= 0);
-
    /* Must own relation. */
-   if (pg_class_ownercheck(relid, GetUserId()))
-       noperm = false;         /* user is allowed to modify this policy */
-   else
+   if (!pg_class_ownercheck(relid, GetUserId()))
        ereport(WARNING,
                (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
                 errmsg("role \"%s\" could not be removed from policy \"%s\" on \"%s\"",
                        GetUserNameFromId(roleid, false),
                        NameStr(((Form_pg_policy) GETSTRUCT(tuple))->polname),
                        RelationGetRelationName(rel))));
-
-   /*
-    * If multiple roles exist on this policy, then remove the one we were
-    * asked to and leave the rest.
-    */
-   if (!noperm && num_roles > 0)
+   else
    {
        int         i,
                    j;
        Oid        *roles = (Oid *) ARR_DATA_PTR(policy_roles);
+       int         num_roles = ARR_DIMS(policy_roles)[0];
        Datum      *role_oids;
        char       *qual_value;
        Node       *qual_expr;
@@ -578,16 +567,22 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
        else
            with_check_qual = NULL;
 
-       /* Rebuild the roles array to then update the pg_policy tuple with */
+       /*
+        * Rebuild the roles array, without any mentions of the target role.
+        * Ordinarily there'd be exactly one, but we must cope with duplicate
+        * mentions, since CREATE/ALTER POLICY historically have allowed that.
+        */
        role_oids = (Datum *) palloc(num_roles * sizeof(Datum));
-       for (i = 0, j = 0; i < ARR_DIMS(policy_roles)[0]; i++)
-           /* Copy over all of the roles which are not the one being removed */
+       for (i = 0, j = 0; i < num_roles; i++)
+       {
            if (roles[i] != roleid)
                role_oids[j++] = ObjectIdGetDatum(roles[i]);
+       }
+       num_roles = j;
 
-       /* We should have only removed the one role */
-       Assert(j == num_roles);
-
+       /* If any roles remain, update the policy entry. */
+       if (num_roles > 0)
+       {
        /* This is the array for the new tuple */
        role_ids = construct_array(role_oids, num_roles, OIDOID,
                                   sizeof(Oid), true, 'i');
@@ -642,8 +637,17 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
 
        heap_freetuple(new_tuple);
 
+       /* Make updates visible */
+       CommandCounterIncrement();
+
        /* Invalidate Relation Cache */
        CacheInvalidateRelcache(rel);
+       }
+       else
+       {
+           /* No roles would remain, so drop the policy instead */
+           keep_policy = false;
+       }
    }
 
    /* Clean up. */
@@ -653,7 +657,7 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
 
    heap_close(pg_policy_rel, RowExclusiveLock);
 
-   return (noperm || num_roles > 0);
+   return keep_policy;
 }
 
 /*

diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 11fa632faa0ab7b75c2746aedefe599972b73c4f..3558ff87d1e472854af6354ec7efb015c121ca04 100644 (file)
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -3906,6 +3906,15 @@ ERROR:  policy "p1" for table "dob_t1" does not exist
 CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
 DROP OWNED BY regress_rls_dob_role1;
 DROP POLICY p1 ON dob_t1; -- should succeed
+-- same cases with duplicate polroles entries
+CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role1 USING (true);
+DROP OWNED BY regress_rls_dob_role1;
+DROP POLICY p1 ON dob_t1; -- should fail, already gone
+ERROR:  policy "p1" for table "dob_t1" does not exist
+CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
+DROP OWNED BY regress_rls_dob_role1;
+DROP POLICY p1 ON dob_t1; -- should succeed
+-- partitioned target
 CREATE POLICY p1 ON dob_t2 TO regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
 DROP OWNED BY regress_rls_dob_role1;
 DROP POLICY p1 ON dob_t2; -- should succeed

diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index c0bceee1eca068c7f48f85949b28f3db3edd196c..585a53a986e377bb34c4d62388f7c4095a168b4a 100644 (file)
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -1760,6 +1760,16 @@ CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role2 USING
 DROP OWNED BY regress_rls_dob_role1;
 DROP POLICY p1 ON dob_t1; -- should succeed
 
+-- same cases with duplicate polroles entries
+CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role1 USING (true);
+DROP OWNED BY regress_rls_dob_role1;
+DROP POLICY p1 ON dob_t1; -- should fail, already gone
+
+CREATE POLICY p1 ON dob_t1 TO regress_rls_dob_role1,regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
+DROP OWNED BY regress_rls_dob_role1;
+DROP POLICY p1 ON dob_t1; -- should succeed
+
+-- partitioned target
 CREATE POLICY p1 ON dob_t2 TO regress_rls_dob_role1,regress_rls_dob_role2 USING (true);
 DROP OWNED BY regress_rls_dob_role1;
 DROP POLICY p1 ON dob_t2; -- should succeed