There's a line of code in the callback phase which raises an error: <div class="hi

Param code should only be required when response_type is code about omniauth_openid_connect HOT 2 CLOSED

omniauth commented on May 27, 2024

Param code should only be required when response_type is code

from omniauth_openid_connect.

Comments (2)

m0n9oose commented on May 27, 2024

But unless params[options.response_type] condition is incorrect if

It looks like it should be executed only if option :response_type is set to code

I'd say we can go with 2nd option, but it's worth to consider extracting check to separate method or at least local variable to make this check a bit more clear, like

valid_code = params.key?('code') && options.response_type == 'code'
return fail!(
  :missing_code,
  OmniAuth::OpenIDConnect::MissingCodeError.new(params['error'])
) unless valid_code

from omniauth_openid_connect.

januszm commented on May 27, 2024

Sure, to keep it simple we can add support for id_token first, like:

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?

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

but that would be a terrible code duplication, so later on we might want to support all possible grant types like this:

SUPPORTED_GRANT_TYPES = %w[code id_token token].freeze

# e.g. for `id_token token`, it is valid if:
(SUPPORTED_GRANT_TYPES & params.keys).sort.join(' ') == options.response_type

# the below would mean we implement some kind of errors array
# and return with fail! from the main callback_phase method if there's at least one
options.response_type.split(' ').each do |grant_type|
  unless params.key?(grant_type) && SUPPORTED_GRANT_TYPES.include?(grant_type)
    error_class = "OmniAuth::OpenIDConnect::Missing#{grant_type.classify}Error".constantize
    errors << { :"missing_#{grant_type}" => error_class.new(params['error']) }
  end
end