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 040b21acc0cc214d8480adfdadc860d0a26d7820..9f17ed9e3003203ed8ffe8981292108a4e16bb90 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"
@@ -418,10 +419,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);
 
@@ -473,31 +473,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;
@@ -571,16 +560,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');
@@ -638,8 +633,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. */
@@ -649,7 +653,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 f82f93ea414cd3d74f7d61d0a9e84ead70b2852f..ceb367e716d1bd37be073d468de909079ca04e7b 100644 (file)
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -3461,6 +3461,14 @@ 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
 DROP USER regress_rls_dob_role1;
 DROP USER regress_rls_dob_role2;
 -- Bug #15708: view + table with RLS should check policies as view owner

diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index 54bf205476351df91d47117547282e5db309175a..96ed83fe62ac316c5eb51e808b291eb8d390d8e1 100644 (file)
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -1572,6 +1572,15 @@ 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
+
 DROP USER regress_rls_dob_role1;
 DROP USER regress_rls_dob_role2;