On Mon, Aug 22, 2022 at 9:29 PM Kyotaro Horiguchi
<horikyota....@gmail.com> wrote:
> Today, I see some error messages have been added, two of which look
> somewhat inconsistent.
>
> commands/user.c
> @707:
> > errmsg("must have admin option on role \"%s\" to add members",
> @1971:
> > errmsg("grantor must have ADMIN OPTION on \"%s\"",
>
> A grep'ing told me that the latter above is the only outlier among 6
> occurrences in total of "admin option/ADMIN OPTION".
>
> Don't we unify them? I slightly prefer "ADMIN OPTION" but no problem
> with them being in small letters. (Attached).
Fair point. There's some ambiguity in my mind about exactly how we
want to refer to this, which is probably why the messages ended up not
being entirely consistent. I feel like it's a little weird that we
talk about ADMIN OPTION as if it were a thing that you can possess.
For example, consider EXPLAIN. If you were trying to troubleshoot a
problem with a query plan, you wouldn't tell them "hey, please run
EXPLAIN, and be sure to use the ANALYZE OPTION". You would tell them
"hey, please run EXPLAIN, and be sure to use the ANALYZE option". In
that case, it's clear that the thing you need to include in the
command is ANALYZE -- which is an option -- not a thing called ANALYZE
OPTION.
In the case of GRANT, that's more ambiguous, because the word OPTION
actually appears in the syntax. But isn't that sort of accidental?
It's quite possible to give someone the right to administer a role
without ever mentioning the OPTION keyword:
rhaas=# create role bob;
CREATE ROLE
rhaas=# create role accounting admin bob;
CREATE ROLE
rhaas=# select roleid::regrole, member::regrole, grantor::regrole,
admin_option from pg_auth_members where roleid =
'accounting'::regrole;
roleid | member | grantor | admin_option
------------+--------+---------+--------------
accounting | bob | rhaas | t
(1 row)
You can't change this after-the-fact with ALTER ROLE or ALTER GROUP,
but if we added that ability, I imagine that the syntax would probably
not involve the OPTION keyword. You'd probably say something like:
ALTER ROLE accounting ADD ADMIN fred, or ALTER GROUP accounting DROP
ADMIN bob.
In short, I'm wondering whether we should regard ADMIN as the name of
the option, but OPTION as part of the GRANT syntax, and hence
capitalize it "ADMIN option". However, if the non-English speakers on
this list have a strong preference for something else I'm certainly
not going to fight about it.
> In passing, I met the following code in the same file.
>
> > if (!have_createrole_privilege() &&
> > !is_admin_of_role(currentUserId, roleid))
> > ereport(ERROR,
> >
> > (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> > errmsg("must have admin option on
> > role \"%s\"",
> > rolename)));
>
> The message seems a bit short that it only mentions admin option while
> omitting CREATEROLE privilege. "must have CREATEROLE privilege or
> admin option on role %s" might be better. Or we could say just
> "insufficient privilege" or "permission denied" in the main error
> message then provide "CREATEROLE privilege or admin option on role %s
> is required" in DETAILS or HINTS message. The message was added by
> c33d575899 along with the have_createrole_privilege() call so it is
> unclear to me whether it is intentional or not.
Yeah, I wasn't sure what to do about this. We do not mention superuser
privileges in every message where they theoretically apply, because it
would make a lot of messages longer for not much benefit. CREATEROLE
is a similar case and I think, but am not sure, that we treat it
similarly. So in my mind it is a judgement call what to do here, and
if other people think that what I picked wasn't best, we can change
it.
For what it's worth, I'm hoping to eventually remove the CREATEROLE
exception here. The superuser exception will remain, of course.
--
Robert Haas
EDB: http://www.enterprisedb.com
