public inbox for [email protected]help / color / mirror / Atom feed
Re: Re: Re: Revoke Connect Privilege from Database not working 6+ messages / 3 participants [nested] [flat]
* Re: Re: Re: Revoke Connect Privilege from Database not working @ 2025-04-07 15:37 David G. Johnston <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: David G. Johnston @ 2025-04-07 15:37 UTC (permalink / raw) To: Ing. Marijo Kristo <[email protected]>; PostgreSQL Bug List <[email protected]> On Mon, Apr 7, 2025 at 7:27 AM Ing. Marijo Kristo <[email protected]> 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 ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Re: Re: Revoke Connect Privilege from Database not working @ 2025-04-07 16:06 Tom Lane <[email protected]> parent: David G. Johnston <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Tom Lane @ 2025-04-07 16:06 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Ing. Marijo Kristo <[email protected]>; PostgreSQL Bug List <[email protected]> "David G. Johnston" <[email protected]> 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 ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Re: Re: Revoke Connect Privilege from Database not working @ 2025-04-07 16:22 David G. Johnston <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: David G. Johnston @ 2025-04-07 16:22 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Ing. Marijo Kristo <[email protected]>; PostgreSQL Bug List <[email protected]> On Mon, Apr 7, 2025 at 9:06 AM Tom Lane <[email protected]> wrote: > "David G. Johnston" <[email protected]> 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. > 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. ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Re: Re: Revoke Connect Privilege from Database not working @ 2025-11-13 16:47 Nathan Bossart <[email protected]> parent: David G. Johnston <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Nathan Bossart @ 2025-11-13 16:47 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Tom Lane <[email protected]>; Ing. Marijo Kristo <[email protected]>; PostgreSQL Bug List <[email protected]> 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 <[email protected]> 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 From 7949dc2dd3bda2b93f0125311fea1d8fa60e6263 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> 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) Attachments: [text/plain] v1-0001-GRANTED-BY.patch (7.3K, 2-v1-0001-GRANTED-BY.patch) download | inline diff: From 7949dc2dd3bda2b93f0125311fea1d8fa60e6263 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> 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) ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Revoke Connect Privilege from Database not working @ 2026-01-20 23:05 Tom Lane <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Tom Lane @ 2026-01-20 23:05 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: David G. Johnston <[email protected]>; Ing. Marijo Kristo <[email protected]>; PostgreSQL Bug List <[email protected]> Nathan Bossart <[email protected]> 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 ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Revoke Connect Privilege from Database not working @ 2026-01-21 15:28 Nathan Bossart <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Nathan Bossart @ 2026-01-21 15:28 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: David G. Johnston <[email protected]>; Ing. Marijo Kristo <[email protected]>; PostgreSQL Bug List <[email protected]> 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 ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-01-21 15:28 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2025-04-07 15:37 Re: Re: Re: Revoke Connect Privilege from Database not working David G. Johnston <[email protected]> 2025-04-07 16:06 ` Tom Lane <[email protected]> 2025-04-07 16:22 ` David G. Johnston <[email protected]> 2025-11-13 16:47 ` Nathan Bossart <[email protected]> 2026-01-20 23:05 ` Tom Lane <[email protected]> 2026-01-21 15:28 ` Nathan Bossart <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox