Skip to content
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

Support oauth2 v2.0.6+ #429

Merged
merged 7 commits into from
Sep 4, 2022
Merged

Conversation

JacobMarq
Copy link
Contributor

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:

  • tests using OAuth2::AccessToken.from_hash() now include access_token
  • tests using OAuth2::AccessToken.new() now include refresh_token
  • OAuth2::AccessToken.from_hash() now require client.options method, so tests for methods that call from_hash() now create a OAuth2::Client object instead of a symbol

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

 - apply changes to test suite for new parameters in  OAuth::Client and AccessToken
 - update access_token['id_token'] to access_token.token in strategies/google_oauth2.rb
 - alter access_token.token.nil? to nil_or_empty() due to aforementioned changes
 - alter test "...when the access token is empty or nil"
 - v2.0.6 fixed regression in v2.0.5 so test client is now initialized with refresh_token
 - update test "...when the access token is empty or nil"
 - remove redundant "let(:client) do" declaration
 - allow OAuth::Client initializations to raise error flags
 - convert unnecessary "let(:client) do" to instance variable
@@ -9,8 +9,8 @@ Gem::Specification.new do |gem|
gem.name = 'omniauth-google-oauth2'
gem.version = OmniAuth::GoogleOauth2::VERSION
gem.license = 'MIT'
gem.summary = %(A Google OAuth2 strategy for OmniAuth 1.x)
gem.description = %(A Google OAuth2 strategy for OmniAuth 1.x. This allows you to login to Google with your ruby app.)
gem.summary = %(A Google OAuth2 strategy for OmniAuth 2.x)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fixed, not sure why all these PR's had the same incorrect change. =\

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went ahead and made the correction. My original PR was based off of the other PR that didn't pass testing and I completely overlooked this more subtle mistake while focusing on the build breaking changes that were included.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#426

The PR I was building off of for reference

@zquestz
Copy link
Owner

zquestz commented Sep 3, 2022

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

@zquestz zquestz merged commit 08ac691 into zquestz:master Sep 4, 2022
@wbotelhos
Copy link

@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
Copy link
Contributor Author

@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
Copy link

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

@wbotelhos
Copy link

It worked, @JacobMarq . Thanks! :)

@JacobMarq JacobMarq deleted the support-oauth2-v2.0.6+ branch December 1, 2022 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants