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

Adding support for NTLMv2 #79

Merged
merged 3 commits into from
Feb 22, 2020
Merged

Adding support for NTLMv2 #79

merged 3 commits into from
Feb 22, 2020

Conversation

ricjd
Copy link
Contributor

@ricjd ricjd commented Jan 23, 2018

No description provided.

@nmarus nmarus self-requested a review January 25, 2018 13:34
@nmarus nmarus self-assigned this Jan 25, 2018
@nmarus nmarus requested a review from kcastrotech January 25, 2018 13:34
@ricjd
Copy link
Contributor Author

ricjd commented Mar 28, 2018

@nmarus @kcastrotech We've been using this for a while now and think it's a good change.

@kcastrotech
Copy link
Contributor

Cool! Code looks good, I'll test it out internally.

@ricjd
Copy link
Contributor Author

ricjd commented May 18, 2018

@kcastrotech hey, wondering if any updates on this? thanks!

@TashiDhundup
Copy link

TashiDhundup commented Mar 17, 2019

Hello @nmarus @ricjd @kcastrotech I have been using this module. But have to upgrade to NTLMv2. Please consider merging this PR to main branch. I have been trying other EWS modules that supports NTLMv2 but does not support HTML and other important features.
Please consider this PR.

- Connects to configured EWS Host and downloads it's WSDL file so it might be concluded that this is "fairly" version agnostic
- After downloading the WSDL file, the wrapper dynamically exposes all EWS SOAP functions
- Attempts to standardize Microsoft's WSDL by modifying the file to include missing service name, port, and bindings
- This DOES NOT work with anything Microsoft Documents as using the EWS Managed API.
- When using NTLMv2 you have to you your Windows username and not your email address

Choose a reason for hiding this comment

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

Suggested change
- When using NTLMv2 you have to you your Windows username and not your email address
- When using NTLMv2 you have to use your Windows username and not your email address

username: config.username,
password: config.password,
domain: config.domain,
worksstation: config.worksstation,

Choose a reason for hiding this comment

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

Suggested change
worksstation: config.worksstation,
workstation: config.workstation,

Comment on lines -344 to -345
This utilizes the [options](https://github.com/SamDecrock/node-http-ntlm#options) available to the underlying NTLM lib.
[Here](https://github.com/SamDecrock/node-http-ntlm#pre-encrypt-the-password) is an example from its README.

Choose a reason for hiding this comment

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

You should replace this old documentation with a reference to the docs for the new library you're using, so that users understand what options to pass in (e.g. they wouldn't know about an option like workstation unless it was documented here).

Comment on lines -358 to -359
nt_password: ntHashedPassword,
lm_password: lmHashedPassword,

Choose a reason for hiding this comment

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

If this is no longer supported, then this is a breaking change and you'll need to bump the version to 4.0.0

@nmarus nmarus merged commit 7b7a4e0 into nmarus:master Feb 22, 2020
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.

5 participants