Add support for response_type id_token

[januszm]

Current implementation will always trigger the failure action in the client for the Implicit grant. It's because we call Omniauth::Strategy .fail! method if parameter code is missing.

Resolves: #31

For the Implicit grant, the id_token or both id_token and access_token may be returned.
NOTICE: when response_type is set to token, then access_token parameter is used (Bearer token).

A much simpler implementation to handle just the 2 cases, code or id_token could look like:

def callback_phase
  # ...
  return fail!(:missing_code, OmniAuth::OpenIDConnect::MissingCodeError.new(params['error'])) unless valid_code?
  return fail!(:missing_id_token, OmniAuth::OpenIDConnect::MissingIdTokenError.new(params['error'])) unless valid_id_token?
  # ...
end

def valid_code?
  options.response_type == 'code' ? params.key?('code') : true
end
def valid_id_token?
  options.response_type == 'id_token' ? params.key('id_token') : true
end

Let me know which option would you prefer to start with

[januszm]

I didn't have a chance and I won't test the token/access_token-only scenario unfortunately.

[januszm]

This looks like a minor version update, new feature, backwards compatible.

[januszm]

Hmm we need to return from the callback_phase just before:

client.authorization_code = authorization_code
access_token
super

Maybe with:

return something unless options.response_type.to_s == 'code'

And for Implicit Grant, super will execute:

def callback_phase
  env['omniauth.auth'] = auth_hash
  call_app!
end

# =>

def auth_hash
  hash = AuthHash.new(:provider => name, :uid => uid)
  hash.info = info unless skip_info?
  hash.credentials = credentials if credentials
  hash.extra = extra if extra
  hash
end

so we need to deal with quite a few things, like

def user_info
  @user_info ||= access_token.userinfo!
end

(define credentials, info and extra variants for Implicit Grant flow)

[krzysiek1507]

It doesn't work when response_type: %i[id_token code]

undefined method `[]' for nil:NilClass
NoMethodError (undefined method `[]' for nil:NilClass):
gems/ruby-2.3.8/bundler/gems/omniauth_openid_connect-3f52ccda27ba/lib/omniauth/strategies/openid_connect.rb:319:in `valid_response_type?'
gems/ruby-2.3.8/bundler/gems/omniauth_openid_connect-3f52ccda27ba/lib/omniauth/strategies/openid_connect.rb:117:in `callback_phase'
omniauth (1.9.0) lib/omniauth/strategy.rb:238:in `callback_call'

[krzysiek1507]

That's weird that this change was needed. 0.3.2 worked with: response_type: %i[id_token code]. I guess id didn't work with response_type: :id_token.

[Eric-Guo]

it didn't work with response_type: :code either

[m0n9oose]

It doesn't work when response_type: %i[id_token code]

Before this change, only code was a valid response type. Now it's code & id_token. So yeah, it's okay if it's not working with multiple values, there's no implementation of that.

That's weird that this change was needed. 0.3.2 worked with: response_type: %i[id_token code]. I guess id didn't work with response_type: :id_token.

Not really, we were ignoring unknown types.

it didn't work with response_type: :code either

Will check, thanks!

[januszm]

Changes requested in the code review did not include handling both symbol and string version of the exception key for invalid response_type param, here:

error_attrs = RESPONSE_TYPE_EXCEPTIONS[options.response_type]
we're not casting the response_type option into string (above, we define exception key as a string ('id_token' => { exception_class: OmniAuth...)

[januszm]

@krzysiek1507 @Eric-Guo try this: #35 , support for both symbol and string was lost during the code review phase for this PR.

[krzysiek1507]

Before this change, only code was a valid response type. Now it's code & id_token. So yeah, it's okay if it's not working with multiple values, there's no implementation of that.

But it worked well! I need it so let me try to implement it.

[m0n9oose]

Still have below error:

318 319  | error_attrs = RESPONSE_TYPE_EXCEPTIONS[options.response_type]         fail!(error_attrs[:key], error_attrs[:exception_class].new(params['error']))

/Users/guochunzhong/git/oss/omniauth_openid_connect/lib/omniauth/strategies/openid_connect.rb:319:in `valid_response_type?' 
/Users/guochunzhong/git/oss/omniauth_openid_connect/lib/omniauth/strategies/openid_connect.rb:117:in `callback_phase' 

make sure you're on latest master version. It looks like you still using same version as before.

[Eric-Guo]

Now another error:

    def raise_out!
      raise(env['omniauth.error'] || OmniAuth::Error.new(env['omniauth.error.type']))
    end

omniauth (1.9.0) lib/omniauth/failure_endpoint.rb:25:in `raise_out!' 
omniauth (1.9.0) lib/omniauth/failure_endpoint.rb:20:in `call' 
omniauth (1.9.0) lib/omniauth/failure_endpoint.rb:12:in `call' 
omniauth (1.9.0) lib/omniauth/strategy.rb:491:in `fail!' 
/Users/guochunzhong/git/oss/omniauth_openid_connect/lib/omniauth/strategies/openid_connect.rb:319:in `valid_response_type?' 
/Users/guochunzhong/git/oss/omniauth_openid_connect/lib/omniauth/strategies/openid_connect.rb:117:in `callback_phase' 

[Eric-Guo]

Acturally, the code in the URL (https://portal.test/auth/openid_connect/callback?code=ZvME77cBxeR9jkg6NsvVH-21O1KhJWAaIkGI5tx5fAg&state=db31263029df1c105792dc8632b90a10)

[krzysiek1507]

To fix response_type: :code/'code' we need to merge #34