InResponseTo SubjectConfirmation validation support

[mradamlacey]

This PR is for support for validation the InResponseTo attribute and limited support for the SubjectConfirmation element of a SAML response.

• The Id attribute of generated SAML requests are cached in the module when support for the InResponseTo validation is turned on
• This cache of ids is set to expire (default is 8 hours)
• When processing a SAML response, the handler code will now:
  • Look for the InResponseTo in the root Response element and validate that it has been seen before (hit in the cache)
  • Look for the InResponseTo attribute in the SubjectConfirmationData element if supplied. This value must match the top level id and hit the cache as well
  • Remove the id from the cache if a response with a matching id has been processed
• Validation for InResponseTo is by default false, perhaps it is better to be more restrictive, although this could break existing clients

[ploer]

Looks pretty good!

Some thoughts:

• I agree with this being off by default. There are too many use cases where it won't work to have the state all be in-memory, such as the common scenario of multiple server instances.
• Maybe it would be simpler to just clean expired records every time we do an add, instead of setting timer events?
• It looks like when using validateInResponseTo, a response without any InResponseTo field is still allowed. This seems like it errors a little bit on the side of usability at the expense of paranoia, but I think I agree with it since we should be making it easy to configure the library to be 'as locked down as my identity provider supports', but still work on any identity providers that don't.
• npm run-script jshint will run the jshint checks for you locally, in order to make sure you will pass the Travis build checks (I always forget to do this though, should probably figure out how to wire it in to mocha)
• It looks like you are parsing out NotBefore/NotOnOrAfter fields in the SubjectConfirmation but then not doing anything with them. Perhaps we should always be checking these (i.e., as part of issue NotBefore and NotOnOrAfter - let me try again #35)?

[mradamlacey]

Regarding the in-memory cache of request ids... maybe the feature could provide the simple in-memory version for simple scenarios by default, but be extensible and allow the caller to provide an implementation for storing, checking for existence, and deleting from some cache of ids. Basically 'Bring Your Own Cache'. This should make the validation checking feasible for multiple servers.

Ok yes, my oversight on not running JSHint. Will get that cleaned up.

And yes, digging into the spec on SubjectConfirmation, I realized those validation timestamps can occur in multiple places in the SAML response. I will submit another PR to supplement issue #35 for this.

[mradamlacey]

latest changes:

• fixed JSHint failures
• provided a way to override the cache provider used when the in memory scenario won't work.
• I kept the timer mechanism to expire old cache values as I thought this belonged with the Cache Provider itself, and that SAML module shouldn't be concerned with this.

[ploer]

Awesome, working on integrating this.

I think, though, that we may want to allow for custom 'serialization providers' rather than custom 'cache providers'. In my thinking, a serialization provider just needs to know how to store and return values, which is all we really need, while a cache provider needs to have more complicated behavior that is aware of the semantics of what is being stored. Might as well keep the additional work as minimal as possible for anyone who needs to build glue code here, since the cache policy should always be the same.

I'll take a stab at that change and see how it works out.

[ploer]

After thinking about this some more, I think your way is better. It would be nice to take care of cache expiration so that anyone using this doesn't have to worry about it... but doing cache expiration efficiently probably requires knowledge of the underlying datastore.

[mradamlacey]

Just added some more commits that merge in from master all the latest to save you the trouble. Wasn't trivial to get the tests working again, it seems when using the useFakeTimers method in sinon it makes using setTimeout a little trickier.

[mradamlacey]

Ok, sorry for the quick succession of changes. Fixed JSHint errors, and realized I had some code duplication and parsing out the SubjectConfirmation elements, I consolidated this.

[ploer]

Oops, I had a very similar set of changes in progress, just hadn't gotten them checked in. :-)

I changed the interface to get rid of the 'exists' method, since 'get' provides the same information and I figure that can make it that much easier for someone to implement their own provider.

Also, I changed saml.js to set the createdAt time in the value field, and check it when it looks up a token. That way it's not relying on the provider's expiration policy to make the tokens invalid, and if a provider doesn't implement expiration, it will grow without bound but they will still expire as expected.

Just landed this; let me know if the reconciled version looks good to you.

[mradamlacey]

Checked out what was merged into master and I think there's one issue:

• Need to merge in my commit from above to fix all the unit tests, made a few changes getting useFakeTimers to work, and also fix a bug that wasn't executing all the 'captured saml responses' tests and fixed some issues in those tests

Other than that, looks good.

[mradamlacey]

One last thing - looks like README.md should be updated to reflect the cache provider API changes.

[ploer]

Updated the README.

I thought I had gotten the useFakeTimers changes sorted out during my merge. I see that you changed the time format, but I think that one's fine either way -- I actually kinda prefer the ISO format since it is easier to match to the xml.

Is there a part of the unit tests that isn't working right now?

[mradamlacey]

On line 119 of test/tests.js - only one of the 'captured saml responses' tests are executed. One of my later commits for this PR fixed this.

Once all of those tests are actually executing, you will see problems with setTimeout actually firing in the ResponseTo tests, and 3 of the 4 saml response tests are failing.

My later commits to this PR should resolve all those test issues.

[ploer]

Ah, got it, thanks. Should be fixed in the latest.