Re: [PATCH] New predefined role pg_manage_extensions

Hi,

On Fri, Jan 17, 2025 at 10:03:17AM +0100, Laurenz Albe wrote:
> On Thu, 2024-10-31 at 22:47 +0100, Michael Banck wrote:
> > Even though there has not been a lot of discussion on this, here is a
> > rebased patch.  I have also added it to the upcoming commitfest.
> 
> I had a look at the patch.

Thanks! And sorry for the long delay...
 
> > --- a/doc/src/sgml/user-manag.sgml
> > +++ b/doc/src/sgml/user-manag.sgml
> > @@ -669,6 +669,17 @@ GRANT pg_signal_backend TO admin_user;
> >       </listitem>
> >      </varlistentry>
> >  
> > +    <varlistentry id="predefined-role-pg-manage-extensions" xreflabel="pg_manage_extensions">
> > +     <term><varname>pg_manage_extensions</varname></term>
> > +     <listitem>
> > +      <para>
> > +       <literal>pg_manage_extensions</literal> allows creating, removing or
> > +       updating extensions, even if the extensions are untrusted or the user is
> > +       not the database owner.
> > +      </para>
> > +     </listitem>
> > +    </varlistentry>
> > +
> 
> The inaccuracy of the database owner has already been mentioned.

Right, I had already changed that in my tree but never sent out an
updated version with this.
 
> Should we say "creating, altering or dropping extensions" to make the connection to
> the respective commands obvious?

Alright, did so.

> > --- a/src/backend/commands/extension.c
> > +++ b/src/backend/commands/extension.c
> > @@ -994,13 +994,14 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
> >     ListCell   *lc2;
> >  
> >     /*
> > -    * Enforce superuser-ness if appropriate.  We postpone these checks until
> > -    * here so that the control flags are correctly associated with the right
> > +    * Enforce superuser-ness/membership of the pg_manage_extensions
> > +    * predefined role if appropriate.  We postpone these checks until here
> > +    * so that the control flags are correctly associated with the right
> >      * script(s) if they happen to be set in secondary control files.
> >      */
> 
> This is just an attempt to improve the English:
> 
>   If appropriate, enforce superuser-ness or membership of the predefined role
>   pg_manage_extensions.

Done.

> > -                    : errhint("Must be superuser to create this extension.")));
> > +                    : errhint("Only roles with privileges of the \"%s\" role are allowed to create this extension.", "pg_manage_extensions")));
> 
> I don't see the point of breaking out the role name from the message.
> We don't do that in other places.

We actually do, I think I modelled it after other predefined roles,
e.g.:

|src/backend/commands/subscriptioncmds.c:				 errdetail("Only roles with privileges of the \"%s\" role may create subscriptions.",
|src/backend/commands/subscriptioncmds.c-						   "pg_create_subscription")));
|--
|src/backend/commands/copy.c:						 errdetail("Only roles with privileges of the \"%s\" role may COPY to or from an external program.",
|src/backend/commands/copy.c-								   "pg_execute_server_program"),
|--
|src/backend/commands/copy.c:						 errdetail("Only roles with privileges of the \"%s\" role may COPY from a file.",
|src/backend/commands/copy.c-								   "pg_read_server_files"),
|--
|src/backend/commands/copy.c:						 errdetail("Only roles with privileges of the \"%s\" role may COPY to a file.",
|src/backend/commands/copy.c-								   "pg_write_server_files"),

However, those are all errdetail, while the previous "Must be superuser
to [...]" is errhint, and that error message has different hints based
on whether the extension is trusted or not...

> Besides, I think that the mention of the superuser should be retained.
> Moreover, I think that commit 8d9978a717 pretty much established that we
> should not quote names if they contain underscores.
> Perhaps:
> 
>   errhint("Must be superuser or member of pg_manage_extensions to create this extension.")));

Alright, I think it makes more sense to have it like that than the
above, so changed it to that.

New version 3 attached.


Michael