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

NotBefore and NotOnOrAfter - let me try again #35

Closed
wants to merge 3 commits into from

Conversation

mradamlacey
Copy link
Contributor

Let me try this again... don't think my changes made it in my previous pull request. Sorry/

This PR provides support for the Assertion conditions for NotBefore and NotOnOrAfter.

Some design decisions I'd love to hear some feedback on:

  • The condition checks are not opt-in - if NotBefore or NotOnOrAfter are present in the SAML response, they will be validated. You can maybe make the argument some clients might not want these checks, but if this is configurable to turn these off, the default should be on at least
  • The timestamp comparisons by default account for some clock skew between client and server. The default is 60 seconds. This might be too much for some, and not enough for others. Maybe the default clock skew should be 0 and keep this as a configuration option?
  • I added sinon to the unit tests to make it easy to mock the current date (since the code now relies on new Date() to compare against the SAML response validity timestamps
    I'd love to help out this great module with this support. Let me know if there's anything else I need to do.

thanks!

@ploer
Copy link
Contributor

ploer commented Jun 5, 2014

This looks great, thanks!

Some thoughts:

  • I am not sure we need to tolerate any clock skew by default -- I notice that many providers create a bracket of allowable time that basically anticipates some clock skew. For instance, in our example okta document they allow +/- 5 minutes from the moment of assertion, and the onelogin example allows +/- 3 minutes. So I think a default of 0 'additional skew' is probably appropriate.
  • I agree that these checks should be enabled by default, but I also think it should be easy to completely disable them. Perhaps a magic value of 'any' for acceptedClockSkewMs?

@mradamlacey
Copy link
Contributor Author

Just added 0 as the default for acceptedClockSkew.

And support for -1 as the magic value that disables all the timestamp checks.

@ploer
Copy link
Contributor

ploer commented Jun 5, 2014

Merged: 6e9571a

I made a couple of superficial changes, but also removed the use of isNAN() (since it seems to me that should be used specifically for checking for NaN values such as 1/0), errored if there are multiple conditions for some reason, and switched to some more succinct error strings. Let me know if you see a problem with any of those.

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.

None yet

2 participants