Adjust interaction of CREATEROLE with role properties.

Previously, a CREATEROLE user without SUPERUSER could not alter
REPLICATION users in any way, and could not set the BYPASSRLS
attribute. However, they could manipulate the CREATEDB property
even if they themselves did not possess it.

With this change, a CREATEROLE user without SUPERUSER can set or
clear the REPLICATION, BYPASSRLS, or CREATEDB property on a new
role or a role that they have rights to manage if and only if
that property is set for their own role.

This implements the standard idea that you can't give permissions
you don't have (but you can give the ones you do have). We might
in the future want to provide more powerful ways to constrain
what a CREATEROLE user can do - for example, to limit whether
CONNECTION LIMIT can be set or the values to which it can be set -
but that is left as future work.

Patch by me, reviewed by Nathan Bossart, Tushar Ahuja, and Neha
Sharma.

Discussion: http://postgr.es/m/CA+TgmobX=LHg_J5aT=0pi9gJy=JdtrUVGAu0zhr-i5v5nNbJDg@mail.gmail.com

7 files changed, 137 insertions, 83 deletions

diff --git a/doc/src/sgml/ref/alter_role.sgml b/doc/src/sgml/ref/alter_role.sgml
index fbb4612e70..ff2b88e9b6 100644
--- a/doc/src/sgml/ref/alter_role.sgml
+++ b/doc/src/sgml/ref/alter_role.sgml
@@ -70,11 +70,14 @@ ALTER ROLE { <replaceable class="parameter">role_specification</replaceable> | A
    <link linkend="sql-revoke"><command>REVOKE</command></link> for that.)
    Attributes not mentioned in the command retain their previous settings.
    Database superusers can change any of these settings for any role.
-   Roles having <literal>CREATEROLE</literal> privilege can change any of these
-   settings except <literal>SUPERUSER</literal>, <literal>REPLICATION</literal>,
-   and <literal>BYPASSRLS</literal>; but only for non-superuser and
-   non-replication roles for which they have been
-   granted <literal>ADMIN OPTION</literal>.
+   Non-superuser roles having <literal>CREATEROLE</literal> privilege can
+   change most of these properties, but only for non-superuser and
+   non-replication roles for which they have been granted
+   <literal>ADMIN OPTION</literal>. Non-superusers cannot change the
+   <literal>SUPERUSER</literal> property and can change the
+   <literal>CREATEDB</literal>, <literal>REPLICATION</literal>, and
+   <literal>BYPASSRLS</literal> properties only if they possess the
+   corresponding property themselves.
    Ordinary roles can only change their own password.
   </para>
 
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index 7ce4e38b45..c101df6e2f 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -109,6 +109,8 @@ in sync when changing the above synopsis!
         <literal>NOCREATEDB</literal> will deny a role the ability to
         create databases. If not specified,
         <literal>NOCREATEDB</literal> is the default.
+        Only superuser roles or roles with <literal>CREATEDB</literal>
+        can specify <literal>CREATEDB</literal>.
        </para>
       </listitem>
      </varlistentry>
@@ -188,8 +190,8 @@ in sync when changing the above synopsis!
         highly privileged role, and should only be used on roles actually
         used for replication. If not specified,
         <literal>NOREPLICATION</literal> is the default.
-        You must be a superuser to create a new role having the
-        <literal>REPLICATION</literal> attribute.
+        Only superuser roles or roles with <literal>REPLICATION</literal>
+        can specify <literal>REPLICATION</literal>.
        </para>
       </listitem>
      </varlistentry>
@@ -201,8 +203,8 @@ in sync when changing the above synopsis!
        <para>
         These clauses determine whether a role bypasses every row-level
         security (RLS) policy.  <literal>NOBYPASSRLS</literal> is the default.
-        You must be a superuser to create a new role having
-        the <literal>BYPASSRLS</literal> attribute.
+        Only superuser roles or roles with <literal>BYPASSRLS</literal>
+        can specify <literal>BYPASSRLS</literal>.
        </para>
 
        <para>
