Create user in `envtool` for PostgreSQL #4133

chilagrow · 2024-02-29T03:57:02Z

Description

To test it, I added --test-enable-new-auth flag in task file run: and run-secured:.
It does not create user for SQLite, that will be handled in a separate PR.

Closes #1877.

Readiness checklist

I added/updated unit tests (and they pass).
I added/updated integration/compatibility tests (and they pass).
I added/updated comments and checked rendering.
I made spot refactorings.
I updated user documentation.
I ran task all, and it passed.
I ensured that PR title is good enough for the changelog.
(for maintainers only) I set Reviewers (@FerretDB/core), Milestone (Next), Labels, Project and project's Sprint fields.
I marked all done items in this checklist.

codecov · 2024-02-29T04:00:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.80%. Comparing base (5a5ca29) to head (0b57bbe).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4133      +/-   ##
==========================================
- Coverage   74.31%   73.80%   -0.51%     
==========================================
  Files         353      353              
  Lines       22738    22739       +1     
==========================================
- Hits        16897    16783     -114     
- Misses       4552     4686     +134     
+ Partials     1289     1270      -19

Files	Coverage Δ
integration/setup/setup.go	`81.25% <100.00%> (ø)`
internal/handler/commands.go	`100.00% <100.00%> (ø)`

... and 12 files with indirect coverage changes

