Обсуждение: CREATE ROLE inheritance details

I am unhappy with the documentation adjustments made to CREATE ROLE in
Postgres 16 by this commit:

    commit e3ce2de09d
    Author: Robert Haas <rhaas@postgresql.org>
    Date:   Thu Aug 25 10:06:02 2022 -0400
    
        Allow grant-level control of role inheritance behavior.
    
        The GRANT statement can now specify WITH INHERIT TRUE or WITH
        INHERIT FALSE to control whether the member inherits the granted
        role's permissions. For symmetry, you can now likewise write
        WITH ADMIN TRUE or WITH ADMIN FALSE to turn ADMIN OPTION on or off.
    
        If a GRANT does not specify WITH INHERIT, the behavior based on
        whether the member role is marked INHERIT or NOINHERIT. This means
        that if all roles are marked INHERIT or NOINHERIT before any role
        grants are performed, the behavior is identical to what we had before;
        otherwise, it's different, because ALTER ROLE [NO]INHERIT now only
        changes the default behavior of future grants, and has no effect on
        existing ones.
    
        Patch by me. Reviewed and testing by Nathan Bossart and Tushar Ahuja,
        with design-level comments from various others.
    
        Discussion: http://postgr.es/m/CA+Tgmoa5Sf4PiWrfxA=sGzDKg0Ojo3dADw=wAHOhR9dggV=RmQ@mail.gmail.com

It seems to have removed important details about how inherit works
beyond GRANT.

Using the attached script, test_inh.sh, I generated SQL queries
contained in attached file test_inh.sql, and this generated this output
showing that the inheritance setting of the role being added as a member
controls the inheritance status of the membership:

         rolname      | Is member of role | inherit_option
    ------------------+-------------------+----------------
     init_1_noinherit | user_1_inherit    | f
     init_1_noinherit | user_2_noinherit  | f
     init_1_noinherit | user_3_inherit    | f
     init_1_noinherit | user_4_noinherit  | f
     init_2_inherit   | user_5_inherit    | t
     init_2_inherit   | user_6_noinherit  | t
     init_2_inherit   | user_7_inherit    | t
     init_2_inherit   | user_8_noinherit  | t
     user_1_inherit   | init_3_noinherit  | t
     user_2_noinherit | init_3_noinherit  | f
     user_3_inherit   | init_4_inherit    | t
     user_4_noinherit | init_4_inherit    | f
     user_5_inherit   | init_3_noinherit  | t
     user_6_noinherit | init_3_noinherit  | f
     user_7_inherit   | init_4_inherit    | t
     user_8_noinherit | init_4_inherit    | f

I have attached a patch to re-add this information, and clarify it.  I
would like to apply this to PG 16 and master.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

On Tue, 2024-01-16 at 17:03 -0500, Bruce Momjian wrote:
> I am unhappy with the documentation adjustments made to CREATE ROLE in
> Postgres 16 by this commit:
>
>     commit e3ce2de09d
>
> I have attached a patch to re-add this information, and clarify it.  I
> would like to apply this to PG 16 and master.

I had to read the text twice before I understood it, but I cannot think
of a simpler way to write it.

Yours,
Laurenz Albe

On Wed, Jan 17, 2024 at 09:28:38AM +0100, Laurenz Albe wrote:
> On Tue, 2024-01-16 at 17:03 -0500, Bruce Momjian wrote:
> > I am unhappy with the documentation adjustments made to CREATE ROLE in
> > Postgres 16 by this commit:
> > 
> >     commit e3ce2de09d
> > 
> > I have attached a patch to re-add this information, and clarify it.  I
> > would like to apply this to PG 16 and master.
> 
> I had to read the text twice before I understood it, but I cannot think
> of a simpler way to write it.

Yeah, I had the same feeling.  A bullet list would be nice but overkill
for a manual page.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

On Wed, Jan 17, 2024 at 02:32:47PM -0700, David G. Johnston wrote:
> I had a go at this.
> 
> I went with a more "bullet item" approach with my wording for INHERIT/
> NOINHERIT.

I tried to address that.

> The entire paragraph regarding how the INHERIT "option" works, as opposed to
> the attribute, seems out of place where it was and the material is already
> covered in the GRANT page.  We should either improve that page or extract this
> level of detail somewhere else, not try to clutter up the CREATE ROLE page with
> it.

Because the CREATE ROLE is the way to create roles, and GRANT does much
more than just grant membership, I ended up moving the details from the
GRANT page to the CREATE ROLE page.

