Horton
Menu

Re: improving user.c error messages

Robert Haas <robertmhaas@gmail.com>

View thread
From: Robert Haas <robertmhaas@gmail.com>
To: Nathan Bossart <nathandbossart@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:42:05Z
Lists: pgsql-hackers 
On Thu, Jan 26, 2023 at 2:14 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> Yeah, it's probably better to say "to alter roles with %s" to refer to
> roles that presently have the attribute and "to change the %s attribute"
> when referring to privileges for the attribute.  I did this in v2, too.
>
> I've also switched from errhint() to errdetail() as suggested by Tom.

This seems fine to me in general but I'm not entirely sure about this part:

@@ -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.

-- 
Robert Haas
EDB: http://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.