Make role grant system more consistent with other privileges.

Previously, membership of role A in role B could be recorded in the
catalog tables only once. This meant that a new grant of role A to
role B would overwrite the previous grant. For other object types, a
new grant of permission on an object - in this case role A - exists
along side the existing grant provided that the grantor is different.
Either grant can be revoked independently of the other, and
permissions remain so long as at least one grant remains. Make role
grants work similarly.

Previously, when granting membership in a role, the superuser could
specify any role whatsoever as the grantor, but for other object types,
the grantor of record must be either the owner of the object, or a
role that currently has privileges to perform a similar GRANT.
Implement the same scheme for role grants, treating the bootstrap
superuser as the role owner since roles do not have owners. This means
that attempting to revoke a grant, or admin option on a grant, can now
fail if there are dependent privileges, and that CASCADE can be used
to revoke these. It also means that you can't grant ADMIN OPTION on
a role back to a user who granted it directly or indirectly to you,
similar to how you can't give WITH GRANT OPTION on a privilege back
to a role which granted it directly or indirectly to you.

Previously, only the superuser could specify GRANTED BY with a user
other than the current user. Relax that rule to allow the grantor
to be any role whose privileges the current user posseses. This
doesn't improve compatibility with what we do for other object types,
where support for GRANTED BY is entirely vestigial, but it makes this
feature more usable and seems to make sense to change at the same time
we're changing related behaviors.

Along the way, fix "ALTER GROUP group_name ADD USER user_name" to
require the same privileges as "GRANT group_name TO user_name".
Previously, CREATEROLE privileges were sufficient for either, but
only the former form was permissible with ADMIN OPTION on the role.
Now, either CREATEROLE or ADMIN OPTION on the role suffices for
either spelling.

Patch by me, reviewed by Stephen Frost.

Discussion: http://postgr.es/m/CA+TgmoaFr-RZeQ+WoQ5nKPv97oT9+aDgK_a5+qWHSgbDsMp1Vg@mail.gmail.com

4 files changed, 109 insertions, 29 deletions

diff --git a/src/test/regress/expected/create_role.out b/src/test/regress/expected/create_role.out
index c2465d0f492..4e67d727603 100644
--- a/src/test/regress/expected/create_role.out
+++ b/src/test/regress/expected/create_role.out
@@ -103,21 +103,9 @@ ERROR:  role "regress_nosuch_recursive" does not exist
 DROP ROLE regress_nosuch_admin_recursive;
 ERROR:  role "regress_nosuch_admin_recursive" does not exist
 DROP ROLE regress_plainrole;
--- fail, can't drop regress_createrole yet, due to outstanding grants
-DROP ROLE regress_createrole;
-ERROR:  role "regress_createrole" cannot be dropped because some objects depend on it
-DETAIL:  privileges for membership of role regress_read_all_data in role pg_read_all_data
-privileges for membership of role regress_write_all_data in role pg_write_all_data
-privileges for membership of role regress_monitor in role pg_monitor
-privileges for membership of role regress_read_all_settings in role pg_read_all_settings
-privileges for membership of role regress_read_all_stats in role pg_read_all_stats
-privileges for membership of role regress_stat_scan_tables in role pg_stat_scan_tables
-privileges for membership of role regress_read_server_files in role pg_read_server_files
-privileges for membership of role regress_write_server_files in role pg_write_server_files
-privileges for membership of role regress_execute_server_program in role pg_execute_server_program
-privileges for membership of role regress_signal_backend in role pg_signal_backend
 -- ok, should be able to drop non-superuser roles we created
 DROP ROLE regress_createdb;
+DROP ROLE regress_createrole;
 DROP ROLE regress_login;
 DROP ROLE regress_inherit;
 DROP ROLE regress_connection_limit;
@@ -137,8 +125,6 @@ DROP ROLE regress_read_server_files;
 DROP ROLE regress_write_server_files;
 DROP ROLE regress_execute_server_program;
 DROP ROLE regress_signal_backend;
--- ok, dropped the other roles first so this is ok now
-DROP ROLE regress_createrole;
 -- fail, role still owns database objects
 DROP ROLE regress_tenant;
 ERROR:  role "regress_tenant" cannot be dropped because some objects depend on it
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 65b4a22ebc5..0154a09262e 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -33,6 +33,54 @@ CREATE USER regress_priv_user8;
 CREATE USER regress_priv_user9;
 CREATE USER regress_priv_user10;
 CREATE ROLE regress_priv_role;
