Allow choosing specific grantors via GRANT/REVOKE ... GRANTED BY.

Except for GRANT and REVOKE on roles, the GRANTED BY clause
currently only accepts the current role to match the SQL standard.
And even if an acceptable grantor (i.e., the current role) is
specified, Postgres ignores it and chooses the "best" grantor for
the command.  Allowing the user to select a specific grantor would
allow better control over the precise behavior of GRANT/REVOKE
statements.  This commit adds that ability.  For consistency with
select_best_grantor(), we only permit choosing grantor roles for
which the current role inherits privileges.

Author: Nathan Bossart <nathandbossart@gmail.com>
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/aRYLkTpazxKhnS_w%40nathan

diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml
index 0e57348d893ef0b6b2c5234e3800c9f8147f439e..67426d42285d38eeaa67af180e0cdac6916c8b42 100644 (file)
--- a/doc/src/sgml/ref/grant.sgml
+++ b/doc/src/sgml/ref/grant.sgml
@@ -158,9 +158,9 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
   </para>
 
   <para>
-   If <literal>GRANTED BY</literal> is specified, the specified grantor must
-   be the current user.  This clause is currently present in this form only
-   for SQL compatibility.
+   If <literal>GRANTED BY</literal> 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 it inherits the privileges of that role.
   </para>
 
   <para>
@@ -325,7 +325,7 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
   <para>
    If <literal>GRANTED BY</literal> is specified, the grant is recorded as
    having been done by the specified role. A user can only attribute a grant
-   to another role if they possess the privileges of that role. The role
+   to another role if it inherits the privileges of that role. The role
    recorded as the grantor must have <literal>ADMIN OPTION</literal> on the
    target role, unless it is the bootstrap superuser. When a grant is recorded
    as having a grantor other than the bootstrap superuser, it depends on the

diff --git a/doc/src/sgml/ref/revoke.sgml b/doc/src/sgml/ref/revoke.sgml
index 948ac534446b800cd92e77915596c8a8d6028f8d..618a204c36f653ad166c99229cc0a430318862d3 100644 (file)
--- 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.
   </para>
 
+  <para>
+   If <literal>GRANTED BY</literal> is specified, only privileges granted by
+   the specified role are revoked.  A role can only revoke grants by another
+   role if it inherits the privileges of that role.
+  </para>
+
   <para>
    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 <command>REVOKE</command> 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 <command>SET ROLE</command> to become the specific
+    it is best practice to use <literal>GRANTED BY</literal> to specify which
     role you want to do the <command>REVOKE</command> 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 52f38480c52ea286f451ac837aa5ecc343dc2662..67424fe3b0c83ff2ce408b0693bf4e928c6c65ef 100644 (file)
--- 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 b9190e700dc55f6ee6c1ba8ac09a2e6600f8a0d0..01caa12eca7055f08d9ba764d36284ff8adbe4f4 100644 (file)
--- 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 ffadd6671670d2be9f5406594ba04d6a3afdd8b7..4e1ea9d1e8e804fadeed9bed636c514323217923 100644 (file)
--- 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 0bd1a5ce5066ab454a9c6a598f8adaee6fe79837..0b9b04e78eeaf3212553c7fd83750c368e3b1e7b 100644 (file)
--- 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 38317e2ed37423194227770f12499e2ba165820e..fa0b65fbba7d8abd68394aac797fb8a8c60b631b 100644 (file)
--- 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 9c9cdd12af5ed5ec35e3c4529d397106c32b74eb..7069e9febb80d43192665c260ebc2ca5a8193f01 100644 (file)
--- 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 e34c65fc1b2ce868b29bb372b5e9cc9db1076d1f..9f21c2945bdd9a6fc479a7d09329862b21c67445 100644 (file)
--- 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;