> We consistently say what the default is for these attribute pairs, do so here
> as well.

I added that, I hope in all the right places.

> Turn the parenthetical in the IN ROLE section into actual assertive
> documentation of what the clause does.  Tweak ROLE and ADMIN as well to fit in
> better.

Yes, that needed help.

> Reword the discussion regarding non-inheritance to be more direct.
> 
> I added mention of the grantee aspect of privileges as a soft way of further
> pointing out that the IN ROLE, ROLE, ADMIN clauses are limited in what they can
> control in the resulting membership grants.
> 
> I choose to use the phrasing "giving ... roles" for both parts of the sentence
> instead of switching to "... roles are given" for the second half.  More of a
> style choice but I didn't think switching really added much and just makes it a
> bit wordier and possibly a bit more effort to mentally parse.

I adjusted that working, but in a different way.  Patch attached.

I also found we didn't document that GRANT can be used to modify a
membership's attributes even after it is created;  I added that.

Just a reminder, this is for PG 16 and master.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

On Sun, Jan 28, 2024 at 10:11:18AM -0700, David G. Johnston wrote:
> On Fri, Jan 26, 2024 at 5:18 PM Bruce Momjian <bruce@momjian.us> wrote:
>  <para>
> +        Role membership with the inherit attribute can automatically use
> +        whatever database privileges have been granted to all roles it
> +        is directly or indirectly a member of, though the chain stops
> +        at memberships lacking the inherit attribute.  Without role
> +        inheritance, the only other value of membership is the use of
> +        <command>SET ROLE</command>, assuming the membership chain has
> +        <literal>SET</literal> attributes.
>         </para>
> 
> 
> I really think it is clearer if we consistently call the property attached to
> the membership an "option" (the grant command calls them options) and restrict
> the word attribute to only when talking about the role property.  The following
> refers to the option.  I dislike burying this description about how the option
> works within create role like this.  It is already in chapter 22 and this
> attribute description should point the reader there, not repeat the
> information.

Okay, I made these changes in the attached version of the patch.

>         <para>
>          In <productname>PostgreSQL</productname> versions before 16,
> -        the <literal>GRANT</literal> statement did not support
> -        <literal>WITH INHERIT</literal>. Therefore, changing this role-level
> -        property would also change the behavior of already-existing grants.
> -        This is no longer the case.
> +        inheritance was a role-level attribute.  It could not be specified
> +        during role addition with <literal>GRANT</literal>, and changing
> +        this role-level property would also change the inheritance behavior
> +        of all existing memberships.  This is no longer the case.
>         </para>
> 
> My first reaction to the wording here is negative.
> 
> I agree that the pre-v16 behavior dynamic should be documented but maybe l
> eave a note with a bit more detail in chapter 22 and leave the following in
> place here:
> 
> Prior to version 16 this attribute directly controlled runtime privilege
> inheritance instead of now only providing a default for when role membership is
> established.

Okay, I simplified the explaination.

> @@ -285,9 +294,13 @@ in sync when changing the above synopsis!
>         <para>
>          The <literal>IN ROLE</literal> clause causes the new role to
>          be automatically added as a member of the specified existing
> -        roles. (Note that there is no option to add the new role as an
> -        administrator; use a separate <command>GRANT</command> command
> -        to do that.)
> +        roles. The new membership will have the <literal>SET</literal>
> +        option enabled and the <literal>ADMIN</literal> option disabled.
> +        The <literal>INHERIT</literal> option will be enabled unless the
> +        <literal>NOINHERIT</literal> attribute is specified.  See the <xref
> +        linkend="sql-grant"/> command, which has additional attribute
> +        control during membership creation and to modify these options
> +        after the new role is created.
>         </para>
> 
> additional attribute control s/b additional option control

Done

> @@ -307,10 +324,10 @@ in sync when changing the above synopsis!
>        <term><literal>ADMIN</literal> <replaceable class="parameter">role_name
> </replaceable></term>
>        <listitem>
>         <para>
> -        The <literal>ADMIN</literal> clause is like <literal>ROLE</literal>,
> -        but the named roles are added to the new role <literal>WITH ADMIN
> -        OPTION</literal>, giving them the right to grant membership in this
> role
> -        to others.
> +        The <literal>ADMIN</literal> clause is similar to
> +        <literal>ROLE</literal>, but the named roles are added as members
> +        of the new role with <literal>ADMIN</literal> enabled, giving
> +        them the right to grant membership in this role to others.
>         </para>
>        </listitem>
>       </varlistentry>
> 
> I was trying to be explicitly clear that the ADMIN clause is effectively
> additive to what ROLE does.  "similar to + but" makes it easier to interpret as
> something that only controls ADMIN, not SET or INHERIT.

