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
Attachments:
lower_ADMIN_OPTION.patchtext/x-patch; charset=us-asciiDownload
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 511ca8d8fd..4a2e5556f1 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -1968,7 +1968,7 @@ check_role_grantor(Oid currentUserId, Oid roleid, Oid grantorId, bool is_grant)
select_best_admin(grantorId, roleid) != grantorId)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("grantor must have ADMIN OPTION on \"%s\"",
+ errmsg("grantor must have admin option on \"%s\"",
GetUserNameFromId(roleid, false))));
}
else
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
Attachments:
Fix-letter-cases-of-ADMIN-option.patchtext/x-patch; charset=us-asciiDownload
diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml
index 350c75bc31..5f661552d8 100644
--- a/doc/src/sgml/information_schema.sgml
+++ b/doc/src/sgml/information_schema.sgml
@@ -183,8 +183,8 @@
<para>
The view <literal>administrable_role_authorizations</literal>
- identifies all roles that the current user has the admin option
- for.
+ identifies all roles that the current user has the <literal>ADMIN</literal>
+ option for.
</para>
<table>
diff --git a/doc/src/sgml/ref/alter_group.sgml b/doc/src/sgml/ref/alter_group.sgml
index b9e641818c..4b528498df 100644
--- 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>
diff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml
index c6a7c603f7..22cd885c82 100644
--- a/doc/src/sgml/ref/createuser.sgml
+++ b/doc/src/sgml/ref/createuser.sgml
@@ -82,7 +82,8 @@ PostgreSQL documentation
<listitem>
<para>
Indicates role that will be immediately added as a member of the new
- role with admin option, giving it the right to grant membership in the
+ role with <literal>ADMIN</literal> option, giving it the right to
+ grant membership in the
new role to others. Multiple roles to add as members (with admin
option) of the new role can be specified by writing multiple
<option>-a</option> switches.
diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml
index 2fd0f34d55..8f0c572051 100644
--- a/doc/src/sgml/ref/grant.sgml
+++ b/doc/src/sgml/ref/grant.sgml
@@ -257,10 +257,10 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
<para>
If <literal>WITH ADMIN OPTION</literal> is specified, the member can
in turn grant membership in the role to others, and revoke membership
- in the role as well. Without the admin option, ordinary users cannot
- do that. A role is not considered to hold <literal>WITH ADMIN
- OPTION</literal> on itself. Database superusers can grant or revoke
- membership in any role to anyone. Roles having
+ in the role as well. Without the <literal>ADMIN</literal> option, ordinary
+ users cannot do that. A role is not considered to hold
+ the <literal>ADMIN</literal> option on itself. Database superusers can
+ grant or revoke membership in any role to anyone. Roles having
<literal>CREATEROLE</literal> privilege can grant or revoke membership
in any role that is not a superuser.
</para>
@@ -269,11 +269,11 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
If <literal>GRANTED BY</literal> is specified, the grant is recorded as
having been done by the specified role. A user can only attribute a grant
to another role if they possess the privileges of that role. The role
- recorded as the grantor must have <literal>ADMIN OPTION</literal> on the
+ recorded as the grantor must have <literal>ADMIN</literal> option on the
target role, unless it is the bootstrap superuser. When a grant is recorded
as having a grantor other than the bootstrap superuser, it depends on the
- grantor continuing to posess <literal>ADMIN OPTION</literal> on the role;
- so, if <literal>ADMIN OPTION</literal> is revoked, dependent grants must
+ grantor continuing to posess <literal>ADMIN</literal> option on the role;
+ so, if <literal>ADMIN</literal> option is revoked, dependent grants must
be revoked as well.
</para>
diff --git a/doc/src/sgml/ref/revoke.sgml b/doc/src/sgml/ref/revoke.sgml
index 16e840458c..d783beaaa4 100644
--- a/doc/src/sgml/ref/revoke.sgml
+++ b/doc/src/sgml/ref/revoke.sgml
@@ -197,7 +197,7 @@ REVOKE [ ADMIN OPTION FOR ]
<para>
When revoking membership in a role, <literal>GRANT OPTION</literal> is instead
- called <literal>ADMIN OPTION</literal>, but the behavior is similar.
+ called <literal>ADMIN</literal> option, but the behavior is similar.
Note that, in releases prior to <productname>PostgreSQL</productname> 16,
dependent privileges were not tracked for grants of role membership,
and thus <literal>CASCADE</literal> had no effect for role membership.
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 511ca8d8fd..f46a7ab930 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -41,7 +41,7 @@
#include "utils/timestamp.h"
/*
- * Removing a role grant - or the admin option on it - might recurse to
+ * Removing a role grant - or the ADMIN option on it - might recurse to
* dependent grants. We use these values to reason about what would need to
* be done in such cases.
*
@@ -662,7 +662,7 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
* superuser. We also insist on superuser to change the BYPASSRLS
* property. Otherwise, if you don't have createrole, you're only allowed
* to (1) change your own password or (2) add members to a role for which
- * you have ADMIN OPTION.
+ * you have ADMIN option.
*/
if (authform->rolsuper || dissuper)
{
@@ -704,7 +704,7 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
if (drolemembers && !is_admin_of_role(GetUserId(), roleid))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must have admin option on role \"%s\" to add members",
+ errmsg("must have ADMIN option on role \"%s\" to add members",
rolename)));
}
@@ -1483,7 +1483,7 @@ roleSpecsToIds(List *memberNames)
* memberSpecs: list of RoleSpec of roles to add (used only for error messages)
* memberIds: OIDs of roles to add
* grantorId: who is granting the membership (InvalidOid if not set explicitly)
- * admin_opt: granting admin option?
+ * admin_opt: granting ADMIN option?
*/
static void
AddRoleMems(const char *rolename, Oid roleid,
@@ -1503,7 +1503,7 @@ AddRoleMems(const char *rolename, Oid roleid,
return;
/*
- * Check permissions: must have createrole or admin option on the role to
+ * Check permissions: must have CREATEROLE or ADMIN option on the role to
* be changed. To mess with a superuser role, you gotta be superuser.
*/
if (superuser_arg(roleid))
@@ -1519,7 +1519,7 @@ AddRoleMems(const char *rolename, Oid roleid,
!is_admin_of_role(currentUserId, roleid))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must have admin option on role \"%s\"",
+ errmsg("must have ADMIN option on role \"%s\"",
rolename)));
}
@@ -1543,7 +1543,7 @@ AddRoleMems(const char *rolename, Oid roleid,
/*
* Only allow changes to this role by one backend at a time, so that we
- * can check integrity constraints like the lack of circular ADMIN OPTION
+ * can check integrity constraints like the lack of circular ADMIN option
* grants without fear of race conditions.
*/
LockSharedObject(AuthIdRelationId, roleid, 0,
@@ -1562,7 +1562,7 @@ AddRoleMems(const char *rolename, Oid roleid,
* TO proposed_datdba" fail if is_member_of_role(pg_database_owner,
* proposed_datdba). Hence, gaining a membership could reduce what a
* role could do. Alternately, one could allow these memberships to
- * complete loops. A role could then have actual WITH ADMIN OPTION on
+ * complete loops. A role could then have actual ADMIN option on
* itself, prompting a decision about is_admin_of_role() treatment of
* the case.
*
@@ -1594,7 +1594,7 @@ AddRoleMems(const char *rolename, Oid roleid,
}
/*
- * Disallow attempts to grant ADMIN OPTION back to a user who granted it
+ * Disallow attempts to grant ADMIN option back to a user who granted it
* to you, similar to what check_circularity does for ACLs. We want the
* chains of grants to remain acyclic, so that it's always possible to use
* REVOKE .. CASCADE to clean up all grants that depend on the one being
@@ -1603,8 +1603,8 @@ AddRoleMems(const char *rolename, Oid roleid,
* NB: This check might look redundant with the check for membership loops
* above, but it isn't. That's checking for role-member loop (e.g. A is a
* member of B and B is a member of A) while this is checking for a
- * member-grantor loop (e.g. A gave ADMIN OPTION on X to B and now B, who
- * has no other source of ADMIN OPTION on X, tries to give ADMIN OPTION on
+ * member-grantor loop (e.g. A gave ADMIN option on X to B and now B, who
+ * has no other source of ADMIN option on X, tries to give ADMIN option on
* X back to A).
*/
if (admin_opt && grantorId != BOOTSTRAP_SUPERUSERID)
@@ -1629,7 +1629,7 @@ AddRoleMems(const char *rolename, Oid roleid,
if (memberid == BOOTSTRAP_SUPERUSERID)
ereport(ERROR,
(errcode(ERRCODE_INVALID_GRANT_OPERATION),
- errmsg("admin option cannot be granted back to your own grantor")));
+ errmsg("ADMIN option cannot be granted back to your own grantor")));
plan_member_revoke(memlist, actions, memberid);
}
@@ -1654,7 +1654,7 @@ AddRoleMems(const char *rolename, Oid roleid,
if (i >= memlist->n_members)
ereport(ERROR,
(errcode(ERRCODE_INVALID_GRANT_OPERATION),
- errmsg("admin option cannot be granted back to your own grantor")));
+ errmsg("ADMIN option cannot be granted back to your own grantor")));
ReleaseSysCacheList(memlist);
}
@@ -1673,7 +1673,7 @@ AddRoleMems(const char *rolename, Oid roleid,
/*
* Check if entry for this role/member already exists; if so, give
- * warning unless we are adding admin option.
+ * warning unless we are adding ADMIN option.
*/
authmem_tuple = SearchSysCache3(AUTHMEMROLEMEM,
ObjectIdGetDatum(roleid),
@@ -1751,7 +1751,7 @@ AddRoleMems(const char *rolename, Oid roleid,
* memberSpecs: list of RoleSpec of roles to del (used only for error messages)
* memberIds: OIDs of roles to del
* grantorId: who is revoking the membership
- * admin_opt: remove admin option only?
+ * admin_opt: remove ADMIN option only?
* behavior: RESTRICT or CASCADE behavior for recursive removal
*/
static void
@@ -1775,7 +1775,7 @@ DelRoleMems(const char *rolename, Oid roleid,
return;
/*
- * Check permissions: must have createrole or admin option on the role to
+ * Check permissions: must have CREATEROLE or ADMIN option on the role to
* be changed. To mess with a superuser role, you gotta be superuser.
*/
if (superuser_arg(roleid))
@@ -1791,7 +1791,7 @@ DelRoleMems(const char *rolename, Oid roleid,
!is_admin_of_role(currentUserId, roleid))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must have admin option on role \"%s\"",
+ errmsg("must have ADMIN option on role \"%s\"",
rolename)));
}
@@ -1862,7 +1862,7 @@ DelRoleMems(const char *rolename, Oid roleid,
}
else
{
- /* Just turn off the admin option */
+ /* Just turn off the ADMIN option */
HeapTuple tuple;
Datum new_record[Natts_pg_auth_members] = {0};
bool new_record_nulls[Natts_pg_auth_members] = {0};
@@ -1891,7 +1891,7 @@ DelRoleMems(const char *rolename, Oid roleid,
* Sanity-check, or infer, the grantor for a GRANT or REVOKE statement
* targeting a role.
*
- * The grantor must always be either a role with ADMIN OPTION on the role in
+ * The grantor must always be either a role with ADMIN option on the role in
* which membership is being granted, or the bootstrap superuser. This is
* similar to the restriction enforced by select_best_grantor, except that
* roles don't have owners, so we regard the bootstrap superuser as the
@@ -1901,8 +1901,8 @@ DelRoleMems(const char *rolename, Oid roleid,
* be passed as InvalidOid, and this function will infer the user to be
* recorded as the grantor. In many cases, this will be the current user, but
* things get more complicated when the current user doesn't possess ADMIN
- * OPTION on the role but rather relies on having CREATEROLE privileges, or
- * on inheriting the privileges of a role which does have ADMIN OPTION. See
+ * option on the role but rather relies on having CREATEROLE privileges, or
+ * on inheriting the privileges of a role which does have ADMIN option. See
* below for details.
*
* If the grantor was specified by the user, then it must be a user that
@@ -1931,7 +1931,7 @@ check_role_grantor(Oid currentUserId, Oid roleid, Oid grantorId, bool is_grant)
return BOOTSTRAP_SUPERUSERID;
/*
- * Otherwise, the grantor must either have ADMIN OPTION on the role or
+ * Otherwise, the grantor must either have ADMIN option on the role or
* inherit the privileges of a role which does. In the former case,
* record the grantor as the current user; in the latter, pick one of
* the roles that is "most directly" inherited by the current role
@@ -1951,7 +1951,7 @@ check_role_grantor(Oid currentUserId, Oid roleid, Oid grantorId, bool is_grant)
* If an explicit grantor is specified, it must be a role whose privileges
* the current user possesses.
*
- * It should also be a role that has ADMIN OPTION on the target role, but
+ * It should also be a role that has ADMIN option on the target role, but
* we check this condition only in case of GRANT. For REVOKE, no matching
* grant should exist anyway, but if it somehow does, let the user get rid
* of it.
@@ -1968,7 +1968,7 @@ check_role_grantor(Oid currentUserId, Oid roleid, Oid grantorId, bool is_grant)
select_best_admin(grantorId, roleid) != grantorId)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("grantor must have ADMIN OPTION on \"%s\"",
+ errmsg("grantor must have ADMIN option on \"%s\"",
GetUserNameFromId(roleid, false))));
}
else
@@ -2012,7 +2012,7 @@ initialize_revoke_actions(CatCList *memlist)
/*
* Figure out what we would need to do in order to revoke a grant, or just the
- * admin option on a grant, given that there might be dependent privileges.
+ * ADMIN option on a grant, given that there might be dependent privileges.
*
* 'memlist' should be a list of all grants for the target role.
*
@@ -2126,7 +2126,7 @@ plan_recursive_revoke(CatCList *memlist, RevokeRoleGrantAction *actions,
actions[index] = RRG_REMOVE_ADMIN_OPTION;
}
- /* Determine whether the member would still have ADMIN OPTION. */
+ /* Determine whether the member would still have ADMIN option. */
for (i = 0; i < memlist->n_members; ++i)
{
HeapTuple am_cascade_tuple;
@@ -2143,7 +2143,7 @@ plan_recursive_revoke(CatCList *memlist, RevokeRoleGrantAction *actions,
}
}
- /* If the member would still have ADMIN OPTION, we need not recurse. */
+ /* If the member would still have ADMIN option, we need not recurse. */
if (would_still_have_admin_option)
return;
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 3e045da31f..02f4744acb 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -4802,7 +4802,7 @@ has_rolinherit(Oid roleid)
*
* If admin_of is not InvalidOid, this function sets *admin_role, either
* to the OID of the first role in the result list that directly possesses
- * ADMIN OPTION on the role corresponding to admin_of, or to InvalidOid if
+ * ADMIN option on the role corresponding to admin_of, or to InvalidOid if
* there is no such role.
*/
static List *
@@ -4819,7 +4819,7 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
if (admin_role != NULL)
*admin_role = InvalidOid;
- /* If cache is valid and ADMIN OPTION not sought, just return the list */
+ /* If cache is valid and ADMIN option not sought, just return the list */
if (cached_role[type] == roleid && !OidIsValid(admin_of) &&
OidIsValid(cached_role[type]))
return cached_roles[type];
@@ -5013,7 +5013,7 @@ is_member_of_role_nosuper(Oid member, Oid role)
/*
* Is member an admin of role? That is, is member the role itself (subject to
- * restrictions below), a member (directly or indirectly) WITH ADMIN OPTION,
+ * restrictions below), a member (directly or indirectly) with ADMIN option,
* or a superuser?
*/
bool
@@ -5024,7 +5024,7 @@ is_admin_of_role(Oid member, Oid role)
if (superuser_arg(member))
return true;
- /* By policy, a role cannot have WITH ADMIN OPTION on itself. */
+ /* By policy, a role cannot have ADMIN option on itself. */
if (member == role)
return false;
@@ -5033,11 +5033,11 @@ is_admin_of_role(Oid member, Oid role)
}
/*
- * Find a role whose privileges "member" inherits which has ADMIN OPTION
+ * Find a role whose privileges "member" inherits which has ADMIN option
* on "role", ignoring super-userness.
*
* There might be more than one such role; prefer one which involves fewer
- * hops. That is, if member has ADMIN OPTION, prefer that over all other
+ * hops. That is, if member has ADMIN option, prefer that over all other
* options; if not, prefer a role from which member inherits more directly
* over more indirect inheritance.
*/
@@ -5046,7 +5046,7 @@ select_best_admin(Oid member, Oid role)
{
Oid admin_role;
- /* By policy, a role cannot have WITH ADMIN OPTION on itself. */
+ /* By policy, a role cannot have ADMIN option on itself. */
if (member == role)
return InvalidOid;
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index e8a2bfa6bd..27ef96941a 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -956,7 +956,7 @@ dumpRoleMembership(PGconn *conn)
/*
* Previous versions of PostgreSQL didn't used to track the grantor very
* carefully in the backend, and the grantor could be any user even if
- * they didn't have ADMIN OPTION on the role, or a user that no longer
+ * they didn't have ADMIN option on the role, or a user that no longer
* existed. To avoid dump and restore failures, don't dump the grantor
* when talking to an old server version.
*/
@@ -981,13 +981,13 @@ dumpRoleMembership(PGconn *conn)
/*
* We can't dump these GRANT commands in arbitary order, because a role
- * that is named as a grantor must already have ADMIN OPTION on the
+ * that is named as a grantor must already have ADMIN option on the
* role for which it is granting permissions, except for the boostrap
* superuser, who can always be named as the grantor.
*
* We handle this by considering these grants role by role. For each role,
* we initially consider the only allowable grantor to be the boostrap
- * superuser. Every time we grant ADMIN OPTION on the role to some user,
+ * superuser. Every time we grant ADMIN option on the role to some user,
* that user also becomes an allowable grantor. We make repeated passes
* over the grants for the role, each time dumping those whose grantors
* are allowable and which we haven't done yet. Eventually this should
@@ -1072,7 +1072,7 @@ dumpRoleMembership(PGconn *conn)
--remaining;
/*
- * If ADMIN OPTION is being granted, remember that grants
+ * If ADMIN option is being granted, remember that grants
* listing this member as the grantor can now be dumped.
*/
if (*admin_option == 't')
diff --git a/src/bin/scripts/t/040_createuser.pl b/src/bin/scripts/t/040_createuser.pl
index 834d258bf8..a9de57f3b7 100644
--- a/src/bin/scripts/t/040_createuser.pl
+++ b/src/bin/scripts/t/040_createuser.pl
@@ -39,7 +39,7 @@ $node->issues_sql_like(
'regress user2', 'regress user #4'
],
qr/statement: CREATE ROLE "regress user #4" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN ADMIN regress_user1,"regress user2";/,
- 'add a role as a member with admin option of the newly created role');
+ 'add a role as a member with ADMIN option of the newly created role');
$node->issues_sql_like(
[
'createuser', '-m',
diff --git a/src/include/catalog/pg_auth_members.h b/src/include/catalog/pg_auth_members.h
index e57ec4f810..f0e04baadd 100644
--- a/src/include/catalog/pg_auth_members.h
+++ b/src/include/catalog/pg_auth_members.h
@@ -33,7 +33,7 @@ CATALOG(pg_auth_members,1261,AuthMemRelationId) BKI_SHARED_RELATION BKI_ROWTYPE_
Oid roleid BKI_LOOKUP(pg_authid); /* ID of a role */
Oid member BKI_LOOKUP(pg_authid); /* ID of a member of that role */
Oid grantor BKI_LOOKUP(pg_authid); /* who granted the membership */
- bool admin_option; /* granted with admin option? */
+ bool admin_option; /* granted with ADMIN option? */
} FormData_pg_auth_members;
/* ----------------
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 0154a09262..56981c85c8 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -37,7 +37,7 @@ CREATE ROLE regress_priv_role;
GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION;
GRANT regress_priv_user1 TO regress_priv_user3 WITH ADMIN OPTION GRANTED BY regress_priv_user2;
GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION GRANTED BY regress_priv_user3;
-ERROR: admin option cannot be granted back to your own grantor
+ERROR: ADMIN option cannot be granted back to your own grantor
-- need CASCADE to revoke grant or admin option if dependent grants exist
REVOKE ADMIN OPTION FOR regress_priv_user1 FROM regress_priv_user2; -- fail
ERROR: dependent privileges exist
@@ -159,7 +159,7 @@ CREATE FUNCTION leak(integer,integer) RETURNS boolean
ALTER FUNCTION leak(integer,integer) OWNER TO regress_priv_user1;
-- test owner privileges
GRANT regress_priv_role TO regress_priv_user1 WITH ADMIN OPTION GRANTED BY regress_priv_role; -- error, doesn't have ADMIN OPTION
-ERROR: grantor must have ADMIN OPTION on "regress_priv_role"
+ERROR: grantor must have ADMIN option on "regress_priv_role"
GRANT regress_priv_role TO regress_priv_user1 WITH ADMIN OPTION GRANTED BY CURRENT_ROLE;
REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY foo; -- error
ERROR: role "foo" does not exist
@@ -1774,7 +1774,7 @@ REFRESH MATERIALIZED VIEW sro_mv;
ERROR: cannot fire deferred trigger within security-restricted operation
CONTEXT: SQL function "mv_action" statement 1
BEGIN; SET CONSTRAINTS ALL IMMEDIATE; REFRESH MATERIALIZED VIEW sro_mv; COMMIT;
-ERROR: must have admin option on role "regress_priv_group2"
+ERROR: must have ADMIN option on role "regress_priv_group2"
CONTEXT: SQL function "unwanted_grant" statement 1
SQL statement "SELECT unwanted_grant()"
PL/pgSQL function sro_trojan() line 1 at PERFORM
@@ -1804,10 +1804,10 @@ CREATE FUNCTION dogrant_ok() RETURNS void LANGUAGE sql SECURITY DEFINER AS
GRANT regress_priv_group2 TO regress_priv_user5; -- ok: had ADMIN OPTION
SET ROLE regress_priv_group2;
GRANT regress_priv_group2 TO regress_priv_user5; -- fails: SET ROLE suspended privilege
-ERROR: must have admin option on role "regress_priv_group2"
+ERROR: must have ADMIN option on role "regress_priv_group2"
SET SESSION AUTHORIZATION regress_priv_user1;
GRANT regress_priv_group2 TO regress_priv_user5; -- fails: no ADMIN OPTION
-ERROR: must have admin option on role "regress_priv_group2"
+ERROR: must have ADMIN option on role "regress_priv_group2"
SELECT dogrant_ok(); -- ok: SECURITY DEFINER conveys ADMIN
NOTICE: role "regress_priv_user5" has already been granted membership in role "regress_priv_group2" by role "regress_priv_user4"
dogrant_ok
@@ -1817,10 +1817,10 @@ NOTICE: role "regress_priv_user5" has already been granted membership in role "
SET ROLE regress_priv_group2;
GRANT regress_priv_group2 TO regress_priv_user5; -- fails: SET ROLE did not help
-ERROR: must have admin option on role "regress_priv_group2"
+ERROR: must have ADMIN option on role "regress_priv_group2"
SET SESSION AUTHORIZATION regress_priv_group2;
GRANT regress_priv_group2 TO regress_priv_user5; -- fails: no self-admin
-ERROR: must have admin option on role "regress_priv_group2"
+ERROR: must have ADMIN option on role "regress_priv_group2"
SET SESSION AUTHORIZATION regress_priv_user4;
DROP FUNCTION dogrant_ok();
REVOKE regress_priv_group2 FROM regress_priv_user5;
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
Attachments:
capitalize-admin-option-v1.patchapplication/octet-stream; name=capitalize-admin-option-v1.patchDownload
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 265a48af7e..243990e8d3 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -726,7 +726,7 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
if (drolemembers && !is_admin_of_role(GetUserId(), roleid))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must have admin option on role \"%s\" to add members",
+ errmsg("must have ADMIN OPTION on role \"%s\" to add members",
rolename)));
}
@@ -1578,7 +1578,7 @@ AddRoleMems(const char *rolename, Oid roleid,
!is_admin_of_role(currentUserId, roleid))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must have admin option on role \"%s\"",
+ errmsg("must have ADMIN OPTION on role \"%s\"",
rolename)));
}
@@ -1688,7 +1688,7 @@ AddRoleMems(const char *rolename, Oid roleid,
if (memberid == BOOTSTRAP_SUPERUSERID)
ereport(ERROR,
(errcode(ERRCODE_INVALID_GRANT_OPERATION),
- errmsg("admin option cannot be granted back to your own grantor")));
+ errmsg("ADMIN OPTION cannot be granted back to your own grantor")));
plan_member_revoke(memlist, actions, memberid);
}
@@ -1713,7 +1713,7 @@ AddRoleMems(const char *rolename, Oid roleid,
if (i >= memlist->n_members)
ereport(ERROR,
(errcode(ERRCODE_INVALID_GRANT_OPERATION),
- errmsg("admin option cannot be granted back to your own grantor")));
+ errmsg("ADMIN OPTION cannot be granted back to your own grantor")));
ReleaseSysCacheList(memlist);
}
@@ -1897,7 +1897,7 @@ DelRoleMems(const char *rolename, Oid roleid,
!is_admin_of_role(currentUserId, roleid))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must have admin option on role \"%s\"",
+ errmsg("must have ADMIN OPTION on role \"%s\"",
rolename)));
}
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/