Subject: Re: Re: Re: Revoke Connect Privilege from Database not working

From: "David G. Johnston" <[email-masked]>
Date: Mon, 7 Apr 2025 08:37:43 -0700

On Mon, Apr 7, 2025 at 7:27 AM Ing. Marijo Kristo <[email-masked]>
wrote:

> Hi,
> here is a full reproducer. Also revoking with the granted by clause does
> not work.
>
> #clean initialization
> postgres=# create database testdb owner postgres;
> CREATE DATABASE
> postgres=# create user test_admin createrole;
> CREATE ROLE
> postgres=# alter user test_admin with password 'test1234';
> ALTER ROLE
> postgres=# grant connect on database testdb to test_admin with grant
> option;
> GRANT
>
> #create user and grant connect privilege with test_admin
> postgres=# set role test_admin;
> SET
> postgres=> create user test_user password 'testuserpw';
> CREATE ROLE
> postgres=> grant connect on database testdb to test_user;
> GRANT
>
> #generate the failure by granting test_admin superuser privileges
> postgres=> reset role;
> RESET
> postgres=# alter user test_admin superuser;
> ALTER ROLE
> postgres=# set role test_admin;
> SET
> postgres=# revoke connect on database testdb from test_user;
> REVOKE
> postgres=# drop user test_user;
> ERROR:  role "test_user" cannot be dropped because some objects depend on
> it
> DETAIL:  privileges for database testdb
>
> #test also with "granted by clause"
> postgres=# revoke connect on database testdb from test_user granted by
> "test_admin";
> REVOKE
>

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.

David J.

# revokescript.psql
create database testdb:v;
create user test_admin:v createrole;
grant connect on database testdb:v to test_admin:v with grant option;
set role test_admin:v;
create user test_user:v password 'testuserpw';
grant connect on database testdb:v to test_user:v;
reset role;
alter user test_admin:v superuser;
set role test_admin:v;
revoke connect on database testdb:v from test_user:v granted by
test_admin:v;
\l+ testdb:v
drop user test_user:v;

> psql postgres --file revokescript.psql -v v=1

---

From: Tom Lane <[email-masked]>
Date: Mon, 07 Apr 2025 12:06:17 -0400

> 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

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 7 Apr 2025 09:22:45 -0700

> Include GRANTED BY in the REVOKE to override the default

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.

revoke connect on database testdb:v
from test_user:v
---------------
granted by test_admin:v;
---^^^^^^^^^

So if we stick with status quo behavior we'd need to write the following
because the ignoring part is a POLA violation:

If a superuser chooses to issue a GRANT or REVOKE command, the command is
performed as though it were issued by the owner of the affected object, and
the granted by clause is ignored.

David J.

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 13 Nov 2025 10:47:14 -0600

> 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
>From 7949dc2dd3bda2b93f0125311fea1d8fa60e6263 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Thu, 13 Nov 2025 09:29:35 -0600
Subject: [PATCH v1 1/1] GRANTED BY

---
 src/backend/catalog/aclchk.c             | 89 +++++++++++++++++-------
 src/backend/utils/adt/acl.c              |  2 +-
 src/include/utils/acl.h                  |  2 +
 src/include/utils/aclchk_internal.h      |  1 +
 src/test/regress/expected/privileges.out |  2 +-
 5 files changed, 68 insertions(+), 28 deletions(-)

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index cd139bd65a6..9ee3ff9e494 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -397,19 +397,15 @@ ExecuteGrantStmt(GrantStmt *stmt)
 
 	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())
+		istmt.grantor = get_rolespec_oid(stmt->grantor, false);
+		if (!has_privs_of_role(GetUserId(), istmt.grantor))
 			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("grantor must be current user")));
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("must inherit privileges of role \"%s\"",
+							GetUserNameFromId(istmt.grantor, false))));
 	}
+	else
+		istmt.grantor = InvalidOid;
 
 	/*
 	 * Turn the regular GrantStmt into the InternalGrant form.
@@ -1538,6 +1534,7 @@ RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
 		istmt.all_privs = true;
 		istmt.privileges = ACL_NO_RIGHTS;
 		istmt.col_privs = NIL;
+		istmt.grantor = InvalidOid;
 		istmt.grantees = list_make1_oid(roleid);
 		istmt.grant_option = false;
 		istmt.behavior = DROP_CASCADE;
@@ -1694,9 +1691,17 @@ 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,
-						merged_acl, ownerId,
-						&grantorId, &avail_goptions);
+	if (OidIsValid(istmt->grantor))
+	{
+		grantorId = istmt->grantor;
+		avail_goptions = aclmask_direct(merged_acl, grantorId, ownerId,
+										ACL_GRANT_OPTION_FOR(col_privileges),
+										ACLMASK_ALL);
+	}
+	else
+		select_best_grantor(GetUserId(), col_privileges,
+							merged_acl, ownerId,
+							&grantorId, &avail_goptions);
 
 	pfree(merged_acl);
 
@@ -1967,9 +1972,17 @@ ExecGrant_Relation(InternalGrant *istmt)
 			ObjectType	objtype;
 
 			/* Determine ID to do the grant as, and available grant options */
