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

[SDK-974] Improved OIDC compliance #1059

Merged
merged 6 commits into from
Dec 11, 2019
Merged

[SDK-974] Improved OIDC compliance #1059

merged 6 commits into from
Dec 11, 2019

Conversation

stevehobbsdev
Copy link
Contributor

@stevehobbsdev stevehobbsdev commented Dec 9, 2019

Changes

  • Bump idtoken-verifier to 2.0.0 for [SDK-974] Improved OIDC compliance idtoken-verifier#77
  • Fixed tests as a result. These were mostly problems with the expiration date claim, is the tests were previously specifying __disableExpirationCheck, which has now been removed from idtoken-verifier
  • Updated the HS256 "nonce mismatch" error to match the IDTV table

Testing

Unit tests were updated, tested manually in Chrome + IE11 to see that the ID token validation checks still passed at runtime.

  • This change adds unit test coverage
  • This change adds integration test coverage

Checklist

@stevehobbsdev stevehobbsdev added this to the vNext milestone Dec 9, 2019
@stevehobbsdev stevehobbsdev requested a review from a team December 9, 2019 12:38
@stevehobbsdev stevehobbsdev added small and removed tiny labels Dec 9, 2019
@joshcanhelp joshcanhelp requested review from joshcanhelp and removed request for a team December 10, 2019 15:44
Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment around the validation happening here. Approving for now as it isn't a blocker.

if (
validationError.error !== 'invalid_token' ||
validationError.errorDescription === 'Nonce does not match.'
(validationError.errorDescription &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider breaking this logic out into a variable for more clarity.

Also ... why are we checking the description as well if we hav an error code for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just modifies existing logic rather than rewriting it. We do have an error code but it's the same error code for every error. I don't think the check here is terrible, but it could be refactored later if desired.

@@ -414,7 +437,8 @@ WebAuth.prototype.validateToken = function(token, nonce, cb) {
jwksURI: this.baseOptions.jwksURI,
audience: this.baseOptions.clientID,
leeway: this.baseOptions.leeway || 0,
__disableExpirationCheck: this.baseOptions.__disableExpirationCheck
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@stevehobbsdev stevehobbsdev merged commit 57ea1c8 into master Dec 11, 2019
@stevehobbsdev stevehobbsdev deleted the feature/idtv branch December 11, 2019 10:21
peterblazejewicz added a commit to peterblazejewicz/DefinitelyTyped that referenced this pull request Jan 13, 2020
elibarzilay pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants