Fix cryptographically insecure ID generation

[tomauth0]

Description

The existing methods of ID generation are not cryptographically secure. This PR introduces the id-generation lib - a lightweight lib for generating suitably random IDs with a given dictionary. Utils.js updated to make use of this lib

Also used the opportunity to cleanup some of the utils file - swapping out vars and removing some unneeded syntax

References

#111

Testing

Unit tests added for ID length and dictionary memebership

• This change adds test coverage for new/changed/fixed functionality

Checklist

• I have added documentation for new/changed functionality in this PR or in auth0.com/docs
• All active GitHub checks for tests, formatting, and security are passing
• The correct base branch is being used, if not master

[jakelacey2012]

I thought I'd have a go at reviewing some code today, let me know what you think of my comments. I don't think I'm comfortable yet in hitting the approve button, so might be best to get another pair of eyes as well :)

[jakelacey2012]

Suggested change

	it('should generate an ID from the alphabet', function() {
	it('should generate an ID including characters from 0 to 9 and the alphabet', function() {

[tomauth0]

So, in this case the 'alphabet' refers to an 'alphabet' parameter on the ID Gen library, as opposed to the english alphabet - it tests that the characters in the resulting ID intersect with the chars passed in the alphabet param on IdGenerator creation

[jakelacey2012]

It looks like this bit of code is the same in the else statement the only difference is that we're unzipping it, I wonder if this bit of code could be abstracted into a function which is passed the result of .toString().

const xml = new xmldom.DOMParser().parseFromString(input);

if (!xml || !xml.documentElement) {
   return callback(new Error('Invalid SAML Request'));
}

configureSigningCredentials(xml, function (err, shouldValidate) {
   if (!shouldValidate) {
          return callback(null, xml);
   }
   checkSignature(xml, callback);
});

I think doing this should mean we can avoid setting xml = null in the next block as well.

[luuuis]

Well spotted. I usually "lift" the optional computation into a local const to keep the computation linear.

const input = new Buffer(samlRequest, 'base64');
const inflateIfNeeded = input[0] === 60 ? (cb) => setImmediate(cb, null, input) : zlib.inflateRaw;

inflateIfNeeded(input, function(err, inflated) {
  // shared logic
});

[jakelacey2012]

Thats nice, I think I prefer that way!

[tomauth0]

This is a great spot and change, but I'm apprehensive to apply it in this particular PR & ticket.
There's a few different areas in the utils file (and across the lib) that could do with a refactor (misspelled opts, etc.). I'll place a ticket on the backlog that references this suggestion and a few other proposals.

[jakelacey2012]

That sounds sensible to me. :)

[luuuis]

Well spotted. I usually "lift" the optional computation into a local const to keep the computation linear.

const input = new Buffer(samlRequest, 'base64');
const inflateIfNeeded = input[0] === 60 ? (cb) => setImmediate(cb, null, input) : zlib.inflateRaw;

inflateIfNeeded(input, function(err, inflated) {
  // shared logic
});