Обсуждение: 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 <replaceable class="parameter">role_name</replaceable>
   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

On Fri, Sep 15, 2023 at 02:36:16PM -0700, Yurii Rashkovskii wrote:
> On Fri, Sep 15, 2023 at 1:47 PM Nathan Bossart <nathandbossart@gmail.com>
> 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

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
   </para>
 
   <para>
-   The specified <replaceable class="parameter">role_name</replaceable>
-   must be a role that the current session user is a member of.
+   The current session user must have the <literal>SET</option> for the
+   specified <replaceable class="parameter">role_name</replaceable>, either
+   directly or indirectly via a chain of memberships with the
+   <literal>SET</literal> option.
    (If the session user is a superuser, any role can be selected.)
   </para>

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Nov 10, 2023 at 11:11 PM Nathan Bossart
<nathandbossart@gmail.com> 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
>    </para>
>
>    <para>
> -   The specified <replaceable class="parameter">role_name</replaceable>
> -   must be a role that the current session user is a member of.
> +   The current session user must have the <literal>SET</option> for the
> +   specified <replaceable class="parameter">role_name</replaceable>, either
> +   directly or indirectly via a chain of memberships with the
> +   <literal>SET</literal> option.
>     (If the session user is a superuser, any role can be selected.)
>    </para>
>
> --
> I have Reviewed the patch. Patch applies neatly without any issues. Documentation build was successful and there was
noSpell-check issue also. I did not find any issues. The patch looks good to me. 
>
>Thanks and Regards,
>Shubham Khanna.

On Fri, Nov 10, 2023 at 12:41 PM Nathan Bossart
<nathandbossart@gmail.com> 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:

  <para>
   Note that when a superuser chooses to <command>SET ROLE</command> to a
   non-superuser role, they lose their superuser privileges.
  </para>

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 <replaceable
class="parameter">role_name</replaceable> 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
>    </para>
>
>    <para>
> -   The specified <replaceable class="parameter">role_name</replaceable>
> -   must be a role that the current session user is a member of.
> +   The current session user must have the <literal>SET</option> for the
> +   specified <replaceable class="parameter">role_name</replaceable>, either
> +   directly or indirectly via a chain of memberships with the
> +   <literal>SET</literal> option.
>     (If the session user is a superuser, any role can be selected.)
>    </para>

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

On Fri, Mar 22, 2024 at 03:58:59PM -0400, Robert Haas wrote:
> On Fri, Nov 10, 2023 at 12:41 PM Nathan Bossart
> <nathandbossart@gmail.com> 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:
> 
>   <para>
>    Note that when a superuser chooses to <command>SET ROLE</command> to a
>    non-superuser role, they lose their superuser privileges.
>   </para>
> 
> 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 <replaceable
> class="parameter">role_name</replaceable> 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
>>    </para>
>>
>>    <para>
>> -   The specified <replaceable class="parameter">role_name</replaceable>
>> -   must be a role that the current session user is a member of.
>> +   The current session user must have the <literal>SET</option> for the
>> +   specified <replaceable class="parameter">role_name</replaceable>, either
>> +   directly or indirectly via a chain of memberships with the
>> +   <literal>SET</literal> option.
>>     (If the session user is a superuser, any role can be selected.)
>>    </para>
> 
> 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

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
>> <nathandbossart@gmail.com> 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
>>>    </para>
>>>
>>>    <para>
>>> -   The specified <replaceable class="parameter">role_name</replaceable>
>>> -   must be a role that the current session user is a member of.
>>> +   The current session user must have the <literal>SET</option> for the
>>> +   specified <replaceable class="parameter">role_name</replaceable>, either
>>> +   directly or indirectly via a chain of memberships with the
>>> +   <literal>SET</literal> option.
>>>     (If the session user is a superuser, any role can be selected.)
>>>    </para>
>> 
>> 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

On Fri, Mar 22, 2024 at 4:51 PM Nathan Bossart <nathandbossart@gmail.com> 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

On Sat, Mar 23, 2024 at 08:37:20AM -0400, Robert Haas wrote:
> On Fri, Mar 22, 2024 at 4:51 PM Nathan Bossart <nathandbossart@gmail.com> 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

> On 24 Mar 2024, at 23:34, Nathan Bossart <nathandbossart@gmail.com> 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
nextCF? 

Thanks!


Best regards, Andrey Borodin.

[0] https://commitfest.postgresql.org/47/4572/

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

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

On Thu, Apr 11, 2024 at 10:03 AM Nathan Bossart
<nathandbossart@gmail.com> 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

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

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

On Thu, Apr 11, 2024 at 1:03 PM Nathan Bossart <nathandbossart@gmail.com> 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

On Thu, Apr 11, 2024 at 01:38:37PM -0400, Robert Haas wrote:
> On Thu, Apr 11, 2024 at 1:03 PM Nathan Bossart <nathandbossart@gmail.com> 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

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

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