Adding initial NTLM support (from https://github.com/vision10/node-soap) #887

insanityinside · 2016-10-12T15:58:07Z

Pulling in changes from @vision10 https://github.com/vision10/node-soap (via @r24y https://github.com/r24y/node-soap/ who did some cleanup) into one clean package for a pull request.

This is a revised version of pull #885.

Cleaned up version of #776 and #741, syntax altered to match project. I've tested it against a live NTLM SOAP server, and it seems to work perfectly. Added basic instructions to the README with respect to syntax usage with the other security types, and regenerated TOC.

There is currently a check in http.js in HttpClient.prototype.request which has an if/else for the ntlm property. I suspect there's a better way to do this, but it's how it was done in the original code. Suggestions and pointers welcome, or move code around as appropriate. The code may need refactoring, but it seems reasonable enough to work with for now, and does seem to be functioning correctly.

Also requires tests implementing.

coveralls · 2016-10-12T16:00:22Z

Coverage decreased (-0.7%) to 92.451% when pulling 2087e05 on microchip:master into 7841b64 on vpulim:master.

coveralls · 2016-10-31T16:38:26Z

Coverage decreased (-0.7%) to 92.466% when pulling 9782794 on microchip:master into 0b1df30 on vpulim:master.

coveralls · 2016-11-01T16:13:18Z

Coverage decreased (-0.7%) to 92.466% when pulling 9d062af on microchip:master into 0b1df30 on vpulim:master.

jsdevel · 2016-11-01T16:29:18Z

@microchip is there a way to add a test?

insanityinside · 2016-11-01T16:34:49Z

I'll take a look. Should be theoretically possible.

vision10 · 2016-11-02T06:55:51Z

I don't know how to make the test but i can provide anything needed for it

ryaninvents · 2016-11-03T16:42:44Z

@microchip thanks so much for helping merge these changes in!

As far as testing, mockrequire looks like it could help here. Feel free to look into it; I may have time to look at this at some point, but probably not for another week or so.

aijanai · 2017-02-20T17:00:34Z

guys, are there news for this?

insanityinside · 2017-02-20T17:07:26Z

@aijanai I've not had chance to do any more with this at work at the moment I'm afraid :(

coveralls · 2017-02-20T17:15:56Z

Coverage decreased (-0.7%) to 92.383% when pulling b3f3582 on microchip:master into bc6992a on vpulim:master.

aijanai · 2017-02-20T17:49:07Z

@microchip

yes yes, it's perfectly ok, but can it just be merged as it is?
The problem is that there is no sane NTLM support for node anywhere. Packages lying around on NPM are just broken, incomplete or undocumented.

Since this is not breaking anything, could we please just get over the 0.7% coverage drop and release it to the public?

insanityinside · 2017-02-20T18:27:31Z

@aijanai For what it's worth, I'm using the fork I made above in production, and it seems to be working perfectly fine. Haven't tested the latest merges yet, but there's no good reason they wouldn't work.

aijanai · 2017-02-21T10:31:18Z

Uhm. Then there must be something I am seriously doing wrong because it keeps crashing with:

events.js:176
    domain.enter();
           ^

TypeError: domain.enter is not a function
    at Request.emit (events.js:176:12)
    at IncomingMessage.<anonymous> (/home/ai_ja_nai/wd-soap-proxy-lambda/node_modules/request/request.js:998:12)
    at emitOne (events.js:96:13)
    at IncomingMessage.emit (events.js:188:7)
    at IncomingMessage.Readable.read (_stream_readable.js:381:10)
    at flow (_stream_readable.js:761:34)
    at resume_ (_stream_readable.js:743:3)
    at _combinedTickCallback (internal/process/next_tick.js:74:11)
    at process._tickCallback (internal/process/next_tick.js:98:9)

My dependencies are:

 "dependencies": {
    "soap": "^0.18.0",
    "soap-ntlm-2": "^0.9.4"
  },
  "devDependencies": {
    "node-soap": "vision10/node-soap"
  },

The code is just this simple 50 lines test script:

var url = process.env.URL
var username = process.env.USER
var password = process.env.PASSWORD
var domain = process.env.DOMAIN

var options = {
      wsdl_options: {
                ntlm: true,
                username: username,
                password: password,
                workstation: "",
                domain: domain
            }
}

var soap = require('soap')

soap.createClient(url, options, function(err, client, body) {
  if(err){
    console.log("ERROR")
    console.log(err)
    return
  }

  client.setSecurity(new soap.NtlmSecurity(options.wsdl_options))

  console.log(client.describe())
})

I took it from the GitHub website and NPM page. Is this how it is supposed to look like?
Many thanks for any help

insanityinside · 2017-02-21T12:18:47Z

@aijanai Wouldn't you need to change the dependency to be microchip/node-soap to use the NTLM merged code? As I said, current code is untested, but I can't see anything that'd immediately break it. (Please note my fork is entirely unsupported, I merged in code changes someone else had made and tidied up a bit, that's it.)

vision10 · 2017-02-21T12:29:14Z

you need to require

var soap = require('soap-ntlm-2'),

not the original soap

coveralls · 2017-04-03T08:31:17Z

Coverage decreased (-0.7%) to 92.436% when pulling 10a1fa0 on microchip:master into 14aa52c on vpulim:master.

coveralls · 2017-09-06T14:49:54Z

Coverage decreased (-0.7%) to 92.47% when pulling b92509e on microchip:master into 01d8585 on vpulim:master.

herom

Please, add tests for the new features 👍

herom · 2017-09-06T21:43:06Z

lib/http.js

+        return callback(err);
+      }
+      // if result is stream
+      if( typeof res.body != 'string') {


please use !== for strict checl

herom · 2017-09-06T21:44:16Z

lib/security/NTLMSecurity.js

+var _ = require('lodash');
+
+function NTLMSecurity(username, password, domain, workstation) {
+    if (typeof username == "object") {


please use ===

herom · 2017-09-06T21:45:28Z

package.json

@@ -14,6 +14,8 @@
    "ejs": "~2.5.5",
    "finalhandler": "^1.0.3",
    "lodash": "^3.10.1",
+    "optional": "^0.1.3",


where do we need this dependency?

I have no idea, it came with the original modification. Happy to remove it if it's not used.

herom · 2017-09-06T21:47:12Z

lib/security/NTLMSecurity.js

+}
+
+NTLMSecurity.prototype.addHeaders = function (headers) {
+  headers.Connection = 'keep-alive';


so, is it intentional, that the method addHeaders does not add headers anywhere but sets a Connection property on the (hopefully available) headers param (Object)?

It looks that way, it's in the original code. It works the same way as in BearerSecurity, which I'd guess is what this code is based on.

ah, I see.... hmmm.....

herom · 2017-09-06T21:50:21Z

lib/security/NTLMSecurity.js

+};
+
+NTLMSecurity.prototype.addOptions = function (options) {
+    _.merge(options, this.defaults);


if you call it this way, only options will be mutated, but this.defaults will stay "like it is" - is this really the behaviour you/we want?

insanityinside · 2017-09-07T09:08:11Z

I'm happy to clean up the code where mentioned, but to be honest, it should probably be refactored in order to be properly testable. I currently don't have the time to do it (or the knowledge of this setup, I'm just fixing up what's already here!)

If I get chance to at some point, I'll happily do it, but in the meantime, if anyone can refactor it, I welcome pull requests.

…n package.json

coveralls · 2017-09-07T09:21:59Z

Coverage decreased (-0.7%) to 92.47% when pulling 6ffce76 on microchip:master into 01d8585 on vpulim:master.

leonfs · 2017-10-05T10:52:35Z

Any idea when this could get merged?

coveralls · 2017-10-05T11:32:16Z

Coverage decreased (-0.4%) to 93.267% when pulling 5aefa9d on insanityinside:master into 9ea32e2 on vpulim:master.

herom · 2017-10-06T06:46:30Z

@leonfs

Any idea when this could get merged?

We need at least a few tests for the new features in order to merge the PR (see our guidelines on Submitting a Pull Request). As @insanityinside said in his previous comment

I currently don't have the time to do it (or the knowledge of this setup, I'm just fixing up what's already here!)... I welcome pull requests.

A PR over at his fork in order to get things cleaned up and tested is very much appreciated 😺

benthepoet · 2018-04-12T15:13:18Z

Just opened a PR at the @insanityinside fork to fix linting issues and added a handful of tests.

insanityinside · 2018-04-12T15:16:23Z

@benthepoet I'll take a look at merging it this evening, then see if I can bring the codebase up to date and merge up. I've not tested it in a bit, but I've got test apps I can run with the updated code.

Fixed linting issues and added unit tests for NTLMSecurity Haven't had chance to test this yet or rebase it on the live branch, but pulling it in is probably a good start!

Adding initial NTLM support (from https://github.com/vision10/node-soap)

9782794

insanityinside force-pushed the master branch from 2087e05 to 9782794 Compare October 31, 2016 16:35

Merge branch 'master' of https://github.com/vpulim/node-soap

9d062af

Merge branch 'master' of https://github.com/vpulim/node-soap

b3f3582

herom added the needs tests label Mar 22, 2017

Merge branch 'master' of https://github.com/vpulim/node-soap

10a1fa0

Merge in changes from upstream as of 2017-09-06

b92509e

herom suggested changes Sep 6, 2017

View reviewed changes

Fix for strict comparison, remove unnecessary "optional" dependency i…

6ffce76

…n package.json

Merge branch 'master' of https://github.com/vpulim/node-soap

6681e48

Ben Hanna added 4 commits April 12, 2018 10:23

Fixed linting issues.

0813345

Added test file for NTLMSecurity.

611460b

Added tests for NTLMSecurity.

e180e0f

Added constructor tests for NTMLSecurity.

e5d3587

Merge pull request #1 from benthepoet/master

7f6fc53

Fixed linting issues and added unit tests for NTLMSecurity Haven't had chance to test this yet or rebase it on the live branch, but pulling it in is probably a good start!

herom approved these changes May 10, 2018

View reviewed changes

Merge branch 'master' into master

5aefa9d

herom merged commit 1c68531 into vpulim:master May 10, 2018

snyk-bot mentioned this pull request Sep 5, 2019

[Snyk] Upgrade soap from 0.24.0 to 0.29.0 MateMalice/node-ews#2

Merged

snyk-bot mentioned this pull request Sep 24, 2019

[Snyk] Upgrade soap from 0.13.0 to 0.29.0 MailOnline/node-google-dfp#3

Open

snyk-bot mentioned this pull request Dec 3, 2019

[Snyk] Upgrade soap from 0.24.0 to 0.30.0 MateMalice/node-ews#7

Closed

snyk-bot mentioned this pull request Mar 15, 2020

[Snyk] Upgrade soap from 0.24.0 to 0.31.0 MateMalice/node-ews#8

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding initial NTLM support (from https://github.com/vision10/node-soap) #887

Adding initial NTLM support (from https://github.com/vision10/node-soap) #887

insanityinside commented Oct 12, 2016 •

edited

Loading

coveralls commented Oct 12, 2016 •

edited

Loading

coveralls commented Oct 31, 2016 •

edited

Loading

coveralls commented Nov 1, 2016 •

edited

Loading

jsdevel commented Nov 1, 2016

insanityinside commented Nov 1, 2016

vision10 commented Nov 2, 2016

ryaninvents commented Nov 3, 2016

aijanai commented Feb 20, 2017

insanityinside commented Feb 20, 2017

coveralls commented Feb 20, 2017 •

edited

Loading

aijanai commented Feb 20, 2017

insanityinside commented Feb 20, 2017

aijanai commented Feb 21, 2017

insanityinside commented Feb 21, 2017

vision10 commented Feb 21, 2017

coveralls commented Apr 3, 2017 •

edited

Loading

coveralls commented Sep 6, 2017 •

edited

Loading

herom left a comment

herom Sep 6, 2017

herom Sep 6, 2017

herom Sep 6, 2017

insanityinside Sep 7, 2017

herom Sep 6, 2017

insanityinside Sep 7, 2017

herom Sep 7, 2017

herom Sep 6, 2017

insanityinside commented Sep 7, 2017

coveralls commented Sep 7, 2017 •

edited

Loading

leonfs commented Oct 5, 2017 •

edited

Loading

coveralls commented Oct 5, 2017 •

edited

Loading

herom commented Oct 6, 2017

benthepoet commented Apr 12, 2018

insanityinside commented Apr 12, 2018

Adding initial NTLM support (from https://github.com/vision10/node-soap) #887

Adding initial NTLM support (from https://github.com/vision10/node-soap) #887

Conversation

insanityinside commented Oct 12, 2016 • edited Loading

coveralls commented Oct 12, 2016 • edited Loading

coveralls commented Oct 31, 2016 • edited Loading

coveralls commented Nov 1, 2016 • edited Loading

jsdevel commented Nov 1, 2016

insanityinside commented Nov 1, 2016

vision10 commented Nov 2, 2016

ryaninvents commented Nov 3, 2016

aijanai commented Feb 20, 2017

insanityinside commented Feb 20, 2017

coveralls commented Feb 20, 2017 • edited Loading

aijanai commented Feb 20, 2017

insanityinside commented Feb 20, 2017

aijanai commented Feb 21, 2017

insanityinside commented Feb 21, 2017

vision10 commented Feb 21, 2017

coveralls commented Apr 3, 2017 • edited Loading

coveralls commented Sep 6, 2017 • edited Loading

herom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

insanityinside commented Sep 7, 2017

coveralls commented Sep 7, 2017 • edited Loading

leonfs commented Oct 5, 2017 • edited Loading

coveralls commented Oct 5, 2017 • edited Loading

herom commented Oct 6, 2017

benthepoet commented Apr 12, 2018

insanityinside commented Apr 12, 2018

insanityinside commented Oct 12, 2016 •

edited

Loading

coveralls commented Oct 12, 2016 •

edited

Loading

coveralls commented Oct 31, 2016 •

edited

Loading

coveralls commented Nov 1, 2016 •

edited

Loading

coveralls commented Feb 20, 2017 •

edited

Loading

coveralls commented Apr 3, 2017 •

edited

Loading

coveralls commented Sep 6, 2017 •

edited

Loading

coveralls commented Sep 7, 2017 •

edited

Loading

leonfs commented Oct 5, 2017 •

edited

Loading

coveralls commented Oct 5, 2017 •

edited

Loading