Feedback-ID: ia2694551:Fastmail
Date: Fri, 27 Jan 2023 19:31:19 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Robert Haas <robertmhaas@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: improving user.c error messages
Message-ID: <20230127183119.e5jtyh4g2md57ctb@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230127000936.GA1998073@nathanxps13>
Archived-At: 
 <https://www.postgresql.org/message-id/20230127183119.e5jtyh4g2md57ctb%40alvherre.pgsql>
Precedence: bulk

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/