Обсуждение: Re:   Re: Re: Revoke Connect Privilege from Database not working

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On master, confirmed that after this command the privilege:
> test_user=c/test_admin (on database testdb) still exists.  That seems like
> a bug. Its at least a POLA violation and I cannot figure out how to read
> the revoke reference page in a way that explains it.

I believe what's going on there is explained by the rule that
"grants and revokes done by a superuser are done as if issued
by the object owner".  So here, what would be revoked is
test_user=c/postgres, which isn't the privilege at issue.
Include GRANTED BY in the REVOKE to override the default
choice of grantor.

IIRC, said rule was invented before we had the GRANTED BY
syntax.  It probably doesn't make as much sense today,
but I'd be very afraid of breaking peoples' work flows
by changing it.

            regards, tom lane

On Mon, Apr 07, 2025 at 09:22:45AM -0700, David G. Johnston wrote:
> On Mon, Apr 7, 2025 at 9:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I believe what's going on there is explained by the rule that
>> "grants and revokes done by a superuser are done as if issued
>> by the object owner".  So here, what would be revoked is
>> test_user=c/postgres, which isn't the privilege at issue.
>> Include GRANTED BY in the REVOKE to override the default
>> choice of grantor.
> 
> The command in question did include "granted by" which is why this is a
> bug.  The explicit granted by specification is being ignored if the
> invoking user is a superuser.

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.  

-- 
nathan

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

On Tue, Jan 20, 2026 at 06:05:41PM -0500, Tom Lane wrote:
> 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.

Yeah, I think doing most of the work in select_best_grantor() is obviously
better.  I recall wondering whether we should check for INHERIT or SET
privilege (or both) on the grantor role, and IIRC I settled on INHERIT
because select_best_grantor() searches through roles we have INHERIT on.

Would you like to handle docs/tests/committing, or shall I?

-- 
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:
> Yeah, I think doing most of the work in select_best_grantor() is obviously
> better.  I recall wondering whether we should check for INHERIT or SET
> privilege (or both) on the grantor role, and IIRC I settled on INHERIT
> because select_best_grantor() searches through roles we have INHERIT on.

Yeah, I mentally had that point as something to check on.  Clearly,
if you have SET ROLE privilege then you can become the target role
and then issue the GRANT, so if we define GRANTED BY like that
then we're not allowing anything that can't be done today.  However,
it feels like INHERIT is a more natural test to make, since AIUI
that is what permits "automatic" use of a role's privileges, and that
seems to be what we'd be doing here.

I'd be interested to hear Robert's opinion on this, or somebody
else who worked on the SET/INHERIT splitup.

Also cc'ing Peter, who put in the current effectively-a-noise-clause
behavior of GRANTED BY, to see if he has standards-compliance or
other concerns about changing this.

> Would you like to handle docs/tests/committing, or shall I?

I'm willing to push it forward if we have consensus to do it.

            regards, tom lane

On Wed, Jan 21, 2026 at 11:57:01AM -0500, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> Yeah, I think doing most of the work in select_best_grantor() is obviously
>> better.  I recall wondering whether we should check for INHERIT or SET
>> privilege (or both) on the grantor role, and IIRC I settled on INHERIT
>> because select_best_grantor() searches through roles we have INHERIT on.
> 
> Yeah, I mentally had that point as something to check on.  Clearly,
> if you have SET ROLE privilege then you can become the target role
> and then issue the GRANT, so if we define GRANTED BY like that
> then we're not allowing anything that can't be done today.  However,
> it feels like INHERIT is a more natural test to make, since AIUI
> that is what permits "automatic" use of a role's privileges, and that
> seems to be what we'd be doing here.

Agreed.

> I'd be interested to hear Robert's opinion on this, or somebody
> else who worked on the SET/INHERIT splitup.
> 
> Also cc'ing Peter, who put in the current effectively-a-noise-clause
> behavior of GRANTED BY, to see if he has standards-compliance or
> other concerns about changing this.

Robert/Peter, do you have any thoughts about expanding GRANT/REVOKE GRANTED
BY like this?  I think it would've helped with a couple of reports received
during this development cycle, and IMHO it'd be a nice little feature for
v19.

-- 
nathan