-
-
Notifications
You must be signed in to change notification settings - Fork 526
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
Add support to SASL SCRAM #72
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this was a nice and simple PR. No complicated logic anywhere to be found.
src/network/connection.js
Outdated
@@ -313,6 +319,11 @@ module.exports = class Connection { | |||
|
|||
// The full payload is loaded, erase the temporary buffer | |||
this.buffer = Buffer.alloc(0) | |||
|
|||
if (this.authHandlers && this.authExpectResponse) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove the second condition
|
||
const finalMessageWithoutProof = this.finalMessageWithoutProof(clientMessageResponse) | ||
const clientProof = await this.clientProof(clientMessageResponse) | ||
const finalMessage = `${finalMessageWithoutProof},p=${clientProof}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this p
, r
, n
etc. is reminding me of node-postgres
. I would suggest to extract them to named constants so that I don't have to read the RFC to understand what they all mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think naming this different will help; it's better to have a close mapping to the RFC. This is also "standardized", anyone reading SCRAM code will easily find the components of the message instead of parsing our understanding of it. WDYT?
Closes #71