From: Tom Lane <tgl@sss.pgh.pa.us>
To: Stephen Frost <sfrost@snowman.net>
cc: Wolfgang Walther <walther@technowledgy.de>,
        pgsql-bugs@lists.postgresql.org
Subject: Re: User with BYPASSRLS privilege can't change password
In-reply-to: <923005.1604420579@sss.pgh.pa.us>
References: <a5548a9f-89ee-3167-129d-162b5985fcf8@technowledgy.de>
 <923005.1604420579@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Tue, 03 Nov 2020 11:22:59 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <956875.1604425251.0@sss.pgh.pa.us>
Date: Tue, 03 Nov 2020 12:44:11 -0500
Message-ID: <956994.1604425451@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <956875.1604425251.1@sss.pgh.pa.us>

I wrote:
> Wolfgang Walther <walther@technowledgy.de> writes:
>> CREATE USER bob BYPASSRLS;
>> SET ROLE bob;
>> ALTER USER bob PASSWORD 'x';
>> -- ERROR: must be superuser to change bypassrls attribute

> Yeah, duplicated here on HEAD.  The error message seems to think
> the command is trying to remove the BYPASSRLS privilege, which
> suggests somebody forgot to copy that flag somewhere where it needs
> to be copied.  Haven't dug further than that.

It's a little more subtle than that, but not much.  Commit 491c029db
copied-and-pasted the logic used to deny non-superusers the privilege
to change anything about a superuser role.  That was certainly not the
intention, because the error message was phrased differently from the
superuser case, but that was the effect.  I propose the attached.

(Hm, looks like this behavior is undocumented, too.)

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-bypassrls-privilege-check.patch";
	charset="us-ascii"
Content-ID: <956875.1604425251.2@sss.pgh.pa.us>
Content-Description: fix-bypassrls-privilege-check.patch

diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 9ce9a66921..5cd479a649 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -709,8 +709,10 @@ AlterRole(AlterRoleStmt *stmt)
 	roleid = authform->oid;
 
 	/*
-	 * To mess with a superuser you gotta be superuser; else you need
-	 * createrole, or just want to change your own password
+	 * 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.  Otherwise, if you don't have createrole, you're only allowed
+	 * to change your own password.
 	 */
 	if (authform->rolsuper || issuper >= 0)
 	{
@@ -726,7 +728,7 @@ AlterRole(AlterRoleStmt *stmt)
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("must be superuser to alter replication users")));
 	}
-	else if (authform->rolbypassrls || bypassrls >= 0)
+	else if (bypassrls >= 0)
 	{
 		if (!superuser())
 			ereport(ERROR,
@@ -739,7 +741,6 @@ AlterRole(AlterRoleStmt *stmt)
 			  createrole < 0 &&
 			  createdb < 0 &&
 			  canlogin < 0 &&
-			  isreplication < 0 &&
 			  !dconnlimit &&
 			  !rolemembers &&
 			  !validUntil &&

------- =_aaaaaaaaaa0--