Re: Revoke Connect Privilege from Database not working - Mailing list pgsql-bugs

Nathan Bossart <nathandbossart@gmail.com> writes:
> This is admittedly a half-formed idea, but perhaps we could have whatever's
> specified in GRANTED BY override select_best_grantor(), like in the
> attached patch.  I've no idea if this is the intention of the standard, but
> it should at least address the reported issue.  FWIW I recently received an
> independent report about the same thing.

Motivated by the discussion at [1], I'd started on the same idea,
but arrived at a rather different refactorization.  I think this
way is nicer (less duplicated logic).  Either way, we need to
address the docs and probably add more regression tests.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/85cd06c6-7b2e-483e-b05d-d5ff87b0168d%40garret.ru

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index a431fc0926f..e31d22ebf7d 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -98,6 +98,7 @@ typedef struct
     AclMode        privileges;
     List       *grantees;
     bool        grant_option;
+    RoleSpec   *grantor;
     DropBehavior behavior;
 } InternalDefaultACL;

@@ -395,22 +396,6 @@ ExecuteGrantStmt(GrantStmt *stmt)
     const char *errormsg;
     AclMode        all_privileges;

-    if (stmt->grantor)
-    {
-        Oid            grantor;
-
-        grantor = get_rolespec_oid(stmt->grantor, false);
-
-        /*
-         * Currently, this clause is only for SQL compatibility, not very
-         * interesting otherwise.
-         */
-        if (grantor != GetUserId())
-            ereport(ERROR,
-                    (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                     errmsg("grantor must be current user")));
-    }
-
     /*
      * Turn the regular GrantStmt into the InternalGrant form.
      */
@@ -438,6 +423,7 @@ ExecuteGrantStmt(GrantStmt *stmt)
     istmt.col_privs = NIL;        /* may get filled below */
     istmt.grantees = NIL;        /* filled below */
     istmt.grant_option = stmt->grant_option;
+    istmt.grantor = stmt->grantor;
     istmt.behavior = stmt->behavior;

     /*
@@ -960,6 +946,7 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
     /* privileges to be filled below */
     iacls.grantees = NIL;        /* filled below */
     iacls.grant_option = action->grant_option;
+    iacls.grantor = action->grantor;
     iacls.behavior = action->behavior;

     /*
@@ -1486,6 +1473,7 @@ RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
         iacls.privileges = ACL_NO_RIGHTS;
         iacls.grantees = list_make1_oid(roleid);
         iacls.grant_option = false;
+        iacls.grantor = NULL;
         iacls.behavior = DROP_CASCADE;

         /* Do it */
@@ -1542,6 +1530,7 @@ RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
         istmt.col_privs = NIL;
         istmt.grantees = list_make1_oid(roleid);
         istmt.grant_option = false;
+        istmt.grantor = NULL;
         istmt.behavior = DROP_CASCADE;

         ExecGrantStmt_oids(&istmt);
@@ -1696,7 +1685,7 @@ ExecGrant_Attribute(InternalGrant *istmt, Oid relOid, const char *relname,
     merged_acl = aclconcat(old_rel_acl, old_acl);

     /* Determine ID to do the grant as, and available grant options */
-    select_best_grantor(GetUserId(), col_privileges,
+    select_best_grantor(istmt->grantor, col_privileges,
                         merged_acl, ownerId,
                         &grantorId, &avail_goptions);

@@ -1969,7 +1958,7 @@ ExecGrant_Relation(InternalGrant *istmt)
             ObjectType    objtype;

             /* Determine ID to do the grant as, and available grant options */
-            select_best_grantor(GetUserId(), this_privileges,
+            select_best_grantor(istmt->grantor, this_privileges,
                                 old_acl, ownerId,
                                 &grantorId, &avail_goptions);

@@ -2184,7 +2173,7 @@ ExecGrant_common(InternalGrant *istmt, Oid classid, AclMode default_privs,
         }

         /* Determine ID to do the grant as, and available grant options */
-        select_best_grantor(GetUserId(), istmt->privileges,
+        select_best_grantor(istmt->grantor, istmt->privileges,
                             old_acl, ownerId,
                             &grantorId, &avail_goptions);

@@ -2339,7 +2328,7 @@ ExecGrant_Largeobject(InternalGrant *istmt)
         }

         /* Determine ID to do the grant as, and available grant options */
-        select_best_grantor(GetUserId(), istmt->privileges,
+        select_best_grantor(istmt->grantor, istmt->privileges,
                             old_acl, ownerId,
                             &grantorId, &avail_goptions);

@@ -2485,7 +2474,7 @@ ExecGrant_Parameter(InternalGrant *istmt)
         }

         /* Determine ID to do the grant as, and available grant options */
-        select_best_grantor(GetUserId(), istmt->privileges,
+        select_best_grantor(istmt->grantor, istmt->privileges,
                             old_acl, ownerId,
                             &grantorId, &avail_goptions);

diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 3a6905f9546..90fa49eacb7 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -5451,6 +5451,10 @@ select_best_admin(Oid member, Oid role)
 /*
  * Select the effective grantor ID for a GRANT or REVOKE operation.
  *
+ * If the GRANT/REVOKE has an explicit GRANTED BY clause, we always use
+ * exactly that role (which may result in granting/revoking no privileges).
+ * Otherwise, we seek a "best" grantor, starting with the current user.
+ *
  * The grantor must always be either the object owner or some role that has
  * been explicitly granted grant options.  This ensures that all granted
  * privileges appear to flow from the object owner, and there are never
@@ -5463,25 +5467,44 @@ select_best_admin(Oid member, Oid role)
  * role has 'em all.  In this case we pick a role with the largest number
  * of desired options.  Ties are broken in favor of closer ancestors.
  *
- * roleId: the role attempting to do the GRANT/REVOKE
+ * grantedBy: the GRANTED BY clause of GRANT/REVOKE, or NULL if none
  * privileges: the privileges to be granted/revoked
  * acl: the ACL of the object in question
  * ownerId: the role owning the object in question
  * *grantorId: receives the OID of the role to do the grant as
- * *grantOptions: receives the grant options actually held by grantorId
- *
- * If no grant options exist, we set grantorId to roleId, grantOptions to 0.
+ * *grantOptions: receives grant options actually held by grantorId (maybe 0)
  */
 void
-select_best_grantor(Oid roleId, AclMode privileges,
+select_best_grantor(const RoleSpec *grantedBy, AclMode privileges,
                     const Acl *acl, Oid ownerId,
                     Oid *grantorId, AclMode *grantOptions)
 {
+    Oid            roleId = GetUserId();
     AclMode        needed_goptions = ACL_GRANT_OPTION_FOR(privileges);
     List       *roles_list;
     int            nrights;
     ListCell   *l;

+    /*
+     * If we have GRANTED BY, resolve it and verify current user is allowed to
+     * specify that role.
+     */
+    if (grantedBy)
+    {
+        Oid            grantor = get_rolespec_oid(grantedBy, false);
+
+        if (!has_privs_of_role(roleId, grantor))
+            ereport(ERROR,
+                    (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                     errmsg("must inherit privileges of role \"%s\"",
+                            GetUserNameFromId(grantor, false))));
+        /* Use exactly that grantor, whether it has privileges or not */
+        *grantorId = grantor;
+        *grantOptions = aclmask_direct(acl, grantor, ownerId,
+                                       needed_goptions, ACLMASK_ALL);
+        return;
+    }
+
     /*
      * The object owner is always treated as having all grant options, so if
      * roleId is the owner it's easy.  Also, if roleId is a superuser it's
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 646d6ced763..b12f21d22e9 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2613,7 +2613,7 @@ typedef struct GrantStmt
     /* privileges == NIL denotes ALL PRIVILEGES */
     List       *grantees;        /* list of RoleSpec nodes */
     bool        grant_option;    /* grant or revoke grant option */
-    RoleSpec   *grantor;
+    RoleSpec   *grantor;        /* GRANTED BY clause, or NULL if none */
     DropBehavior behavior;        /* drop behavior (for REVOKE) */
 } GrantStmt;

diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index ec01fd581cf..9da62a7aa76 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -223,7 +223,7 @@ extern void check_rolespec_name(const RoleSpec *role, const char *detail_msg);
 extern HeapTuple get_rolespec_tuple(const RoleSpec *role);
 extern char *get_rolespec_name(const RoleSpec *role);

-extern void select_best_grantor(Oid roleId, AclMode privileges,
+extern void select_best_grantor(const RoleSpec *grantedBy, AclMode privileges,
                                 const Acl *acl, Oid ownerId,
                                 Oid *grantorId, AclMode *grantOptions);

diff --git a/src/include/utils/aclchk_internal.h b/src/include/utils/aclchk_internal.h
index 38317e2ed37..fa0b65fbba7 100644
--- a/src/include/utils/aclchk_internal.h
+++ b/src/include/utils/aclchk_internal.h
@@ -38,6 +38,7 @@ typedef struct
     List       *col_privs;
     List       *grantees;
     bool        grant_option;
+    RoleSpec   *grantor;
     DropBehavior behavior;
 } InternalGrant;

diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index daafaa94fde..997c4b68f47 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -321,7 +321,7 @@ SELECT pg_get_acl(0, 0, 0); -- null
 (1 row)

 GRANT TRUNCATE ON atest2 TO regress_priv_user4 GRANTED BY regress_priv_user5;  -- error
-ERROR:  grantor must be current user
+ERROR:  must inherit privileges of role "regress_priv_user5"
 SET SESSION AUTHORIZATION regress_priv_user2;
 SELECT session_user, current_user;
     session_user    |    current_user