-
Notifications
You must be signed in to change notification settings - Fork 126
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
Discovery issues / bug #32
Comments
I can confirm that this code does not work, when the |
After reading the OIDC spec again, I am pretty sure that the issuer should be specified including the scheme part.
Since the issuer is also used to verify the ID token, those rules also have to apply here. Therefore I suggest to update the README (currently reading: "e.g. |
* Add support for response_type id_token * Simplify the response_type validation * Remove unnecessary statements from README
It looks like the logic here is expecting a
discover
option but the option is fordiscovery
:https://github.com/jjbohn/omniauth-openid-connect/blob/master/lib/omniauth/strategies/openid_connect.rb#L129
When I enabled discovery (after updating that logic to
discovery
) I noticed my issuer was failing to be discovered because it was missing https here as this logic expects a full URI (https://github.com/nov/openid_connect/blob/master/lib/openid_connect/discovery/provider/config/resource.rb#L11):https://github.com/jjbohn/omniauth-openid-connect/blob/master/lib/omniauth/strategies/openid_connect.rb#L79
When I added the https I noticed it failed here because the decoding expected the issuer to not have a protocol:
https://github.com/jjbohn/omniauth-openid-connect/blob/master/lib/omniauth/strategies/openid_connect.rb#L162
I noticed the readme shows an issuer without the protocol and given the openid configuration data from the discovery doc shows it without the https guessing it should be this way so the bug is probably just in that the discovery logic should append the protocol itself? (https://accounts.google.com/.well-known/openid-configuration)
New to OpenID Connect, sorry for the trouble.
The text was updated successfully, but these errors were encountered: