Horton
Menu

Re: improving user.c error messages

Nathan Bossart <nathandbossart@gmail.com>

View thread
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
Date: 2023-01-26T19:59:29Z
Lists: pgsql-hackers 
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

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Improve several permission-related error messages.

  2. Integrate superuser check into has_rolreplication()

  3. Small code simplification

  4. Adjust interaction of CREATEROLE with role properties.

  5. Add new GUC reserved_connections.

  6. Rename ReservedBackends variable to SuperuserReservedConnections.

  7. Update docs and error message for superuser_reserved_connections.

  8. Add new GUC createrole_self_grant.

  9. Restrict the privileges of CREATEROLE users.

  10. Add a SET option to the GRANT command.