SHOW ALL does not honor pg_read_all_settings membership

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

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

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

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

On 17 April 2018 at 04:28, Michael Paquier <michael@paquier.xyz> 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

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

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

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

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

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

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

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