While we're here,
On 2023-Jan-26, Nathan Bossart wrote:
> @@ -838,7 +867,8 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
> if (!should_be_super && roleid == BOOTSTRAP_SUPERUSERID)
> ereport(ERROR,
>
> (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> - errmsg("permission denied: bootstrap
> user must be superuser")));
> + errmsg("permission denied to alter
> role"),
> + errdetail("The bootstrap user must be
> superuser.")));
I think this one isn't using the right errcode; this is not a case of
insufficient privileges. There's no priv you can acquire that lets you
do it. So I'd change it to unsupported operation.
I was confused a bit by this one:
> /* an unprivileged user can change their own password */
> if (dpassword && roleid != currentUserId)
> ereport(ERROR,
> (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> - errmsg("must have CREATEROLE privilege to change another
> user's password")));
> + errmsg("permission denied to alter role"),
> + errdetail("To change another role's password, you must
> have %s privilege and %s on the role.",
> + "CREATEROLE", "ADMIN OPTION")));
> }
In no other message we say what operation is being attempted in the
errdetail; all the others start with "You must have" and that's it.
However, looking closer I think this one being different is okay,
because the errmsg() you're using is vague, and I think the error report
would be confusing if you were to remove the "To change another role's
password" bit.
The patch looks good to me.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
