Skip to content
This repository was archived by the owner on Feb 4, 2022. It is now read-only.

NODE-1435: move all auth to connect #406

Merged
merged 8 commits into from
Feb 22, 2019
Merged

Conversation

mbroadst
Copy link
Member

NOTE: this PR is a little big until the previous one is reviewed and merged

This moves all authentication in the entire driver into the connect method, greatly simplifying our view of auth in the module. Top-level methods still exist on all of the topologies, but they are no-ops that immediately call back with no error.

Copy link
Contributor

@daprahamian daprahamian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for the most part. My main hold-up is that this looks like it is technically a breaking change for the Core API. Is it possible that someone is still relying on being able to connect w.out auth, and then later pass the auth credentials to connect?


function connect(options, callback) {
if (AUTH_PROVIDERS == null) {
AUTH_PROVIDERS = defaultAuthProviders(options.bson);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checking that we do not allow people to change their BSON impl on the fly

@mbroadst mbroadst force-pushed the NODE-1435/move-auth-to-connect branch from 19070bb to fa99c8f Compare February 22, 2019 19:28
Copy link
Contributor

@kvwalker kvwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me

@mbroadst mbroadst merged commit be884c2 into master Feb 22, 2019
@mbroadst mbroadst deleted the NODE-1435/move-auth-to-connect branch February 22, 2019 21:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants