Thread: Add --{no-,}bypassrls flags to createuser

Hi,

Add --{no-,}bypassrls flags to createuser.
The following is an example of execution.
--
$ createuser a --bypassrls
$ psql -c "\du a"
            List of roles
  Role name | Attributes | Member of
-----------+------------+-----------
  a         | Bypass RLS | {}

-- 

Do you think?

Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

At Wed, 13 Apr 2022 14:51:35 +0900, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote in 
> Hi,
> 
> Add --{no-,}bypassrls flags to createuser.
> The following is an example of execution.
> --
> $ createuser a --bypassrls
> $ psql -c "\du a"
>            List of roles
>  Role name | Attributes | Member of
> -----------+------------+-----------
>  a         | Bypass RLS | {}
> 
> -- 
> 
> Do you think?

It is sensible to rig createuser command with full capability of
CREATE ROLE is reasonable.

Only --replication is added by commit 9b8aff8c19 (2010) since
8ae0d476a9 (2005). BYPASSRLS and NOBYPASSRLS were introduced by
491c029dbc (2014) but it seems to have forgotten to add the
corresponding createuser options.

By a quick search, found a few other CREATE ROLE optinos that are not
supported by createuser.

VALID UNTIL
ROLE  (IN ROLE is -g/--role)
ADMIN

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Apr 13, 2022 at 03:46:25PM +0900, Kyotaro Horiguchi wrote:
> It is sensible to rig createuser command with full capability of
> CREATE ROLE is reasonable.
>
> Only --replication is added by commit 9b8aff8c19 (2010) since
> 8ae0d476a9 (2005). BYPASSRLS and NOBYPASSRLS were introduced by
> 491c029dbc (2014) but it seems to have forgotten to add the
> corresponding createuser options.
>
> By a quick search, found a few other CREATE ROLE optinos that are not
> supported by createuser.

My question is: is BYPASSRLS common enough to justify having a switch
to createuser?  As the development cycle of 15 has just finished and
that we are in feature freeze, you may want to hold on new patches for
a bit.  The next commit fest is planned for July.
--
Michael

At Wed, 13 Apr 2022 16:10:01 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Wed, Apr 13, 2022 at 03:46:25PM +0900, Kyotaro Horiguchi wrote:
> > It is sensible to rig createuser command with full capability of
> > CREATE ROLE is reasonable.
> > 
> > Only --replication is added by commit 9b8aff8c19 (2010) since
> > 8ae0d476a9 (2005). BYPASSRLS and NOBYPASSRLS were introduced by
> > 491c029dbc (2014) but it seems to have forgotten to add the
> > corresponding createuser options.
> > 
> > By a quick search, found a few other CREATE ROLE optinos that are not
> > supported by createuser.
> 
> My question is: is BYPASSRLS common enough to justify having a switch
> to createuser?  As the development cycle of 15 has just finished and
> that we are in feature freeze, you may want to hold on new patches for
> a bit.  The next commit fest is planned for July.

I don't think there's a definitive criteria (other than feasibility)
for whether each CREATE ROLE option should have the correspondent
option in the createuser command.  I don't see a clear reason why
createuser command should not have the option.

As far as schedules are concerned, I don't think this has anything to
do with 15.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Apr 13, 2022 at 4:35 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> I don't think there's a definitive criteria (other than feasibility)
> for whether each CREATE ROLE option should have the correspondent
> option in the createuser command.  I don't see a clear reason why
> createuser command should not have the option.

+1.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On 2022-04-13 17:35, Kyotaro Horiguchi wrote:
> At Wed, 13 Apr 2022 16:10:01 +0900, Michael Paquier
> <michael@paquier.xyz> wrote in
>> On Wed, Apr 13, 2022 at 03:46:25PM +0900, Kyotaro Horiguchi wrote:
>> > It is sensible to rig createuser command with full capability of
>> > CREATE ROLE is reasonable.
>> >
>> > Only --replication is added by commit 9b8aff8c19 (2010) since
>> > 8ae0d476a9 (2005). BYPASSRLS and NOBYPASSRLS were introduced by
>> > 491c029dbc (2014) but it seems to have forgotten to add the
>> > corresponding createuser options.
>> >
>> > By a quick search, found a few other CREATE ROLE optinos that are not
>> > supported by createuser.
>> 
>> My question is: is BYPASSRLS common enough to justify having a switch
>> to createuser?  As the development cycle of 15 has just finished and
>> that we are in feature freeze, you may want to hold on new patches for
>> a bit.  The next commit fest is planned for July.
> 
> I don't think there's a definitive criteria (other than feasibility)
> for whether each CREATE ROLE option should have the correspondent
> option in the createuser command.  I don't see a clear reason why
> createuser command should not have the option.

