Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support speculative authentication in scram-sha and x509 #2353

Merged
merged 1 commit into from
May 7, 2020

Conversation

mbroadst
Copy link
Member

@mbroadst mbroadst commented May 5, 2020

this is the port to 4.0

This introduces a new auth provider stage prepare which allows a provider to alter the handshake document before its used for the initial handshake. The SCRAM-SHA and X509 providers have been improved to use the prepare stage in order to speculatively authenticate, potentially reducing the roundtrips of connection authentication.

NODE-2487

@mbroadst mbroadst requested review from reggi and emadum May 5, 2020 23:00
This introduces a new auth provider stage `prepare` which allows
a provider to alter the handshake document before its used for the
initial handshake. The SCRAM-SHA and X509 providers have been
improved to use the prepare stage in order to speculatively
authenticate, potentially reducing the roundtrips of connection
authentication.

NODE-2487
@mbroadst mbroadst force-pushed the NODE-2487/master/speculative-authentication branch from 996c97a to f754f15 Compare May 5, 2020 23:02
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

LGTM with one nit.

I was having a hard time reading the diffs on this PR until I remembered the VSCode extension, really helpful for situations like this!

return;
}

const { connection, credentials } = authContext;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could go a bit further with destructuring here and get rid of credentials, which isn't used again past here.
const { connection, credentials: { username, password, mechanismProperties } } = authContext;

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought we decided against the deep destructuring in one of Neal's PRs, because it would hose TypeScript?

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly, but I don't recall that discussion. Do you remember the details of how it would hose Typescript (or perhaps you do @nbbeeken)?

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no issue with doing this with TS. The discussion we had is that you can't use this nested destructuring with ES6 imports, which is potentially something we'd have to use in some far off future. So I had reverted some of the requires to just use one layer of destructuring, but that didn't make it in.

Copy link
Contributor

@reggi reggi left a comment

Choose a reason for hiding this comment

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

LGTM

@mbroadst mbroadst merged commit f71f09b into master May 7, 2020
@mbroadst mbroadst deleted the NODE-2487/master/speculative-authentication branch May 7, 2020 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants