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

Improved OIDC compliance #97

Merged
merged 3 commits into from
Nov 19, 2019
Merged

Improved OIDC compliance #97

merged 3 commits into from
Nov 19, 2019

Conversation

davidpatrick
Copy link
Contributor

@davidpatrick davidpatrick commented Nov 5, 2019

Changes

This update improves the SDK support for OpenID Connect. In particular, it modifies the sign in verification phase by substituting backchannel based checks with id_token validation.

References

Internal Docs

Testing

  • This change adds test coverage

  • This change has been tested on the latest stable version of Node.js

Checklist

@davidpatrick davidpatrick added medium Medium review CH: Added labels Nov 5, 2019
@davidpatrick davidpatrick requested a review from a team November 5, 2019 00:08
@lbalmaceda lbalmaceda changed the title Adds ID Token Validation Improved OIDC compliance Nov 5, 2019
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.

@davidpatrick - I might be missing something here ... added a number of comments here and I'll pause before continuing the review.

Also, it looks like you're testing the different parts of the verification but not the whole process (so we can make sure all this logic is for sure getting called). I was looking for a test to show me how jwt.verify() was getting called. The existing tests should help show if that's even possible and how.

lib/index.js Outdated Show resolved Hide resolved
lib/jwt.js Outdated Show resolved Hide resolved
lib/jwt.js Outdated Show resolved Hide resolved
lib/jwt.js Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
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.

Have a number of fixes that need to happen.

I'm just curious on the actual verification logic ... why not just copy in the JS sample? Would have saved some extra work and kept it standard. Besides the signature check, can use it almost exactly as-is.

lib/index.js Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
lib/jwt.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
test/jwt.test.js Outdated Show resolved Hide resolved
test/jwt.test.js Outdated Show resolved Hide resolved
test/jwt.test.js Outdated Show resolved Hide resolved
test/jwt.test.js Outdated Show resolved Hide resolved
test/strategy.test.js Outdated Show resolved Hide resolved
@davidpatrick davidpatrick force-pushed the idtoken_validation branch 3 times, most recently from aa3e358 to a96de9a Compare November 14, 2019 21:33
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.

Few more fixes here. Might want to get a Passport app running with the quickstart and make sure this branch works for you as well.

lib/jwt.js Show resolved Hide resolved
lib/jwt.js Outdated Show resolved Hide resolved
lib/jwt.js Outdated Show resolved Hide resolved
lib/jwt.js Outdated Show resolved Hide resolved
lib/index.js Show resolved Hide resolved
lib/index.js Show resolved Hide resolved
joshcanhelp
joshcanhelp previously approved these changes Nov 18, 2019
@joshcanhelp
Copy link
Contributor

@gkwang - Ready for your review!

lib/jwt.js Outdated Show resolved Hide resolved
lib/verifyWrapper.js Show resolved Hide resolved
@gkwang
Copy link

gkwang commented Nov 18, 2019

Can we also make sure we bump passport-oauth2's version to 1.5.0 before we release a new version? It contains all the PKCE changes. jaredhanson/passport-oauth2@v1.4.0...jaredhanson:v1.5.0
It doesn't have to be in this particular PR.

gkwang
gkwang previously approved these changes Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CH: Added medium Medium review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants