From f7b34b528cead1c89398476a0d615aa8aa84dd6b Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Thu, 22 Feb 2024 14:25:30 +0100 Subject: [PATCH 01/27] Unmark showCredentials as ignored property of usersInfo --- internal/handler/msg_usersinfo.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/handler/msg_usersinfo.go b/internal/handler/msg_usersinfo.go index 774c0f262cc7..fdd01323831d 100644 --- a/internal/handler/msg_usersinfo.go +++ b/internal/handler/msg_usersinfo.go @@ -59,7 +59,7 @@ func (h *Handler) MsgUsersInfo(ctx context.Context, msg *wire.OpMsg) (*wire.OpMs common.Ignored( document, h.L, - "showCredentials", "showCustomData", "showPrivileges", + "showCustomData", "showPrivileges", "showAuthenticationRestrictions", "comment", "filter", ) @@ -168,6 +168,10 @@ func (h *Handler) MsgUsersInfo(ctx context.Context, msg *wire.OpMsg) (*wire.OpMs return nil, lazyerrors.Error(err) } + if matches { + res.Append(v) + } + if v.Has("credentials") { credentials := must.NotFail(v.Get("credentials")).(*types.Document) if credentialsKeys := credentials.Keys(); len(credentialsKeys) > 0 { @@ -183,10 +187,6 @@ func (h *Handler) MsgUsersInfo(ctx context.Context, msg *wire.OpMsg) (*wire.OpMs if !showCredentials { v.Remove("credentials") } - - if matches { - res.Append(v) - } } var reply wire.OpMsg From 477ab174c4047a9dbf13cf9ce9d3252a24d99f45 Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Thu, 22 Feb 2024 14:27:44 +0100 Subject: [PATCH 02/27] wip --- internal/handler/msg_getparameter.go | 2 +- internal/handler/msg_hello.go | 126 ++++++++++++++++++++++++--- 2 files changed, 113 insertions(+), 15 deletions(-) diff --git a/internal/handler/msg_getparameter.go b/internal/handler/msg_getparameter.go index 47180050a34e..1194d7eca763 100644 --- a/internal/handler/msg_getparameter.go +++ b/internal/handler/msg_getparameter.go @@ -52,7 +52,7 @@ func (h *Handler) MsgGetParameter(ctx context.Context, msg *wire.OpMsg) (*wire.O // "settableAtStartup", , //)), "authenticationMechanisms", must.NotFail(types.NewDocument( - "value", must.NotFail(types.NewArray("PLAIN")), + "value", must.NotFail(types.NewArray("SCRAM-SHA-1", "SCRAM-SHA-256", "PLAIN")), "settableAtRuntime", false, "settableAtStartup", true, )), diff --git a/internal/handler/msg_hello.go b/internal/handler/msg_hello.go index 9845961f024c..68efa24c54f3 100644 --- a/internal/handler/msg_hello.go +++ b/internal/handler/msg_hello.go @@ -16,10 +16,14 @@ package handler import ( "context" + "errors" + "strings" "time" "github.com/FerretDB/FerretDB/internal/handler/common" + "github.com/FerretDB/FerretDB/internal/handler/handlererrors" "github.com/FerretDB/FerretDB/internal/types" + "github.com/FerretDB/FerretDB/internal/util/iterator" "github.com/FerretDB/FerretDB/internal/util/lazyerrors" "github.com/FerretDB/FerretDB/internal/util/must" "github.com/FerretDB/FerretDB/internal/wire" @@ -36,21 +40,115 @@ func (h *Handler) MsgHello(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, e return nil, lazyerrors.Error(err) } + saslSupportedMechs, err := common.GetOptionalParam(doc, "saslSupportedMechs", "") + if err != nil { + return nil, lazyerrors.Error(err) + } + + var saslSupportedMechsResp *types.Array + + if saslSupportedMechs != "" { + db, username, ok := strings.Cut(saslSupportedMechs, ".") + if !ok { + return nil, handlererrors.NewCommandErrorMsg( + handlererrors.ErrBadValue, + "UserName must contain a '.' separated database.user pair", + ) + } + // If username is empty, MongoDB doesn't send back a saslSupportedMechs property but + // still sends the response normally. + if username != "" { + mechs, err := h.getUserSupportedMechs(ctx, db, username) + if err != nil { + return nil, lazyerrors.Error(err) + } + + if len(mechs) > 0 { + saslSupportedMechsResp = must.NotFail(types.NewArray()) + for _, k := range mechs { + saslSupportedMechsResp.Append(k) + } + } + } + } + + resp := must.NotFail(types.NewDocument( + "isWritablePrimary", true, + "maxBsonObjectSize", int32(types.MaxDocumentLen), + "maxMessageSizeBytes", int32(wire.MaxMsgLen), + "maxWriteBatchSize", int32(100000), + "localTime", time.Now(), + "connectionId", int32(42), + "minWireVersion", common.MinWireVersion, + "maxWireVersion", common.MaxWireVersion, + "readOnly", false, + )) + + if saslSupportedMechsResp != nil { + resp.Set("saslSupportedMechs", saslSupportedMechsResp) + } + + resp.Set("ok", float64(1)) + var reply wire.OpMsg - must.NoError(reply.SetSections(wire.MakeOpMsgSection( - must.NotFail(types.NewDocument( - "isWritablePrimary", true, - "maxBsonObjectSize", int32(types.MaxDocumentLen), - "maxMessageSizeBytes", int32(wire.MaxMsgLen), - "maxWriteBatchSize", int32(100000), - "localTime", time.Now(), - "connectionId", int32(42), - "minWireVersion", common.MinWireVersion, - "maxWireVersion", common.MaxWireVersion, - "readOnly", false, - "ok", float64(1), - )), - ))) + must.NoError(reply.SetSections(wire.MakeOpMsgSection(resp))) return &reply, nil } + +// getUserSupportedMechs for a given user. +func (h *Handler) getUserSupportedMechs(ctx context.Context, db, username string) ([]string, error) { + adminDB, err := h.b.Database("admin") + if err != nil { + return nil, lazyerrors.Error(err) + } + + usersCol, err := adminDB.Collection("system.users") + if err != nil { + return nil, lazyerrors.Error(err) + } + + filter, err := usersInfoFilter(false, true, db, []usersInfoPair{ + {username: username, db: db}, + }) + if err != nil { + return nil, lazyerrors.Error(err) + } + + // Filter isn't being passed to the query as we are filtering after retrieving all data + // from the database due to limitations of the internal/backends filters. + qr, err := usersCol.Query(ctx, nil) + if err != nil { + return nil, lazyerrors.Error(err) + } + + defer qr.Iter.Close() + + for { + _, v, err := qr.Iter.Next() + + if errors.Is(err, iterator.ErrIteratorDone) { + break + } + + if err != nil { + return nil, lazyerrors.Error(err) + } + + matches, err := common.FilterDocument(v, filter) + if err != nil { + return nil, lazyerrors.Error(err) + } + + if !matches { + continue + } + + if v.Has("credentials") { + credentials := must.NotFail(v.Get("credentials")).(*types.Document) + return credentials.Keys(), nil + } + } + + return nil, nil +} From 2208a62f9b6d68bbf6eedee4a403b51ee0a4c301 Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Fri, 23 Feb 2024 09:23:29 +0100 Subject: [PATCH 03/27] failure example --- integration/hello_command_test.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 integration/hello_command_test.go diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go new file mode 100644 index 000000000000..91a24ff27ce9 --- /dev/null +++ b/integration/hello_command_test.go @@ -0,0 +1,24 @@ +package integration + +import ( + "testing" + + "go.mongodb.org/mongo-driver/bson" + + "github.com/FerretDB/FerretDB/integration/setup" + "github.com/FerretDB/FerretDB/integration/shareddata" + "github.com/stretchr/testify/require" +) + +func TestHelloFail(t *testing.T) { + t.Parallel() + + ctx, collection := setup.Setup(t, shareddata.Scalars, shareddata.Composites) + db := collection.Database() + + payload := ConvertDocument(t, bson.D{ + {"hello", "1"}, + }) + + require.NoError(t, db.RunCommand(ctx, payload).Err()) +} From b35ac423423b5ffdb874a23d220030ee9fbbf2b1 Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Mon, 26 Feb 2024 09:37:28 +0100 Subject: [PATCH 04/27] wip --- integration/hello_command_test.go | 143 +++++++++++++++++++++++++++++- internal/handler/msg_hello.go | 2 +- 2 files changed, 140 insertions(+), 5 deletions(-) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index 91a24ff27ce9..8b37a9eb376f 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -3,22 +3,157 @@ package integration import ( "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.mongodb.org/mongo-driver/bson" "github.com/FerretDB/FerretDB/integration/setup" "github.com/FerretDB/FerretDB/integration/shareddata" - "github.com/stretchr/testify/require" + "github.com/FerretDB/FerretDB/internal/types" + "github.com/FerretDB/FerretDB/internal/util/must" ) -func TestHelloFail(t *testing.T) { +func TestHello(t *testing.T) { t.Parallel() ctx, collection := setup.Setup(t, shareddata.Scalars, shareddata.Composites) db := collection.Database() - payload := ConvertDocument(t, bson.D{ + var res bson.D + + require.NoError(t, db.RunCommand(ctx, bson.D{ {"hello", "1"}, + }).Decode(&res)) + + actual := ConvertDocument(t, res) + + assert.Equal(t, actual.Keys(), []string{ + "isWritablePrimary", + "maxBsonObjectSize", + "maxMessageSizeBytes", + "maxWriteBatchSize", + "localTime", + "connectionId", + "minWireVersion", + "maxWireVersion", + "readOnly", + "ok", }) +} + +func TestHelloWithSupportedMechs(t *testing.T) { + t.Parallel() + + ctx, collection := setup.Setup(t, shareddata.Scalars, shareddata.Composites) + db := collection.Database() + + usersPayload := []bson.D{ + { + {"createUser", "hello_user"}, + {"roles", bson.A{}}, + {"pwd", "hello_password"}, + }, + { + {"createUser", "hello_user_plain"}, + {"roles", bson.A{}}, + {"pwd", "hello_password"}, + {"mechanisms", bson.A{"PLAIN"}}, + }, + { + {"createUser", "hello_user_scram1"}, + {"roles", bson.A{}}, + {"pwd", "hello_password"}, + {"mechanisms", bson.A{"SCRAM-SHA-1"}}, + }, + { + {"createUser", "hello_user_scram256"}, + {"roles", bson.A{}}, + {"pwd", "hello_password"}, + {"mechanisms", bson.A{"SCRAM-SHA-256"}}, + }, + } + + for _, u := range usersPayload { + require.NoError(t, db.RunCommand(ctx, u).Err()) + } + + testCases := []struct { + username string + db string + mechs *types.Array + err bool + }{ + { + username: "not_found", + db: db.Name(), + }, + { + username: "another_db", + db: db.Name() + "_not_found", + }, + { + username: "hello_user", + db: db.Name(), + mechs: must.NotFail(types.NewArray("SCRAM-SHA-1", "SCRAM-SHA-256")), + }, + { + username: "hello_user_plain", + db: db.Name(), + mechs: must.NotFail(types.NewArray("PLAIN")), + }, + { + username: "hello_user_scram1", + db: db.Name(), + mechs: must.NotFail(types.NewArray("SCRAM-SHA-1")), + }, + { + username: "hello_user_scram256", + db: db.Name(), + mechs: must.NotFail(types.NewArray("SCRAM-SHA-256")), + }, + } + + for _, tc := range testCases { + tc := tc + + t.Run(tc.username, func(t *testing.T) { + t.Parallel() + + var res bson.D + + err := db.RunCommand(ctx, bson.D{ + {"hello", "1"}, + {"saslSupportedMechs", tc.db + "." + tc.username}, + }).Decode(&res) + + if tc.err { + require.Error(t, err) + } + + actual := ConvertDocument(t, res) + + keys := []string{ + "isWritablePrimary", + "maxBsonObjectSize", + "maxMessageSizeBytes", + "maxWriteBatchSize", + "localTime", + "connectionId", + "minWireVersion", + "maxWireVersion", + "readOnly", + } + + if tc.mechs != nil { + keys = append(keys, "saslSupportedMechs") + mechanisms := must.NotFail(actual.Get("saslSupportedMechs")) + assert.Equal(t, tc.mechs, mechanisms) + } else { + assert.False(t, actual.Has("saslSupportedMechs")) + } - require.NoError(t, db.RunCommand(ctx, payload).Err()) + keys = append(keys, "ok") + assert.Equal(t, keys, actual.Keys()) + }) + } } diff --git a/internal/handler/msg_hello.go b/internal/handler/msg_hello.go index 68efa24c54f3..1421bf6fafdd 100644 --- a/internal/handler/msg_hello.go +++ b/internal/handler/msg_hello.go @@ -108,7 +108,7 @@ func (h *Handler) getUserSupportedMechs(ctx context.Context, db, username string return nil, lazyerrors.Error(err) } - filter, err := usersInfoFilter(false, true, db, []usersInfoPair{ + filter, err := usersInfoFilter(false, false, db, []usersInfoPair{ {username: username, db: db}, }) if err != nil { From 136ecf95067e8190a6fdc61701810d0e9cd2d04d Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Mon, 26 Feb 2024 09:58:20 +0100 Subject: [PATCH 05/27] wip --- integration/hello_command_test.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index 8b37a9eb376f..3c75784e373f 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.mongodb.org/mongo-driver/bson" + "go.mongodb.org/mongo-driver/bson/primitive" "github.com/FerretDB/FerretDB/integration/setup" "github.com/FerretDB/FerretDB/integration/shareddata" @@ -53,12 +54,6 @@ func TestHelloWithSupportedMechs(t *testing.T) { {"roles", bson.A{}}, {"pwd", "hello_password"}, }, - { - {"createUser", "hello_user_plain"}, - {"roles", bson.A{}}, - {"pwd", "hello_password"}, - {"mechanisms", bson.A{"PLAIN"}}, - }, { {"createUser", "hello_user_scram1"}, {"roles", bson.A{}}, @@ -73,6 +68,15 @@ func TestHelloWithSupportedMechs(t *testing.T) { }, } + if !setup.IsMongoDB(t) { + usersPayload = append(usersPayload, primitive.D{ + {"createUser", "hello_user_plain"}, + {"roles", bson.A{}}, + {"pwd", "hello_password"}, + {"mechanisms", bson.A{"PLAIN"}}, + }) + } + for _, u := range usersPayload { require.NoError(t, db.RunCommand(ctx, u).Err()) } @@ -121,6 +125,10 @@ func TestHelloWithSupportedMechs(t *testing.T) { var res bson.D + if tc.mechs != nil && tc.mechs.Contains("PLAIN") { + setup.SkipForMongoDB(t, "PLAIN authentication mechanism is not support by MongoDB") + } + err := db.RunCommand(ctx, bson.D{ {"hello", "1"}, {"saslSupportedMechs", tc.db + "." + tc.username}, From 196356183fff7bbd20bf75f480bbe610826c473b Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Mon, 26 Feb 2024 10:11:30 +0100 Subject: [PATCH 06/27] wip --- integration/hello_command_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index 3c75784e373f..2eb8084e4526 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -161,7 +161,11 @@ func TestHelloWithSupportedMechs(t *testing.T) { } keys = append(keys, "ok") - assert.Equal(t, keys, actual.Keys()) + if setup.IsMongoDB(t) { + assert.Contains(t, keys, actual.Keys()) + } else { + assert.Equal(t, keys, actual.Keys()) + } }) } } From 5e3e377ab5c1fa632c5219959b590ffe409712cd Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Mon, 26 Feb 2024 10:16:43 +0100 Subject: [PATCH 07/27] wip --- integration/hello_command_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index 2eb8084e4526..1f25d6ca3454 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -1,3 +1,17 @@ +// Copyright 2021 FerretDB Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package integration import ( From 46bcdad0d5ed1eeec4b8e676e8a2b5c96398643c Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Mon, 26 Feb 2024 10:25:32 +0100 Subject: [PATCH 08/27] wip --- integration/hello_command_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index 1f25d6ca3454..2d55f994d61c 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -42,7 +42,7 @@ func TestHello(t *testing.T) { actual := ConvertDocument(t, res) - assert.Equal(t, actual.Keys(), []string{ + assert.Contains(t, actual.Keys(), []string{ "isWritablePrimary", "maxBsonObjectSize", "maxMessageSizeBytes", @@ -95,7 +95,7 @@ func TestHelloWithSupportedMechs(t *testing.T) { require.NoError(t, db.RunCommand(ctx, u).Err()) } - testCases := []struct { + testCases := []struct { //nolint:vet // used for test only username string db string mechs *types.Array From a0e504f8f394f21de83d2289e46248115be967bf Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Mon, 26 Feb 2024 15:13:09 +0100 Subject: [PATCH 09/27] wip --- integration/hello_command_test.go | 34 ++++++++++++++++--------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index 2d55f994d61c..48a84f0f66e4 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -42,18 +42,20 @@ func TestHello(t *testing.T) { actual := ConvertDocument(t, res) - assert.Contains(t, actual.Keys(), []string{ - "isWritablePrimary", - "maxBsonObjectSize", - "maxMessageSizeBytes", - "maxWriteBatchSize", - "localTime", - "connectionId", - "minWireVersion", - "maxWireVersion", - "readOnly", - "ok", - }) + if !setup.IsMongoDB(t) { + assert.Equal(t, actual.Keys(), []string{ + "isWritablePrimary", + "maxBsonObjectSize", + "maxMessageSizeBytes", + "maxWriteBatchSize", + "localTime", + "connectionId", + "minWireVersion", + "maxWireVersion", + "readOnly", + "ok", + }) + } } func TestHelloWithSupportedMechs(t *testing.T) { @@ -169,15 +171,15 @@ func TestHelloWithSupportedMechs(t *testing.T) { if tc.mechs != nil { keys = append(keys, "saslSupportedMechs") mechanisms := must.NotFail(actual.Get("saslSupportedMechs")) - assert.Equal(t, tc.mechs, mechanisms) + if !setup.IsMongoDB(t) { + assert.Equal(t, tc.mechs, mechanisms) + } } else { assert.False(t, actual.Has("saslSupportedMechs")) } keys = append(keys, "ok") - if setup.IsMongoDB(t) { - assert.Contains(t, keys, actual.Keys()) - } else { + if !setup.IsMongoDB(t) { assert.Equal(t, keys, actual.Keys()) } }) From 01282a9cfaea690ecd413b758ab78d6dec9d1c7d Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Mon, 26 Feb 2024 15:58:53 +0100 Subject: [PATCH 10/27] wip --- integration/hello_command_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index 48a84f0f66e4..ecceb60140f6 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -171,6 +171,7 @@ func TestHelloWithSupportedMechs(t *testing.T) { if tc.mechs != nil { keys = append(keys, "saslSupportedMechs") mechanisms := must.NotFail(actual.Get("saslSupportedMechs")) + if !setup.IsMongoDB(t) { assert.Equal(t, tc.mechs, mechanisms) } @@ -179,6 +180,7 @@ func TestHelloWithSupportedMechs(t *testing.T) { } keys = append(keys, "ok") + if !setup.IsMongoDB(t) { assert.Equal(t, keys, actual.Keys()) } From 2c834b479d1bbd07bb6e36572ca6508725319397 Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Mon, 26 Feb 2024 16:30:27 +0100 Subject: [PATCH 11/27] wip --- integration/commands_administration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/commands_administration_test.go b/integration/commands_administration_test.go index db4b9245e849..85c14a9c1111 100644 --- a/integration/commands_administration_test.go +++ b/integration/commands_administration_test.go @@ -858,7 +858,7 @@ func TestGetParameterCommandAuthenticationMechanisms(t *testing.T) { require.NoError(t, err) expected := bson.D{ - {"authenticationMechanisms", bson.A{"PLAIN"}}, + {"authenticationMechanisms", bson.A{"SCRAM-SHA-1", "SCRAM-SHA-256", "PLAIN"}}, {"ok", float64(1)}, } require.Equal(t, expected, res) From 78b41d70f4f74c004b916a38f6efde13270a5bf1 Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Tue, 27 Feb 2024 12:55:44 +0100 Subject: [PATCH 12/27] wip --- integration/hello_command_test.go | 40 +++++++++++++++++++++---------- internal/handler/msg_usersinfo.go | 8 +++---- 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index ecceb60140f6..b7c9c0932a2a 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -42,19 +42,27 @@ func TestHello(t *testing.T) { actual := ConvertDocument(t, res) + keys := []string{ + "isWritablePrimary", + "maxBsonObjectSize", + "maxMessageSizeBytes", + "maxWriteBatchSize", + "localTime", + "connectionId", + "minWireVersion", + "maxWireVersion", + "readOnly", + "ok", + } + if !setup.IsMongoDB(t) { - assert.Equal(t, actual.Keys(), []string{ - "isWritablePrimary", - "maxBsonObjectSize", - "maxMessageSizeBytes", - "maxWriteBatchSize", - "localTime", - "connectionId", - "minWireVersion", - "maxWireVersion", - "readOnly", - "ok", - }) + assert.Equal(t, actual.Keys(), keys) + } else { + // MongoDB have additional keys, + // so just check if the keys exists on it. + for _, wantKey := range keys { + assert.Contains(t, actual.Keys(), wantKey) + } } } @@ -182,7 +190,13 @@ func TestHelloWithSupportedMechs(t *testing.T) { keys = append(keys, "ok") if !setup.IsMongoDB(t) { - assert.Equal(t, keys, actual.Keys()) + assert.Equal(t, actual.Keys(), keys) + } else { + // MongoDB have additional keys, + // so just check if the keys exists on it. + for _, wantKey := range keys { + assert.Contains(t, actual.Keys(), wantKey) + } } }) } diff --git a/internal/handler/msg_usersinfo.go b/internal/handler/msg_usersinfo.go index fdd01323831d..a49b1f55def2 100644 --- a/internal/handler/msg_usersinfo.go +++ b/internal/handler/msg_usersinfo.go @@ -168,10 +168,6 @@ func (h *Handler) MsgUsersInfo(ctx context.Context, msg *wire.OpMsg) (*wire.OpMs return nil, lazyerrors.Error(err) } - if matches { - res.Append(v) - } - if v.Has("credentials") { credentials := must.NotFail(v.Get("credentials")).(*types.Document) if credentialsKeys := credentials.Keys(); len(credentialsKeys) > 0 { @@ -187,6 +183,10 @@ func (h *Handler) MsgUsersInfo(ctx context.Context, msg *wire.OpMsg) (*wire.OpMs if !showCredentials { v.Remove("credentials") } + + if matches { + res.Append(v) + } } var reply wire.OpMsg From 5a77a3dca7491e75464ca208b0f00a264b3a1d49 Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Tue, 27 Feb 2024 16:46:25 +0100 Subject: [PATCH 13/27] wip --- integration/hello_command_test.go | 69 +++++++++++-------------------- 1 file changed, 23 insertions(+), 46 deletions(-) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index b7c9c0932a2a..7b380695e9df 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -16,6 +16,7 @@ package integration import ( "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -42,28 +43,17 @@ func TestHello(t *testing.T) { actual := ConvertDocument(t, res) - keys := []string{ - "isWritablePrimary", - "maxBsonObjectSize", - "maxMessageSizeBytes", - "maxWriteBatchSize", - "localTime", - "connectionId", - "minWireVersion", - "maxWireVersion", - "readOnly", - "ok", - } - - if !setup.IsMongoDB(t) { - assert.Equal(t, actual.Keys(), keys) - } else { - // MongoDB have additional keys, - // so just check if the keys exists on it. - for _, wantKey := range keys { - assert.Contains(t, actual.Keys(), wantKey) - } - } + assert.Equal(t, must.NotFail(actual.Get("isWritablePrimary")), true) + assert.Equal(t, must.NotFail(actual.Get("maxBsonObjectSize")), int32(16777216)) + assert.Equal(t, must.NotFail(actual.Get("maxMessageSizeBytes")), int32(48000000)) + assert.Equal(t, must.NotFail(actual.Get("maxMessageSizeBytes")), int32(48000000)) + assert.Equal(t, must.NotFail(actual.Get("maxWriteBatchSize")), int32(100000)) + assert.IsType(t, must.NotFail(actual.Get("localTime")), time.Time{}) + assert.IsType(t, must.NotFail(actual.Get("connectionId")), int32(1)) + assert.Equal(t, must.NotFail(actual.Get("minWireVersion")), int32(0)) + assert.Equal(t, must.NotFail(actual.Get("maxWireVersion")), int32(21)) + assert.Equal(t, must.NotFail(actual.Get("readOnly")), false) + assert.Equal(t, must.NotFail(actual.Get("ok")), float64(1)) } func TestHelloWithSupportedMechs(t *testing.T) { @@ -164,20 +154,7 @@ func TestHelloWithSupportedMechs(t *testing.T) { actual := ConvertDocument(t, res) - keys := []string{ - "isWritablePrimary", - "maxBsonObjectSize", - "maxMessageSizeBytes", - "maxWriteBatchSize", - "localTime", - "connectionId", - "minWireVersion", - "maxWireVersion", - "readOnly", - } - if tc.mechs != nil { - keys = append(keys, "saslSupportedMechs") mechanisms := must.NotFail(actual.Get("saslSupportedMechs")) if !setup.IsMongoDB(t) { @@ -187,17 +164,17 @@ func TestHelloWithSupportedMechs(t *testing.T) { assert.False(t, actual.Has("saslSupportedMechs")) } - keys = append(keys, "ok") - - if !setup.IsMongoDB(t) { - assert.Equal(t, actual.Keys(), keys) - } else { - // MongoDB have additional keys, - // so just check if the keys exists on it. - for _, wantKey := range keys { - assert.Contains(t, actual.Keys(), wantKey) - } - } + assert.Equal(t, must.NotFail(actual.Get("isWritablePrimary")), true) + assert.Equal(t, must.NotFail(actual.Get("maxBsonObjectSize")), int32(16777216)) + assert.Equal(t, must.NotFail(actual.Get("maxMessageSizeBytes")), int32(48000000)) + assert.Equal(t, must.NotFail(actual.Get("maxMessageSizeBytes")), int32(48000000)) + assert.Equal(t, must.NotFail(actual.Get("maxWriteBatchSize")), int32(100000)) + assert.IsType(t, must.NotFail(actual.Get("localTime")), time.Time{}) + assert.IsType(t, must.NotFail(actual.Get("connectionId")), int32(1)) + assert.Equal(t, must.NotFail(actual.Get("minWireVersion")), int32(0)) + assert.Equal(t, must.NotFail(actual.Get("maxWireVersion")), int32(21)) + assert.Equal(t, must.NotFail(actual.Get("readOnly")), false) + assert.Equal(t, must.NotFail(actual.Get("ok")), float64(1)) }) } } From 76efeee0d32fd71f5d4e4652e2220c9d3b604140 Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Tue, 27 Feb 2024 19:13:06 +0100 Subject: [PATCH 14/27] wip --- internal/handler/msg_hello.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/internal/handler/msg_hello.go b/internal/handler/msg_hello.go index 1421bf6fafdd..eb90d6cbc272 100644 --- a/internal/handler/msg_hello.go +++ b/internal/handler/msg_hello.go @@ -55,8 +55,7 @@ func (h *Handler) MsgHello(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, e "UserName must contain a '.' separated database.user pair", ) } - // If username is empty, MongoDB doesn't send back a saslSupportedMechs property but - // still sends the response normally. + if username != "" { mechs, err := h.getUserSupportedMechs(ctx, db, username) if err != nil { @@ -115,8 +114,6 @@ func (h *Handler) getUserSupportedMechs(ctx context.Context, db, username string return nil, lazyerrors.Error(err) } - // Filter isn't being passed to the query as we are filtering after retrieving all data - // from the database due to limitations of the internal/backends filters. qr, err := usersCol.Query(ctx, nil) if err != nil { return nil, lazyerrors.Error(err) From 830386df212cb21b7e778b20b4e4c67ad6af42c4 Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Wed, 28 Feb 2024 02:31:58 +0100 Subject: [PATCH 15/27] wip --- integration/hello_command_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index 7b380695e9df..63a894774e1b 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -156,10 +156,7 @@ func TestHelloWithSupportedMechs(t *testing.T) { if tc.mechs != nil { mechanisms := must.NotFail(actual.Get("saslSupportedMechs")) - - if !setup.IsMongoDB(t) { - assert.Equal(t, tc.mechs, mechanisms) - } + assert.Equal(t, tc.mechs, mechanisms) } else { assert.False(t, actual.Has("saslSupportedMechs")) } From 2736c4a8d6a281dbf897d9be26525633109f311d Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Wed, 28 Feb 2024 02:46:29 +0100 Subject: [PATCH 16/27] wip --- integration/hello_command_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index 63a894774e1b..f06d54d6ef65 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -156,7 +156,7 @@ func TestHelloWithSupportedMechs(t *testing.T) { if tc.mechs != nil { mechanisms := must.NotFail(actual.Get("saslSupportedMechs")) - assert.Equal(t, tc.mechs, mechanisms) + assert.EqualValues(t, tc.mechs, mechanisms) } else { assert.False(t, actual.Has("saslSupportedMechs")) } From e769c9099c5fea779b086ae0bbf62e636e51be0b Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Wed, 28 Feb 2024 02:57:33 +0100 Subject: [PATCH 17/27] wip --- integration/hello_command_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index f06d54d6ef65..4afb83282371 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -156,7 +156,7 @@ func TestHelloWithSupportedMechs(t *testing.T) { if tc.mechs != nil { mechanisms := must.NotFail(actual.Get("saslSupportedMechs")) - assert.EqualValues(t, tc.mechs, mechanisms) + assert.ElementsMatch(t, tc.mechs, mechanisms) } else { assert.False(t, actual.Has("saslSupportedMechs")) } From 87bcf668bc0bbdb9835b22640216e05f994695f8 Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Wed, 28 Feb 2024 03:14:00 +0100 Subject: [PATCH 18/27] wip --- integration/hello_command_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index 4afb83282371..ae44645f890c 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -156,7 +156,7 @@ func TestHelloWithSupportedMechs(t *testing.T) { if tc.mechs != nil { mechanisms := must.NotFail(actual.Get("saslSupportedMechs")) - assert.ElementsMatch(t, tc.mechs, mechanisms) + assert.True(t, mechanisms.(*types.Array).ContainsAll(tc.mechs)) } else { assert.False(t, actual.Has("saslSupportedMechs")) } From 92ebe16f6a50b0f2d00157f2b260c7ca91a0939b Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Sun, 3 Mar 2024 01:28:45 +0100 Subject: [PATCH 19/27] wip --- integration/hello_command_test.go | 68 +++++----- integration/users/usersinfo_test.go | 129 ++++++++++++++----- website/docs/reference/supported-commands.md | 2 +- 3 files changed, 136 insertions(+), 63 deletions(-) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index ae44645f890c..5f33dfdcc34a 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -95,46 +95,47 @@ func TestHelloWithSupportedMechs(t *testing.T) { require.NoError(t, db.RunCommand(ctx, u).Err()) } - testCases := []struct { //nolint:vet // used for test only - username string - db string - mechs *types.Array - err bool + testCases := map[string]struct { //nolint:vet // used for test only + user string + mechs *types.Array + err string }{ - { - username: "not_found", - db: db.Name(), + "NotFound": { + user: db.Name() + ".not_found", }, - { - username: "another_db", - db: db.Name() + "_not_found", + "AnotherDB": { + user: db.Name() + "_not_found.another_db", }, - { - username: "hello_user", - db: db.Name(), - mechs: must.NotFail(types.NewArray("SCRAM-SHA-1", "SCRAM-SHA-256")), + "HelloUser": { + user: db.Name() + ".hello_user", + mechs: must.NotFail(types.NewArray("SCRAM-SHA-1", "SCRAM-SHA-256")), }, - { - username: "hello_user_plain", - db: db.Name(), - mechs: must.NotFail(types.NewArray("PLAIN")), + "HelloUserPlain": { + user: db.Name() + ".hello_user_plain", + mechs: must.NotFail(types.NewArray("PLAIN")), }, - { - username: "hello_user_scram1", - db: db.Name(), - mechs: must.NotFail(types.NewArray("SCRAM-SHA-1")), + "HelloUserSCRAM1": { + user: db.Name() + ".hello_user_scram1", + mechs: must.NotFail(types.NewArray("SCRAM-SHA-1")), }, - { - username: "hello_user_scram256", - db: db.Name(), - mechs: must.NotFail(types.NewArray("SCRAM-SHA-256")), + "HelloUserSCRAM256": { + user: db.Name() + ".hello_user_scram256", + mechs: must.NotFail(types.NewArray("SCRAM-SHA-256")), + }, + "EmptyUsername": { + user: db.Name() + ".", + mechs: nil, + }, + "MissingSeparator": { + user: db.Name(), + err: "UserName must contain a '.' separated database.user pair", }, } - for _, tc := range testCases { - tc := tc + for name, tc := range testCases { + tc, name := tc, name - t.Run(tc.username, func(t *testing.T) { + t.Run(name, func(t *testing.T) { t.Parallel() var res bson.D @@ -145,11 +146,12 @@ func TestHelloWithSupportedMechs(t *testing.T) { err := db.RunCommand(ctx, bson.D{ {"hello", "1"}, - {"saslSupportedMechs", tc.db + "." + tc.username}, + {"saslSupportedMechs", tc.user}, }).Decode(&res) - if tc.err { - require.Error(t, err) + if tc.err != "" { + require.ErrorContains(t, err, tc.err) + return } actual := ConvertDocument(t, res) diff --git a/integration/users/usersinfo_test.go b/integration/users/usersinfo_test.go index e41b1f8c8ce0..fbd426c81f39 100644 --- a/integration/users/usersinfo_test.go +++ b/integration/users/usersinfo_test.go @@ -87,6 +87,28 @@ func TestUsersinfo(t *testing.T) { }, }, }, + { + dbSuffix: "allbackends", + payloads: []bson.D{ + { + {"createUser", "WithSCRAMSHA1"}, + {"roles", bson.A{}}, + {"pwd", "pwd1"}, + {"mechanisms", bson.A{"SCRAM-SHA-1"}}, + }, + }, + }, + { + dbSuffix: "allbackends", + payloads: []bson.D{ + { + {"createUser", "WithSCRAMSHA256"}, + {"roles", bson.A{}}, + {"pwd", "pwd1"}, + {"mechanisms", bson.A{"SCRAM-SHA-256"}}, + }, + }, + }, } dbPrefix := testutil.DatabaseName(t) @@ -117,13 +139,15 @@ func TestUsersinfo(t *testing.T) { } testCases := map[string]struct { //nolint:vet // for readability - dbSuffix string - payload bson.D - err *mongo.CommandError - altMessage string - expected bson.D - hasUser map[string]struct{} - skipForMongoDB string // optional, skip test for MongoDB backend with a specific reason + dbSuffix string + payload bson.D + err *mongo.CommandError + altMessage string + expected bson.D + hasUser map[string]struct{} + + showCredentials []string // showCredentials list the credentials types expected to be returned + skipForMongoDB string // optional, skip test for MongoDB backend with a specific reason }{ "NoUserFound": { dbSuffix: "no_users", @@ -182,6 +206,7 @@ func TestUsersinfo(t *testing.T) { {"usersInfo", "WithPLAIN"}, {"showCredentials", true}, }, + showCredentials: []string{"PLAIN"}, expected: bson.D{ {"users", bson.A{ bson.D{ @@ -195,6 +220,44 @@ func TestUsersinfo(t *testing.T) { }, skipForMongoDB: "Only MongoDB Enterprise offers PLAIN", }, + "WithSCRAMSHA1": { + dbSuffix: "allbackends", + payload: bson.D{ + {"usersInfo", "WithSCRAMSHA1"}, + {"showCredentials", true}, + }, + showCredentials: []string{"SCRAM-SHA-1"}, + expected: bson.D{ + {"users", bson.A{ + bson.D{ + {"_id", "TestUsersinfo.WithSCRAMSHA1"}, + {"user", "scramsha1"}, + {"db", "TestUsersinfo"}, + {"roles", bson.A{}}, + }, + }}, + {"ok", float64(1)}, + }, + }, + "WithSCRAMSHA256": { + dbSuffix: "allbackends", + payload: bson.D{ + {"usersInfo", "WithSCRAMSHA256"}, + {"showCredentials", true}, + }, + showCredentials: []string{"SCRAM-SHA-256"}, + expected: bson.D{ + {"users", bson.A{ + bson.D{ + {"_id", "TestUsersinfo.WithSCRAMSHA256"}, + {"user", "scramsha256"}, + {"db", "TestUsersinfo"}, + {"roles", bson.A{}}, + }, + }}, + {"ok", float64(1)}, + }, + }, "FromSameDatabase": { dbSuffix: "_example", payload: bson.D{{ @@ -513,35 +576,27 @@ func TestUsersinfo(t *testing.T) { require.True(t, (tc.hasUser == nil) != (tc.expected == nil)) payload := integration.ConvertDocument(t, tc.payload) - var showCredentials bool - if payload.Has("showCredentials") { - showCredentials = must.NotFail(payload.Get("showCredentials")).(bool) - } - if showCredentials { - if !setup.IsMongoDB(t) { - cred, ok := actualUser.Get("credentials") - assert.Nil(t, ok, "credentials not found") - assertPlainCredentials(t, "PLAIN", cred.(*types.Document)) + if tc.showCredentials != nil { + cred, ok := actualUser.Get("credentials") + assert.Nil(t, ok, "credentials not found") + + for _, typ := range tc.showCredentials { + switch typ { + case "PLAIN": + assertPlainCredentials(t, "PLAIN", cred.(*types.Document)) + case "SCRAM-SHA-1": + assertSCRAMSHA1Credentials(t, "SCRAM-SHA-1", cred.(*types.Document)) + case "SCRAM-SHA-256": + assertSCRAMSHA256Credentials(t, "SCRAM-SHA-256", cred.(*types.Document)) + } } } else { assert.False(t, actualUser.Has("credentials")) } if payload.Has("mechanisms") { - payloadMechanisms := must.NotFail(payload.Get("mechanisms")).(*types.Array) - - if payloadMechanisms.Contains("PLAIN") { - assertPlainCredentials(t, "PLAIN", must.NotFail(actualUser.Get("credentials")).(*types.Document)) - } - - if payloadMechanisms.Contains("SCRAM-SHA-1") { - assertSCRAMSHA1Credentials(t, "SCRAM-SHA-1", must.NotFail(actualUser.Get("credentials")).(*types.Document)) - } - - if payloadMechanisms.Contains("SCRAM-SHA-256") { - assertSCRAMSHA256Credentials(t, "SCRAM-SHA-256", must.NotFail(actualUser.Get("credentials")).(*types.Document)) - } + assertUsersinfoMechanisms(t, payload, actualUser) } foundUsers[must.NotFail(actualUser.Get("_id")).(string)] = struct{}{} @@ -568,3 +623,19 @@ func TestUsersinfo(t *testing.T) { }) } } + +func assertUsersinfoMechanisms(t *testing.T, payload, user *types.Document) { + payloadMechanisms := must.NotFail(payload.Get("mechanisms")).(*types.Array) + + if payloadMechanisms.Contains("PLAIN") { + assertPlainCredentials(t, "PLAIN", must.NotFail(user.Get("credentials")).(*types.Document)) + } + + if payloadMechanisms.Contains("SCRAM-SHA-1") { + assertSCRAMSHA1Credentials(t, "SCRAM-SHA-1", must.NotFail(user.Get("credentials")).(*types.Document)) + } + + if payloadMechanisms.Contains("SCRAM-SHA-256") { + assertSCRAMSHA256Credentials(t, "SCRAM-SHA-256", must.NotFail(user.Get("credentials")).(*types.Document)) + } +} diff --git a/website/docs/reference/supported-commands.md b/website/docs/reference/supported-commands.md index 40d659de3e81..3cc2802758c6 100644 --- a/website/docs/reference/supported-commands.md +++ b/website/docs/reference/supported-commands.md @@ -205,7 +205,7 @@ Related [issue](https://github.com/FerretDB/FerretDB/issues/78). | | `digestPassword` | ⚠️ | | | | `comment` | ⚠️ | | | `usersInfo` | | ✅ | | -| | `showCredentials` | ⚠️ | | +| | `showCredentials` | ✅ | | | | `showCustomData` | ⚠️ | | | | `showPrivileges` | ⚠️ | | | | `showAuthenticationRestrictions` | ⚠️ | | From 0173295838f53737cee6e5e63f83e57f439a26dd Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Tue, 5 Mar 2024 20:27:45 +0100 Subject: [PATCH 20/27] Fix `saslContinue` crashing due to not found authentication conversation (#4129) --- integration/users/connection_test.go | 4 ++-- internal/handler/msg_saslcontinue.go | 9 ++++++++- internal/handler/msg_saslstart.go | 2 +- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/integration/users/connection_test.go b/integration/users/connection_test.go index a499ced063ad..af03b512a1df 100644 --- a/integration/users/connection_test.go +++ b/integration/users/connection_test.go @@ -101,7 +101,7 @@ func TestAuthentication(t *testing.T) { mechanisms: []string{"SCRAM-SHA-256"}, connectionMechanism: "SCRAM-SHA-256", userNotFound: true, - errorMessage: "Authentication failed", + errorMessage: "Authentication failed.", topologyError: true, }, "BadPassword": { @@ -111,7 +111,7 @@ func TestAuthentication(t *testing.T) { connectionMechanism: "SCRAM-SHA-256", wrongPassword: true, topologyError: true, - errorMessage: "Authentication failed", + errorMessage: "Authentication failed.", }, "MechanismNotSet": { username: "user_mechanism_not_set", diff --git a/internal/handler/msg_saslcontinue.go b/internal/handler/msg_saslcontinue.go index 672412770add..25024817d7c3 100644 --- a/internal/handler/msg_saslcontinue.go +++ b/internal/handler/msg_saslcontinue.go @@ -44,11 +44,18 @@ func (h *Handler) MsgSASLContinue(ctx context.Context, msg *wire.OpMsg) (*wire.O conv := conninfo.Get(ctx).Conv() + if conv == nil { + return nil, handlererrors.NewCommandErrorMsg( + handlererrors.ErrAuthenticationFailed, + "Authentication failed.", + ) + } + response, err := conv.Step(string(payload)) if err != nil { return nil, handlererrors.NewCommandErrorMsg( handlererrors.ErrAuthenticationFailed, - "Authentication failed", + "Authentication failed.", ) } diff --git a/internal/handler/msg_saslstart.go b/internal/handler/msg_saslstart.go index 965a60cae3de..7e1208f4a790 100644 --- a/internal/handler/msg_saslstart.go +++ b/internal/handler/msg_saslstart.go @@ -245,7 +245,7 @@ func (h *Handler) scramCredentialLookup(ctx context.Context, username, dbName, m return nil, handlererrors.NewCommandErrorMsg( handlererrors.ErrAuthenticationFailed, - "Authentication failed", + "Authentication failed.", ) } From efd071ba6a460738b95b5d409b120f0e941f63bc Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Tue, 5 Mar 2024 22:26:58 +0100 Subject: [PATCH 21/27] code review remarks --- integration/commands_administration_test.go | 1 - integration/create_test.go | 1 - integration/hello_command_test.go | 17 +++++----- integration/query_test.go | 1 - integration/setup/helpers.go | 2 +- integration/users/usersinfo_test.go | 34 ++++--------------- .../handler/handlerparams/typecode_test.go | 1 - 7 files changed, 15 insertions(+), 42 deletions(-) diff --git a/integration/commands_administration_test.go b/integration/commands_administration_test.go index 85c14a9c1111..b0ed592e1be5 100644 --- a/integration/commands_administration_test.go +++ b/integration/commands_administration_test.go @@ -252,7 +252,6 @@ func TestCommandsAdministrationListDatabases(t *testing.T) { } for name, tc := range testCases { - tc, name := tc, name t.Run(name, func(t *testing.T) { t.Parallel() diff --git a/integration/create_test.go b/integration/create_test.go index 2b2cc836ff96..47b2f70bda79 100644 --- a/integration/create_test.go +++ b/integration/create_test.go @@ -333,7 +333,6 @@ func TestCreateCappedCommandInvalidSpec(t *testing.T) { }, }, } { - tc, name := tc, name t.Run(name, func(t *testing.T) { t.Parallel() diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index 5f33dfdcc34a..0de9742d9795 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -133,8 +133,6 @@ func TestHelloWithSupportedMechs(t *testing.T) { } for name, tc := range testCases { - tc, name := tc, name - t.Run(name, func(t *testing.T) { t.Parallel() @@ -156,13 +154,6 @@ func TestHelloWithSupportedMechs(t *testing.T) { actual := ConvertDocument(t, res) - if tc.mechs != nil { - mechanisms := must.NotFail(actual.Get("saslSupportedMechs")) - assert.True(t, mechanisms.(*types.Array).ContainsAll(tc.mechs)) - } else { - assert.False(t, actual.Has("saslSupportedMechs")) - } - assert.Equal(t, must.NotFail(actual.Get("isWritablePrimary")), true) assert.Equal(t, must.NotFail(actual.Get("maxBsonObjectSize")), int32(16777216)) assert.Equal(t, must.NotFail(actual.Get("maxMessageSizeBytes")), int32(48000000)) @@ -174,6 +165,14 @@ func TestHelloWithSupportedMechs(t *testing.T) { assert.Equal(t, must.NotFail(actual.Get("maxWireVersion")), int32(21)) assert.Equal(t, must.NotFail(actual.Get("readOnly")), false) assert.Equal(t, must.NotFail(actual.Get("ok")), float64(1)) + + if tc.mechs == nil { + assert.False(t, actual.Has("saslSupportedMechs")) + return + } + + mechanisms := must.NotFail(actual.Get("saslSupportedMechs")) + assert.True(t, mechanisms.(*types.Array).ContainsAll(tc.mechs)) }) } } diff --git a/integration/query_test.go b/integration/query_test.go index dcb83fc7d1fb..2c5c3e5487ce 100644 --- a/integration/query_test.go +++ b/integration/query_test.go @@ -728,7 +728,6 @@ func TestQueryCommandLimitPushDown(t *testing.T) { limitPushdown: noPushdown, }, } { - tc, name := tc, name t.Run(name, func(t *testing.T) { t.Parallel() diff --git a/integration/setup/helpers.go b/integration/setup/helpers.go index 07abd7ed8fc0..f6d3dc6bf3be 100644 --- a/integration/setup/helpers.go +++ b/integration/setup/helpers.go @@ -89,7 +89,7 @@ func FailsForMongoDB(tb testtb.TB, reason string) testtb.TB { // SkipForMongoDB skips the current test for MongoDB. // -// Use [FailsForMongoDB] in new code. +// Deprecated: Use [FailsForMongoDB] in new code. func SkipForMongoDB(tb testtb.TB, reason string) { tb.Helper() diff --git a/integration/users/usersinfo_test.go b/integration/users/usersinfo_test.go index fbd426c81f39..944cc1a7a766 100644 --- a/integration/users/usersinfo_test.go +++ b/integration/users/usersinfo_test.go @@ -532,7 +532,6 @@ func TestUsersinfo(t *testing.T) { } for name, tc := range testCases { - name, tc := name, tc t.Run(name, func(t *testing.T) { t.Parallel() @@ -575,28 +574,23 @@ func TestUsersinfo(t *testing.T) { require.True(t, (tc.hasUser == nil) != (tc.expected == nil)) - payload := integration.ConvertDocument(t, tc.payload) - if tc.showCredentials != nil { - cred, ok := actualUser.Get("credentials") - assert.Nil(t, ok, "credentials not found") + cred := must.NotFail(actualUser.Get("credentials")).(*types.Document) for _, typ := range tc.showCredentials { switch typ { case "PLAIN": - assertPlainCredentials(t, "PLAIN", cred.(*types.Document)) + assertPlainCredentials(t, "PLAIN", cred) case "SCRAM-SHA-1": - assertSCRAMSHA1Credentials(t, "SCRAM-SHA-1", cred.(*types.Document)) + assertSCRAMSHA1Credentials(t, "SCRAM-SHA-1", cred) case "SCRAM-SHA-256": - assertSCRAMSHA256Credentials(t, "SCRAM-SHA-256", cred.(*types.Document)) + assertSCRAMSHA256Credentials(t, "SCRAM-SHA-256", cred) } } - } else { - assert.False(t, actualUser.Has("credentials")) } - if payload.Has("mechanisms") { - assertUsersinfoMechanisms(t, payload, actualUser) + if len(tc.showCredentials) == 0 { + assert.False(t, actualUser.Has("credentials")) } foundUsers[must.NotFail(actualUser.Get("_id")).(string)] = struct{}{} @@ -623,19 +617,3 @@ func TestUsersinfo(t *testing.T) { }) } } - -func assertUsersinfoMechanisms(t *testing.T, payload, user *types.Document) { - payloadMechanisms := must.NotFail(payload.Get("mechanisms")).(*types.Array) - - if payloadMechanisms.Contains("PLAIN") { - assertPlainCredentials(t, "PLAIN", must.NotFail(user.Get("credentials")).(*types.Document)) - } - - if payloadMechanisms.Contains("SCRAM-SHA-1") { - assertSCRAMSHA1Credentials(t, "SCRAM-SHA-1", must.NotFail(user.Get("credentials")).(*types.Document)) - } - - if payloadMechanisms.Contains("SCRAM-SHA-256") { - assertSCRAMSHA256Credentials(t, "SCRAM-SHA-256", must.NotFail(user.Get("credentials")).(*types.Document)) - } -} diff --git a/internal/handler/handlerparams/typecode_test.go b/internal/handler/handlerparams/typecode_test.go index b57a60487cca..5fe5ec084223 100644 --- a/internal/handler/handlerparams/typecode_test.go +++ b/internal/handler/handlerparams/typecode_test.go @@ -51,7 +51,6 @@ func TestHasSameTypeElements(t *testing.T) { same: true, }, } { - tc, name := tc, name t.Run(name, func(t *testing.T) { t.Parallel() From fae330e5dace5e6debedab1cb9782268c688ddbf Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Tue, 5 Mar 2024 23:22:02 +0100 Subject: [PATCH 22/27] wip --- integration/hello_command_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index 0de9742d9795..fac38074734e 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -139,7 +139,7 @@ func TestHelloWithSupportedMechs(t *testing.T) { var res bson.D if tc.mechs != nil && tc.mechs.Contains("PLAIN") { - setup.SkipForMongoDB(t, "PLAIN authentication mechanism is not support by MongoDB") + setup.FailsForMongoDB(t, "PLAIN authentication mechanism is not support by MongoDB") } err := db.RunCommand(ctx, bson.D{ From 22a2be4dad3c512ea7d7e902e70688db2aa9cf4c Mon Sep 17 00:00:00 2001 From: Henrique Vicente Date: Wed, 6 Mar 2024 00:12:31 +0100 Subject: [PATCH 23/27] wip --- integration/hello_command_test.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index fac38074734e..2f13a84bf10c 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -27,6 +27,7 @@ import ( "github.com/FerretDB/FerretDB/integration/shareddata" "github.com/FerretDB/FerretDB/internal/types" "github.com/FerretDB/FerretDB/internal/util/must" + "github.com/FerretDB/FerretDB/internal/util/testutil/testtb" ) func TestHello(t *testing.T) { @@ -133,15 +134,17 @@ func TestHelloWithSupportedMechs(t *testing.T) { } for name, tc := range testCases { - t.Run(name, func(t *testing.T) { - t.Parallel() + t.Run(name, func(tt *testing.T) { + tt.Parallel() - var res bson.D + var t testtb.TB = tt if tc.mechs != nil && tc.mechs.Contains("PLAIN") { - setup.FailsForMongoDB(t, "PLAIN authentication mechanism is not support by MongoDB") + t = setup.FailsForMongoDB(t, "PLAIN authentication mechanism is not support by MongoDB") } + var res bson.D + err := db.RunCommand(ctx, bson.D{ {"hello", "1"}, {"saslSupportedMechs", tc.user}, From b930a1bc8f179bf700b2f19010cb89ab9128115e Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Wed, 6 Mar 2024 16:40:40 +0900 Subject: [PATCH 24/27] update test --- integration/hello_command_test.go | 24 ++++++++++++++++-------- integration/users/usersinfo_test.go | 20 +++++++++++++------- 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index 2f13a84bf10c..7dd44e52cfe3 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/require" "go.mongodb.org/mongo-driver/bson" "go.mongodb.org/mongo-driver/bson/primitive" + "go.mongodb.org/mongo-driver/mongo" "github.com/FerretDB/FerretDB/integration/setup" "github.com/FerretDB/FerretDB/integration/shareddata" @@ -99,7 +100,9 @@ func TestHelloWithSupportedMechs(t *testing.T) { testCases := map[string]struct { //nolint:vet // used for test only user string mechs *types.Array - err string + + err *mongo.CommandError + failsForMongoDB string }{ "NotFound": { user: db.Name() + ".not_found", @@ -112,8 +115,9 @@ func TestHelloWithSupportedMechs(t *testing.T) { mechs: must.NotFail(types.NewArray("SCRAM-SHA-1", "SCRAM-SHA-256")), }, "HelloUserPlain": { - user: db.Name() + ".hello_user_plain", - mechs: must.NotFail(types.NewArray("PLAIN")), + user: db.Name() + ".hello_user_plain", + mechs: must.NotFail(types.NewArray("PLAIN")), + failsForMongoDB: "PLAIN authentication mechanism is not support by MongoDB", }, "HelloUserSCRAM1": { user: db.Name() + ".hello_user_scram1", @@ -129,7 +133,11 @@ func TestHelloWithSupportedMechs(t *testing.T) { }, "MissingSeparator": { user: db.Name(), - err: "UserName must contain a '.' separated database.user pair", + err: &mongo.CommandError{ + Code: 2, + Name: "BadValue", + Message: "UserName must contain a '.' separated database.user pair", + }, }, } @@ -139,8 +147,8 @@ func TestHelloWithSupportedMechs(t *testing.T) { var t testtb.TB = tt - if tc.mechs != nil && tc.mechs.Contains("PLAIN") { - t = setup.FailsForMongoDB(t, "PLAIN authentication mechanism is not support by MongoDB") + if tc.failsForMongoDB != "" { + t = setup.FailsForMongoDB(t, tc.failsForMongoDB) } var res bson.D @@ -150,8 +158,8 @@ func TestHelloWithSupportedMechs(t *testing.T) { {"saslSupportedMechs", tc.user}, }).Decode(&res) - if tc.err != "" { - require.ErrorContains(t, err, tc.err) + if tc.err != nil { + AssertEqualCommandError(t, *tc.err, err) return } diff --git a/integration/users/usersinfo_test.go b/integration/users/usersinfo_test.go index 944cc1a7a766..a0a073f357b3 100644 --- a/integration/users/usersinfo_test.go +++ b/integration/users/usersinfo_test.go @@ -27,6 +27,7 @@ import ( "github.com/FerretDB/FerretDB/internal/types" "github.com/FerretDB/FerretDB/internal/util/must" "github.com/FerretDB/FerretDB/internal/util/testutil" + "github.com/FerretDB/FerretDB/internal/util/testutil/testtb" ) // createUser creates a bson.D command payload to create an user with the given username and password. @@ -147,7 +148,7 @@ func TestUsersinfo(t *testing.T) { hasUser map[string]struct{} showCredentials []string // showCredentials list the credentials types expected to be returned - skipForMongoDB string // optional, skip test for MongoDB backend with a specific reason + failsForMongoDB string // if set, fails for MongoDB backend }{ "NoUserFound": { dbSuffix: "no_users", @@ -218,7 +219,7 @@ func TestUsersinfo(t *testing.T) { }}, {"ok", float64(1)}, }, - skipForMongoDB: "Only MongoDB Enterprise offers PLAIN", + failsForMongoDB: "Only MongoDB Enterprise offers PLAIN", }, "WithSCRAMSHA1": { dbSuffix: "allbackends", @@ -532,11 +533,13 @@ func TestUsersinfo(t *testing.T) { } for name, tc := range testCases { - t.Run(name, func(t *testing.T) { - t.Parallel() + t.Run(name, func(tt *testing.T) { + tt.Parallel() - if tc.skipForMongoDB != "" { - setup.SkipForMongoDB(t, tc.skipForMongoDB) + var t testtb.TB = tt + + if tc.failsForMongoDB != "" { + t = setup.FailsForMongoDB(t, tc.failsForMongoDB) } var res bson.D @@ -575,7 +578,10 @@ func TestUsersinfo(t *testing.T) { require.True(t, (tc.hasUser == nil) != (tc.expected == nil)) if tc.showCredentials != nil { - cred := must.NotFail(actualUser.Get("credentials")).(*types.Document) + actualUser, err := actualUser.Get("credentials") + require.NoError(t, err) + + cred := actualUser.(*types.Document) for _, typ := range tc.showCredentials { switch typ { From 444a39d628ad4eb91c5a61726aafc3d29ae4cd11 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Wed, 6 Mar 2024 16:41:36 +0900 Subject: [PATCH 25/27] flatten if statements --- internal/handler/msg_hello.go | 56 +++++++++++++++++------------------ 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/internal/handler/msg_hello.go b/internal/handler/msg_hello.go index eb90d6cbc272..bc7fe2de60ca 100644 --- a/internal/handler/msg_hello.go +++ b/internal/handler/msg_hello.go @@ -45,32 +45,7 @@ func (h *Handler) MsgHello(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, e return nil, lazyerrors.Error(err) } - var saslSupportedMechsResp *types.Array - - if saslSupportedMechs != "" { - db, username, ok := strings.Cut(saslSupportedMechs, ".") - if !ok { - return nil, handlererrors.NewCommandErrorMsg( - handlererrors.ErrBadValue, - "UserName must contain a '.' separated database.user pair", - ) - } - - if username != "" { - mechs, err := h.getUserSupportedMechs(ctx, db, username) - if err != nil { - return nil, lazyerrors.Error(err) - } - - if len(mechs) > 0 { - saslSupportedMechsResp = must.NotFail(types.NewArray()) - for _, k := range mechs { - saslSupportedMechsResp.Append(k) - } - } - } - } - + var reply wire.OpMsg resp := must.NotFail(types.NewDocument( "isWritablePrimary", true, "maxBsonObjectSize", int32(types.MaxDocumentLen), @@ -83,13 +58,36 @@ func (h *Handler) MsgHello(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, e "readOnly", false, )) - if saslSupportedMechsResp != nil { + if saslSupportedMechs == "" { + resp.Set("ok", float64(1)) + must.NoError(reply.SetSections(wire.MakeOpMsgSection(resp))) + + return &reply, nil + } + + db, username, ok := strings.Cut(saslSupportedMechs, ".") + if !ok { + return nil, handlererrors.NewCommandErrorMsg( + handlererrors.ErrBadValue, + "UserName must contain a '.' separated database.user pair", + ) + } + + mechs, err := h.getUserSupportedMechs(ctx, db, username) + if err != nil { + return nil, lazyerrors.Error(err) + } + + saslSupportedMechsResp := must.NotFail(types.NewArray()) + for _, k := range mechs { + saslSupportedMechsResp.Append(k) + } + + if saslSupportedMechsResp.Len() != 0 { resp.Set("saslSupportedMechs", saslSupportedMechsResp) } resp.Set("ok", float64(1)) - - var reply wire.OpMsg must.NoError(reply.SetSections(wire.MakeOpMsgSection(resp))) return &reply, nil From d44e886ecd17262ed455027c157e83600a3199f0 Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Wed, 6 Mar 2024 17:11:49 +0900 Subject: [PATCH 26/27] update test --- integration/users/usersinfo_test.go | 67 ++++++++++++++++------------- 1 file changed, 37 insertions(+), 30 deletions(-) diff --git a/integration/users/usersinfo_test.go b/integration/users/usersinfo_test.go index a0a073f357b3..6bfdb77483ac 100644 --- a/integration/users/usersinfo_test.go +++ b/integration/users/usersinfo_test.go @@ -140,15 +140,14 @@ func TestUsersinfo(t *testing.T) { } testCases := map[string]struct { //nolint:vet // for readability - dbSuffix string - payload bson.D - err *mongo.CommandError - altMessage string - expected bson.D - hasUser map[string]struct{} - + dbSuffix string + payload bson.D + err *mongo.CommandError + altMessage string + expected bson.D + hasUser map[string]struct{} showCredentials []string // showCredentials list the credentials types expected to be returned - failsForMongoDB string // if set, fails for MongoDB backend + failsForMongoDB string }{ "NoUserFound": { dbSuffix: "no_users", @@ -577,33 +576,41 @@ func TestUsersinfo(t *testing.T) { require.True(t, (tc.hasUser == nil) != (tc.expected == nil)) - if tc.showCredentials != nil { - actualUser, err := actualUser.Get("credentials") - require.NoError(t, err) - - cred := actualUser.(*types.Document) - - for _, typ := range tc.showCredentials { - switch typ { - case "PLAIN": - assertPlainCredentials(t, "PLAIN", cred) - case "SCRAM-SHA-1": - assertSCRAMSHA1Credentials(t, "SCRAM-SHA-1", cred) - case "SCRAM-SHA-256": - assertSCRAMSHA256Credentials(t, "SCRAM-SHA-256", cred) - } - } - } + id, err := actualUser.Get("_id") + require.NoError(t, err) + + // when `forAllDBs` is set true, it may contain more users from other databases, + // so we check expected users were found rather than exact match + foundUsers[id.(string)] = struct{}{} + + userIDV, err := actualUser.Get("userId") + require.NoError(t, err) - if len(tc.showCredentials) == 0 { + userID := userIDV.(types.Binary) + assert.Equal(t, userID.Subtype.String(), types.BinaryUUID.String(), "uuid subtype") + assert.Equal(t, 16, len(userID.B), "UUID length") + + if tc.showCredentials == nil { assert.False(t, actualUser.Has("credentials")) + + continue } - foundUsers[must.NotFail(actualUser.Get("_id")).(string)] = struct{}{} + credV, err := actualUser.Get("credentials") + require.NoError(t, err) - uuid := must.NotFail(actualUser.Get("userId")).(types.Binary) - assert.Equal(t, uuid.Subtype.String(), types.BinaryUUID.String(), "uuid subtype") - assert.Equal(t, 16, len(uuid.B), "UUID length") + cred := credV.(*types.Document) + + for _, typ := range tc.showCredentials { + switch typ { + case "PLAIN": + assertPlainCredentials(t, "PLAIN", cred) + case "SCRAM-SHA-1": + assertSCRAMSHA1Credentials(t, "SCRAM-SHA-1", cred) + case "SCRAM-SHA-256": + assertSCRAMSHA256Credentials(t, "SCRAM-SHA-256", cred) + } + } } if tc.hasUser != nil { From 76990ef54b3ed38185587229ed0ee867613eb44e Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Wed, 6 Mar 2024 17:19:40 +0900 Subject: [PATCH 27/27] update test to not panic for failForMongoDB --- integration/hello_command_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/integration/hello_command_test.go b/integration/hello_command_test.go index 7dd44e52cfe3..081e55d73c69 100644 --- a/integration/hello_command_test.go +++ b/integration/hello_command_test.go @@ -182,7 +182,8 @@ func TestHelloWithSupportedMechs(t *testing.T) { return } - mechanisms := must.NotFail(actual.Get("saslSupportedMechs")) + mechanisms, err := actual.Get("saslSupportedMechs") + require.NoError(t, err) assert.True(t, mechanisms.(*types.Array).ContainsAll(tc.mechs)) }) }