diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml index 0e57348d893..1eb5a96cc57 100644 --- a/doc/src/sgml/ref/grant.sgml +++ b/doc/src/sgml/ref/grant.sgml @@ -158,9 +158,9 @@ GRANT role_name [, ...] TO - If GRANTED BY is specified, the specified grantor must - be the current user. This clause is currently present in this form only - for SQL compatibility. + If GRANTED BY is specified, the grant is recorded as + having been done by the specified role. A role can only attribute a grant + to another role if they possess the privileges of that role. diff --git a/doc/src/sgml/ref/revoke.sgml b/doc/src/sgml/ref/revoke.sgml index 948ac534446..244e8becfa2 100644 --- a/doc/src/sgml/ref/revoke.sgml +++ b/doc/src/sgml/ref/revoke.sgml @@ -181,6 +181,12 @@ REVOKE [ { ADMIN | INHERIT | SET } OPTION FOR ] Otherwise, both the privilege and the grant option are revoked. + + If GRANTED BY is specified, only privileges granted by + the specified role are revoked. A role can only revoke grants by another + role if they possess the privileges of that role. + + If a user holds a privilege with grant option and has granted it to other users then the privileges held by those other users are @@ -282,7 +288,7 @@ REVOKE [ { ADMIN | INHERIT | SET } OPTION FOR ] If the role executing REVOKE holds privileges indirectly via more than one role membership path, it is unspecified which containing role will be used to perform the command. In such cases - it is best practice to use SET ROLE to become the specific + it is best practice to use GRANTED BY to specify which role you want to do the REVOKE as. Failure to do so might lead to revoking privileges other than the ones you intended, or not revoking anything at all. diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 52f38480c52..67424fe3b0c 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; @@ -398,22 +399,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. */ @@ -441,6 +426,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; /* @@ -973,6 +959,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; /* @@ -1503,6 +1490,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 */ @@ -1559,6 +1547,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); @@ -1713,7 +1702,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); @@ -1998,7 +1987,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); @@ -2213,7 +2202,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); @@ -2368,7 +2357,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); @@ -2514,7 +2503,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 b9190e700dc..01caa12eca7 100644 --- a/src/backend/utils/adt/acl.c +++ b/src/backend/utils/adt/acl.c @@ -5481,6 +5481,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 @@ -5493,25 +5497,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 ffadd667167..4e1ea9d1e8e 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -2672,7 +2672,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 0bd1a5ce506..0b9b04e78ee 100644 --- a/src/include/utils/acl.h +++ b/src/include/utils/acl.h @@ -224,7 +224,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 9c9cdd12af5..7069e9febb8 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 @@ -3621,3 +3621,61 @@ SELECT * FROM information_schema.table_privileges t DROP TABLE grantor_test1, grantor_test2, grantor_test3; DROP ROLE regress_grantor1, regress_grantor2, regress_grantor3; +-- GRANTED BY +CREATE ROLE regress_grantor1; +CREATE ROLE regress_grantor2 ROLE regress_grantor1; +CREATE ROLE regress_grantor3 ROLE regress_grantor1; +CREATE ROLE regress_grantor4 ROLE regress_grantor1; +CREATE ROLE regress_grantor5; +CREATE TABLE grantor_test (); +GRANT SELECT ON grantor_test TO regress_grantor2 WITH GRANT OPTION; +GRANT UPDATE ON grantor_test TO regress_grantor3 WITH GRANT OPTION; +GRANT SELECT, UPDATE ON grantor_test TO regress_grantor4 WITH GRANT OPTION; +SET ROLE regress_grantor1; +GRANT SELECT, UPDATE ON grantor_test TO regress_grantor5; +SELECT * FROM information_schema.table_privileges t + WHERE grantor LIKE 'regress_grantor%' ORDER BY ROW(t.*); + grantor | grantee | table_catalog | table_schema | table_name | privilege_type | is_grantable | with_hierarchy +------------------+------------------+---------------+--------------+--------------+----------------+--------------+---------------- + regress_grantor4 | regress_grantor5 | regression | public | grantor_test | SELECT | NO | YES + regress_grantor4 | regress_grantor5 | regression | public | grantor_test | UPDATE | NO | NO +(2 rows) + +REVOKE SELECT, UPDATE ON grantor_test FROM regress_grantor5; +GRANT SELECT, UPDATE ON grantor_test TO regress_grantor5 GRANTED BY regress_grantor2; +WARNING: not all privileges were granted for "grantor_test" +GRANT SELECT, UPDATE ON grantor_test TO regress_grantor5 GRANTED BY regress_grantor3; +WARNING: not all privileges were granted for "grantor_test" +SELECT * FROM information_schema.table_privileges t + WHERE grantor LIKE 'regress_grantor%' ORDER BY ROW(t.*); + grantor | grantee | table_catalog | table_schema | table_name | privilege_type | is_grantable | with_hierarchy +------------------+------------------+---------------+--------------+--------------+----------------+--------------+---------------- + regress_grantor2 | regress_grantor5 | regression | public | grantor_test | SELECT | NO | YES + regress_grantor3 | regress_grantor5 | regression | public | grantor_test | UPDATE | NO | NO +(2 rows) + +REVOKE SELECT, UPDATE ON grantor_test FROM regress_grantor5 GRANTED BY regress_grantor2; +WARNING: not all privileges could be revoked for "grantor_test" +WARNING: not all privileges could be revoked for column "tableoid" of relation "grantor_test" +WARNING: not all privileges could be revoked for column "cmax" of relation "grantor_test" +WARNING: not all privileges could be revoked for column "xmax" of relation "grantor_test" +WARNING: not all privileges could be revoked for column "cmin" of relation "grantor_test" +WARNING: not all privileges could be revoked for column "xmin" of relation "grantor_test" +WARNING: not all privileges could be revoked for column "ctid" of relation "grantor_test" +REVOKE SELECT, UPDATE ON grantor_test FROM regress_grantor5 GRANTED BY regress_grantor3; +WARNING: not all privileges could be revoked for "grantor_test" +WARNING: not all privileges could be revoked for column "tableoid" of relation "grantor_test" +WARNING: not all privileges could be revoked for column "cmax" of relation "grantor_test" +WARNING: not all privileges could be revoked for column "xmax" of relation "grantor_test" +WARNING: not all privileges could be revoked for column "cmin" of relation "grantor_test" +WARNING: not all privileges could be revoked for column "xmin" of relation "grantor_test" +WARNING: not all privileges could be revoked for column "ctid" of relation "grantor_test" +SELECT * FROM information_schema.table_privileges t + WHERE grantor LIKE 'regress_grantor%' ORDER BY ROW(t.*); + grantor | grantee | table_catalog | table_schema | table_name | privilege_type | is_grantable | with_hierarchy +---------+---------+---------------+--------------+------------+----------------+--------------+---------------- +(0 rows) + +RESET ROLE; +DROP TABLE grantor_test; +DROP ROLE regress_grantor1, regress_grantor2, regress_grantor3, regress_grantor4, regress_grantor5; diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql index e34c65fc1b2..9f21c2945bd 100644 --- a/src/test/regress/sql/privileges.sql +++ b/src/test/regress/sql/privileges.sql @@ -2211,3 +2211,37 @@ SELECT * FROM information_schema.table_privileges t DROP TABLE grantor_test1, grantor_test2, grantor_test3; DROP ROLE regress_grantor1, regress_grantor2, regress_grantor3; + +-- GRANTED BY +CREATE ROLE regress_grantor1; +CREATE ROLE regress_grantor2 ROLE regress_grantor1; +CREATE ROLE regress_grantor3 ROLE regress_grantor1; +CREATE ROLE regress_grantor4 ROLE regress_grantor1; +CREATE ROLE regress_grantor5; +CREATE TABLE grantor_test (); +GRANT SELECT ON grantor_test TO regress_grantor2 WITH GRANT OPTION; +GRANT UPDATE ON grantor_test TO regress_grantor3 WITH GRANT OPTION; +GRANT SELECT, UPDATE ON grantor_test TO regress_grantor4 WITH GRANT OPTION; +SET ROLE regress_grantor1; + +GRANT SELECT, UPDATE ON grantor_test TO regress_grantor5; + +SELECT * FROM information_schema.table_privileges t + WHERE grantor LIKE 'regress_grantor%' ORDER BY ROW(t.*); + +REVOKE SELECT, UPDATE ON grantor_test FROM regress_grantor5; +GRANT SELECT, UPDATE ON grantor_test TO regress_grantor5 GRANTED BY regress_grantor2; +GRANT SELECT, UPDATE ON grantor_test TO regress_grantor5 GRANTED BY regress_grantor3; + +SELECT * FROM information_schema.table_privileges t + WHERE grantor LIKE 'regress_grantor%' ORDER BY ROW(t.*); + +REVOKE SELECT, UPDATE ON grantor_test FROM regress_grantor5 GRANTED BY regress_grantor2; +REVOKE SELECT, UPDATE ON grantor_test FROM regress_grantor5 GRANTED BY regress_grantor3; + +SELECT * FROM information_schema.table_privileges t + WHERE grantor LIKE 'regress_grantor%' ORDER BY ROW(t.*); + +RESET ROLE; +DROP TABLE grantor_test; +DROP ROLE regress_grantor1, regress_grantor2, regress_grantor3, regress_grantor4, regress_grantor5; -- 2.50.1 (Apple Git-155)