createuser doesn't tell default settings for some options

Started by Kyotaro Horiguchiover 3 years ago3 messages
#1Kyotaro Horiguchi
horikyota.ntt@gmail.com
3 attachment(s)

(I suppose this is a pg15 issue)

createuser --help shows the following help text.

--bypassrls role can bypass row-level security (RLS) policy
--no-bypassrls role cannot bypass row-level security (RLS) policy
--replication role can initiate replication
--no-replication role cannot initiate replication

For other options the text tells which one is the default, which I
think the two options also should have the same.

-r, --createrole role can create new roles
-R, --no-createrole role cannot create roles (default)

In correspondence, it seems to me that the command should explicitly
place the default value (of the command's own) in generated SQL
command even if the corresponding command line options are omitted, as
createrole and so do. (attached first)

The interacitive mode doesn't cover all options, but I'm not sure what
we should do to the mode since I don't have a clear idea of how the
mode is used. In the attached only --bypassrls is arbirarily added.
The remaining options omitted in the interactive mode are: password,
valid-until, role, member and replication. (attached second)

The ternary options are checked against decimal 0, but it should use
TRI_DEFAULT instead. (attached third)

I tempted to check no ternary options remains set to TRY_DEFAULT
before generating SQL command, but I didn't that in the attached.

What do you think about this?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Attachments:

0001-Fix-handling-of-default-option-values-in-createuser.patchtext/x-patch; charset=us-asciiDownload
From 835f5e14dc40b8ef3c93bdc976477c38a63d018b Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Wed, 10 Aug 2022 15:03:44 +0900
Subject: [PATCH 1/3] Fix handling of default option values in createuser

Add description of which one is the default between two complementary
options of --bypassrls and --replication in the help text. In
correspondence let the command always include the tokens corresponding
to every options of that kind in the SQL command sent to server.
---
 src/bin/scripts/createuser.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index 991930a1ae..afde9bed5f 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -270,6 +270,12 @@ main(int argc, char *argv[])
 			createrole = TRI_NO;
 	}
 
+	if (bypassrls == 0)
+		bypassrls = TRI_NO;
+
+	if (replication == 0)
+		replication = TRI_NO;
+
 	if (inherit == 0)
 		inherit = TRI_YES;
 
@@ -432,9 +438,10 @@ help(const char *progname)
 	printf(_("  --interactive             prompt for missing role name and attributes rather\n"
 			 "                            than using defaults\n"));
 	printf(_("  --bypassrls               role can bypass row-level security (RLS) policy\n"));
-	printf(_("  --no-bypassrls            role cannot bypass row-level security (RLS) policy\n"));
+	printf(_("  --no-bypassrls            role cannot bypass row-level security (RLS) policy\n"
+			 "                            (default)\n"));
 	printf(_("  --replication             role can initiate replication\n"));
-	printf(_("  --no-replication          role cannot initiate replication\n"));
+	printf(_("  --no-replication          role cannot initiate replication (default)\n"));
 	printf(_("  -?, --help                show this help, then exit\n"));
 	printf(_("\nConnection options:\n"));
 	printf(_("  -h, --host=HOSTNAME       database server host or socket directory\n"));
-- 
2.31.1

0002-Add-bypassrls-to-interactive-mode-in-createuser.patchtext/x-patch; charset=us-asciiDownload
From c2285ccc738260d57895d4b9cfc77ea9feaebb26 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Wed, 10 Aug 2022 15:04:43 +0900
Subject: [PATCH 2/3] Add bypassrls to interactive mode in createuser

---
 src/bin/scripts/createuser.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index afde9bed5f..9789bab034 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -271,7 +271,12 @@ main(int argc, char *argv[])
 	}
 
 	if (bypassrls == 0)
-		bypassrls = TRI_NO;
+	{
+		if (interactive && yesno_prompt("Shall the new role be allowed to bypass row-level security policy?"))
+			bypassrls = TRI_YES;
+		else
+			bypassrls = TRI_NO;
+	}
 
 	if (replication == 0)
 		replication = TRI_NO;
-- 
2.31.1

0003-Use-ternary-value-against-ternary-variables.patchtext/x-patch; charset=us-asciiDownload
From 71a52953717371f8fa77e50f966410ac16581852 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Wed, 10 Aug 2022 14:21:47 +0900
Subject: [PATCH 3/3] Use ternary value against ternary variables

