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

[peregrinator]

First off, thanks for continuing to maintain this code!

We currently have a 'code' based implementation working with this gem. When upgrading to the latest code we're running into issues with the following lines:

https://github.com/m0n9oose/omniauth_openid_connect/blob/e05e60ddbaa4374d6375eecd441fec46232c59fe/lib/omniauth/strategies/openid_connect.rb#L121-L124

We don't have an 'id_token' param and so decode_id_token attempts to parse nil which causes an error. A simple conditional check of the response type before calling decode_id_token fixes the error.

Wanted to raise it here though in case there is something I am missing. Thanks!

[iknowu10]

we have the same issue, and would be grateful if that easy fix can go it quickly.

[carlossilva]

Yep, same issue here. Simple fix to not have to monkey patch that class would be appreciated.

[m0n9oose]

@peregrinator @iknowu10 @carlossilva could you guys test this branch if it fixes the issue and there are no regressions?

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

[carlossilva]

Thank you @m0n9oose, works like a charm!

On a related note, in the callback_phase even for a non id_token response type we have an id token coming in the rack request request.env['omniauth.auth']['credentials']['id_token'] do you feel your gem should verify the id_token if there is one in this case as well?

[m0n9oose]

On a related note, in the callback_phase even for a non id_token response type we have an id token coming in the rack request request.env['omniauth.auth']['credentials']['id_token'] do you feel your gem should verify the id_token if there is one in this case as well?

Sorry, I'm not sure if I understand this. With this fix gem will try to verify id_token
only if your app has response_type = :id_token setting. If you use response_type = :code gem will skip verification step.

[carlossilva]

Right, but in my case even with response_type = :code I still get a response from the IDP that includes an id_token. Not necessary at all, most important is that your fix works and would be great to merge back!

Thanks again.

[m0n9oose]

I see. Not sure if this is good idea. Verification can throw an exception if somethings wrong with id_token and it could be a problem if you have response_type = :code and still getting an error because of unwanted id_token.

[carlossilva]

Sounds good, a merge would be great! Thanks @m0n9oose

[m0n9oose]

@peregrinator @iknowu10 any news?

[peregrinator]

Apologies - I’m currently traveling so won’t be able to test right away but the code changes look good to me. 👍

[m0n9oose]

@peregrinator I'd like to wait a bit more to allow you to test this branch on your app and make sure we haven't introduced new regressions. I have no live application and can't reproduce it in the wild.

[iceraluk]

@m0n9oose I've tested this and it all works fine now! Would be great if it can be merged in.

[peregrinator]

We've been running our fork with these changes for a bit now and haven't seen any problems. Should be good to merge!

[ahukkanen]

Bumped into this issue as well and the fix in the branch fix_redundant_token_verification seems to fix this.

[stanhu]

This pull request worked for me!

[stanhu]

@m0n9oose Would you be so kind as to merge this and cut a new release? Thanks!