GODRIVER-3215 Fix default auth source for auth specified via ClientOptions.#1764
Conversation
mongodb-drivers-pr-bot
Bot
added
the
review-priority-low
API Change Report./x/mongo/driver/topologycompatible changesConvertCreds: added |
matthewdale
force-pushed
the
godriver3215-default-external
branch
from
1643840 to
2422048
Compare
matthewdale
force-pushed
the
godriver3215-default-external
branch
from
2422048 to
d6a25ad
Compare
matthewdale
changed the title
blink1073
added
review-priority-urgent
|
|
||
| // NewAuthenticator returns a [driver.Authenticator] configured with the given | ||
| // credential and HTTP client. It returns nil if cred is nil. | ||
| func NewAuthenticator(cred *options.Credential, httpClient *http.Client) (driver.Authenticator, error) { |
There was a problem hiding this comment.
I suggest repurposing this function with the goal of converting options.Credential to auth.Cred. And constructing the Authenticator using the auth package where needed. For example,
authCreds := topology.ConvertCreds(clientOpt.Auth)
client.authenticator, err = auth.CreateAuthenticator(clientOpt.Auth.AuthMechanism, authCreds, clientOpt.HTTPClient)This way we limit the number of functions that return the Authenticator interface, making a future refactor to correct this anti-pattern more straightforward: GODRIVER-3316
There was a problem hiding this comment.
Seems reasonable, will try that refactor.
|
|
||
| // Test that convertOIDCArgs exhaustively copies all fields of a driver.OIDCArgs | ||
| // into an options.OIDCArgs. | ||
| func TestConvertOIDCArgs(t *testing.T) { |
There was a problem hiding this comment.
Suggest parallelizing the entire test
| authenticator := PlainAuthenticator{ | ||
| Username: "user", | ||
| Password: "pencil", | ||
| Source: "$external", |
There was a problem hiding this comment.
We use this so much, what are your thoughts on constantizing?
There was a problem hiding this comment.
The tradeoff is more indirection for lower typo risk. Since the source of truth is the server APIs, there is no risk of out-of-sync constants in the Go Driver. Seems like a good idea overall, will update 👍
prestonvasquez
left a comment
prestonvasquez
left a comment
There was a problem hiding this comment.
LGTM, see suggestions 👍
Co-authored-by: Preston Vasquez <prestonvasquez@icloud.com>
|
drivers-pr-bot please backport to master |
This didn't quite work yet, I'll follow up. |
|
I don't think I have enough golang-foo to cherry-pick this one myself. |
…tions. (mongodb#1764) Co-authored-by: Preston Vasquez <prestonvasquez@icloud.com> Co-authored-by: Steven Silvester <steven.silvester@ieee.org> (cherry picked from commit 18d1b19)
3 participants
GODRIVER-3215
Summary
ClientOptionsto a new functiontopology.NewAuthenticator. Use it everywhere that needs to create an authenticator fromClientOptions.convertOIDCArgsinto thetopologypackage.topology.NewConfigWithAuthenticator, but actually has no effect currently, so no default auth sources are being set.Background & Motivation
Currently, if auth mechanism "MONGODB-AWS" is set using
ClientOptions.SetAuth, the default auth source is set to "admin" instead of "$external". The result is a confusing error messageThere was a further regression (that hasn't been released yet) caused by #1678, which effectively skips all of the default auth source logic. Refactor the authenticator creation logic and the default auth source logic to make similar regressions more obvious.