+-- circular ADMIN OPTION grants should be disallowed
+GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION;
+GRANT regress_priv_user1 TO regress_priv_user3 WITH ADMIN OPTION GRANTED BY regress_priv_user2;
+GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION GRANTED BY regress_priv_user3;
+ERROR:  admin option cannot be granted back to your own grantor
+-- need CASCADE to revoke grant or admin option if dependent grants exist
+REVOKE ADMIN OPTION FOR regress_priv_user1 FROM regress_priv_user2; -- fail
+ERROR:  dependent privileges exist
+HINT:  Use CASCADE to revoke them too.
+REVOKE regress_priv_user1 FROM regress_priv_user2; -- fail
+ERROR:  dependent privileges exist
+HINT:  Use CASCADE to revoke them too.
+SELECT member::regrole, admin_option FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole;
+       member       | admin_option 
+--------------------+--------------
+ regress_priv_user2 | t
+ regress_priv_user3 | t
+(2 rows)
+
+BEGIN;
+REVOKE ADMIN OPTION FOR regress_priv_user1 FROM regress_priv_user2 CASCADE;
+SELECT member::regrole, admin_option FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole;
+       member       | admin_option 
+--------------------+--------------
+ regress_priv_user2 | f
+(1 row)
+
+ROLLBACK;
+REVOKE regress_priv_user1 FROM regress_priv_user2 CASCADE;
+SELECT member::regrole, admin_option FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole;
+ member | admin_option 
+--------+--------------
+(0 rows)
+
+-- inferred grantor must be a role with ADMIN OPTION
+GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION;
+GRANT regress_priv_user2 TO regress_priv_user3;
+SET ROLE regress_priv_user3;
+GRANT regress_priv_user1 TO regress_priv_user4;
+SELECT grantor::regrole FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole and member = 'regress_priv_user4'::regrole;
+      grantor       
+--------------------
+ regress_priv_user2
+(1 row)
+
+RESET ROLE;
+REVOKE regress_priv_user2 FROM regress_priv_user3;
+REVOKE regress_priv_user1 FROM regress_priv_user2 CASCADE;
 -- test GRANTED BY with DROP OWNED and REASSIGN OWNED
 GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION;
 GRANT regress_priv_user1 TO regress_priv_user3 GRANTED BY regress_priv_user2;
@@ -68,15 +116,17 @@ CREATE USER regress_priv_user5;
 GRANT pg_read_all_data TO regress_priv_user6;
 GRANT pg_write_all_data TO regress_priv_user7;
 GRANT pg_read_all_settings TO regress_priv_user8 WITH ADMIN OPTION;
+GRANT regress_priv_user9 TO regress_priv_user8;
 SET SESSION AUTHORIZATION regress_priv_user8;
 GRANT pg_read_all_settings TO regress_priv_user9 WITH ADMIN OPTION;
 SET SESSION AUTHORIZATION regress_priv_user9;
 GRANT pg_read_all_settings TO regress_priv_user10;
 SET SESSION AUTHORIZATION regress_priv_user8;
-REVOKE pg_read_all_settings FROM regress_priv_user10;
+REVOKE pg_read_all_settings FROM regress_priv_user10 GRANTED BY regress_priv_user9;
 REVOKE ADMIN OPTION FOR pg_read_all_settings FROM regress_priv_user9;
 REVOKE pg_read_all_settings FROM regress_priv_user9;
 RESET SESSION AUTHORIZATION;
+REVOKE regress_priv_user9 FROM regress_priv_user8;
 REVOKE ADMIN OPTION FOR pg_read_all_settings FROM regress_priv_user8;
 SET SESSION AUTHORIZATION regress_priv_user8;
 SET ROLE pg_read_all_settings;
@@ -87,11 +137,20 @@ DROP USER regress_priv_user10;
 DROP USER regress_priv_user9;
 DROP USER regress_priv_user8;
 CREATE GROUP regress_priv_group1;
-CREATE GROUP regress_priv_group2 WITH USER regress_priv_user1, regress_priv_user2;
+CREATE GROUP regress_priv_group2 WITH ADMIN regress_priv_user1 USER regress_priv_user2;
 ALTER GROUP regress_priv_group1 ADD USER regress_priv_user4;
+GRANT regress_priv_group2 TO regress_priv_user2 GRANTED BY regress_priv_user1;
+SET SESSION AUTHORIZATION regress_priv_user1;
+ALTER GROUP regress_priv_group2 ADD USER regress_priv_user2;
+NOTICE:  role "regress_priv_user2" has already been granted membership in role "regress_priv_group2" by role "regress_priv_user1"
 ALTER GROUP regress_priv_group2 ADD USER regress_priv_user2;	-- duplicate
