Timestamp Optional

[jayharper]

Added the option to generate a timestamp when creating a soap client. Also added a new test in request response sample to test this option.

[herom]

Thanks a lot for your time and effort @jayharper 👍

In order to get your PR merged, please squash your commits like pointed out in our Guidelines on Submitting a Pull Request.

[jayharper]

My apologies for not squashing to a single commit originally.

[jsdevel]

Looks good @jayharper ! 👍 from me @herom

[herom]

As the passwordType String param was not only replaced by an options Object parameter, we should do something like this when setting this._passwordType:

options = options || {};

this._passwordType = typeof options === 'string' ? options : options.passwordType;
this._hasTimeStamp = options.hasTimeStamp;

Also, I think we should update the README in order to let the people know, that an options Object is expected and what it could contain.

[jayharper]

@herom take a look at my new updates. My goal was not to break implementations utilizing the passwordType string parameter. The options object is not required but rather optional. The passwordType property default is PasswordText and the hasTimeStamp property will default to true.

[herom]

I just addressed some concerns I had regarding the newly introduced options Object. That's just my 2 cents but I think this would be not also a "nice to have" but also a "must have" if we "break" the constructor like this. 😸

[herom]

thanks a lot for your patience @jayharper 👍

I think it's completely viable to set this._passwordType and this._hasTimeStamp properties once without setting them and resetting them within the next line. So if you please - as a last step - could change the "double set"-lines with "one-liners" like:

if (typeof options === 'string') {
  this._passwordType = options ? options : 'PasswordText';
} else {
  this._passwordType = options.passwordType ? options.passwordType : 'PasswordText';
}

this._hasTimeStamp = !!options.hasTimeStamp; // implicit Boolean conversion!

This would a) shorten the code and b) make it more readable. Thanks a lot for your understanding 😃

[jayharper]

@herom I understand your desire to shorten the code and make it more readable, unfortunately your suggested code does not pass the original and the two new propogate basic security tests I have implemented. Could I ask you to check out my commit and run the tests yourself to help me find a solution?

[herom]

Somehow I was not able to fork your repo and create a PR there, so here's the (working & tested) snippet for the WSSecurity constructor, I hope this helps 😃

"use strict";

var crypto = require('crypto');
var passwordDigest = require('../utils').passwordDigest;
var validPasswordTypes = ['PasswordDigest', 'PasswordText']; // <-- this is new!

function WSSecurity(username, password, options) {
  options = options || {};
  this._username = username;
  this._password = password;

  //must account for backward compatibility for passwordType String param as well as object options defaults: passwordType = 'PasswordText', hasTimeStamp = true
  if (typeof options === 'string') {
    this._passwordType = options ? options : 'PasswordText';
  } else {
    this._passwordType = options.passwordType ? options.passwordType : 'PasswordText';
  }

  if (validPasswordTypes.indexOf(this._passwordType) === -1) {
    this._passwordType = 'PasswordText';
  }

  this._hasTimeStamp = options.hasTimeStamp || typeof options.hasTimeStamp === 'boolean' ? !!options.hasTimeStamp : true;
}

[jayharper]

@herom thanks for cleaning this up 👍. I have incorporated your changes and we should now be good to go.

[herom]

Thanks a ton 👍 👯

[jsdevel]

👯