zquestz / omniauth-google-oauth2

Oauth2 strategy for Google
1.45k stars 413 forks source link

Support oauth2 v2.0.6+ #429

Closed JacobMarq closed 2 years ago

JacobMarq commented 2 years ago

Solves issue #425 to provide complete support for latest version v2.0.6+ !! Breaking changes for OAuth2 versions <2.0.6

Reasons for changes


OAuth2 v2.0.5 introduced a breaking change. addressed with the following changes:

# ./lib/omniauth/strategies/google-oauth2.rb
hash[:id_token] = access_token.token
if !options[:skip_jwt] && !nil_or_empty(access_token.token)
  decoded = ::JWT.decode(access_token.token, nil, false).first

#  Initializing AccessToken with nil id_token, using workaround or v2.0.6+, returns empty string instead of nil
def nil_or_empty(obj)
  obj.is_a?(String) ? obj.empty? : obj.nil?
end

NOTE: I'm not certain if there are edge cases where access_token.token may still return nil, given the changes to oauth2, so nil_or_empty handles checking for both.

As well as a regression that prevents initializing AccessToken without 'id_token':

# oauth2/lib/oauth2/access_token.rb v2.0.5
if @client.options[:raise_errors] && (@token.nil? || @token.empty?)
  error = Error.new(opts)
  raise(error)
end

Fixed for refresh_token work flows in v2.0.6:

# oauth2/lib/oauth2/access_token.rb v.2.0.6
no_tokens = (@token.nil? || @token.empty?) && (@refresh_token.nil? || @refresh_token.empty?)
if no_tokens
  if @client.options[:raise_errors]
    error = Error.new(opts)
    raise(error)
  else
    warn('OAuth2::AccessToken has no token')
  end
end

This code led to multiple test failures that required the following changes:

I am open to and appreciate any feedback on the changes.

zquestz commented 2 years ago

This looks great, should be mergeable once the OmniAuth version change is reverted.

wbotelhos commented 2 years ago

@JacobMarq Now I'm receiving Invalid segment encoding when I try to log in. Any idea or doc on how to upgrade to this last version? Thank you! :)

JacobMarq commented 2 years ago

@wbotelhos Sounds similar to an issue another user was having #430. A recent PR #431, merged shortly after your comment, should fix things for you. If you are still experiencing any problems, I'd be happy to help!

patatepartie commented 2 years ago

@JacobMarq I had the same error as @wbotelhos and version 1.1.1 fixed it. Thank you all for fixing it so quickly!

wbotelhos commented 1 year ago

It worked, @JacobMarq . Thanks! :)