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

1 files changed, 38 insertions, 44 deletions

diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 4d193a6f9a4..3a92e930c06 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))