Thread: [PATCH] Use role name "system_user" instead of "user" for unsafe_tests

Hi,

While playing with a new single board computer (VisionFive 2) I
discovered that postgresql:unsafe_tests suite fails like this:

```
--- /home/user/projects/postgresql/src/test/modules/unsafe_tests/expected/rolenames.out
2023-04-11 14:58:57.844550612 +0000
+++ /home/user/projects/postgresql/build/testrun/unsafe_tests/regress/results/rolenames.out
    2023-04-11 17:54:22.999024391 +0000
@@ -53,6 +53,7 @@
 CREATE ROLE "current_user";
 CREATE ROLE "session_user";
 CREATE ROLE "user";
+ERROR:  role "user" already exists
 RESET client_min_messages;
 CREATE ROLE current_user; -- error
 ERROR:  CURRENT_USER cannot be used as a role name here
@@ -1089,4 +1090,5 @@
 DROP OWNED BY regress_testrol0, "Public", "current_role",
"current_user", regress_testrol1, regress_testrol2, regress_testrolx
CASCADE;
 DROP ROLE regress_testrol0, regress_testrol1, regress_testrol2,
regress_testrolx;
 DROP ROLE "Public", "None", "current_role", "current_user",
"session_user", "user";
+ERROR:  current user cannot be dropped
 DROP ROLE regress_role_haspriv, regress_role_nopriv;
```

This happens because the developers of this SBC choose the default
username "user", which I had no reason to change.

Test merely checks that we can distinguish a username "user" from the
USER keyword. Maybe it's worth replacing "user" with "system_user"? It
is also a keyword but is a less likely choice for the OS user name.

-- 
Best regards,
Aleksander Alekseev

Hi,

While playing with a new single board computer (VisionFive 2) I
discovered that postgresql:unsafe_tests suite fails like this:

```
--- /home/user/projects/postgresql/src/test/modules/unsafe_tests/expected/rolenames.out
2023-04-11 14:58:57.844550612 +0000
+++ /home/user/projects/postgresql/build/testrun/unsafe_tests/regress/results/rolenames.out    2023-04-11 17:54:22.999024391 +0000
@@ -53,6 +53,7 @@ CREATE ROLE "current_user"; CREATE ROLE "session_user"; CREATE ROLE "user";
+ERROR:  role "user" already exists RESET client_min_messages; CREATE ROLE current_user; -- error ERROR:  CURRENT_USER cannot be used as a role name here
@@ -1089,4 +1090,5 @@ DROP OWNED BY regress_testrol0, "Public", "current_role",
"current_user", regress_testrol1, regress_testrol2, regress_testrolx
CASCADE; DROP ROLE regress_testrol0, regress_testrol1, regress_testrol2,
regress_testrolx; DROP ROLE "Public", "None", "current_role", "current_user",
"session_user", "user";
+ERROR:  current user cannot be dropped DROP ROLE regress_role_haspriv, regress_role_nopriv;
```

This happens because the developers of this SBC choose the default
username "user", which I had no reason to change.

Test merely checks that we can distinguish a username "user" from the
USER keyword. Maybe it's worth replacing "user" with "system_user"? It
is also a keyword but is a less likely choice for the OS user name.

I don't think we can protect against all possible user names. Wouldn't it be better to run the tests under an OS user with a different name, like "marmaduke"? ("user" is a truly terrible default user name).

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi Andrew,

> I don't think we can protect against all possible user names. Wouldn't it be better to run the tests under an OS user
witha different name, like "marmaduke"? ("user" is a truly terrible default user name).
 

100% agree. The point is not to protect against all possible user
names but merely to reduce the likelihood of the problem. For this
particular test there is no difference which keyword to use for the
test. I realize this is a minor problem, however the fix is trivial
too.

-- 
Best regards,
Aleksander Alekseev

Aleksander Alekseev <aleksander@timescale.com> writes:
>> I don't think we can protect against all possible user names. Wouldn't it be better to run the tests under an OS
userwith a different name, like "marmaduke"? ("user" is a truly terrible default user name). 

> 100% agree. The point is not to protect against all possible user
> names but merely to reduce the likelihood of the problem.

It only reduces the likelihood if you assume that "system_user"
is less likely than "user" as a choice of OS user name to run
the tests under.  That seems like a debatable assumption;
perhaps it's actually *more* likely.

Whether we need to have a test for this at all is perhaps a
more interesting argument.

            regards, tom lane

Hi,

> Whether we need to have a test for this at all is perhaps a
> more interesting argument.

This was my initial thought but since somebody put it there I assumed
this is a very important test.

Any objections if we remove the tests for "user"?

-- 
Best regards,
Aleksander Alekseev

On Wed, Apr 12, 2023 at 03:30:03PM +0300, Aleksander Alekseev wrote:
> Any objections if we remove the tests for "user"?

Based on some rather-recent experience in this area with
COERCE_SQL_SYNTAX, the relationship between the SQL keywords and the
way they can handled internally could be tricky if this area of the
code is touched.  So I would choose to keep these tests, FWIW.
--
Michael

Hi,

> On Wed, Apr 12, 2023 at 03:30:03PM +0300, Aleksander Alekseev wrote:
> > Any objections if we remove the tests for "user"?
>
> Based on some rather-recent experience in this area with
> COERCE_SQL_SYNTAX, the relationship between the SQL keywords and the
> way they can handled internally could be tricky if this area of the
> code is touched.  So I would choose to keep these tests, FWIW.

Thanks for the feedback. I see this is a controversial topic so in the
interest of saving our time I'm withdrawing the patch.

-- 
Best regards,
Aleksander Alekseev