Horton
Menu

Re: improving user.c error messages

Alvaro Herrera <alvherre@alvh.no-ip.org>

View thread
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>
Date: 2023-01-27T18:31:19Z
Lists: pgsql-hackers 
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/

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.