Thread: Letter case of "admin option"

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


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.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2022-Aug-23, Kyotaro Horiguchi wrote:

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

As a translator, it makes a huge difference to have them in upper vs.
lower case.  In the former case I would keep it untranslated, while in
the latter I would translate it.  Given that these are keywords to use
in a command, I think making them uppercase is the better approach.

I see several other messages using "admin option" in lower case in
user.c.  The Spanish translation contains one translation already and it
is somewhat disappointing; I would prefer to have it as uppercase there
too.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Mon, Aug 22, 2022 at 9:29 PM Kyotaro Horiguchi
<horikyota.ntt@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

Thanks for the comment.

At Tue, 23 Aug 2022 09:58:47 -0400, Robert Haas <robertmhaas@gmail.com> wrote in 
> On Mon, Aug 22, 2022 at 9:29 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> 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?

Yeah I think so.  My intension is to let the translators do their work
more mechanically.  A capital-letter word is automatically recognized
as a keyword then can be copied as-is.

I would translate "ADMIN OPTION" to "ADMIN OPTION" in Japanese but
"admin option" is translated to "管理者オプション" which is a bit hard
for the readers to come up with the connection to "ADMIN OPTION" (or
ADMIN <roles>). I guess this is somewhat simliar to use "You need to
give capability to administrate the role" to suggest users to add WITH
ADMIN OPTION to the role.

Maybe Álvaro has a similar difficulty on it.

> It's quite possible to give someone the right to administer a role
> without ever mentioning the OPTION keyword:

Mmm.. Fair point.

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

"ADMIN option" which is translated into "ADMINオプション" is fine by
me.  I hope Álvaro thinks the same way.

What do you think about the attached?


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

If it were simply "permission denied", I don't think about the details
then seek for the way to allow that. But I don't mean to fight this
for now.

For the record, I would prefer the follwoing message for this sort of
failure.

ERROR:  permission denied
DETAILS: CREATEROLE or ADMIN option is required for the role.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2022-Aug-25, Kyotaro Horiguchi wrote:

> At Tue, 23 Aug 2022 09:58:47 -0400, Robert Haas <robertmhaas@gmail.com> wrote in 

> I would translate "ADMIN OPTION" to "ADMIN OPTION" in Japanese but
> "admin option" is translated to "管理者オプション" which is a bit hard
> for the readers to come up with the connection to "ADMIN OPTION" (or
> ADMIN <roles>). I guess this is somewhat simliar to use "You need to
> give capability to administrate the role" to suggest users to add WITH
> ADMIN OPTION to the role.
> 
> Maybe Álvaro has a similar difficulty on it.

Exactly.

I ran a quick poll in a Spanish community.  Everyone who responded (not
many admittedly) agreed with this idea -- they find the message clearer
if the keyword is mentioned explicitly in the translation.

> > 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.
> 
> "ADMIN option" which is translated into "ADMINオプション" is fine by
> me.  I hope Álvaro thinks the same way.

Hmm, but our docs say that the option is called ADMIN OPTION, don't
they?  And I think the standard sees it the same way.  You cannot invoke
it without the word OPTION.  I understand the point of view, but I don't
think it is clearer done that way.  It is different for example with
INHERIT; we could say "the INHERIT option" making the word "option"
translatable in that phrase.  But then you don't have to add that word
in the command.

> What do you think about the attached?

I prefer the <literal>ADMIN OPTION</literal> interpretation (both for
docs and error messages).  I think it's clearer that way, given that the
syntax is what it is.

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

I'm not opposed to moving that part of detail/hint, but I would prefer
that it says "the CREATEROLE privilege or ADMIN OPTION".

> --- a/doc/src/sgml/ref/alter_group.sgml
> +++ b/doc/src/sgml/ref/alter_group.sgml
> @@ -55,7 +55,7 @@ ALTER GROUP <replaceable class="parameter">group_name</replaceable> RENAME TO <r
>     <link linkend="sql-revoke"><command>REVOKE</command></link>. Note that
>     <command>GRANT</command> and <command>REVOKE</command> have additional
>     options which are not available with this command, such as the ability
> -   to grant and revoke <literal>ADMIN OPTION</literal>, and the ability to
> +   to grant and revoke <literal>ADMIN</literal> option, and the ability to
>     specify the grantor.
>    </para>

I think the original reads better.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)

On Thu, Aug 25, 2022 at 4:58 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> I ran a quick poll in a Spanish community.  Everyone who responded (not
> many admittedly) agreed with this idea -- they find the message clearer
> if the keyword is mentioned explicitly in the translation.

Makes sense. I didn't really doubt that ADMIN should be capitalized, I
just wasn't sure about OPTION.

> > > 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.
> >
> > "ADMIN option" which is translated into "ADMINオプション" is fine by
> > me.  I hope Álvaro thinks the same way.
>
> Hmm, but our docs say that the option is called ADMIN OPTION, don't
> they?  And I think the standard sees it the same way.  You cannot invoke
> it without the word OPTION.  I understand the point of view, but I don't
> think it is clearer done that way.  It is different for example with
> INHERIT; we could say "the INHERIT option" making the word "option"
> translatable in that phrase.  But then you don't have to add that word
> in the command.

It's going to be a little strange of we have ADMIN OPTION and INHERIT
option, isn't it? But we can try it.

One thing I have noticed, though, is that there are a lot of existing
references to ADMIN OPTION in code comments. If we decide on anything
else here we're going to have quite a few things to tidy up. Not that
that's a big deal I guess, but it's something to think about.

--
Robert Haas
EDB: http://www.enterprisedb.com

Here's a patch changing all occurrences of "admin option" in error
messages to "ADMIN OPTION".

Two of these five messages also exist in previous releases; the other
three are new.

I'm not sure if this is our final conclusion on what we want to do
here, so please speak up if you don't agree.

Thanks,

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On 2022-Aug-26, Robert Haas wrote:

> Here's a patch changing all occurrences of "admin option" in error
> messages to "ADMIN OPTION".
> 
> Two of these five messages also exist in previous releases; the other
> three are new.
> 
> I'm not sure if this is our final conclusion on what we want to do
> here, so please speak up if you don't agree.

Thanks -- this is my personal preference, as well as speaking on behalf
of a few people who considered the matter from a user's point of view.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/