Re: User with BYPASSRLS privilege can't change password

Greetings,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> I wrote:
> > Wolfgang Walther <walther(at)technowledgy(dot)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.)

The intent was certainly that non-superusers shouldn't be able to tweak
the BYPASSRLS bit, but it wasn't intended to prevent users who have
BYPASSRLS from being able to change their own password.

I certainly recall the discussion about that requirement and we should
document it.

> 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,

This change looks correct, we shouldn't be worrying about what's already
been set on the role.

> @@ -739,7 +741,6 @@ AlterRole(AlterRoleStmt *stmt)
> createrole < 0 &&
> createdb < 0 &&
> canlogin < 0 &&
> - isreplication < 0 &&
> !dconnlimit &&
> !rolemembers &&
> !validUntil &&

This seems like an independent change..? Not saying it's wrong though.

Thanks,

Stephen