Thank you for the review!
I created a new patch containing 'VALID UNTIL', 'ADMIN', and 'ROLE'.

To add the ROLE clause, the originally existing --role option 
(corresponding to the IN ROLE clause) is changed to the --in-role 
option. Would this not be good from a backward compatibility standpoint?


> As far as schedules are concerned, I don't think this has anything to
> do with 15.

I have registered this patch for the July commit fest.


-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

> On 14 Apr 2022, at 09:42, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote:

> To add the ROLE clause, the originally existing --role option (corresponding to the IN ROLE clause) is changed to the
--in-roleoption. Would this not be good from a backward compatibility standpoint? 

-    printf(_("  -g, --role=ROLE           new role will be a member of this role\n"));
+    printf(_("  -g, --in-role=ROLE        new role will be a member of this role\n"));
+    printf(_("  -G, --role=ROLE           this role will be a member of new role\n"));

Won't this make existing scripts to behave differently after an upgrade?  That
seems like something we should avoid at all costs.

--
Daniel Gustafsson        https://vmware.com/

On 2022-04-14 18:57, Daniel Gustafsson wrote:
>> On 14 Apr 2022, at 09:42, Shinya Kato <Shinya11.Kato@oss.nttdata.com> 
>> wrote:
> 
>> To add the ROLE clause, the originally existing --role option 
>> (corresponding to the IN ROLE clause) is changed to the --in-role 
>> option. Would this not be good from a backward compatibility 
>> standpoint?
> 
> -    printf(_("  -g, --role=ROLE           new role will be a member of
> this role\n"));
> +    printf(_("  -g, --in-role=ROLE        new role will be a member of
> this role\n"));
> +    printf(_("  -G, --role=ROLE           this role will be a member of
> new role\n"));
> 
> Won't this make existing scripts to behave differently after an 
> upgrade?  That
> seems like something we should avoid at all costs.

I understand. For backward compatibility, I left the ROLE clause option 
as it is and changed the IN ROLE clause option to --membership option.


-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

At Fri, 15 Apr 2022 14:55:48 +0900, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote in 
> I understand. For backward compatibility, I left the ROLE clause
> option as it is and changed the IN ROLE clause option to --membership
> option.

Thanks!

-    printf(_("  -g, --role=ROLE           new role will be a member of this role\n"));
+    printf(_("  -g, --role=ROLE        new role will be a member of this role\n"));

This looks lik an unexpected change.  We shoudl preserve it, but *I*
think that we can add a synonym of the old --role for
understandability/memorability.  (By the way "-g" looks like coming
from "group", which looks somewhat strange..)

> printf(_("  -b, --belongs-to=ROLE     new role will be a member of this role\n"));

+    printf(_("  -m, --membership=ROLE     this role will be a member of new role\n"));

membership sounds somewhat obscure, it seems *to me* members is clearer

> printf(_("  -m, --member=ROLE        new role will be a member of this role\n"));

I'd like to hear others' opinions.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Apr 15, 2022 at 2:33 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> > printf(_("  -b, --belongs-to=ROLE     new role will be a member of this role\n"));
>
> +       printf(_("  -m, --membership=ROLE     this role will be a member of new role\n"));
>
> membership sounds somewhat obscure, it seems *to me* members is clearer
>
> > printf(_("  -m, --member=ROLE        new role will be a member of this role\n"));
>
> I'd like to hear others' opinions.

I think that we need to preserve consistency with the SQL syntax as
much as possible -- and neither MEMBER nor MEMBERSHIP nor BELONGS_TO
appear in that syntax. A lot of the terminology in this area seems
poorly chosen and confusing to me, but having two ways to refer to
something probably won't be an improvement even if the second name is
better-chosen than the first one.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Thanks!

At Mon, 18 Apr 2022 09:59:48 -0400, Robert Haas <robertmhaas@gmail.com> wrote in 
> On Fri, Apr 15, 2022 at 2:33 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> > > printf(_("  -b, --belongs-to=ROLE     new role will be a member of this role\n"));
> >
> > +       printf(_("  -m, --membership=ROLE     this role will be a member of new role\n"));
> >
> > membership sounds somewhat obscure, it seems *to me* members is clearer
> >
> > > printf(_("  -m, --member=ROLE        new role will be a member of this role\n"));
> >
> > I'd like to hear others' opinions.
> 
> I think that we need to preserve consistency with the SQL syntax as
> much as possible -- and neither MEMBER nor MEMBERSHIP nor BELONGS_TO
> appear in that syntax. A lot of the terminology in this area seems
> poorly chosen and confusing to me, but having two ways to refer to
> something probably won't be an improvement even if the second name is
> better-chosen than the first one.

Hmm.. So, "-r/--role" and "-m/--member(ship)" is the (least worse) way
to go?  Or we can give up adding -m for the reason of being hard to
name it..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Apr 18, 2022 at 9:50 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> Hmm.. So, "-r/--role" and "-m/--member(ship)" is the (least worse) way
> to go?  Or we can give up adding -m for the reason of being hard to
> name it..

Hmm, yeah, I hadn't quite realized what the problem was when I wrote
that. I honestly don't know what to do about that. Renaming the
existing option is not great, but having the syntax diverge between
SQL and CLI is not great either. Giving up is also not great. Not sure
what is best.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

At Tue, 19 Apr 2022 12:13:51 -0400, Robert Haas <robertmhaas@gmail.com> wrote in 
> On Mon, Apr 18, 2022 at 9:50 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> > Hmm.. So, "-r/--role" and "-m/--member(ship)" is the (least worse) way
> > to go?  Or we can give up adding -m for the reason of being hard to
> > name it..
> 
> Hmm, yeah, I hadn't quite realized what the problem was when I wrote
> that. I honestly don't know what to do about that. Renaming the
> existing option is not great, but having the syntax diverge between
> SQL and CLI is not great either. Giving up is also not great. Not sure
> what is best.

Exactly..  So I'm stuckX(

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Apr 19, 2022 at 12:13:51PM -0400, Robert Haas wrote:
> On Mon, Apr 18, 2022 at 9:50 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
>> Hmm.. So, "-r/--role" and "-m/--member(ship)" is the (least worse) way
>> to go?  Or we can give up adding -m for the reason of being hard to
>> name it..
>
> Hmm, yeah, I hadn't quite realized what the problem was when I wrote
> that. I honestly don't know what to do about that. Renaming the
> existing option is not great, but having the syntax diverge between
> SQL and CLI is not great either. Giving up is also not great. Not sure
> what is best.

Changing one existing option to mean something entirely different
should be avoided, as this could lead to silent breakages.  As the
origin of the problem is that the option --role means "IN ROLE" in the
SQL grammar, we could keep around --role for compatibility while
marking it deprecated, and add two new options whose names would be
more consistent with each other.  One choice could be --role-name and
--in-role-name, where --in-role-name maps to the older --role, just to
give an idea.
--
Michael

On Thu, Apr 21, 2022 at 12:30 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Tue, Apr 19, 2022 at 12:13:51PM -0400, Robert Haas wrote:
> > On Mon, Apr 18, 2022 at 9:50 PM Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote:
> >> Hmm.. So, "-r/--role" and "-m/--member(ship)" is the (least worse) way
> >> to go?  Or we can give up adding -m for the reason of being hard to
> >> name it..
> >
> > Hmm, yeah, I hadn't quite realized what the problem was when I wrote
> > that. I honestly don't know what to do about that. Renaming the
> > existing option is not great, but having the syntax diverge between
> > SQL and CLI is not great either. Giving up is also not great. Not sure
> > what is best.
>
> Changing one existing option to mean something entirely different
> should be avoided, as this could lead to silent breakages.  As the
> origin of the problem is that the option --role means "IN ROLE" in the
> SQL grammar, we could keep around --role for compatibility while
> marking it deprecated, and add two new options whose names would be
> more consistent with each other.  One choice could be --role-name and
> --in-role-name, where --in-role-name maps to the older --role, just to
> give an idea.

I don't think that having both --role and --role-name, doing different
things, is going to be clear at all.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Apr 21, 2022 at 01:21:57PM -0700, David G. Johnston wrote:
> I'm ok with -m/--member as well (like with --role only one role can be
> specified per switch instance so member, not membership, the later meaning,
> at least for me, the collective).
> 
> That -m doesn't match --role-to is no worse than -g not matching --role, a
> short option seems worthwhile, and the -m (membership) mnemonic should be
> simple to pick-up.
> 
> I don't see the addition of "-name" to the option name being beneficial.
> 
> Yes, the standard doesn't use the "TO" prefix for "ROLE" - but taking that
> liberty for consistency here is very appealing and there isn't another SQL
> clause that it would be confused with.

+1 for "member".  It might not be perfect, but IMO it's the clearest
option.

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

Thank you for the reviews!

On 2022-04-26 05:19, Nathan Bossart wrote:

> -    printf(_("  -g, --role=ROLE           new role will be a member of 
> this role\n"));
> +    printf(_("  -g, --role=ROLE        new role will be a member of this 
> role\n"));
> This looks lik an unexpected change.

I fixed it.


>> I'm ok with -m/--member as well (like with --role only one role can be
>> specified per switch instance so member, not membership, the later 
>> meaning,
>> at least for me, the collective).
>> 
>> That -m doesn't match --role-to is no worse than -g not matching 
>> --role, a
>> short option seems worthwhile, and the -m (membership) mnemonic should 
>> be
>> simple to pick-up.
>> 
>> I don't see the addition of "-name" to the option name being 
>> beneficial.
>> 
>> Yes, the standard doesn't use the "TO" prefix for "ROLE" - but taking 
>> that
>> liberty for consistency here is very appealing and there isn't another 
>> SQL
>> clause that it would be confused with.
> 
> +1 for "member".  It might not be perfect, but IMO it's the clearest
> option.

Thanks! I changed the option "--membership" to "--member".


For now, I also think "-m / --member" is the best choice, although it is 
ambiguous:(
I'd like to hear others' opinions.

regards


--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Thu, Apr 28, 2022 at 03:06:30PM +0900, Shinya Kato wrote:
> On 2022-04-26 05:19, Nathan Bossart wrote:
>> +1 for "member".  It might not be perfect, but IMO it's the clearest
>> option.
> 
> Thanks! I changed the option "--membership" to "--member".

Thanks for the new patch!  Would you mind adding some tests for the new
options?

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

Dear Shinya,

Too bad there's no --comment parameter to do COMMENT ON ROLE name IS 'Comment';

As you already make such changes in createuser, I would like to ask for an additional --comment parameter
that will allow sysadmins to set a comment with additional information about the new DB user.
psql is scary for some. :-)

Overall a very useful patch. I needed bypassrls several times recently.

Shinya Kato wrote on 4/28/2022 8:06 AM:

Thank you for the reviews!

On 2022-04-26 05:19, Nathan Bossart wrote:

-    printf(_("  -g, --role=ROLE           new role will be a member of this role\n"));
+    printf(_("  -g, --role=ROLE        new role will be a member of this role\n"));
This looks lik an unexpected change.

I fixed it.

I'm ok with -m/--member as well (like with --role only one role can be
specified per switch instance so member, not membership, the later meaning,
at least for me, the collective).

That -m doesn't match --role-to is no worse than -g not matching --role, a
short option seems worthwhile, and the -m (membership) mnemonic should be
simple to pick-up.

I don't see the addition of "-name" to the option name being beneficial.

Yes, the standard doesn't use the "TO" prefix for "ROLE" - but taking that
liberty for consistency here is very appealing and there isn't another SQL
clause that it would be confused with.

+1 for "member".  It might not be perfect, but IMO it's the clearest
option.

Thanks! I changed the option "--membership" to "--member".


For now, I also think "-m / --member" is the best choice, although it is ambiguous:(
I'd like to hear others' opinions.

regards


--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Thanks for reviews and comments!

On 2022-05-06 07:08, Przemysław Sztoch wrote:

> Thanks for the new patch!  Would you mind adding some tests for the new
> options?

I created a new patch to test the new options!
However, not all option tests exist, so it may be necessary to consider 
whether to actually add this test.


> Too bad there's no --comment parameter to do COMMENT ON ROLE name IS
> 'Comment';
> 
> As you already make such changes in createuser, I would like to ask
> for an additional --comment parameter
> that will allow sysadmins to set a comment with additional information
> about the new DB user.
> psql is scary for some. :-)

Since the createuser command is a wrapper for the CREATE ROLE command, I 
do not think it is appropriate to add options that the CREATE ROLE 
command does not have.


-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Thu, May 19, 2022 at 10:35:23AM +0900, Shinya Kato wrote:
> I created a new patch to test the new options!

Thanks for the new patch!  I attached a new version with a few small
changes.  What do you think?

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

David G. Johnston wrote on 5/19/2022 3:46 AM:

I think that this feature is at least worth considering - but absent an existing command that does this I would agree that doing so constitutes a separate feature.

As an aside, I'd rather overcome this particular objection by having the CREATE object command all accept an optional "COMMENT IS" clause.

David J.

The createuser command is typically used by IT personnel unfamiliar with SQL and unfamiliar with psql.
They often use this command because software installation procedures require it.
Lack of comment then causes more mess in the configuration of larger servers.
I still think it's worth adding the --comment argument to execute the next SQL statement by createuser.
This will simplify the setup scripts and installation instructions for the final software.

I believe that it is not worth dividing it into a separate program.

The same --comment argument is needed for the createdb command.

At Sun, 22 May 2022 09:55:37 +0200, Przemysław Sztoch <przemyslaw@sztoch.pl> wrote in 
> David G. Johnston wrote on 5/19/2022 3:46 AM:
> > As an aside, I'd rather overcome this particular objection by having
> > the CREATE object command all accept an optional "COMMENT IS" clause.
> >
> I believe that it is not worth dividing it into a separate program.
> 
> The same --comment argument is needed for the createdb command.

David didn't say that it should be another "program", but said it
should be another "patch/development", because how we implement the
--comment feature is apparently controversial.

It doesn't seem to be explicity mentioned that "createuser is mere a
shell-substitute for the SQL CREATE ROLE", but I feel the same with
Shinya that it is. We could directly invoke "COMMENT ON" from
createuser command, but I think it is not the way to go in that light.

We can either add COMMENT clause only to "CREATE ROLE" , or "COMMENT
IS" clause to all (or most of) "CREATE object" commands, or something
others. (Perhaps "COMMETN IS" requires "ALTER object" handle comments,
and I'm not sure how we think about the difference of it from "comment
on" command.)  We might return to "comment on" in the end..

Anyway, after fixing that issue we will modify the createrole command
so that it uses the new SQL feature. I find no hard obstacles in
reaching there in the 16 cycle.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2022-05-21 06:45, Nathan Bossart wrote:
> On Thu, May 19, 2022 at 10:35:23AM +0900, Shinya Kato wrote:
>> I created a new patch to test the new options!
> 
> Thanks for the new patch!  I attached a new version with a few small
> changes.  What do you think?

Thanks for updating the patch!
It looks good to me.


On 2022-05-23 10:32, Kyotaro Horiguchi wrote:
> Anyway, after fixing that issue we will modify the createrole command
> so that it uses the new SQL feature. I find no hard obstacles in
> reaching there in the 16 cycle.

+1.


-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Fri, May 20, 2022 at 02:45:19PM -0700, Nathan Bossart wrote:
> Thanks for the new patch!  I attached a new version with a few small
> changes.  What do you think?

So you have settled down to --member to emulate the clause ROLE.
Well, this choice is fine by me at the end.

> +$node->issues_sql_like(
> +    [ 'createuser', 'regress_role2', '-a', 'regress_user1' ],
> +    qr/statement: CREATE ROLE regress_role2 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN ADMIN
regress_user1;/,
> +    'add a role as a member with admin option of the newly created role');
> +$node->issues_sql_like(
> +    [ 'createuser', 'regress_role3', '-m', 'regress_user1' ],
> +    qr/statement: CREATE ROLE regress_role3 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN ROLE regress_user1;/,
> +    'add a role as a member of the newly created role');

May I ask for the addition of tests when one specifies multiple
switches for --admin and --member?  This would check the code path
where you build a list of role names.  You could check fancier string
patterns, while on it, to look after the use of fmtId(), say with
role names that include whitespaces or such.
--
Michael

On 2022-05-23 16:29, Michael Paquier wrote:
>> +$node->issues_sql_like(
>> +    [ 'createuser', 'regress_role2', '-a', 'regress_user1' ],
>> +    qr/statement: CREATE ROLE regress_role2 NOSUPERUSER NOCREATEDB 
>> NOCREATEROLE INHERIT LOGIN ADMIN regress_user1;/,
>> +    'add a role as a member with admin option of the newly created 
>> role');
>> +$node->issues_sql_like(
>> +    [ 'createuser', 'regress_role3', '-m', 'regress_user1' ],
>> +    qr/statement: CREATE ROLE regress_role3 NOSUPERUSER NOCREATEDB 
>> NOCREATEROLE INHERIT LOGIN ROLE regress_user1;/,
>> +    'add a role as a member of the newly created role');
> 
> May I ask for the addition of tests when one specifies multiple
> switches for --admin and --member?  This would check the code path
> where you build a list of role names.  You could check fancier string
> patterns, while on it, to look after the use of fmtId(), say with
> role names that include whitespaces or such.

Thanks!
I changed to the test that describes multiple "-m".
It seems to be working without any problems, how about it?


-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Mon, May 23, 2022 at 11:55:43PM +0900, Shinya Kato wrote:
> On 2022-05-23 16:29, Michael Paquier wrote:
>> May I ask for the addition of tests when one specifies multiple
>> switches for --admin and --member?  This would check the code path
>> where you build a list of role names.  You could check fancier string
>> patterns, while on it, to look after the use of fmtId(), say with
>> role names that include whitespaces or such.
> 
> Thanks!
> I changed to the test that describes multiple "-m".
> It seems to be working without any problems, how about it?

Michael also requested a test for multiple -a switches and for fancier
string patterns.  Once that is taken care of, I think this can be marked as
ready-for-committer.

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

On Mon, May 23, 2022 at 09:37:35AM -0700, Nathan Bossart wrote:
> Michael also requested a test for multiple -a switches and for fancier
> string patterns.  Once that is taken care of, I think this can be marked as
> ready-for-committer.

Looking at v7, this means to extend the tests to process lists for
--admin with more name patterns.  And while on it, we could do the
same for the existing command for --role, but this one is on me, being
overly-pedantic while looking at the patch :)
--
Michael

On 2022-05-24 11:09, Michael Paquier wrote:
> On Mon, May 23, 2022 at 09:37:35AM -0700, Nathan Bossart wrote:
>> Michael also requested a test for multiple -a switches and for fancier
>> string patterns.  Once that is taken care of, I think this can be 
>> marked as
>> ready-for-committer.
> 
> Looking at v7, this means to extend the tests to process lists for
> --admin with more name patterns.  And while on it, we could do the
> same for the existing command for --role, but this one is on me, being
> overly-pedantic while looking at the patch :)

Thanks! I fixed it.


-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Tue, May 24, 2022 at 08:07:31PM +0900, Shinya Kato wrote:
> On 2022-05-24 11:09, Michael Paquier wrote:
>> On Mon, May 23, 2022 at 09:37:35AM -0700, Nathan Bossart wrote:
>> > Michael also requested a test for multiple -a switches and for fancier
>> > string patterns.  Once that is taken care of, I think this can be
>> > marked as
>> > ready-for-committer.
>> 
>> Looking at v7, this means to extend the tests to process lists for
>> --admin with more name patterns.  And while on it, we could do the
>> same for the existing command for --role, but this one is on me, being
>> overly-pedantic while looking at the patch :)
> 
> Thanks! I fixed it.

We're still missing some "fancier" string patterns in the tests, but we
might just be nitpicking at this point.

I noticed that the cfbot tests for this are failing for Windows.  I've
looked at the relevant logs a bit, and I'm not sure what is going on.  The
expected log messages are indeed missing, but I haven't found any clues for
why those test cases are skipped.

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

At Tue, 24 May 2022 10:09:10 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
> We're still missing some "fancier" string patterns in the tests, but we
> might just be nitpicking at this point.

Such "fancier" strings should be properly handled by FmtId() and
appendStringLiteralConn.  If this is a privilege escalating command,
we should have ones but this is not.

> I noticed that the cfbot tests for this are failing for Windows.  I've
> looked at the relevant logs a bit, and I'm not sure what is going on.  The
> expected log messages are indeed missing, but I haven't found any clues for
> why those test cases are skipped.

createuser command complains like this.

> # Running: createuser regress_user4 -a regress_user1 -a regress_user2
> createuser: error: too many command-line arguments (first is "-a")
> hint: Try "createuser --help" for more information.

It seems like '-a' is not recognised as an option parameter.

(Fortunately, the ActiveState installer looks like having been fixed,
 but something's still wrong..)

I reproduced the same failure at my hand and identified the
cause. Windows' version of getopt_long seems to dislike that
non-optional parameters precedes options.

> createuser <user name to create> <options>

The test succeeded if I moved the <user name to create> to the end of
command line.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, May 25, 2022 at 11:07:52AM +0900, Kyotaro Horiguchi wrote:
> I reproduced the same failure at my hand and identified the
> cause. Windows' version of getopt_long seems to dislike that
> non-optional parameters precedes options.

Tweaking the list of arguments in some commands kicked by the TAP
tests to satisfy our implementation of getopt_long() has been the
origin of a couple of portability fixes, like ffd3980.
--
Michael

On 2022-05-25 12:47, Michael Paquier wrote:
> On Wed, May 25, 2022 at 11:07:52AM +0900, Kyotaro Horiguchi wrote:
>> I reproduced the same failure at my hand and identified the
>> cause. Windows' version of getopt_long seems to dislike that
>> non-optional parameters precedes options.
> 
> Tweaking the list of arguments in some commands kicked by the TAP
> tests to satisfy our implementation of getopt_long() has been the
> origin of a couple of portability fixes, like ffd3980.

Thanks! I fixed it.


On 2022-05-25 11:07, Kyotaro Horiguchi wrote:
> At Tue, 24 May 2022 10:09:10 -0700, Nathan Bossart
> <nathandbossart@gmail.com> wrote in
>> We're still missing some "fancier" string patterns in the tests, but 
>> we
>> might just be nitpicking at this point.
> 
> Such "fancier" strings should be properly handled by FmtId() and
> appendStringLiteralConn.  If this is a privilege escalating command,
> we should have ones but this is not.

Sorry, I didn't quite understand the "fancier" pattern. Is a string like 
this patch correct?


-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

At Thu, 26 May 2022 14:16:37 +0900, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote in 
> On 2022-05-25 12:47, Michael Paquier wrote:
> > On Wed, May 25, 2022 at 11:07:52AM +0900, Kyotaro Horiguchi wrote:
> >> I reproduced the same failure at my hand and identified the
> >> cause. Windows' version of getopt_long seems to dislike that
> >> non-optional parameters precedes options.
> > Tweaking the list of arguments in some commands kicked by the TAP
> > tests to satisfy our implementation of getopt_long() has been the
> > origin of a couple of portability fixes, like ffd3980.
> 
> Thanks! I fixed it.
> 
> 
> On 2022-05-25 11:07, Kyotaro Horiguchi wrote:
> > At Tue, 24 May 2022 10:09:10 -0700, Nathan Bossart
> > <nathandbossart@gmail.com> wrote in
> >> We're still missing some "fancier" string patterns in the tests, but
> >> we
> >> might just be nitpicking at this point.
> > Such "fancier" strings should be properly handled by FmtId() and
> > appendStringLiteralConn.  If this is a privilege escalating command,
> > we should have ones but this is not.
> 
> Sorry, I didn't quite understand the "fancier" pattern. Is a string
> like this patch correct?

FWIW, the "fancy" here causes me to think about something likely to
cause syntax breakage of the query to be sent.

createuser -a 'user"1' -a 'user"2' 'user"3'
createuser -v "2023-1-1'; DROP TABLE public.x; select '" hoge

BUT, thses should be prevented by the functions enumerated above. So,
I don't think we need them.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, May 26, 2022 at 02:16:37PM +0900, Shinya Kato wrote:
> On 2022-05-25 11:07, Kyotaro Horiguchi wrote:
>> At Tue, 24 May 2022 10:09:10 -0700, Nathan Bossart
>> <nathandbossart@gmail.com> wrote in
>> > We're still missing some "fancier" string patterns in the tests, but
>> > we
>> > might just be nitpicking at this point.
>> 
>> Such "fancier" strings should be properly handled by FmtId() and
>> appendStringLiteralConn.  If this is a privilege escalating command,
>> we should have ones but this is not.
> 
> Sorry, I didn't quite understand the "fancier" pattern. Is a string like
> this patch correct?

Yes, thanks.  I'm marking this as ready-for-committer.

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

On Thu, May 26, 2022 at 04:47:46PM +0900, Kyotaro Horiguchi wrote:
> FWIW, the "fancy" here causes me to think about something likely to
> cause syntax breakage of the query to be sent.
>
> createuser -a 'user"1' -a 'user"2' 'user"3'
> createuser -v "2023-1-1'; DROP TABLE public.x; select '" hoge

That would be mostly using spaces here, to make sure that quoting is
correctly applied.

> BUT, thses should be prevented by the functions enumerated above. So,
> I don't think we need them.

Mostly.  For example, the test for --valid-until can use a timestamp
with spaces to validate the use of appendStringLiteralConn().  A
second thing is that --member was checked, but not --admin, so I have
renamed regress_user2 to "regress user2" for that to apply a maximum
of coverage, and applied the patch.

One thing that I found annoying is that this made the list of options
of createuser much harder to follow.  That's not something caused by
this patch as many options have accumulated across the years and there
is a kind pattern where the connection options were listed first, but
I have cleaned up that while on it.  A second area where this could be
done is createdb, as it could be easily expanded if the backend query
gains support for more stuff, but that can happen when it makes more
sense.
--
Michael

Hi,
Thanks to the developers and reviewers.
The attached small patch fixes the message in "createuser --help" command. The patch has changed to specify a time
stampfor the --valid-for option. I don't think the SGML description needs to be modified. 

Regards,
Noriyoshi Shinoda
-----Original Message-----
From: Michael Paquier <michael@paquier.xyz>
Sent: Wednesday, July 13, 2022 12:25 PM
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: Shinya11.Kato@oss.nttdata.com; nathandbossart@gmail.com; przemyslaw@sztoch.pl; david.g.johnston@gmail.com;
robertmhaas@gmail.com;daniel@yesql.se; pgsql-hackers@postgresql.org 
Subject: Re: Add --{no-,}bypassrls flags to createuser

On Thu, May 26, 2022 at 04:47:46PM +0900, Kyotaro Horiguchi wrote:
> FWIW, the "fancy" here causes me to think about something likely to
> cause syntax breakage of the query to be sent.
>
> createuser -a 'user"1' -a 'user"2' 'user"3'
> createuser -v "2023-1-1'; DROP TABLE public.x; select '" hoge

That would be mostly using spaces here, to make sure that quoting is correctly applied.

> BUT, thses should be prevented by the functions enumerated above. So,
> I don't think we need them.

Mostly.  For example, the test for --valid-until can use a timestamp with spaces to validate the use of
appendStringLiteralConn(). A second thing is that --member was checked, but not --admin, so I have renamed
regress_user2to "regress user2" for that to apply a maximum of coverage, and applied the patch. 

One thing that I found annoying is that this made the list of options of createuser much harder to follow.  That's not
somethingcaused by this patch as many options have accumulated across the years and there is a kind pattern where the
connectionoptions were listed first, but I have cleaned up that while on it.  A second area where this could be done is
createdb,as it could be easily expanded if the backend query gains support for more stuff, but that can happen when it
makesmore sense. 
--
Michael

On Wed, Jul 13, 2022 at 08:14:28AM +0000, Shinoda, Noriyoshi (PN Japan FSIP) wrote:
> The attached small patch fixes the message in "createuser --help" command. The patch has changed to specify a time
stampfor the --valid-for option. I don't think the SGML description needs to be modified.
 

Good catch.  Apart from a nitpick about the indentation, your patch looks
reasonable to me.

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

On Wed, Jul 13, 2022 at 11:38:03AM -0700, Nathan Bossart wrote:
> On Wed, Jul 13, 2022 at 08:14:28AM +0000, Shinoda, Noriyoshi (PN Japan FSIP) wrote:
>> The attached small patch fixes the message in "createuser --help"
>> command. The patch has changed to specify a time stamp for the
>> --valid-for option. I don't think the SGML description needs to be
>> modified.

Thanks, Shinoda-san.  Fixed.

> Good catch.  Apart from a nitpick about the indentation, your patch looks
> reasonable to me.

FWIW, one can check that with a simple `git diff --check` or similar
to see what was going wrong here.  This simple trick allows me to find
quickly formatting issues in any patch posted.
--
Michael