Thread: SHOW ALL does not honor pg_read_all_settings membership
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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