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