mirror of
https://git.postgresql.org/git/postgresql.git
synced 2024-08-31 15:47:22 +02:00
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
This commit is contained in:
parent
4b8b3562e1
commit
ea5ae3ae1a
@ -17,6 +17,7 @@
|
|||||||
#include "access/htup.h"
|
#include "access/htup.h"
|
||||||
#include "access/htup_details.h"
|
#include "access/htup_details.h"
|
||||||
#include "access/sysattr.h"
|
#include "access/sysattr.h"
|
||||||
|
#include "access/xact.h"
|
||||||
#include "catalog/catalog.h"
|
#include "catalog/catalog.h"
|
||||||
#include "catalog/dependency.h"
|
#include "catalog/dependency.h"
|
||||||
#include "catalog/indexing.h"
|
#include "catalog/indexing.h"
|
||||||
@ -424,10 +425,9 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
|
|||||||
Oid relid;
|
Oid relid;
|
||||||
Relation rel;
|
Relation rel;
|
||||||
ArrayType *policy_roles;
|
ArrayType *policy_roles;
|
||||||
int num_roles;
|
|
||||||
Datum roles_datum;
|
Datum roles_datum;
|
||||||
bool attr_isnull;
|
bool attr_isnull;
|
||||||
bool noperm = true;
|
bool keep_policy = true;
|
||||||
|
|
||||||
Assert(classid == PolicyRelationId);
|
Assert(classid == PolicyRelationId);
|
||||||
|
|
||||||
@ -480,31 +480,20 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
|
|||||||
|
|
||||||
policy_roles = DatumGetArrayTypePCopy(roles_datum);
|
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. */
|
/* Must own relation. */
|
||||||
if (pg_class_ownercheck(relid, GetUserId()))
|
if (!pg_class_ownercheck(relid, GetUserId()))
|
||||||
noperm = false; /* user is allowed to modify this policy */
|
|
||||||
else
|
|
||||||
ereport(WARNING,
|
ereport(WARNING,
|
||||||
(errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
|
(errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
|
||||||
errmsg("role \"%s\" could not be removed from policy \"%s\" on \"%s\"",
|
errmsg("role \"%s\" could not be removed from policy \"%s\" on \"%s\"",
|
||||||
GetUserNameFromId(roleid, false),
|
GetUserNameFromId(roleid, false),
|
||||||
NameStr(((Form_pg_policy) GETSTRUCT(tuple))->polname),
|
NameStr(((Form_pg_policy) GETSTRUCT(tuple))->polname),
|
||||||
RelationGetRelationName(rel))));
|
RelationGetRelationName(rel))));
|
||||||
|
else
|
||||||
/*
|
|
||||||
* If multiple roles exist on this policy, then remove the one we were
|
|
||||||
* asked to and leave the rest.
|
|
||||||
*/
|
|
||||||
if (!noperm && num_roles > 0)
|
|
||||||
{
|
{
|
||||||
int i,
|
int i,
|
||||||
j;
|
j;
|
||||||
Oid *roles = (Oid *) ARR_DATA_PTR(policy_roles);
|
Oid *roles = (Oid *) ARR_DATA_PTR(policy_roles);
|
||||||
|
int num_roles = ARR_DIMS(policy_roles)[0];
|
||||||
Datum *role_oids;
|
Datum *role_oids;
|
||||||
char *qual_value;
|
char *qual_value;
|
||||||
Node *qual_expr;
|
Node *qual_expr;
|
||||||
@ -578,16 +567,22 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
|
|||||||
else
|
else
|
||||||
with_check_qual = NULL;
|
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));
|
role_oids = (Datum *) palloc(num_roles * sizeof(Datum));
|
||||||
for (i = 0, j = 0; i < ARR_DIMS(policy_roles)[0]; i++)
|
for (i = 0, j = 0; i < num_roles; i++)
|
||||||
/* Copy over all of the roles which are not the one being removed */
|
{
|
||||||
if (roles[i] != roleid)
|
if (roles[i] != roleid)
|
||||||
role_oids[j++] = ObjectIdGetDatum(roles[i]);
|
role_oids[j++] = ObjectIdGetDatum(roles[i]);
|
||||||
|
}
|
||||||
|
num_roles = j;
|
||||||
|
|
||||||
/* We should have only removed the one role */
|
/* If any roles remain, update the policy entry. */
|
||||||
Assert(j == num_roles);
|
if (num_roles > 0)
|
||||||
|
{
|
||||||
/* This is the array for the new tuple */
|
/* This is the array for the new tuple */
|
||||||
role_ids = construct_array(role_oids, num_roles, OIDOID,
|
role_ids = construct_array(role_oids, num_roles, OIDOID,
|
||||||
sizeof(Oid), true, 'i');
|
sizeof(Oid), true, 'i');
|
||||||
@ -642,9 +637,18 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
|
|||||||
|
|
||||||
heap_freetuple(new_tuple);
|
heap_freetuple(new_tuple);
|
||||||
|
|
||||||
|
/* Make updates visible */
|
||||||
|
CommandCounterIncrement();
|
||||||
|
|
||||||
/* Invalidate Relation Cache */
|
/* Invalidate Relation Cache */
|
||||||
CacheInvalidateRelcache(rel);
|
CacheInvalidateRelcache(rel);
|
||||||
}
|
}
|
||||||
|
else
|
||||||
|
{
|
||||||
|
/* No roles would remain, so drop the policy instead */
|
||||||
|
keep_policy = false;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/* Clean up. */
|
/* Clean up. */
|
||||||
systable_endscan(sscan);
|
systable_endscan(sscan);
|
||||||
@ -653,7 +657,7 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
|
|||||||
|
|
||||||
heap_close(pg_policy_rel, RowExclusiveLock);
|
heap_close(pg_policy_rel, RowExclusiveLock);
|
||||||
|
|
||||||
return (noperm || num_roles > 0);
|
return keep_policy;
|
||||||
}
|
}
|
||||||
|
|
||||||
/*
|
/*
|
||||||
|
@ -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);
|
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 OWNED BY regress_rls_dob_role1;
|
||||||
DROP POLICY p1 ON dob_t1; -- should succeed
|
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);
|
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 OWNED BY regress_rls_dob_role1;
|
||||||
DROP POLICY p1 ON dob_t2; -- should succeed
|
DROP POLICY p1 ON dob_t2; -- should succeed
|
||||||
|
@ -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 OWNED BY regress_rls_dob_role1;
|
||||||
DROP POLICY p1 ON dob_t1; -- should succeed
|
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);
|
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 OWNED BY regress_rls_dob_role1;
|
||||||
DROP POLICY p1 ON dob_t2; -- should succeed
|
DROP POLICY p1 ON dob_t2; -- should succeed
|
||||||
|
Loading…
Reference in New Issue
Block a user