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

From: "David G. Johnston" <[email-masked]>
Date: Mon, 7 Apr 2025 06:42:28 -0700

On Monday, April 7, 2025, Ing. Marijo Kristo <[email-masked]>
wrote:
>
>
> Seems like a bug to me.
> Can someone else verifiy this ?
>

It would help greatly if you create a reproducer that starts from a clean
install, creates the roles and database, and demonstrates the issue.

> postgres=# \du vault_admin;
>             List of roles
>   Role name  |       Attributes
> -------------+------------------------
> vault_admin | Superuser, Create role
>
> postgres=# set role vault_admin;
>

You are setting role to another role that has superuser which is basically
pointless.

Use “granted by” in your revoke command.  If that works this isn’t a bug.

David J.

---

From: "Ing. Marijo Kristo" <[email-masked]>
Date: Mon, 7 Apr 2025 14:27:41 +0000 (UTC)

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 postgres=# drop user test_user; ERROR: role "test_user" cannot be dropped because some objects depend on it DETAIL: privileges for database testdb #fix by removing superuser privilege from test_admin postgres=# reset role; RESET postgres=# alter user test_admin nosuperuser; ALTER ROLE postgres=# set role test_admin; SET postgres=> revoke connect on database testdb from test_user; REVOKE postgres=> drop role test_user; DROP ROLE Best Regards Marijo Kristo David G. Johnston <[email-masked]> schrieb am 7. Apr. 2025 um 15:42: On Monday, April 7, 2025, Ing. Marijo Kristo < [email-masked] > wrote: Seems like a bug to me. Can someone else verifiy this ? It would help greatly if you create a reproducer that starts from a clean install, creates the roles and database, and demonstrates the issue. postgres=# \du vault_admin; List of roles Role name | Attributes -------------+---------------- -------- vault_admin | Superuser, Create role postgres=# set role vault_admin; You are setting role to another role that has superuser which is basically pointless. Use “granted by” in your revoke command. If that works this isn’t a bug. David J.

---

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

---

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

---

From: Tom Lane <[email-masked]>
Date: Wed, 21 Jan 2026 11:57:01 -0500

> 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

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 6 Mar 2026 16:01:50 -0600

> seems to be what we'd be doing here.

Agreed.

> 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

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 17 Mar 2026 19:50:33 -0500

I went ahead and tried adding docs, tests, and a commit message.  The
documentation for these commands might need a revamp.  They seem to meander
a bit, probably due to decades of organic development.  But that's probably
not this patch's problem.

-- 
nathan

---

From: Tom Lane <[email-masked]>
Date: Wed, 18 Mar 2026 12:36:17 -0400

Nathan Bossart <[email-masked]> writes:
> I went ahead and tried adding docs, tests, and a commit message.

Thanks!  I was about to conclude that "silence means assent" and
do that work, but you beat me to it.  Your changes look fine,
except that where you have

+   ... A role can only attribute a grant
+   to another role if they possess the privileges of that role.

the word "possess" seems a little ambiguous --- it's not clear whether
it means SET or INHERIT privileges.  The grammar nerd in me doesn't
like "they" either.  How about s/they possess/it inherits/ ?
(likewise in revoke.sgml)

> The

Agreed, seems like a task for another day.

			regards, tom lane

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 18 Mar 2026 12:47:17 -0500

> except that where you have

Thanks for reviewing.

> (likewise in revoke.sgml)

This crossed my mind when copy/pasting from the "GRANT on Roles" section,
but I obviously didn't do anything about it.  Fixed in the attached.

-- 
nathan

---

From: Tom Lane <[email-masked]>
Date: Wed, 18 Mar 2026 13:58:55 -0400

> but I obviously didn't do anything about it.  Fixed in the attached.

v4 LGTM.

			regards, tom lane

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 19 Mar 2026 11:44:30 -0500

> v4 LGTM.

Thanks, committed.

-- 
nathan