Re: DROP OWNED BY fails to drop privileges granted by non-owners (was Re: [GENERAL] Bug, Feature, or what else?) - Mailing list 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;              /*
DetermineID 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
andnew ACLs so we can correct 
--- 1895,1962 ----             AclObjectKind aclkind;              /* Determine ID to do the grant as, and available
grantoptions */ 
!             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