createuser.c uses '0' against a trivalue.  It should use TRI_DEFAULT
instead.
---
 src/bin/scripts/createuser.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index 9789bab034..4a2a751f7e 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -239,7 +239,7 @@ main(int argc, char *argv[])
 		free(pw2);
 	}
 
-	if (superuser == 0)
+	if (superuser == TRI_DEFAULT)
 	{
 		if (interactive && yesno_prompt("Shall the new role be a superuser?"))
 			superuser = TRI_YES;
@@ -254,7 +254,7 @@ main(int argc, char *argv[])
 		createrole = TRI_YES;
 	}
 
-	if (createdb == 0)
+	if (createdb == TRI_DEFAULT)
 	{
 		if (interactive && yesno_prompt("Shall the new role be allowed to create databases?"))
 			createdb = TRI_YES;
@@ -262,7 +262,7 @@ main(int argc, char *argv[])
 			createdb = TRI_NO;
 	}
 
-	if (createrole == 0)
+	if (createrole == TRI_DEFAULT)
 	{
 		if (interactive && yesno_prompt("Shall the new role be allowed to create more new roles?"))
 			createrole = TRI_YES;
@@ -270,7 +270,7 @@ main(int argc, char *argv[])
 			createrole = TRI_NO;
 	}
 
-	if (bypassrls == 0)
+	if (bypassrls == TRI_DEFAULT)
 	{
 		if (interactive && yesno_prompt("Shall the new role be allowed to bypass row-level security policy?"))
 			bypassrls = TRI_YES;
@@ -278,13 +278,13 @@ main(int argc, char *argv[])
 			bypassrls = TRI_NO;
 	}
 
-	if (replication == 0)
+	if (replication == TRI_DEFAULT)
 		replication = TRI_NO;
 
-	if (inherit == 0)
+	if (inherit == TRI_DEFAULT)
 		inherit = TRI_YES;
 
-	if (login == 0)
+	if (login == TRI_DEFAULT)
 		login = TRI_YES;
 
 	cparams.dbname = NULL;		/* this program lacks any dbname option... */
-- 
2.31.1

