Date: Thu, 16 Mar 2023 09:27:49 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, Tom Lane <tgl@sss.pgh.pa.us>,
	Robert Haas <robertmhaas@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: improving user.c error messages
Message-ID: <20230316162749.GA792895@nathanxps13>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <73ab0059-a652-e44a-73c6-36f350782078@enterprisedb.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20230316162749.GA792895%40nathanxps13>
Precedence: bulk

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