-NOTICE:  role "regress_priv_user2" is already a member of role "regress_priv_group2"
+NOTICE:  role "regress_priv_user2" has already been granted membership in role "regress_priv_group2" by role "regress_priv_user1"
+ALTER GROUP regress_priv_group2 DROP USER regress_priv_user2;
+ALTER USER regress_priv_user2 PASSWORD 'verysecret'; -- not permitted
+ERROR:  must have CREATEROLE privilege to change another user's password
+RESET SESSION AUTHORIZATION;
 ALTER GROUP regress_priv_group2 DROP USER regress_priv_user2;
+REVOKE ADMIN OPTION FOR regress_priv_group2 FROM regress_priv_user1;
 GRANT regress_priv_group2 TO regress_priv_user4 WITH ADMIN OPTION;
 -- prepare non-leakproof function for later
 CREATE FUNCTION leak(integer,integer) RETURNS boolean
@@ -99,9 +158,13 @@ CREATE FUNCTION leak(integer,integer) RETURNS boolean
   LANGUAGE internal IMMUTABLE STRICT;  -- but deliberately not LEAKPROOF
 ALTER FUNCTION leak(integer,integer) OWNER TO regress_priv_user1;
 -- test owner privileges
+GRANT regress_priv_role TO regress_priv_user1 WITH ADMIN OPTION GRANTED BY regress_priv_role; -- error, doesn't have ADMIN OPTION
+ERROR:  grantor must have ADMIN OPTION on "regress_priv_role"
 GRANT regress_priv_role TO regress_priv_user1 WITH ADMIN OPTION GRANTED BY CURRENT_ROLE;
 REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY foo; -- error
-REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY regress_priv_user2; -- error
+ERROR:  role "foo" does not exist
+REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY regress_priv_user2; -- warning, noop
+WARNING:  role "regress_priv_user1" has not been granted membership in role "regress_priv_role" by role "regress_priv_user2"
 REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY CURRENT_USER;
 REVOKE regress_priv_role FROM regress_priv_user1 GRANTED BY CURRENT_ROLE;
 DROP ROLE regress_priv_role;
@@ -1746,7 +1809,7 @@ SET SESSION AUTHORIZATION regress_priv_user1;
 GRANT regress_priv_group2 TO regress_priv_user5; -- fails: no ADMIN OPTION
 ERROR:  must have admin option on role "regress_priv_group2"
 SELECT dogrant_ok();			-- ok: SECURITY DEFINER conveys ADMIN
-NOTICE:  role "regress_priv_user5" is already a member of role "regress_priv_group2"
+NOTICE:  role "regress_priv_user5" has already been granted membership in role "regress_priv_group2" by role "regress_priv_user4"
  dogrant_ok 
 ------------
  
diff --git a/src/test/regress/sql/create_role.sql b/src/test/regress/sql/create_role.sql
index b696628238a..292dc087975 100644
--- a/src/test/regress/sql/create_role.sql
+++ b/src/test/regress/sql/create_role.sql
@@ -98,11 +98,9 @@ DROP ROLE regress_nosuch_recursive;
 DROP ROLE regress_nosuch_admin_recursive;
 DROP ROLE regress_plainrole;
 
--- fail, can't drop regress_createrole yet, due to outstanding grants
-DROP ROLE regress_createrole;
-
 -- ok, should be able to drop non-superuser roles we created
 DROP ROLE regress_createdb;
+DROP ROLE regress_createrole;
 DROP ROLE regress_login;
 DROP ROLE regress_inherit;
 DROP ROLE regress_connection_limit;
@@ -123,9 +121,6 @@ DROP ROLE regress_write_server_files;
 DROP ROLE regress_execute_server_program;
 DROP ROLE regress_signal_backend;
 
--- ok, dropped the other roles first so this is ok now
-DROP ROLE regress_createrole;
-
 -- fail, role still owns database objects
 DROP ROLE regress_tenant;
 
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 66834e32a7e..b4ef20f738e 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -37,6 +37,32 @@ CREATE USER regress_priv_user9;
 CREATE USER regress_priv_user10;
 CREATE ROLE regress_priv_role;
 
+-- circular ADMIN OPTION grants should be disallowed
+GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION;
+GRANT regress_priv_user1 TO regress_priv_user3 WITH ADMIN OPTION GRANTED BY regress_priv_user2;
+GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION GRANTED BY regress_priv_user3;
+
+-- need CASCADE to revoke grant or admin option if dependent grants exist
+REVOKE ADMIN OPTION FOR regress_priv_user1 FROM regress_priv_user2; -- fail
+REVOKE regress_priv_user1 FROM regress_priv_user2; -- fail
+SELECT member::regrole, admin_option FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole;
+BEGIN;
+REVOKE ADMIN OPTION FOR regress_priv_user1 FROM regress_priv_user2 CASCADE;
+SELECT member::regrole, admin_option FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole;
+ROLLBACK;
+REVOKE regress_priv_user1 FROM regress_priv_user2 CASCADE;
+SELECT member::regrole, admin_option FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole;
+
+-- inferred grantor must be a role with ADMIN OPTION
+GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION;
+GRANT regress_priv_user2 TO regress_priv_user3;
+SET ROLE regress_priv_user3;
+GRANT regress_priv_user1 TO regress_priv_user4;
+SELECT grantor::regrole FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole and member = 'regress_priv_user4'::regrole;
+RESET ROLE;
+REVOKE regress_priv_user2 FROM regress_priv_user3;
+REVOKE regress_priv_user1 FROM regress_priv_user2 CASCADE;
+
 -- test GRANTED BY with DROP OWNED and REASSIGN OWNED
 GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION;
 GRANT regress_priv_user1 TO regress_priv_user3 GRANTED BY regress_priv_user2;
@@ -67,6 +93,7 @@ CREATE USER regress_priv_user5;
 GRANT pg_read_all_data TO regress_priv_user6;
 GRANT pg_write_all_data TO regress_priv_user7;
 GRANT pg_read_all_settings TO regress_priv_user8 WITH ADMIN OPTION;
+GRANT regress_priv_user9 TO regress_priv_user8;
 
 SET SESSION AUTHORIZATION regress_priv_user8;
 GRANT pg_read_all_settings TO regress_priv_user9 WITH ADMIN OPTION;
@@ -75,11 +102,12 @@ SET SESSION AUTHORIZATION regress_priv_user9;
 GRANT pg_read_all_settings TO regress_priv_user10;
 
 SET SESSION AUTHORIZATION regress_priv_user8;
-REVOKE pg_read_all_settings FROM regress_priv_user10;
+REVOKE pg_read_all_settings FROM regress_priv_user10 GRANTED BY regress_priv_user9;
 REVOKE ADMIN OPTION FOR pg_read_all_settings FROM regress_priv_user9;
 REVOKE pg_read_all_settings FROM regress_priv_user9;
 
 RESET SESSION AUTHORIZATION;
+REVOKE regress_priv_user9 FROM regress_priv_user8;
 REVOKE ADMIN OPTION FOR pg_read_all_settings FROM regress_priv_user8;
 
 SET SESSION AUTHORIZATION regress_priv_user8;
@@ -94,12 +122,19 @@ DROP USER regress_priv_user9;
 DROP USER regress_priv_user8;
 
 CREATE GROUP regress_priv_group1;
-CREATE GROUP regress_priv_group2 WITH USER regress_priv_user1, regress_priv_user2;
+CREATE GROUP regress_priv_group2 WITH ADMIN regress_priv_user1 USER regress_priv_user2;
 
 ALTER GROUP regress_priv_group1 ADD USER regress_priv_user4;
 
+GRANT regress_priv_group2 TO regress_priv_user2 GRANTED BY regress_priv_user1;
+SET SESSION AUTHORIZATION regress_priv_user1;
+ALTER GROUP regress_priv_group2 ADD USER regress_priv_user2;
 ALTER GROUP regress_priv_group2 ADD USER regress_priv_user2;	-- duplicate
 ALTER GROUP regress_priv_group2 DROP USER regress_priv_user2;
+ALTER USER regress_priv_user2 PASSWORD 'verysecret'; -- not permitted
+RESET SESSION AUTHORIZATION;
+ALTER GROUP regress_priv_group2 DROP USER regress_priv_user2;
+REVOKE ADMIN OPTION FOR regress_priv_group2 FROM regress_priv_user1;
 GRANT regress_priv_group2 TO regress_priv_user4 WITH ADMIN OPTION;
 
 -- prepare non-leakproof function for later
@@ -110,9 +145,10 @@ ALTER FUNCTION leak(integer,integer) OWNER TO regress_priv_user1;
 
 -- test owner privileges
 
+GRANT regress_priv_role TO regress_priv_user1 WITH ADMIN OPTION GRANTED BY regress_priv_role; -- error, doesn't have ADMIN OPTION
 GRANT regress_priv_role TO regress_priv_user1 WITH ADMIN OPTION GRANTED BY CURRENT_ROLE;
 REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY foo; -- error
-REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY regress_priv_user2; -- error
+REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY regress_priv_user2; -- warning, noop
 REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY CURRENT_USER;
 REVOKE regress_priv_role FROM regress_priv_user1 GRANTED BY CURRENT_ROLE;
 DROP ROLE regress_priv_role;