noodlefrenzy / node-amqp10

amqp10 is a promise-based, AMQP 1.0 compliant node.js client
MIT License
134 stars 56 forks source link

Separate SASL mechanism and add "registerSaslMechanism" API on the client #349

Closed pierreca closed 6 years ago

pierreca commented 6 years ago

This adds the ability to "inject" a SASL mechanism and associated handler in the library, which effectively unlocks the ability to have complex SASL challenge/response exchanges.


This change is Reviewable

amarzavery commented 6 years ago

Travis is failing for some weird reason in 0.10 version of node.js. Have restarted the ci job. Btw, do we want to support 0.10 or should we remove this from the build matrix?

amarzavery commented 6 years ago

Otherwise everything LGTM

princjef commented 6 years ago

travis is failing for 0.10 because there are a couple of arrow functions

amarzavery commented 6 years ago

oops. @pierreca can you replace the arrow functions?

pierreca commented 6 years ago

Review status: 0 of 11 files reviewed at latest revision, 6 unresolved discussions.


lib/sasl/sasl.js, line 74 at r1 (raw file):

Previously, pierreca (Pierre Cauchois) wrote…
that's exactly why. I'm not sure we want to start exporting our frames types. It's worth revisiting this decision now though since changing it later would likely be a breaking change.

Done.


lib/sasl/sasl.js, line 91 at r1 (raw file):

Previously, princjef (Jeff Principe) wrote…
any reason for using an arrow function + `self`? i'm guessing this is just a typo and you meant to do `function (responseContent) {` instead (though i'd be all for switching to arrow functions if/when we drop support for old versions of node). same for the `.catch` after this

Done.


lib/sasl/sasl.js, line 69 at r2 (raw file):

Previously, princjef (Jeff Principe) wrote…
i think that's reasonable

Done.


lib/sasl/sasl_anonymous.js, line 19 at r2 (raw file):

Previously, amarzavery (Amar Zavery) wrote…
same as below

Done.


lib/sasl/sasl_plain.js, line 24 at r2 (raw file):

Previously, amarzavery (Amar Zavery) wrote…
This is much simpler and does the same thing ```javascript return Promise.resolve(); ```

Done.


Comments from Reviewable

princjef commented 6 years ago

Reviewed 2 of 10 files at r1, 9 of 9 files at r4. Review status: all files reviewed at latest revision, 4 unresolved discussions.


Comments from Reviewable

princjef commented 6 years ago

LGTM

pierreca commented 6 years ago

Review status: all files reviewed at latest revision, 4 unresolved discussions.


lib/sasl/sasl.js, line 90 at r4 (raw file):

Previously, princjef (Jeff Principe) wrote…
nit: parentheses shouldn't be necessary

Done.


Comments from Reviewable