Date: Thu, 26 Jan 2023 11:59:29 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
	tgl@sss.pgh.pa.us
Subject: Re: improving user.c error messages
Message-ID: <20230126195929.GB1702315@nathanxps13>
References: <20230126002251.GA1506128@nathanxps13>
 <20230126090739.nspznasnb4x3v5l4@alvherre.pgsql>
 <20230126191358.GA1660026@nathanxps13>
 <CA+TgmoYZcZ-PUp7muZEG20Jz4BtgUSngA3p7k=Nk-_8FayO5xQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+TgmoYZcZ-PUp7muZEG20Jz4BtgUSngA3p7k=Nk-_8FayO5xQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20230126195929.GB1702315%40nathanxps13>
Precedence: bulk

On Thu, Jan 26, 2023 at 02:42:05PM -0500, Robert Haas wrote:
> @@ -758,16 +776,13 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
>   {
>   /* things an unprivileged user certainly can't do */
>   if (dinherit || dcreaterole || dcreatedb || dcanlogin || dconnlimit ||
> - dvalidUntil || disreplication || dbypassRLS)
> + dvalidUntil || disreplication || dbypassRLS ||
> + (dpassword && roleid != currentUserId))
>   ereport(ERROR,
>   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> - errmsg("permission denied")));
> -
> - /* 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("You must have %s privilege and %s on role \"%s\".",
> +    "CREATEROLE", "ADMIN OPTION", rolename)));
>   }
>   else if (!superuser())
>   {
> 
> Basically my question is whether having one error message for all of
> those cases is good enough, or whether we should be trying harder. I
> don't mind if the conclusion is that it's OK as-is, and I'm not
> entirely sure what would be better. But when I was working on this
> code, all of those cases OR'd together feeding into a single error
> message seemed a little sketchy to me, so I am wondering what others
> think.

I wondered the same thing, but I hesitated because I didn't want to change
too much in a patch for error messaging.  I can give it a try.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com