SWIFT-1161 MongoConnectionString authentication options support #631
Conversation
bynn
changed the title
| case "authSource": | ||
| self.credential?.source = value | ||
| case "authMechanism": | ||
| if value == "GSSAPI", self.credential?.source == self.database { |
There was a problem hiding this comment.
I think it would be helpful to leave a comment here explaining why you set it to nil
There was a problem hiding this comment.
now that I think about it... we only have to do this because we preemptively fill in the default DB before calling this method, right?
I think it would be better if we could just hold off on populating that field until we're sure it's actually relevant to do so (maybe you could pass the default DB name into this method or something)
There was a problem hiding this comment.
now only sets it when it needs to
Co-authored-by: Kaitlin Mahar <kaitlin.mahar@mongodb.com>
| try MongoConnectionString.percentDecode(from: userInfoExists[1]) : nil | ||
| ) | ||
| } | ||
| guard let authAndOptions = identifiersAndOptions.count == 2 ? |
There was a problem hiding this comment.
nit: this variable actually contains the auth DB and options right? (authAndOptions sounds like maybe it contains the user info too)
There was a problem hiding this comment.
changed to authDBAndOptions
| } | ||
| self.credential?.mechanismProperties = authMechanismProperties | ||
| default: | ||
| break |
There was a problem hiding this comment.
this should never happen right? throwing an InternalError might be appropriate
There was a problem hiding this comment.
i wasn't sure on what to write but i tried to write something in the terms of MongoUnreachableError
| } | ||
|
|
||
| private mutating func applyAndValidateAuthOptions(authOptions: [String: String]) throws { | ||
| guard self.credential != nil else { |
There was a problem hiding this comment.
so I realized x509 auth actually does not require a username, and neither does AWS:
https://github.com/mongodb/specifications/blob/master/source/auth/auth.rst#mongocredential-properties-1
https://github.com/mongodb/specifications/blob/master/source/auth/auth.rst#mongocredential-properties-5
there is a test case for that - "should recognize the mechanism with no username (MONGODB-X509)" I would think we would fail it?
There was a problem hiding this comment.
It was never setting the mechanism because the credential was empty but I'm not sure why authTest wouldn't catch that
There was a problem hiding this comment.
for MONGO-AWS it didn't exist in the MongoCredential so i didn't write anything for it. Does the swift driver support it?
There was a problem hiding this comment.
Also currently, im duplicating a lot of code and the whole point of having a separate validate and apply was that we didn't have to do that so I'm not sure how to make this cleaner.
| case "authSource": | ||
| self.credential?.source = value | ||
| case "authMechanism": | ||
| if value == "GSSAPI", self.credential?.source == self.database { |
There was a problem hiding this comment.
now that I think about it... we only have to do this because we preemptively fill in the default DB before calling this method, right?
I think it would be better if we could just hold off on populating that field until we're sure it's actually relevant to do so (maybe you could pass the default DB name into this method or something)
|
we're closing this PR now as it now longer merges cleanly, however we will keep this branch around for when we have time to revisit the project, so we can use Bynn's work. |
This currently doesn't have the accepted behavior for specifying the database and the authSource and I wanted to get some feedback on how I can improve the design