diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c index fbbbd4914b..e5e4e15e5f 100644 --- 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 11fa632faa..3558ff87d1 100644 --- 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 c0bceee1ec..585a53a986 100644 --- 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