@@ -391,19 +393,6 @@ in sync when changing the above synopsis!
   </para>
 
   <para>
-   Be careful with the <literal>CREATEROLE</literal> privilege. There is no concept of
-   inheritance for the privileges of a <literal>CREATEROLE</literal>-role. That
-   means that even if a role does not have a certain privilege but is allowed
-   to create other roles, it can easily create another role with different
-   privileges than its own (except for creating roles with superuser
-   privileges). For example, if the role <quote>user</quote> has the
-   <literal>CREATEROLE</literal> privilege but not the <literal>CREATEDB</literal> privilege,
-   nonetheless it can create a new role with the <literal>CREATEDB</literal>
-   privilege. Therefore, regard roles that have the <literal>CREATEROLE</literal>
-   privilege as almost-superuser-roles.
-  </para>
-
-  <para>
    <productname>PostgreSQL</productname> includes a program <xref
    linkend="app-createuser"/> that has
    the same functionality as <command>CREATE ROLE</command> (in fact,
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 518ffca09a..1f4ce2fb9c 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -121,7 +121,6 @@ static bool get_db_info(const char *name, LOCKMODE lockmode,
 						Oid *dbTablespace, char **dbCollate, char **dbCtype, char **dbIculocale,
 						char *dbLocProvider,
 						char **dbCollversion);
-static bool have_createdb_privilege(void);
 static void remove_dbtablespaces(Oid db_id);
 static bool check_db_file_conflict(Oid db_id);
 static int	errdetail_busy_db(int notherbackends, int npreparedxacts);
@@ -2742,7 +2741,7 @@ get_db_info(const char *name, LOCKMODE lockmode,
 }
 
 /* Check if current user has createdb privileges */
-static bool
+bool
 have_createdb_privilege(void)
 {
 	bool		result = false;
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 4d193a6f9a..3a92e930c0 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -311,33 +311,28 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
 		bypassrls = boolVal(dbypassRLS->arg);
 
 	/* Check some permissions first */
-	if (issuper)
+	if (!superuser_arg(currentUserId))
 	{
-		if (!superuser())
+		if (!has_createrole_privilege(currentUserId))
+			ereport(ERROR,
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("permission denied to create role")));
+		if (issuper)
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("must be superuser to create superusers")));
-	}
-	else if (isreplication)
-	{
-		if (!superuser())
+		if (createdb && !have_createdb_privilege())
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-					 errmsg("must be superuser to create replication users")));
-	}
-	else if (bypassrls)
-	{
-		if (!superuser())
+					 errmsg("must have createdb permission to create createdb users")));
+		if (isreplication && !has_rolreplication(currentUserId))
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-					 errmsg("must be superuser to create bypassrls users")));
-	}
-	else
-	{
-		if (!have_createrole_privilege())
+					 errmsg("must have replication permission to create replication users")));
+		if (bypassrls && !has_bypassrls_privilege(currentUserId))
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-					 errmsg("permission denied to create role")));
+					 errmsg("must have bypassrls to create bypassrls users")));
 	}
 
 	/*
@@ -748,32 +743,11 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
 	rolename = pstrdup(NameStr(authform->rolname));
 	roleid = authform->oid;
 
-	/*
-	 * To mess with a superuser or replication role in any way you gotta be
-	 * superuser.  We also insist on superuser to change the BYPASSRLS
-	 * property.
-	 */
-	if (authform->rolsuper || dissuper)
-	{
-		if (!superuser())
-			ereport(ERROR,
-					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-					 errmsg("must be superuser to alter superuser roles or change superuser attribute")));
-	}
-	else if (authform->rolreplication || disreplication)
-	{
-		if (!superuser())
-			ereport(ERROR,
-					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-					 errmsg("must be superuser to alter replication roles or change replication attribute")));
-	}
-	else if (dbypassRLS)
-	{
-		if (!superuser())
-			ereport(ERROR,
-					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-					 errmsg("must be superuser to change bypassrls attribute")));
-	}
+	/* To mess with a superuser in any way you gotta be superuser. */
+	if (!superuser() && (authform->rolsuper || dissuper))
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("must be superuser to alter superuser roles or change superuser attribute")));
 
 	/*
 	 * Most changes to a role require that you both have CREATEROLE privileges
@@ -784,7 +758,7 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
 	{
 		/* things an unprivileged user certainly can't do */
 		if (dinherit || dcreaterole || dcreatedb || dcanlogin || dconnlimit ||
-			dvalidUntil)
+			dvalidUntil || disreplication || dbypassRLS)
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("permission denied")));
@@ -795,6 +769,26 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("must have CREATEROLE privilege to change another user's password")));
 	}
