Re: SET ROLE documentation improvement
On Fri, Apr 12, 2024 at 09:54:24AM +0900, Michael Paquier wrote: > On Thu, Apr 11, 2024 at 02:21:49PM -0500, Nathan Bossart wrote: >> No objections here. I'll give this a few days for others to comment. I'm >> not particularly interested in back-patching this since it's arguably not >> fixing anything that's incorrect, but if anyone really wants me to, I will. > > HEAD looks fine based on what I'm reading in the patch. If there are > more voices in favor of a backpatch, it could always happen later. Committed. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: SET ROLE documentation improvement
On Thu, Apr 11, 2024 at 02:21:49PM -0500, Nathan Bossart wrote: > No objections here. I'll give this a few days for others to comment. I'm > not particularly interested in back-patching this since it's arguably not > fixing anything that's incorrect, but if anyone really wants me to, I will. HEAD looks fine based on what I'm reading in the patch. If there are more voices in favor of a backpatch, it could always happen later. -- Michael signature.asc Description: PGP signature
Re: SET ROLE documentation improvement
On Thu, Apr 11, 2024 at 01:38:37PM -0400, Robert Haas wrote: > On Thu, Apr 11, 2024 at 1:03 PM Nathan Bossart > wrote: >> While it's fresh on my mind, I very hastily hacked together a draft of what >> I believe is the remaining work. > > That looks fine to me. And if others agree, I think it's fine to just > commit this now, post-freeze. It's only a doc change, and a > back-patchable one if you want to go that route. No objections here. I'll give this a few days for others to comment. I'm not particularly interested in back-patching this since it's arguably not fixing anything that's incorrect, but if anyone really wants me to, I will. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: SET ROLE documentation improvement
On Thu, Apr 11, 2024 at 1:03 PM Nathan Bossart wrote: > On Thu, Apr 11, 2024 at 11:48:30AM -0500, Nathan Bossart wrote: > > On Thu, Apr 11, 2024 at 11:36:52AM -0400, Robert Haas wrote: > >> I suggest that we close the existing CF entry as committed and > >> somebody can start a new one for whatever remains. I think that will > >> be less confusing. > > > > Done: https://commitfest.postgresql.org/48/4923/. > > While it's fresh on my mind, I very hastily hacked together a draft of what > I believe is the remaining work. That looks fine to me. And if others agree, I think it's fine to just commit this now, post-freeze. It's only a doc change, and a back-patchable one if you want to go that route. -- Robert Haas EDB: http://www.enterprisedb.com
Re: SET ROLE documentation improvement
On Thu, Apr 11, 2024 at 11:48:30AM -0500, Nathan Bossart wrote: > On Thu, Apr 11, 2024 at 11:36:52AM -0400, Robert Haas wrote: >> I suggest that we close the existing CF entry as committed and >> somebody can start a new one for whatever remains. I think that will >> be less confusing. > > Done: https://commitfest.postgresql.org/48/4923/. While it's fresh on my mind, I very hastily hacked together a draft of what I believe is the remaining work. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com >From bb3aa06b6e55b403489afafcc8b7608516fd7b40 Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Thu, 11 Apr 2024 12:01:21 -0500 Subject: [PATCH v3 1/1] further improvements to SET ROLE docs --- doc/src/sgml/ref/set_role.sgml | 10 -- 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/doc/src/sgml/ref/set_role.sgml b/doc/src/sgml/ref/set_role.sgml index 083e6dc6ea..9557bb77ab 100644 --- a/doc/src/sgml/ref/set_role.sgml +++ b/doc/src/sgml/ref/set_role.sgml @@ -37,7 +37,10 @@ RESET ROLE written as either an identifier or a string literal. After SET ROLE, permissions checking for SQL commands is carried out as though the named role were the one that had logged - in originally. + in originally. Note that SET ROLE and + SET SESSION AUTHORIZATION are exceptions; permissions + checks for those continue to use the current session user and the initial + session user (the authenticated user), respectively. @@ -88,11 +91,6 @@ RESET ROLE exercised either with or without SET ROLE. - - Note that when a superuser chooses to SET ROLE to a - non-superuser role, they lose their superuser privileges. - - SET ROLE has effects comparable to SET SESSION AUTHORIZATION, but the privilege -- 2.25.1
Re: SET ROLE documentation improvement
On Thu, Apr 11, 2024 at 11:36:52AM -0400, Robert Haas wrote: > I suggest that we close the existing CF entry as committed and > somebody can start a new one for whatever remains. I think that will > be less confusing. Done: https://commitfest.postgresql.org/48/4923/. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: SET ROLE documentation improvement
On Thu, Apr 11, 2024 at 10:03 AM Nathan Bossart wrote: > On Thu, Apr 11, 2024 at 10:33:15AM +0900, Michael Paquier wrote: > > On Tue, Apr 09, 2024 at 09:21:39AM +0300, Andrey M. Borodin wrote: > >> Can I ask you please to help me with determining status of CF item > >> [0]. Is it committed or there's something to move to next CF? > > > > Only half of the patch has been applied as of 3330a8d1b792. Yurii and > > Nathan, could you follow up with the rest? Moving the patch to the > > next CF makes sense, and the last thread update is rather recent. > > AFAICT there are two things remaining: > > * Remove the "they lose their superuser privileges" note. > * Note that SET ROLE and SET SESSION AUTHORIZATION are exceptions. > > While I think these are good changes, I don't sense any urgency here, so > I'm treating this as v18 material at this point. I suggest that we close the existing CF entry as committed and somebody can start a new one for whatever remains. I think that will be less confusing. -- Robert Haas EDB: http://www.enterprisedb.com
Re: SET ROLE documentation improvement
On Thu, Apr 11, 2024 at 10:33:15AM +0900, Michael Paquier wrote: > On Tue, Apr 09, 2024 at 09:21:39AM +0300, Andrey M. Borodin wrote: >> Can I ask you please to help me with determining status of CF item >> [0]. Is it committed or there's something to move to next CF? > > Only half of the patch has been applied as of 3330a8d1b792. Yurii and > Nathan, could you follow up with the rest? Moving the patch to the > next CF makes sense, and the last thread update is rather recent. AFAICT there are two things remaining: * Remove the "they lose their superuser privileges" note. * Note that SET ROLE and SET SESSION AUTHORIZATION are exceptions. While I think these are good changes, I don't sense any urgency here, so I'm treating this as v18 material at this point. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: SET ROLE documentation improvement
On Tue, Apr 09, 2024 at 09:21:39AM +0300, Andrey M. Borodin wrote: > Can I ask you please to help me with determining status of CF item > [0]. Is it committed or there's something to move to next CF? Only half of the patch has been applied as of 3330a8d1b792. Yurii and Nathan, could you follow up with the rest? Moving the patch to the next CF makes sense, and the last thread update is rather recent. -- Michael signature.asc Description: PGP signature
Re: SET ROLE documentation improvement
> On 24 Mar 2024, at 23:34, Nathan Bossart wrote: > > Committed that part. Hi Nathan and Yurii! Can I ask you please to help me with determining status of CF item [0]. Is it committed or there's something to move to next CF? Thanks! Best regards, Andrey Borodin. [0] https://commitfest.postgresql.org/47/4572/
Re: SET ROLE documentation improvement
On Sat, Mar 23, 2024 at 08:37:20AM -0400, Robert Haas wrote: > On Fri, Mar 22, 2024 at 4:51 PM Nathan Bossart > wrote: >> Actually, shouldn't this one be back-patched to v16? If so, I'd do that >> one separately from the other changes we are discussing. > > Sure, that seems fine. Committed that part. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: SET ROLE documentation improvement
On Fri, Mar 22, 2024 at 4:51 PM Nathan Bossart wrote: > Actually, shouldn't this one be back-patched to v16? If so, I'd do that > one separately from the other changes we are discussing. Sure, that seems fine. -- Robert Haas EDB: http://www.enterprisedb.com
Re: SET ROLE documentation improvement
On Fri, Mar 22, 2024 at 03:45:06PM -0500, Nathan Bossart wrote: > On Fri, Mar 22, 2024 at 03:58:59PM -0400, Robert Haas wrote: >> On Fri, Nov 10, 2023 at 12:41 PM Nathan Bossart >> wrote: >>> I still think we should update the existing note about privileges for >>> SET/RESET ROLE to something like the following: >>> >>> diff --git a/doc/src/sgml/ref/set_role.sgml b/doc/src/sgml/ref/set_role.sgml >>> index 13bad1bf66..c91a95f5af 100644 >>> --- a/doc/src/sgml/ref/set_role.sgml >>> +++ b/doc/src/sgml/ref/set_role.sgml >>> @@ -41,8 +41,10 @@ RESET ROLE >>> >>> >>> >>> - The specified role_name >>> - must be a role that the current session user is a member of. >>> + The current session user must have the SET for the >>> + specified role_name, either >>> + directly or indirectly via a chain of memberships with the >>> + SET option. >>> (If the session user is a superuser, any role can be selected.) >>> >> >> This is a good change; I should have done this when SET was added. > > Cool. Actually, shouldn't this one be back-patched to v16? If so, I'd do that one separately from the other changes we are discussing. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: SET ROLE documentation improvement
On Fri, Mar 22, 2024 at 03:58:59PM -0400, Robert Haas wrote: > On Fri, Nov 10, 2023 at 12:41 PM Nathan Bossart > wrote: >> Looking again, I'm kind of hesitant to add too much qualification to this >> note about losing superuser privileges. > > The note in question is: > > >Note that when a superuser chooses to SET ROLE to a >non-superuser role, they lose their superuser privileges. > > > It's not entirely clear to me what the point of this note is. I think > we could consider removing it entirely, on the theory that it's just > poorly-stated special case of what's already been said in the > description, i.e. "permissions checking for SQL commands is carried > out as though the named role were the one that had logged in > originally" and "The specified class="parameter">role_name must be a role that the > current session user is a member of." +1. IMHO these kinds of special mentions of SUPERUSER tend to be redundant, and, as evidenced by this thread, confusing. I'll update the patch. > I think it's also possible that what the author of this paragraph > meant was that role attributes like CREATEDB, CREATEROLE, REPLICATION, > and SUPERUSER follow the current user, not the session user. If we > think that was the point of this paragraph, we could make it say that > more clearly. However, I'm not sure that really needs to be mentioned, > because "permissions checking for SQL commands is carried out as > though the named role were the one that had logged in originally" > seems to cover that ground along with everything else. +1 >> I still think we should update the existing note about privileges for >> SET/RESET ROLE to something like the following: >> >> diff --git a/doc/src/sgml/ref/set_role.sgml b/doc/src/sgml/ref/set_role.sgml >> index 13bad1bf66..c91a95f5af 100644 >> --- a/doc/src/sgml/ref/set_role.sgml >> +++ b/doc/src/sgml/ref/set_role.sgml >> @@ -41,8 +41,10 @@ RESET ROLE >> >> >> >> - The specified role_name >> - must be a role that the current session user is a member of. >> + The current session user must have the SET for the >> + specified role_name, either >> + directly or indirectly via a chain of memberships with the >> + SET option. >> (If the session user is a superuser, any role can be selected.) >> > > This is a good change; I should have done this when SET was added. Cool. > Another change we could consider is revising "permissions checking for > SQL commands is carried out as though the named role were the one that > had logged in originally" to mention that SET ROLE and SET SESSION > AUTHORIZATION are exceptions. That seems like a resonable idea, although it might require a few rounds of wordsmithing. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: SET ROLE documentation improvement
On Fri, Nov 10, 2023 at 12:41 PM Nathan Bossart wrote: > On Tue, Sep 26, 2023 at 08:33:25AM -0700, Yurii Rashkovskii wrote: > > This is a good start, indeed. I've amended my patch to include it. > > Thanks for the new patch. > > Looking again, I'm kind of hesitant to add too much qualification to this > note about losing superuser privileges. The note in question is: Note that when a superuser chooses to SET ROLE to a non-superuser role, they lose their superuser privileges. It's not entirely clear to me what the point of this note is. I think we could consider removing it entirely, on the theory that it's just poorly-stated special case of what's already been said in the description, i.e. "permissions checking for SQL commands is carried out as though the named role were the one that had logged in originally" and "The specified role_name must be a role that the current session user is a member of." I think it's also possible that what the author of this paragraph meant was that role attributes like CREATEDB, CREATEROLE, REPLICATION, and SUPERUSER follow the current user, not the session user. If we think that was the point of this paragraph, we could make it say that more clearly. However, I'm not sure that really needs to be mentioned, because "permissions checking for SQL commands is carried out as though the named role were the one that had logged in originally" seems to cover that ground along with everything else. > I still think we should update the existing note about privileges for > SET/RESET ROLE to something like the following: > > diff --git a/doc/src/sgml/ref/set_role.sgml b/doc/src/sgml/ref/set_role.sgml > index 13bad1bf66..c91a95f5af 100644 > --- a/doc/src/sgml/ref/set_role.sgml > +++ b/doc/src/sgml/ref/set_role.sgml > @@ -41,8 +41,10 @@ RESET ROLE > > > > - The specified role_name > - must be a role that the current session user is a member of. > + The current session user must have the SET for the > + specified role_name, either > + directly or indirectly via a chain of memberships with the > + SET option. > (If the session user is a superuser, any role can be selected.) > This is a good change; I should have done this when SET was added. Another change we could consider is revising "permissions checking for SQL commands is carried out as though the named role were the one that had logged in originally" to mention that SET ROLE and SET SESSION AUTHORIZATION are exceptions. -- Robert Haas EDB: http://www.enterprisedb.com
Re: SET ROLE documentation improvement
On Fri, Nov 10, 2023 at 11:11 PM Nathan Bossart wrote: > > On Tue, Sep 26, 2023 at 08:33:25AM -0700, Yurii Rashkovskii wrote: > > This is a good start, indeed. I've amended my patch to include it. > > Thanks for the new patch. > > Looking again, I'm kind of hesitant to add too much qualification to this > note about losing superuser privileges. If we changed it to > > Note that when a superuser chooses to SET ROLE to a non-superuser > role, > they lose their superuser privileges, except for the privilege to > change to another role again using SET ROLE or RESET ROLE. > > it almost seems to imply that a non-superuser role could obtain the ability > to switch to any role if they first SET ROLE to a superuser. In practice, > that's true because they could just give the session role SUPERUSER, but I > don't think that's the intent of this section. > > I thought about changing it to something like > > Note that when a superuser chooses to SET ROLE to a non-superuser > role, > they lose their superuser privileges. However, if the current session > user is a superuser, they retain the ability to set the current user > identifier to any role via SET ROLE and RESET ROLE. > > but it seemed weird to me to single out superusers here when it's always > true that the current session user retains the ability to SET ROLE to any > role they have the SET option on. That is already covered above in the > "Description" section, so I don't really see the need to belabor the point > by adding qualifications to the "Notes" section. ISTM the point of these > couple of paragraphs in the "Notes" section is to explain the effects on > privileges for schemas, tables, etc. > > I still think we should update the existing note about privileges for > SET/RESET ROLE to something like the following: > > diff --git a/doc/src/sgml/ref/set_role.sgml b/doc/src/sgml/ref/set_role.sgml > index 13bad1bf66..c91a95f5af 100644 > --- a/doc/src/sgml/ref/set_role.sgml > +++ b/doc/src/sgml/ref/set_role.sgml > @@ -41,8 +41,10 @@ RESET ROLE > > > > - The specified role_name > - must be a role that the current session user is a member of. > + The current session user must have the SET for the > + specified role_name, either > + directly or indirectly via a chain of memberships with the > + SET option. > (If the session user is a superuser, any role can be selected.) > > > -- > I have Reviewed the patch. Patch applies neatly without any issues. > Documentation build was successful and there was no Spell-check issue also. I > did not find any issues. The patch looks good to me. > >Thanks and Regards, >Shubham Khanna.
Re: SET ROLE documentation improvement
On Tue, Sep 26, 2023 at 08:33:25AM -0700, Yurii Rashkovskii wrote: > This is a good start, indeed. I've amended my patch to include it. Thanks for the new patch. Looking again, I'm kind of hesitant to add too much qualification to this note about losing superuser privileges. If we changed it to Note that when a superuser chooses to SET ROLE to a non-superuser role, they lose their superuser privileges, except for the privilege to change to another role again using SET ROLE or RESET ROLE. it almost seems to imply that a non-superuser role could obtain the ability to switch to any role if they first SET ROLE to a superuser. In practice, that's true because they could just give the session role SUPERUSER, but I don't think that's the intent of this section. I thought about changing it to something like Note that when a superuser chooses to SET ROLE to a non-superuser role, they lose their superuser privileges. However, if the current session user is a superuser, they retain the ability to set the current user identifier to any role via SET ROLE and RESET ROLE. but it seemed weird to me to single out superusers here when it's always true that the current session user retains the ability to SET ROLE to any role they have the SET option on. That is already covered above in the "Description" section, so I don't really see the need to belabor the point by adding qualifications to the "Notes" section. ISTM the point of these couple of paragraphs in the "Notes" section is to explain the effects on privileges for schemas, tables, etc. I still think we should update the existing note about privileges for SET/RESET ROLE to something like the following: diff --git a/doc/src/sgml/ref/set_role.sgml b/doc/src/sgml/ref/set_role.sgml index 13bad1bf66..c91a95f5af 100644 --- a/doc/src/sgml/ref/set_role.sgml +++ b/doc/src/sgml/ref/set_role.sgml @@ -41,8 +41,10 @@ RESET ROLE - The specified role_name - must be a role that the current session user is a member of. + The current session user must have the SET for the + specified role_name, either + directly or indirectly via a chain of memberships with the + SET option. (If the session user is a superuser, any role can be selected.) -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: SET ROLE documentation improvement
On Mon, Sep 25, 2023 at 3:09 PM Nathan Bossart wrote: > On Fri, Sep 15, 2023 at 02:36:16PM -0700, Yurii Rashkovskii wrote: > > On Fri, Sep 15, 2023 at 1:47 PM Nathan Bossart > > > wrote: > >> I think another issue is that the aforementioned note doesn't mention > the > >> new SET option added in 3d14e17. > > > > How do you think we should word it in that note to make it useful? > > Maybe something like this: > > The current session user must have the SET option for the specified > role_name, either directly or indirectly via a chain of memberships > with the SET option. > This is a good start, indeed. I've amended my patch to include it. -- Y. V2-0001-Minor-improvement-to-SET-ROLE-documentation.patch Description: Binary data
Re: SET ROLE documentation improvement
On Fri, Sep 15, 2023 at 02:36:16PM -0700, Yurii Rashkovskii wrote: > On Fri, Sep 15, 2023 at 1:47 PM Nathan Bossart > wrote: >> I think another issue is that the aforementioned note doesn't mention the >> new SET option added in 3d14e17. > > How do you think we should word it in that note to make it useful? Maybe something like this: The current session user must have the SET option for the specified role_name, either directly or indirectly via a chain of memberships with the SET option. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: SET ROLE documentation improvement
On Fri, Sep 15, 2023 at 1:47 PM Nathan Bossart wrote: > On Fri, Sep 15, 2023 at 11:26:16AM -0700, Yurii Rashkovskii wrote: > > I believe SET ROLE documentation makes a slightly incomplete statement > > about what happens when a superuser uses SET ROLE. > > > > The documentation reading suggests that the superuser would lose all > their > > privileges. However, they still retain the ability to use `SET ROLE` > again. > > > > The attached patch adds this bit to the documentation. > > IMO this is arguably covered by the following note: > >The specified role_name >must be a role that the current session user is a member of. >(If the session user is a superuser, any role can be selected.) > > I agree that this may be considered sufficient coverage, but I believe that giving contextual clarification goes a long way to help people understand. Documentation reading can be challenging. > But I don't see a big issue with clarifying things further as you propose. > > I think another issue is that the aforementioned note doesn't mention the > new SET option added in 3d14e17. > How do you think we should word it in that note to make it useful? -- Y.
Re: SET ROLE documentation improvement
On Fri, Sep 15, 2023 at 11:26:16AM -0700, Yurii Rashkovskii wrote: > I believe SET ROLE documentation makes a slightly incomplete statement > about what happens when a superuser uses SET ROLE. > > The documentation reading suggests that the superuser would lose all their > privileges. However, they still retain the ability to use `SET ROLE` again. > > The attached patch adds this bit to the documentation. IMO this is arguably covered by the following note: The specified role_name must be a role that the current session user is a member of. (If the session user is a superuser, any role can be selected.) But I don't see a big issue with clarifying things further as you propose. I think another issue is that the aforementioned note doesn't mention the new SET option added in 3d14e17. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
