SHOW ALL does not honor pg_read_all_settings membership

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

SHOW ALL does not honor pg_read_all_settings membership

Laurenz Albe
I noticed that commit 25fff40798fc4ac11a241bfd9ab0c45c085e2212 forgot
to teach SHOW ALL to show all GUCs when the user belongs to pg_read_all_settings.

Patch attached; I think this should be backpatched.

Yours,
Laurenz Albe

0001-Teach-SHOW-ALL-to-honor-pg_read_all_settings-members.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SHOW ALL does not honor pg_read_all_settings membership

Laurenz Albe
On Thu, 2018-03-01 at 16:22 +0100, I wrote:
> I noticed that commit 25fff40798fc4ac11a241bfd9ab0c45c085e2212 forgot
> to teach SHOW ALL to show all GUCs when the user belongs to pg_read_all_settings.
>
> Patch attached; I think this should be backpatched.

Now that the dust from the last commitfest is settling, I'll make a second
attempt to attract attention for this small bug fix.

The original commit was Simon's.

Yours,
Laurenz Albe

Reply | Threaded
Open this post in threaded view
|

Re: SHOW ALL does not honor pg_read_all_settings membership

Michael Paquier-2
On Mon, Apr 16, 2018 at 02:32:10PM +0200, Laurenz Albe wrote:
> Now that the dust from the last commitfest is settling, I'll make a second
> attempt to attract attention for this small bug fix.
>
> The original commit was Simon's.

Thanks for the ping.

This was new as of v10, so this cannot be listed as an open item still I
have added that under the section for older bugs, because you are right
as far as I can see.

GetConfigOption is wrong by the way, as restrict_superuser means that
all members of the group pg_read_all_settings can read
GUC_SUPERUSER_ONLY params, and not only superusers, so the comment at
least needs a fix, the variable ought to be renamed as well.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SHOW ALL does not honor pg_read_all_settings membership

Laurenz Albe
Michael Paquier wrote:

> On Mon, Apr 16, 2018 at 02:32:10PM +0200, Laurenz Albe wrote:
> > Now that the dust from the last commitfest is settling, I'll make a second
> > attempt to attract attention for this small bug fix.
> >
> > The original commit was Simon's.
>
> Thanks for the ping.
>
> This was new as of v10, so this cannot be listed as an open item still I
> have added that under the section for older bugs, because you are right
> as far as I can see.
>
> GetConfigOption is wrong by the way, as restrict_superuser means that
> all members of the group pg_read_all_settings can read
> GUC_SUPERUSER_ONLY params, and not only superusers, so the comment at
> least needs a fix, the variable ought to be renamed as well.
Thanks for the review!

I agree; here is a patch for that.

Yours,
Laurenz Albe

0002-Fix-comments-and-a-parameter-name.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SHOW ALL does not honor pg_read_all_settings membership

Michael Paquier-2
On Fri, Apr 20, 2018 at 01:21:46PM +0200, Laurenz Albe wrote:
> I agree; here is a patch for that.

Thanks for taking care of that as well this looks fine to me.  Note to
committer: this needs to be merged with the first patch actually fixing
the SHOW ALL issue.

All the four core callers of GetConfigOption() actually do not use
restrict_superuser set to true...  Modules may use this option, so of
course let's not remove it.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SHOW ALL does not honor pg_read_all_settings membership

Simon Riggs
In reply to this post by Michael Paquier-2
On 17 April 2018 at 04:28, Michael Paquier <[hidden email]> wrote:

> On Mon, Apr 16, 2018 at 02:32:10PM +0200, Laurenz Albe wrote:
>> Now that the dust from the last commitfest is settling, I'll make a second
>> attempt to attract attention for this small bug fix.
>>
>> The original commit was Simon's.
>
> Thanks for the ping.
>
> This was new as of v10, so this cannot be listed as an open item still I
> have added that under the section for older bugs, because you are right
> as far as I can see.

OK, agreed, its a bug.

Any objections to backpatch to v10?

> GetConfigOption is wrong by the way, as restrict_superuser means that
> all members of the group pg_read_all_settings can read
> GUC_SUPERUSER_ONLY params, and not only superusers, so the comment at
> least needs a fix, the variable ought to be renamed as well.

OK

--
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: SHOW ALL does not honor pg_read_all_settings membership

Michael Paquier-2
On Thu, May 31, 2018 at 07:05:58PM +0100, Simon Riggs wrote:
> Any objections to backpatch to v10?

A backpatch is acceptable in my opinion.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SHOW ALL does not honor pg_read_all_settings membership

Alvaro Herrera-9
On 2018-May-31, Michael Paquier wrote:

> On Thu, May 31, 2018 at 07:05:58PM +0100, Simon Riggs wrote:
> > Any objections to backpatch to v10?
>
> A backpatch is acceptable in my opinion.

Agreed on backpatching.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: SHOW ALL does not honor pg_read_all_settings membership

Alvaro Herrera-9
BTW a further bug here seems to be that "select * from pg_settings()"
does not show the source file/line for members of the role, which seems
to be documented to occur.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: SHOW ALL does not honor pg_read_all_settings membership

Alvaro Herrera-9
On 2018-Jun-08, Alvaro Herrera wrote:

> BTW a further bug here seems to be that "select * from pg_settings()"
> does not show the source file/line for members of the role, which seems
> to be documented to occur.

And I think this fixes it.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

0001-fix-sourcefile-line-display-for-pg_read_all_settings.patch (972 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SHOW ALL does not honor pg_read_all_settings membership

Alvaro Herrera-9
In reply to this post by Laurenz Albe
On 2018-Mar-01, Laurenz Albe wrote:

> I noticed that commit 25fff40798fc4ac11a241bfd9ab0c45c085e2212 forgot
> to teach SHOW ALL to show all GUCs when the user belongs to pg_read_all_settings.
>
> Patch attached; I think this should be backpatched.

Done, with the further fixes downthread.  Thanks!

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: SHOW ALL does not honor pg_read_all_settings membership

Michael Paquier-2
In reply to this post by Alvaro Herrera-9
On Fri, Jun 08, 2018 at 03:13:57PM -0400, Alvaro Herrera wrote:
> And I think this fixes it.

-    if (conf->source == PGC_S_FILE && superuser())
+    if (conf->source == PGC_S_FILE &&
+        is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS))

Thanks Alvaro!  This bit in GetConfigOptionByNum() looks fine to me.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SHOW ALL does not honor pg_read_all_settings membership

Laurenz Albe
In reply to this post by Alvaro Herrera-9
Alvaro Herrera wrote:
> On 2018-Mar-01, Laurenz Albe wrote:
>
> > I noticed that commit 25fff40798fc4ac11a241bfd9ab0c45c085e2212 forgot
> > to teach SHOW ALL to show all GUCs when the user belongs to pg_read_all_settings.
> >
> > Patch attached; I think this should be backpatched.
>
> Done, with the further fixes downthread.  Thanks!

Thank you!

Laurenz Albe