-			select_best_grantor(GetUserId(), this_privileges,
-								old_acl, ownerId,
-								&grantorId, &avail_goptions);
+			if (OidIsValid(istmt->grantor))
+			{
+				grantorId = istmt->grantor;
+				avail_goptions = aclmask_direct(old_acl, grantorId, ownerId,
+												ACL_GRANT_OPTION_FOR(this_privileges),
+												ACLMASK_ALL);
+			}
+			else
+				select_best_grantor(GetUserId(), this_privileges,
+									old_acl, ownerId,
+									&grantorId, &avail_goptions);
 
 			switch (pg_class_tuple->relkind)
 			{
@@ -2182,9 +2195,17 @@ 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,
-							old_acl, ownerId,
-							&grantorId, &avail_goptions);
+		if (OidIsValid(istmt->grantor))
+		{
+			grantorId = istmt->grantor;
+			avail_goptions = aclmask_direct(old_acl, grantorId, ownerId,
+											ACL_GRANT_OPTION_FOR(istmt->privileges),
+											ACLMASK_ALL);
+		}
+		else
+			select_best_grantor(GetUserId(), istmt->privileges,
+								old_acl, ownerId,
+								&grantorId, &avail_goptions);
 
 		nameDatum = SysCacheGetAttrNotNull(cacheid, tuple,
 										   get_object_attnum_name(classid));
@@ -2337,9 +2358,17 @@ ExecGrant_Largeobject(InternalGrant *istmt)
 		}
 
 		/* Determine ID to do the grant as, and available grant options */
-		select_best_grantor(GetUserId(), istmt->privileges,
-							old_acl, ownerId,
-							&grantorId, &avail_goptions);
+		if (OidIsValid(istmt->grantor))
+		{
+			grantorId = istmt->grantor;
+			avail_goptions = aclmask_direct(old_acl, grantorId, ownerId,
+											ACL_GRANT_OPTION_FOR(istmt->privileges),
+											ACLMASK_ALL);
+		}
+		else
+			select_best_grantor(GetUserId(), istmt->privileges,
+								old_acl, ownerId,
+								&grantorId, &avail_goptions);
 
 		/*
 		 * Restrict the privileges to what we can actually grant, and emit the
@@ -2483,9 +2512,17 @@ ExecGrant_Parameter(InternalGrant *istmt)
 		}
 
 		/* Determine ID to do the grant as, and available grant options */
-		select_best_grantor(GetUserId(), istmt->privileges,
-							old_acl, ownerId,
-							&grantorId, &avail_goptions);
+		if (OidIsValid(istmt->grantor))
+		{
+			grantorId = istmt->grantor;
+			avail_goptions = aclmask_direct(old_acl, grantorId, ownerId,
+											ACL_GRANT_OPTION_FOR(istmt->privileges),
+											ACLMASK_ALL);
+		}
+		else
+			select_best_grantor(GetUserId(), istmt->privileges,
+								old_acl, ownerId,
+								&grantorId, &avail_goptions);
 
 		/*
 		 * Restrict the privileges to what we can actually grant, and emit the
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index fbcd64a2609..bf078888b0f 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -1473,7 +1473,7 @@ aclmask(const Acl *acl, Oid roleid, Oid ownerId,
  * This is exactly like aclmask() except that we consider only privileges
  * held *directly* by roleid, not those inherited via role membership.
  */
-static AclMode
+AclMode
 aclmask_direct(const Acl *acl, Oid roleid, Oid ownerId,
 			   AclMode mask, AclMaskHow how)
 {
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 01ae5b719fd..50d3b42bfcc 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -207,6 +207,8 @@ extern bool aclequal(const Acl *left_acl, const Acl *right_acl);
 
 extern AclMode aclmask(const Acl *acl, Oid roleid, Oid ownerId,
 					   AclMode mask, AclMaskHow how);
+extern AclMode aclmask_direct(const Acl *acl, Oid roleid, Oid ownerId,
+							  AclMode mask, AclMaskHow how);
 extern int	aclmembers(const Acl *acl, Oid **roleids);
 
 extern bool has_privs_of_role(Oid member, Oid role);
diff --git a/src/include/utils/aclchk_internal.h b/src/include/utils/aclchk_internal.h
index 62af290779a..051a9630256 100644
--- a/src/include/utils/aclchk_internal.h
+++ b/src/include/utils/aclchk_internal.h
@@ -36,6 +36,7 @@ typedef struct
 	bool		all_privs;
 	AclMode		privileges;
 	List	   *col_privs;
+	Oid			grantor;
 	List	   *grantees;
 	bool		grant_option;
 	DropBehavior behavior;
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    
-- 
2.39.5 (Apple Git-154)

---

From: Tom Lane <[email-masked]>
Date: Tue, 20 Jan 2026 18:05:41 -0500

> 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;

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 21 Jan 2026 09:28:53 -0600

> 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