Flag	Coverage Δ
filter-true	`67.51% <100.00%> (-0.31%)`	⬇️
hana-1	`?`
integration	`67.51% <100.00%> (-0.31%)`	⬇️
mongodb-1	`4.75% <25.00%> (+<0.01%)`	⬆️
postgresql-1	`48.72% <100.00%> (+0.53%)`	⬆️
postgresql-2	`50.07% <100.00%> (+1.48%)`	⬆️
postgresql-3	`48.07% <100.00%> (-1.02%)`	⬇️
sqlite-1	`47.66% <75.00%> (+0.32%)`	⬆️
sqlite-2	`48.99% <75.00%> (+1.21%)`	⬆️
sqlite-3	`47.47% <75.00%> (-0.71%)`	⬇️
unit	`32.59% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

chilagrow · 2024-02-29T08:32:46Z

cmd/envtool/envtool.go

+//
+// Without this, once the first user is created, the authentication fails
+// as username/password does not exist in admin.system.users collection.
+func setupUser(ctx context.Context, logger *zap.SugaredLogger, postgreSQLPort uint16) error {


This does not replace setupUser() in integration test.
~~Because this function creates user in ferretdb database which postgreSQL schema, postgres://username@127.0.0.1:5432/ferretdb?pool_max_conns=50.~~

~~Each integration test create its own schema like so, postgres://username@127.0.0.1:5432/testauthentication?pool_max_conns=50.~~

Thanks for the clarification! I was worried this could jeopardize running tests directly with go test for a moment 😄

It's still there for SQLite. It only has handler level authentication, and once the first user is created without this code, auth would fail.

It will be handled in a separate PR.

chilagrow · 2024-02-29T08:35:20Z

Taskfile.yml

@@ -419,7 +419,7 @@ tasks:
        --proxy-addr=127.0.0.1:47017
        --mode=diff-normal
        --handler=pg
-        --postgresql-url='postgres://127.0.0.1:5433/ferretdb?search_path='
+        --postgresql-url='postgres://username:password@127.0.0.1:5433/ferretdb?search_path='


For run-secured to work, what other better thing can I do?

That target was supposed to be for the "old" authentication that does not need credentials in the PostgreSQL URI. Why are they needed?

It's because postgres started on port 5433 is created from postgres_secured docker https://github.com/FerretDB/FerretDB/blob/main/docker-compose.yml#L28.
Unlike postgres docker which has POSTGRES_HOST_AUTH_METHOD=trust, postgres_secured doesn't have trust so username/password is required to connect to it.

chilagrow · 2024-03-01T03:57:54Z

integration/commands_authentication_test.go

@@ -27,12 +33,55 @@ import (
 func TestCommandsAuthenticationLogout(t *testing.T) {


Logout does not fail on authentication check, if user is not authenticated, it just returns success.
This test was updated to use newly created user, because if it uses default username/password user and logs out in this test, t.Cleanup to drop collection fails due to not authenticate.

chilagrow · 2024-03-01T07:06:06Z

integration/setup/setup.go

@@ -334,7 +334,8 @@ func insertBenchmarkProvider(tb testtb.TB, ctx context.Context, collection *mong
 func setupUser(tb testtb.TB, ctx context.Context, client *mongo.Client) {
 	tb.Helper()

-	if IsMongoDB(tb) {
+	if IsPostgreSQL(tb) || IsMongoDB(tb) {
+		// the user is created in env tool for PostgreSQL


For SQLite we need something similar to have a database with default user. Will be handle in a separate PR.

And also for MySQL.

Yes that's true. MySQL hasn't been handled. let me see the status of MySQL backend

MySQL and HANA backends are not yet supported, no need to do anything for them

cmd/envtool/user.json

rumyantseva

LGTM

AlekSi · 2024-03-06T16:24:40Z

Taskfile.yml

@@ -419,7 +419,7 @@ tasks:
        --proxy-addr=127.0.0.1:47017
        --mode=diff-normal
        --handler=pg
-        --postgresql-url='postgres://127.0.0.1:5433/ferretdb?search_path='
+        --postgresql-url='postgres://username:password@127.0.0.1:5433/ferretdb?search_path='


That target was supposed to be for the "old" authentication that does not need credentials in the PostgreSQL URI. Why are they needed?

AlekSi · 2024-03-06T16:29:07Z

cmd/envtool/envtool.go

+	err = setupUserInPostgres(ctx, logger, "postgres://username:password@127.0.0.1:5433/ferretdb")
+	if err != nil {
+		return err
+	}
+
+	return setupUserInPostgres(ctx, logger, "postgres://username:password@127.0.0.1:5433/template1")


I don't understand why those calls are not inside setupAnyPostgres and what we are doing with credentials there

AlekSi · 2024-03-06T16:31:17Z

cmd/envtool/envtool.go

+	if _, err = dbPool.Exec(ctx, q); err != nil && (!errors.As(err, &pgErr) || pgErr.Code != pgerrcode.DuplicateSchema) {
+		return err
+	}
+
+	if err == nil {


I think we could simply that by:

if errors.As(err, &pgErr) && pgErr.Code == pgerrcode.DuplicateSchema { err = nil } if err != nil { return err }

AlekSi · 2024-03-06T16:33:48Z

cmd/envtool/envtool.go

+	}
+
+	if err == nil {
+		q = `CREATE UNIQUE INDEX "system_users_aff2f7ce__id__67399184_idx" ON admin.system_users_aff2f7ce (((_jsonb->'_id')))`


Yikes!..
I wonder if we should step away from that and just implement the localhost exception that we are already working on.

Yes unfortunately this is something needed to be exposed.
It was copy and paste from log output, logic is too difficult to copy accurately 🙈

You know what, let me leave this PR for now and progress with local host exception.

AlekSi · 2024-03-06T16:35:00Z

integration/setup/setup.go

@@ -334,7 +334,8 @@ func insertBenchmarkProvider(tb testtb.TB, ctx context.Context, collection *mong
 func setupUser(tb testtb.TB, ctx context.Context, client *mongo.Client) {
 	tb.Helper()

-	if IsMongoDB(tb) {
+	if IsPostgreSQL(tb) || IsMongoDB(tb) {
+		// the user is created in env tool for PostgreSQL


MySQL and HANA backends are not yet supported, no need to do anything for them

AlekSi · 2024-03-07T10:32:03Z

internal/handler/commands.go

-			Handler: h.MsgLogout,
-			Help:    "Logs out from the current session.",
+			Handler:   h.MsgLogout,
+			anonymous: true,


Let's not forget to port fixes from that PR

Thanks yep that has been included in #4156

initial

36c81c0

mergify bot assigned chilagrow Feb 29, 2024

chilagrow added 6 commits February 29, 2024 14:39

return error if address is already used

d3a2bef

Merge branch 'main' into setup-user-env-tool

167ccf6

update comment

2043e92

use port number

b2f6205

setup user as part of postgresql setup

ff25f6f

update task file

d68a777

chilagrow commented Feb 29, 2024

View reviewed changes

chilagrow added the code/chore Code maintenance improvements label Feb 29, 2024

chilagrow added 5 commits March 1, 2024 09:44

use mongo driver instead of command

1f1c728

create user in template1

af0bdff

remove timeout from test

563e165

fix tests related to having admin database

96d34ef

fix logout test

9db3718

chilagrow commented Mar 1, 2024

View reviewed changes

chilagrow added 4 commits March 1, 2024 12:59

close client

34bd803

replace test

f41adb2

fix name

80bf3a0

sqlite user is still created in test setup

aba7e4e

chilagrow commented Mar 1, 2024

View reviewed changes

chilagrow marked this pull request as ready for review March 1, 2024 07:42

chilagrow requested review from AlekSi and a team as code owners March 1, 2024 07:42

chilagrow requested review from rumyantseva, a team, henvic and noisersup March 1, 2024 07:42

henvic previously approved these changes Mar 1, 2024

View reviewed changes

AlekSi changed the title ~~Create user in env tool for PostgreSQL~~ Create user in envtool for PostgreSQL Mar 1, 2024

chilagrow marked this pull request as draft March 1, 2024 09:48

auto-merge was automatically disabled March 1, 2024 09:48
Pull request was converted to draft

use sql directly

6192940

chilagrow dismissed henvic’s stale review via 6192940 March 5, 2024 03:52

chilagrow added 2 commits March 5, 2024 13:01

update comment

4c4add2

Merge branch 'main' into setup-user-env-tool

8d5ca45

chilagrow marked this pull request as ready for review March 5, 2024 04:09

chilagrow enabled auto-merge (squash) March 5, 2024 04:13

chilagrow requested a review from henvic March 5, 2024 05:33

henvic reviewed Mar 5, 2024

View reviewed changes

cmd/envtool/user.json Outdated Show resolved Hide resolved

henvic previously approved these changes Mar 5, 2024

View reviewed changes

renaming

5278da8

chilagrow dismissed henvic’s stale review via 5278da8 March 5, 2024 07:05

henvic previously approved these changes Mar 5, 2024

View reviewed changes

chilagrow added 2 commits March 6, 2024 10:34

Merge branch 'main' into setup-user-env-tool

3134785

use require in test for not panicing when adding new backend

5801476

chilagrow dismissed henvic’s stale review via 5801476 March 6, 2024 01:44

undo

afae527

chilagrow mentioned this pull request Mar 6, 2024

Create user in envtool for SQLite #4147

Closed

9 tasks

rumyantseva approved these changes Mar 6, 2024

View reviewed changes

chilagrow requested a review from henvic March 6, 2024 09:31

Merge branch 'main' into setup-user-env-tool

0b57bbe

AlekSi reviewed Mar 6, 2024

View reviewed changes

chilagrow mentioned this pull request Mar 7, 2024

Support localhost exception #4156

Merged

9 tasks

chilagrow closed this Mar 7, 2024

auto-merge was automatically disabled March 7, 2024 09:50
Pull request was closed

AlekSi reviewed Mar 7, 2024

View reviewed changes

AlekSi removed this from the Next milestone Mar 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create user in `envtool` for PostgreSQL #4133

Create user in `envtool` for PostgreSQL #4133

chilagrow commented Feb 29, 2024 •

edited

codecov bot commented Feb 29, 2024 •

edited

chilagrow Feb 29, 2024 •

edited

henvic Feb 29, 2024

chilagrow Mar 1, 2024

chilagrow Feb 29, 2024

AlekSi Mar 6, 2024

chilagrow Mar 7, 2024

chilagrow Mar 1, 2024 •

edited

chilagrow Mar 1, 2024

henvic Mar 1, 2024 •

edited

chilagrow Mar 1, 2024

AlekSi Mar 6, 2024

rumyantseva left a comment

AlekSi Mar 6, 2024

AlekSi Mar 6, 2024

AlekSi Mar 6, 2024

AlekSi Mar 6, 2024

chilagrow Mar 7, 2024

AlekSi Mar 6, 2024

AlekSi Mar 7, 2024

chilagrow Mar 7, 2024

		@@ -27,12 +33,55 @@ import (
		func TestCommandsAuthenticationLogout(t *testing.T) {

Create user in envtool for PostgreSQL #4133

Create user in envtool for PostgreSQL #4133

Conversation

chilagrow commented Feb 29, 2024 • edited

Description

Readiness checklist

codecov bot commented Feb 29, 2024 • edited

Codecov Report

chilagrow Feb 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chilagrow Mar 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henvic Mar 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rumyantseva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Create user in `envtool` for PostgreSQL #4133

Create user in `envtool` for PostgreSQL #4133

chilagrow commented Feb 29, 2024 •

edited

codecov bot commented Feb 29, 2024 •

edited

chilagrow Feb 29, 2024 •

edited

chilagrow Mar 1, 2024 •

edited

henvic Mar 1, 2024 •

edited