+	else if (!superuser())
+	{
+		/*
+		 * Even if you have both CREATEROLE and ADMIN OPTION on a role, you
+		 * can only change the CREATEDB, REPLICATION, or BYPASSRLS attributes
+		 * if they are set for your own role (or you are the superuser).
+		 */
+		if (dcreatedb && !have_createdb_privilege())
+			ereport(ERROR,
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("must have createdb privilege to change createdb attribute")));
+		if (disreplication && !has_rolreplication(currentUserId))
+			ereport(ERROR,
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("must have replication privilege to change replication attribute")));
+		if (dbypassRLS && !has_bypassrls_privilege(currentUserId))
+			ereport(ERROR,
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("must have bypassrls privilege to change bypassrls attribute")));
+	}
 
 	/* To add members to a role, you need ADMIN OPTION. */
 	if (drolemembers && !is_admin_of_role(currentUserId, roleid))
diff --git a/src/include/commands/dbcommands.h b/src/include/commands/dbcommands.h
index 9b8818315a..5fbc3ca752 100644
--- a/src/include/commands/dbcommands.h
+++ b/src/include/commands/dbcommands.h
@@ -30,6 +30,7 @@ extern ObjectAddress AlterDatabaseOwner(const char *dbname, Oid newOwnerId);
 
 extern Oid	get_database_oid(const char *dbname, bool missing_ok);
 extern char *get_database_name(Oid dbid);
+extern bool have_createdb_privilege(void);
 
 extern void check_encoding_locale_matches(int encoding, const char *collate, const char *ctype);
 
diff --git a/src/test/regress/expected/create_role.out b/src/test/regress/expected/create_role.out
index cd49feabb3..c8beb36bab 100644
--- a/src/test/regress/expected/create_role.out
+++ b/src/test/regress/expected/create_role.out
@@ -2,19 +2,51 @@
 CREATE ROLE regress_role_super SUPERUSER;
 CREATE ROLE regress_role_admin CREATEDB CREATEROLE REPLICATION BYPASSRLS;
 GRANT CREATE ON DATABASE regression TO regress_role_admin WITH GRANT OPTION;
+CREATE ROLE regress_role_limited_admin CREATEROLE;
 CREATE ROLE regress_role_normal;
--- fail, only superusers can create users with these privileges
-SET SESSION AUTHORIZATION regress_role_admin;
+-- fail, CREATEROLE user can't give away role attributes without having them
+SET SESSION AUTHORIZATION regress_role_limited_admin;
 CREATE ROLE regress_nosuch_superuser SUPERUSER;
 ERROR:  must be superuser to create superusers
 CREATE ROLE regress_nosuch_replication_bypassrls REPLICATION BYPASSRLS;
-ERROR:  must be superuser to create replication users
+ERROR:  must have replication permission to create replication users
 CREATE ROLE regress_nosuch_replication REPLICATION;
-ERROR:  must be superuser to create replication users
+ERROR:  must have replication permission to create replication users
 CREATE ROLE regress_nosuch_bypassrls BYPASSRLS;