#2Daniel Gustafsson
daniel@yesql.se
In reply to: Kyotaro Horiguchi (#1)
Re: createuser doesn't tell default settings for some options

On 10 Aug 2022, at 08:12, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

(I suppose this is a pg15 issue)

createuser --help shows the following help text.

--bypassrls role can bypass row-level security (RLS) policy
--no-bypassrls role cannot bypass row-level security (RLS) policy
--replication role can initiate replication
--no-replication role cannot initiate replication

For other options the text tells which one is the default, which I
think the two options also should have the same.

Agreed. For --no-replication the docs in createuser.sgml should fixed to
include a "This is the default" sentence like the others have as well.

The interacitive mode doesn't cover all options, but I'm not sure what
we should do to the mode since I don't have a clear idea of how the
mode is used. In the attached only --bypassrls is arbirarily added.
The remaining options omitted in the interactive mode are: password,
valid-until, role, member and replication. (attached second)

I'm not convinced that we should add more to the interactive mode, it's IMO
mostly a backwards compat option for ancient (pre-9.2) createuser where this
was automatically done. Back then we had this in the documentation which has
since been removed:

"You will be prompted for a name and other missing information if it is not
specified on the command line."

The ternary options are checked against decimal 0, but it should use
TRI_DEFAULT instead. (attached third)

Agreed, nice catch.

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

#3Daniel Gustafsson
daniel@yesql.se
In reply to: Daniel Gustafsson (#2)
1 attachment(s)
Re: createuser doesn't tell default settings for some options

On 10 Aug 2022, at 10:28, Daniel Gustafsson <daniel@yesql.se> wrote:

On 10 Aug 2022, at 08:12, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

(I suppose this is a pg15 issue)

createuser --help shows the following help text.

--bypassrls role can bypass row-level security (RLS) policy
--no-bypassrls role cannot bypass row-level security (RLS) policy
--replication role can initiate replication
--no-replication role cannot initiate replication

For other options the text tells which one is the default, which I
think the two options also should have the same.

Agreed. For --no-replication the docs in createuser.sgml should fixed to
include a "This is the default" sentence like the others have as well.

The ternary options are checked against decimal 0, but it should use
TRI_DEFAULT instead. (attached third)

Agreed, nice catch.

Attached is my proposal for this, combining your 0001 and 0003 patches with
some docs and test fixups to match.

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

Attachments:

v2-0001-Fix-handling-of-default-option-values-in-createus.patchapplication/octet-stream; name=v2-0001-Fix-handling-of-default-option-values-in-createus.patch; x-unix-mode=0644Download
From 386aa4cbdf4c6fcef83c6188b84bca851d000287 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Mon, 21 Nov 2022 13:55:09 +0100
Subject: [PATCH v2] Fix handling of default option values in createuser

Add description of which one is the default between two complementary
options of --bypassrls and --replication in the help text and docs. In
correspondence let the command always include the tokens corresponding
to every options of that kind in the SQL command sent to server. Tests
are updated accordingly.

Also fix the checks of some trivalue vars which were using literal zero
for checking default value instead of the enum label TRI_DEFAULT. While
not a bug, since TRI_DEFAULT is defined as zero, fixing improves read-
ability improved readability (and avoid bugs if the enum is changed).

Author: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://postgr.es/m/20220810.151243.1073197628358749087.horikyota.ntt@gmail.com
---
 doc/src/sgml/ref/createuser.sgml    |  2 +-
 src/bin/scripts/createuser.c        | 21 ++++++++++++++-------
 src/bin/scripts/t/040_createuser.pl | 18 +++++++++---------
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml
index c6a7c603f7..736f6336cd 100644
--- a/doc/src/sgml/ref/createuser.sgml
+++ b/doc/src/sgml/ref/createuser.sgml
@@ -341,7 +341,7 @@ PostgreSQL documentation
        <para>
         The new user will not have the <literal>REPLICATION</literal>
         privilege, which is described more fully in the documentation for <xref
-        linkend="sql-createrole"/>.
+        linkend="sql-createrole"/>.  This is the default.
        </para>
       </listitem>
      </varlistentry>
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index 991930a1ae..6c702e197a 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -239,7 +239,7 @@ main(int argc, char *argv[])
 		free(pw2);
 	}
 
-	if (superuser == 0)
+	if (superuser == TRI_DEFAULT)
 	{
 		if (interactive && yesno_prompt("Shall the new role be a superuser?"))
 			superuser = TRI_YES;
@@ -254,7 +254,7 @@ main(int argc, char *argv[])
 		createrole = TRI_YES;
 	}
 
-	if (createdb == 0)
+	if (createdb == TRI_DEFAULT)
 	{
 		if (interactive && yesno_prompt("Shall the new role be allowed to create databases?"))
 			createdb = TRI_YES;
@@ -262,7 +262,7 @@ main(int argc, char *argv[])
 			createdb = TRI_NO;
 	}
 
-	if (createrole == 0)
+	if (createrole == TRI_DEFAULT)
 	{
 		if (interactive && yesno_prompt("Shall the new role be allowed to create more new roles?"))
 			createrole = TRI_YES;
@@ -270,10 +270,16 @@ main(int argc, char *argv[])
 			createrole = TRI_NO;
 	}
 
-	if (inherit == 0)
+	if (bypassrls == TRI_DEFAULT)
+		bypassrls = TRI_NO;
+
+	if (replication == TRI_DEFAULT)
+		replication = TRI_NO;
+
+	if (inherit == TRI_DEFAULT)
 		inherit = TRI_YES;
 
-	if (login == 0)
+	if (login == TRI_DEFAULT)
 		login = TRI_YES;
 
 	cparams.dbname = NULL;		/* this program lacks any dbname option... */
@@ -432,9 +438,10 @@ help(const char *progname)
 	printf(_("  --interactive             prompt for missing role name and attributes rather\n"
 			 "                            than using defaults\n"));
 	printf(_("  --bypassrls               role can bypass row-level security (RLS) policy\n"));
-	printf(_("  --no-bypassrls            role cannot bypass row-level security (RLS) policy\n"));
+	printf(_("  --no-bypassrls            role cannot bypass row-level security (RLS) policy\n"
+			 "                            (default)\n"));
 	printf(_("  --replication             role can initiate replication\n"));
-	printf(_("  --no-replication          role cannot initiate replication\n"));
+	printf(_("  --no-replication          role cannot initiate replication (default)\n"));
 	printf(_("  -?, --help                show this help, then exit\n"));
 	printf(_("\nConnection options:\n"));
 	printf(_("  -h, --host=HOSTNAME       database server host or socket directory\n"));
diff --git a/src/bin/scripts/t/040_createuser.pl b/src/bin/scripts/t/040_createuser.pl
index 834d258bf8..138490ecd2 100644
--- a/src/bin/scripts/t/040_createuser.pl
+++ b/src/bin/scripts/t/040_createuser.pl
@@ -18,19 +18,19 @@ $node->start;
 
 $node->issues_sql_like(
 	[ 'createuser', 'regress_user1' ],
-	qr/statement: CREATE ROLE regress_user1 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN;/,
+	qr/statement: CREATE ROLE regress_user1 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS;/,
 	'SQL CREATE USER run');
 $node->issues_sql_like(
 	[ 'createuser', '-L', 'regress_role1' ],
-	qr/statement: CREATE ROLE regress_role1 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN;/,
+	qr/statement: CREATE ROLE regress_role1 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS;/,
 	'create a non-login role');
 $node->issues_sql_like(
 	[ 'createuser', '-r', 'regress user2' ],
-	qr/statement: CREATE ROLE "regress user2" NOSUPERUSER NOCREATEDB CREATEROLE INHERIT LOGIN;/,
+	qr/statement: CREATE ROLE "regress user2" NOSUPERUSER NOCREATEDB CREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS;/,
 	'create a CREATEROLE user');
 $node->issues_sql_like(
 	[ 'createuser', '-s', 'regress_user3' ],
-	qr/statement: CREATE ROLE regress_user3 SUPERUSER CREATEDB CREATEROLE INHERIT LOGIN;/,
+	qr/statement: CREATE ROLE regress_user3 SUPERUSER CREATEDB CREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS;/,
 	'create a superuser');
 $node->issues_sql_like(
 	[
@@ -38,7 +38,7 @@ $node->issues_sql_like(
 		'regress_user1', '-a',
 		'regress user2', 'regress user #4'
 	],
-	qr/statement: CREATE ROLE "regress user #4" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN ADMIN regress_user1,"regress user2";/,
+	qr/statement: CREATE ROLE "regress user #4" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS ADMIN regress_user1,"regress user2";/,
 	'add a role as a member with admin option of the newly created role');
 $node->issues_sql_like(
 	[
@@ -46,19 +46,19 @@ $node->issues_sql_like(
 		'regress_user3',   '-m',
 		'regress user #4', 'REGRESS_USER5'
 	],
-	qr/statement: CREATE ROLE "REGRESS_USER5" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN ROLE regress_user3,"regress user #4";/,
+	qr/statement: CREATE ROLE "REGRESS_USER5" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS ROLE regress_user3,"regress user #4";/,
 	'add a role as a member of the newly created role');
 $node->issues_sql_like(
 	[ 'createuser', '-v', '2029 12 31', 'regress_user6' ],
-	qr/statement: CREATE ROLE regress_user6 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN VALID UNTIL \'2029 12 31\';/,
+	qr/statement: CREATE ROLE regress_user6 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS VALID UNTIL \'2029 12 31\';/,
 	'create a role with a password expiration date');
 $node->issues_sql_like(
 	[ 'createuser', '--bypassrls', 'regress_user7' ],
-	qr/statement: CREATE ROLE regress_user7 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN BYPASSRLS;/,
+	qr/statement: CREATE ROLE regress_user7 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION BYPASSRLS;/,
 	'create a BYPASSRLS role');
 $node->issues_sql_like(
 	[ 'createuser', '--no-bypassrls', 'regress_user8' ],
-	qr/statement: CREATE ROLE regress_user8 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOBYPASSRLS;/,
+	qr/statement: CREATE ROLE regress_user8 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS;/,
 	'create a role without BYPASSRLS');
 
 $node->command_fails([ 'createuser', 'regress_user1' ],
-- 
2.32.1 (Apple Git-133)