Okay, I used new wording.

> The ADMIN clause behaves like ROLE but the ADMIN option is enabled.
> 
> And modify ROLE as such:
> 
> +        <literal>INHERIT</literal> enabled in the new membership.  New
> +        memberships will have the <literal>ADMIN</literal> option disabled.
> +  Use the ADMIN clause instead if you want the admin option enabled.
>  
>     This variant of the <command>GRANT</command> command grants membership
> -   in a role to one or more other roles.  Membership in a role is significant
> +   in a role to one or more other roles, and the modification of
> +   membership attributes.  Membership in a role is significant
> 
> and (allows) the modification of three membership options, set, inherit, and
> admin, described in chapter 22.  Membership in a role is significant...

Done.

> To modify that attributes of
> +   an existing membership, simply specify the membership with updated
> +   attribute values.
> 
> attribute s/b option

Done.

>    <para>
> @@ -275,15 +278,13 @@ GRANT <replaceable class="parameter">role_name</
> replaceable> [, ...] TO <replace
>    </para>
>  
>    <para>
> +   The <literal>INHERIT</literal> option controls the inheritance status
> +   of the new membership;  see <xref linkend="sql-createrole"/> for
> +   details on inheritance.  If it is set to <literal>TRUE</literal>,
> +   it causes the new member to inherit from the granted role. If
> +   set to <literal>FALSE</literal>, the new member does not inherit.
> +   If unspecified, it defaults to the inheritance status of the role
> +   being added.
>    </para>
>  
> Suggest linking to chapter 22, not create role.
K
Done.

> Unspecified has two outcomes: if the grant is establishing a new membership the
> inheritance "attribute" value of the role being added is used, if the grant is
> altering an existing membership the current value of the option is retained.

Done.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

On Tue, Jan 30, 2024 at 01:49:42PM -0700, David G. Johnston wrote:
> Almost there I think.  The comment regarding seeing GRANT really applies to IN
> ROLE, ROLE, and ADMIN.  Repeating it seemed a poor choice and upon thinking
> further it really makes sense to consider how this command and GRANT work
> together part of the description of create role.  So I moved that commentary to
> a new description paragraph.

I liked all your changes so applied your composite patch.  You can see
the results here:

    https://momjian.us/pgsql_docs/

These improvements will appear in the PG 16 minor release which will be
packaged on Monday.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

On Tue, Jan 30, 2024 at 01:49:42PM -0700, David G. Johnston wrote:
> +   The membership grants created by the
> +   <literal>IN ROLE</literal>, <literal>ROLE</literal>, and <literal>ADMIN</literal>
> +   clauses have the role executing this command as the grantee.

This should be s/grantee/grantor/, right?

> +   If unspecified when create a new role membership this defaults to
> +   the inheritance attribute of the role being added.

Need s/create/creating/.  Missing comma.  While phrase "role being added"
isn't wrong, the rest of the paragraph uses "member" and uses "granted role"
to refer to the member-of role.  Putting those together:

  If unspecified when creating a new role membership, this defaults to the
  inheritance attribute of the new member.

On Wed, Apr 03, 2024 at 06:11:47PM -0700, David G. Johnston wrote:
> On Sat, Mar 30, 2024 at 11:16 PM Noah Misch <noah@leadboat.com> wrote:
> > On Tue, Jan 30, 2024 at 01:49:42PM -0700, David G. Johnston wrote:
> > > +   The membership grants created by the
> > > +   <literal>IN ROLE</literal>, <literal>ROLE</literal>, and
> > <literal>ADMIN</literal>
> > > +   clauses have the role executing this command as the grantee.
> >
> > This should be s/grantee/grantor/, right?
> 
> Correct.
> 
> > > +   If unspecified when create a new role membership this defaults to
> > > +   the inheritance attribute of the role being added.
> >
> > Need s/create/creating/.  Missing comma.  While phrase "role being added"
> > isn't wrong, the rest of the paragraph uses "member" and uses "granted
> > role"
> > to refer to the member-of role.  Putting those together:
> >
> >   If unspecified when creating a new role membership, this defaults to the
> >   inheritance attribute of the new member.
> 
> I like that better.

Pushed as 68ba46d.  Thanks for reviewing.