public inbox for pgsql-sql@postgresql.org
help / color / mirror / Atom feedFrom: Tom Lane <tgl@sss.pgh.pa.us>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: David G. Johnston <david.g.johnston@gmail.com>
Cc: Ing. Marijo Kristo <marijo.kristo@icloud.com>
Cc: PostgreSQL Bug List <pgsql-bugs@lists.postgresql.org>
Subject: Re: Revoke Connect Privilege from Database not working
Date: Tue, 20 Jan 2026 18:05:41 -0500
Message-ID: <1933586.1768950341@sss.pgh.pa.us> (raw)
In-Reply-To: <aRYLkTpazxKhnS_w@nathan>
References: <CAKFQuwa7m2smqqpgPetw=i8Aj-xqg9Zjc5Z2aX3AUwNh96WnXw@mail.gmail.com>
<d9bf666c-4d11-4196-99a8-b71d01d9ad40@me.com>
<CAKFQuwbB-ZKtN_p_y5sWa2MrTuy5=pRNPWSj1Ud4HHvTuhb54w@mail.gmail.com>
<3467676.1744041977@sss.pgh.pa.us>
<CAKFQuwbpC5w6sUq8gZQATrviZUT4bYpxW+=2uH6sWWMg7fWjzg@mail.gmail.com>
<aRYLkTpazxKhnS_w@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
Attachments:
[text/x-diff] v2-0001-GRANTED-BY.patch (8.4K, 2-v2-0001-GRANTED-BY.patch)
download | inline diff:
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
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-sql@postgresql.org
Cc: tgl@sss.pgh.pa.us, nathandbossart@gmail.com, david.g.johnston@gmail.com, marijo.kristo@icloud.com, pgsql-bugs@lists.postgresql.org
Subject: Re: Revoke Connect Privilege from Database not working
In-Reply-To: <1933586.1768950341@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox