From: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Adrian Klaver <adrian(dot)klaver(at)gmail(dot)com>, Andreas Kretschmer <andreas(at)a-kretschmer(dot)de>, pgsql-hackers(at)postgreSQL(dot)org |
Subject: | Re: DROP OWNED BY fails to drop privileges granted by non-owners (was Re: [GENERAL] Bug, Feature, or what else?) |
Date: | 2013-03-22 23:12:02 |
Message-ID: | 20130322231202.GB3701@alvh.no-ip.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-general pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Joe Van Dyk | 2013-03-23 01:22:44 | Group by -- precedence question |
Previous Message | Ryan Kelly | 2013-03-22 20:57:14 | Re: PostgreSQL EXCLUDE USING error: Data type integer has no default operator class |
From | Date | Subject | |
---|---|---|---|
Next Message | Adriano Lange | 2013-03-22 23:35:43 | SDP query optimizer |
Previous Message | Stas Kelvich | 2013-03-22 23:10:38 | Cube extension improvement, GSoC |