Re: DROP OWNED BY fails to drop privileges granted by non-owners (was Re: [GENERAL] Bug, Feature, or what else?)

Tom Lane escribió:

> I believe the problem is that DROP OWNED for privileges is implemented
> by calling REVOKE. As noted upthread, when a superuser does REVOKE,
> it's executed as though the object owner did the REVOKE, so only
> privileges granted directly by the object owner go away. In this
> particular example, "DROP OWNED BY u1" makes the grant to u1 go away,
> and then the grant to u2 goes away via cascade ... but "DROP OWNED BY
> u2" fails to accomplish anything at all, because postgres never granted
> anything directly to u2.
>
> We haven't seen this reported before, probably because the use of
> GRANT OPTIONS isn't very common, but AFAICS it's been wrong since
> the invention of DROP OWNED.

So it seems.

I have been mulling this over today, and it seems to me that one
way to fix it would be to have ExecGrant_Objecttype() loop over all
possible grantors when determining what to revoke when it's called by
DROP OWNED. A proof-of-concept is below (RemoveRoleFromObjectACL would
set istmt.all_grantors to true, whereas ExecuteGrantStmt leaves it as
false and behaves as today).

I am not sure about the restrict_and_check_grant() call I left out in
the middle of the operation; it seems to me that if we limit DROP OWNED
to be called only by a superuser, we can get away without that check.
Or maybe we need a new version of that routine that will only apply the
bitmask and not raise any error messages.

The patch below is just for ExecGrant_Relation(), but as far as I can
tell all the ExecGrant_Objecttype() routines do pretty much the same
here, so I think it'd be better to refactor the select_best_grantor/
restrict_and_check_grant/merge_acl_with_grant sequence into a common
function, and then have that function apply the loop for all grantors.

Thoughts?

***************
*** 1891,1932 **** ExecGrant_Relation(InternalGrant *istmt)
AclObjectKind aclkind;

/* Determine ID to do the grant as, and available grant options */
! select_best_grantor(GetUserId(), this_privileges,
! old_acl, ownerId,
! &grantorId, &avail_goptions);
!
! switch (pg_class_tuple->relkind)
{
! case RELKIND_SEQUENCE:
! aclkind = ACL_KIND_SEQUENCE;
! break;
! default:
! aclkind = ACL_KIND_CLASS;
! break;
}

! /*
! * Restrict the privileges to what we can actually grant, and emit
! * the standards-mandated warning and error messages.
! */
! this_privileges =
! restrict_and_check_grant(istmt->is_grant, avail_goptions,
! istmt->all_privs, this_privileges,
! relOid, grantorId, aclkind,
! NameStr(pg_class_tuple->relname),
! 0, NULL);

! /*
! * Generate new ACL.
! */
! new_acl = merge_acl_with_grant(old_acl,
! istmt->is_grant,
! istmt->grant_option,
! istmt->behavior,
! istmt->grantees,
! this_privileges,
! grantorId,
! ownerId);

/*
* We need the members of both old and new ACLs so we can correct
--- 1895,1962 ----
AclObjectKind aclkind;

/* Determine ID to do the grant as, and available grant options */
! if (!istmt->all_grantors)
{
! select_best_grantor(GetUserId(), this_privileges,
! old_acl, ownerId,
! &grantorId, &avail_goptions);
!
! switch (pg_class_tuple->relkind)
! {
! case RELKIND_SEQUENCE:
! aclkind = ACL_KIND_SEQUENCE;
! break;
! default:
! aclkind = ACL_KIND_CLASS;
! break;
! }
!
! /*
! * Restrict the privileges to what we can actually grant, and emit
! * the standards-mandated warning and error messages.
! */
! this_privileges =
! restrict_and_check_grant(istmt->is_grant, avail_goptions,
! istmt->all_privs, this_privileges,
! relOid, grantorId, aclkind,
! NameStr(pg_class_tuple->relname),
! 0, NULL);
!
! /*
! * Generate new ACL.
! */
! new_acl = merge_acl_with_grant(old_acl,
! istmt->is_grant,
! istmt->grant_option,
! istmt->behavior,
! istmt->grantees,
! this_privileges,
! grantorId,
! ownerId);
}
+ else
+ {
+ int ngrantors;
+ Oid *grantors;
+ int i;
+
+ ngrantors = aclgrantors(old_acl, istmt->grantees, &grantors);

! new_acl = aclcopy(old_acl);
! for (i = 0; i < ngrantors; i++)
! {
! Oid grantorId = grantors[i];

! new_acl = merge_acl_with_grant(new_acl,
! istmt->is_grant,
! istmt->grant_option,
! istmt->behavior,
! istmt->grantees,
! this_privileges,
! grantorId,
! ownerId);
! }
! }

/*
* We need the members of both old and new ACLs so we can correct

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services