PMM-9320: fix username/password incorrect escape #372
Conversation
mongo_fix/mongo_fix.go
Outdated
|
|
||
| clientOptions := options.Client(). | ||
| ApplyURI(dsn). | ||
| SetAuth(creds) //Workaround for PMM-9320 |
| ApplyURI(dsn). | ||
| opts, err := mongo_fix.ClientForDSN(dsn) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
it is test, so we can panic
There was a problem hiding this comment.
okay, here we return error, so I think we can just propagate error.
but what about other cases, like in func OpenTestMongoDB(tb testing.TB, dsn string) *mongo.Client {
we don't have error in the API, do you think we should refactor API or we can panic (in tests only)
There was a problem hiding this comment.
we can call require.NoError(tb, err) in that places. so it will stop test with failed result
mongo_fix/mongo_fix.go
Outdated
| @@ -0,0 +1,27 @@ | |||
| package mongo_fix | |||
There was a problem hiding this comment.
called mongo_fix not to clash with mongo driver
mongo_fix/mongo_fix.go
Outdated
| // Workaround for PMM-9320 | ||
| username := parsedDsn.User.Username() | ||
| password, _ := parsedDsn.User.Password() | ||
| if username != "" || password != "" { |
There was a problem hiding this comment.
we need to set auth only when creds are provided, otherwise auth fill fail
utils/tests/mongodb.go
Outdated
| client, err := mongo.Connect(context.Background(), options.Client().ApplyURI(dsn)) | ||
| opts, err := mongo_fix.ClientForDSN(dsn) | ||
| if err != nil { | ||
| panic(err) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
mongo_fix/mongo_fix.go
Outdated
| "go.mongodb.org/mongo-driver/mongo/options" | ||
| ) | ||
|
|
||
| // ClientForDSN applies URI to Client |
There was a problem hiding this comment.
🚫 [golangci-lint] reported by reviewdog 🐶
Comment should end in a period (godot)
BupycHuk
left a comment
BupycHuk
left a comment
There was a problem hiding this comment.
Looks good to me. Can we add a few tests with this kind of DSNs?
| ApplyURI(dsn). | ||
| opts, err := mongo_fix.ClientForDSN(dsn) | ||
| if err != nil { | ||
| panic(err) |
mongo_fix/mongo_fix.go
Outdated
| ) | ||
|
|
||
| // ClientForDSN applies URI to Client. | ||
| func ClientForDSN(dsn string) (*options.ClientOptions, error) { |
There was a problem hiding this comment.
probably we should call it ClientOptionsForDSN or include mongo.Connect to this function
There was a problem hiding this comment.
yeah, ClientOptions is more correct
sure, will do |
PMM-9320
Password and Username are escaped According to RFC 3986 §3.2.1
// The RFC allows ';', ':', '&', '=', '+', '$', and ',' in
// userinfo, so we must escape only '@', '/', and '?'.
// The parsing of userinfo treats ':' as special so we must escape
// that too.
go1.18.1/src/net/url/url.go:143
But in MongoDB driver they decided to unscape it as encodePathSegment
using url.PathUnescape
here mongo-go-driver/x/mongo/driver/connstring/connstring.go:236
and
here ext/mongo-go-driver/x/mongo/driver/connstring/connstring.go:249
It can also be fixed with this PR
Related PRs:
FB: