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

Do not try to verify id_token when id_token is not used #41

Merged
merged 1 commit into from
Nov 9, 2019

Conversation

m0n9oose
Copy link
Collaborator

@m0n9oose m0n9oose commented Sep 7, 2019

fix #40

@m0n9oose m0n9oose force-pushed the fix_redundant_token_verification branch from 7114c6c to a6779dd Compare September 7, 2019 10:04
@Sutto
Copy link
Contributor

Sutto commented Sep 19, 2019

+1 to this - We hit this trying to integrate a Keycloak SSO client with our app, as the default flow won't return an id_token, so without this it crashes with an internal exception.

@m0n9oose
Copy link
Collaborator Author

@Sutto have you tried to use branch with fix?

gem 'omniauth_openid_connect', git: 'https://github.com/m0n9oose/omniauth_openid_connect.git', branch: 'fix_redundant_token_verification'

@Sutto
Copy link
Contributor

Sutto commented Sep 20, 2019

Yehp! The fix works perfectly in our case, thank you - It'd be nice to see this in a released gem version, so we don't have to depend on a git repo.

Thanks for building this btw!

@swapab
Copy link

swapab commented Sep 26, 2019

@m0n9oose Thanks for maintaining omniauth_openid_connect gem.

The proposed change complies to openid-specs and should be merged. But, there is slight caveat which probably needs to be addressed as well.
The call valid_response_type? should be removed. According to the openid-documentation and examples response_type is not returned in the Location header of the response.

Rather the validation should be in request_phase instead ofcallback_phase.
The RFC also says the same: https://tools.ietf.org/html/rfc6749


Let me know what you think about it and I can create a new pull request.

@m0n9oose
Copy link
Collaborator Author

@m0n9oose Thanks for maintaining omniauth_openid_connect gem.

The proposed change complies to openid-specs and should be merged. But, there is slight caveat which probably needs to be addressed as well.
The call valid_response_type? should be removed. According to the openid-documentation and examples response_type is not returned in the Location header of the response.

Rather the validation should be in request_phase instead ofcallback_phase.
The RFC also says the same: https://tools.ietf.org/html/rfc6749

Let me know what you think about it and I can create a new pull request.

@swapab thanks for your feedback and efforts to gather all this information, this is an excellent example of productive discussion!

I would be very appreciated if you can fork this repo and open a new pull request with additional improvements.

@m0n9oose
Copy link
Collaborator Author

@swapab any luck with implementation according to the RFC?

@m0n9oose m0n9oose merged commit d76e82b into master Nov 9, 2019
@m0n9oose m0n9oose deleted the fix_redundant_token_verification branch November 9, 2019 09:11
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.

Issue with parsing id_token parameter in 'code' response_type use cases
3 participants