NTLM authentication requires domain to be upper case #417
Conversation
|
@arthurschreiber @arobson - Please take a look. |
| @@ -39,6 +39,12 @@ class Connection extends EventEmitter { | |||
| super(); | |||
|
|
|||
| this.config = config; | |||
|
|
|||
| if (typeof (config.domain) === 'string') { | |||
| this.config = JSON.parse(JSON.stringify(config)); | |||
There was a problem hiding this comment.
I don't think this line here is quite right - what's the reason for serializing and deserializing the config here?
There was a problem hiding this comment.
I did that protect the 'config' parameter being passed in. Assigning config directly to this.config and then modifying this.config.domain would essentially modify config.domain which I figured would be an unintended side-effect.
There was a problem hiding this comment.
I'll and send an update to the pull request with comments added... once I figure how :)
|
One small nitpick here, other than that, I think this looks good! 👍 |
|
@arthurschreiber Pushed a change with a comment on why I did the JSON serialize/de-serialize. Please take a look. |
|
login7-payload-test seems to be failing on Node v6 on Travis CI. Failure is not related to my change as I only changed comments in the latest update. I couldn't repro it with and without my changes on my Windows box with Node v6 as well as older versions. |
tvrprasad
force-pushed
the
uppercase-domain
branch
from
225a867
to
87a7b41
Compare
|
@tvrprasad Thanks for the explanation! 👍 I agree that the current tedious behavior here is wrong, and we should not modify the object that got passed in. Unfortunately, I don't agree with the implementation that you proposed, as it's a "hack" in my eyes - it abuses JSON serialization to create a copy of the config object. If you remove that change from the PR, I'll merge it. I'd rather fix the "not modifying the passed in config" issue in a separate PR. How does that sound? |
As mentioned in #432, this is because Node.JS 6.4.0 shipped with a bug. Once Node.js 6.5.0 gets released, this will be fixed. I'll pin the AppVeyor builds to 6.3.x for the time being. |
|
@arthurschreiber Updated PR to not do serialize/de-serialize. Opened issue #435 to protect config parameter being passed in a non-hacky manner. I don't know at the moment what the non-hacky way to do that may be. Please add comments to the new issue if you have suggestions. Thanks. |
|
Thanks! ❤️ Could you also merge the lastest changes from master? |
…e upper case. Convert config.domain to upper in Connection constructor to handle this. Add tests.
… protecting config parameter being passed in, as part of a separate PR.
tvrprasad
force-pushed
the
uppercase-domain
branch
from
2b5229c
to
23dc7df
Compare
|
@arthurschreiber Done - merged to latest from master. |
|
@arthurschreiber Anything blocking from getting this committed to master? Thanks. |
|
@arthurschreiber Can you merge this change? Keen to see my first commit :) |
|
Thanks @tvrprasad! ❤️ |
This addresses the issue:
#414
The issues speaks to FDQN in addition to the pull request. But I couldn't repro the FDQN failure. It seems to work for me just fine.
@arthurschreiber @arobson - This is a follow-up to our discussion on #246.