-ERROR:  must be superuser to create bypassrls users
--- ok, having CREATEROLE is enough to create users with these privileges
+ERROR:  must have bypassrls to create bypassrls users
+CREATE ROLE regress_nosuch_createdb CREATEDB;
+ERROR:  must have createdb permission to create createdb users
+-- ok, can create a role without any special attributes
+CREATE ROLE regress_role_limited;
+-- fail, can't give it in any of the restricted attributes
+ALTER ROLE regress_role_limited SUPERUSER;
+ERROR:  must be superuser to alter superuser roles or change superuser attribute
+ALTER ROLE regress_role_limited REPLICATION;
+ERROR:  must have replication privilege to change replication attribute
+ALTER ROLE regress_role_limited CREATEDB;
+ERROR:  must have createdb privilege to change createdb attribute
+ALTER ROLE regress_role_limited BYPASSRLS;
+ERROR:  must have bypassrls privilege to change bypassrls attribute
+DROP ROLE regress_role_limited;
+-- ok, can give away these role attributes if you have them
+SET SESSION AUTHORIZATION regress_role_admin;
+CREATE ROLE regress_replication_bypassrls REPLICATION BYPASSRLS;
+CREATE ROLE regress_replication REPLICATION;
+CREATE ROLE regress_bypassrls BYPASSRLS;
 CREATE ROLE regress_createdb CREATEDB;
+-- ok, can toggle these role attributes off and on if you have them
+ALTER ROLE regress_replication NOREPLICATION;
+ALTER ROLE regress_replication REPLICATION;
+ALTER ROLE regress_bypassrls NOBYPASSRLS;
+ALTER ROLE regress_bypassrls BYPASSRLS;
+ALTER ROLE regress_createdb NOCREATEDB;
+ALTER ROLE regress_createdb CREATEDB;
+-- fail, can't toggle SUPERUSER
+ALTER ROLE regress_createdb SUPERUSER;
+ERROR:  must be superuser to alter superuser roles or change superuser attribute
+ALTER ROLE regress_createdb NOSUPERUSER;
+ERROR:  must be superuser to alter superuser roles or change superuser attribute
+-- ok, having CREATEROLE is enough to create users with these privileges
 CREATE ROLE regress_createrole CREATEROLE NOINHERIT;
 GRANT CREATE ON DATABASE regression TO regress_createrole WITH GRANT OPTION;
 CREATE ROLE regress_login LOGIN;
@@ -53,9 +85,9 @@ ERROR:  permission denied to create database
 CREATE ROLE regress_plainrole;
 -- ok, roles with CREATEROLE can create new roles with it
 CREATE ROLE regress_rolecreator CREATEROLE;
--- ok, roles with CREATEROLE can create new roles with privilege they lack
-CREATE ROLE regress_hasprivs CREATEDB CREATEROLE LOGIN INHERIT
-	CONNECTION LIMIT 5;
+-- ok, roles with CREATEROLE can create new roles with different role
+-- attributes, including CREATEROLE
+CREATE ROLE regress_hasprivs CREATEROLE LOGIN INHERIT CONNECTION LIMIT 5;
 -- ok, we should be able to modify a role we created
 COMMENT ON ROLE regress_hasprivs IS 'some comment';
 ALTER ROLE regress_hasprivs RENAME TO regress_tenant;
@@ -164,6 +196,9 @@ DROP ROLE regress_plainrole;
 -- must revoke privileges before dropping role
 REVOKE CREATE ON DATABASE regression FROM regress_createrole CASCADE;
 -- ok, should be able to drop non-superuser roles we created
+DROP ROLE regress_replication_bypassrls;
+DROP ROLE regress_replication;
+DROP ROLE regress_bypassrls;
 DROP ROLE regress_createdb;
 DROP ROLE regress_createrole;
 DROP ROLE regress_login;
diff --git a/src/test/regress/sql/create_role.sql b/src/test/regress/sql/create_role.sql
index 6b90336da2..19031b4612 100644
--- a/src/test/regress/sql/create_role.sql
+++ b/src/test/regress/sql/create_role.sql
@@ -2,17 +2,47 @@
 CREATE ROLE regress_role_super SUPERUSER;
 CREATE ROLE regress_role_admin CREATEDB CREATEROLE REPLICATION BYPASSRLS;
 GRANT CREATE ON DATABASE regression TO regress_role_admin WITH GRANT OPTION;
+CREATE ROLE regress_role_limited_admin CREATEROLE;
 CREATE ROLE regress_role_normal;
 
--- fail, only superusers can create users with these privileges
-SET SESSION AUTHORIZATION regress_role_admin;
+-- fail, CREATEROLE user can't give away role attributes without having them
+SET SESSION AUTHORIZATION regress_role_limited_admin;
 CREATE ROLE regress_nosuch_superuser SUPERUSER;
 CREATE ROLE regress_nosuch_replication_bypassrls REPLICATION BYPASSRLS;
 CREATE ROLE regress_nosuch_replication REPLICATION;
 CREATE ROLE regress_nosuch_bypassrls BYPASSRLS;
+CREATE ROLE regress_nosuch_createdb CREATEDB;
 
--- ok, having CREATEROLE is enough to create users with these privileges
+-- ok, can create a role without any special attributes
+CREATE ROLE regress_role_limited;
+
+-- fail, can't give it in any of the restricted attributes
+ALTER ROLE regress_role_limited SUPERUSER;
+ALTER ROLE regress_role_limited REPLICATION;
+ALTER ROLE regress_role_limited CREATEDB;
+ALTER ROLE regress_role_limited BYPASSRLS;
+DROP ROLE regress_role_limited;
+
+-- ok, can give away these role attributes if you have them
+SET SESSION AUTHORIZATION regress_role_admin;
+CREATE ROLE regress_replication_bypassrls REPLICATION BYPASSRLS;
+CREATE ROLE regress_replication REPLICATION;
+CREATE ROLE regress_bypassrls BYPASSRLS;
 CREATE ROLE regress_createdb CREATEDB;
+
+-- ok, can toggle these role attributes off and on if you have them
+ALTER ROLE regress_replication NOREPLICATION;
+ALTER ROLE regress_replication REPLICATION;
+ALTER ROLE regress_bypassrls NOBYPASSRLS;
+ALTER ROLE regress_bypassrls BYPASSRLS;
+ALTER ROLE regress_createdb NOCREATEDB;
+ALTER ROLE regress_createdb CREATEDB;
+
+-- fail, can't toggle SUPERUSER
+ALTER ROLE regress_createdb SUPERUSER;
+ALTER ROLE regress_createdb NOSUPERUSER;
+
+-- ok, having CREATEROLE is enough to create users with these privileges
 CREATE ROLE regress_createrole CREATEROLE NOINHERIT;
 GRANT CREATE ON DATABASE regression TO regress_createrole WITH GRANT OPTION;
 CREATE ROLE regress_login LOGIN;
@@ -56,9 +86,9 @@ CREATE ROLE regress_plainrole;
 -- ok, roles with CREATEROLE can create new roles with it
 CREATE ROLE regress_rolecreator CREATEROLE;
 
--- ok, roles with CREATEROLE can create new roles with privilege they lack
-CREATE ROLE regress_hasprivs CREATEDB CREATEROLE LOGIN INHERIT
-	CONNECTION LIMIT 5;
+-- ok, roles with CREATEROLE can create new roles with different role
+-- attributes, including CREATEROLE
+CREATE ROLE regress_hasprivs CREATEROLE LOGIN INHERIT CONNECTION LIMIT 5;
 
 -- ok, we should be able to modify a role we created
 COMMENT ON ROLE regress_hasprivs IS 'some comment';
@@ -150,6 +180,9 @@ DROP ROLE regress_plainrole;
 REVOKE CREATE ON DATABASE regression FROM regress_createrole CASCADE;
 
 -- ok, should be able to drop non-superuser roles we created
+DROP ROLE regress_replication_bypassrls;
+DROP ROLE regress_replication;
+DROP ROLE regress_bypassrls;
 DROP ROLE regress_createdb;
 DROP ROLE regress_createrole;
 DROP ROLE regress_login;