From: Nathan Bossart <nathandbossart@gmail.com>
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: improving user.c error messages
Date: Thu, 16 Mar 2023 09:27:49 -0700
Message-ID: <20230316162749.GA792895@nathanxps13> (raw)
In-Reply-To: <73ab0059-a652-e44a-73c6-36f350782078@enterprisedb.com>
References: <20230207201009.GA450353@nathanxps13>
<e1bbef97-ed13-f6ad-8d21-b2ebc251394b@enterprisedb.com>
<20230220190210.GA3481922@nathanxps13>
<20230220225852.GA3940888@nathanxps13>
<abf97f0d-79b4-217c-2ea8-d081648d9a6d@enterprisedb.com>
<20230309175846.GA3626988@nathanxps13>
<20230310000313.GA3992372@nathanxps13>
<b0084395-5379-3872-035a-a974aa5845fb@enterprisedb.com>
<20230316154858.GA791987@nathanxps13>
<73ab0059-a652-e44a-73c6-36f350782078@enterprisedb.com>
On Thu, Mar 16, 2023 at 04:59:53PM +0100, Peter Eisentraut wrote:
> On 16.03.23 16:48, Nathan Bossart wrote:
>> > I think the following change in DropRole() is incorrect:
>> >
>> > if (!is_admin_of_role(GetUserId(), roleid))
>> > ereport(ERROR,
>> > (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>> > - errmsg("must have admin option on role \"%s\"",
>> > - role)));
>> > + errmsg("permission denied to drop role"),
>> > + errdetail("Only roles with the %s attribute and the %s
>> > option on role \"%s\" may drop this role.",
>> > + "CREATEROLE", "ADMIN",
>> > NameStr(roleform->rolname))));
>> >
>> > The message does not reflect what check is actually performed. (Perhaps
>> > this was confused with a similar but not exactly the same check in
>> > RenameRole().)
>> Hm. Is your point that we should only mention the admin option here? I
>> mentioned both createrole and admin option in this message (and the
>> createrole check above this point) in an attempt to avoid giving partial
>> information.
>
> AFAICT, the mention of CREATEROLE is incorrect, because the code doesn't
> actually check for the CREATEROLE attribute.
There is a createrole check at the top of DropRole():
/*
* DROP ROLE
*/
void
DropRole(DropRoleStmt *stmt)
{
Relation pg_authid_rel,
pg_auth_members_rel;
ListCell *item;
List *role_addresses = NIL;
if (!have_createrole_privilege())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to drop role")));
Granted, no one will see the admin option error unless they at least have
createrole, so we could leave it out, but my intent was to list the full
set of privileges required to drop the role to avoid ambiguity.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, peter.eisentraut@enterprisedb.com, alvherre@alvh.no-ip.org, tgl@sss.pgh.pa.us, robertmhaas@gmail.com
Subject: Re: improving user.c error messages
In-Reply-To: <20230316162749.GA792895@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox