Skip to content

Commit

Permalink
Merge pull request #70 from intridea/dont-override-callback-url
Browse files Browse the repository at this point in the history 
Don't override callback_url Attempt to correct #28
  • Loading branch information
@sferik
sferik committed Oct 21, 2015
2 parents 8f3b9e3 + 85fdbe1 commit 2615267
Showing 1 changed file with 0 additions and 4 deletions.
4 changes: 0 additions & 4 deletions lib/omniauth/strategies/oauth2.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ def client
::OAuth2::Client.new(options.client_id, options.client_secret, deep_symbolize(options.client_options))
end

def callback_url
full_host + script_name + callback_path
end

credentials do
hash = {"token" => access_token.token}
hash.merge!("refresh_token" => access_token.refresh_token) if access_token.expires? && access_token.refresh_token
Expand Down

21 comments on commit 2615267

@libermans
Copy link

Choose a reason for hiding this comment

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

@sferik The redefinition of callback_url was deleted from OmniAuth::Strategies::OAuth2 while it was used in the strategy to get redirect_uri, which should be the same redirect_uri which we sent to facebook without params (code, etc). That's why the callback_url was redefined. Right now omniauth-facebook login, and omniauth-google login don't work.

The fastest fix would be to create a new function in the strategy: redirect_uri, and define it as a "full_host + script_name + callback_path", and then use it in build_access_token and request_phase methods (instead of callback_url).

@libermans
Copy link

Choose a reason for hiding this comment

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

Checked this solution with omniauth-facebook. It works. Although we should check how callback_url was used by all the gems...

@libermans
Copy link

Choose a reason for hiding this comment

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

https://github.com/search?l=ruby&q=%22callback_url%22+%22OmniAuth%3A%3AStrategies%3A%3AOAuth2%22&ref=searchresults&type=Code&utf8=%E2%9C%93

321 files...

The biggest problem could be with these projects, which redefined build_access_token
https://github.com/search?utf8=%E2%9C%93&q=%22callback_url%22+%22OmniAuth%3A%3AStrategies%3A%3AOAuth2%22+build_access_token&type=Code&ref=searchresults

For example basecamp:
https://github.com/Verano/omniauth-basecamp/blob/9bcb88e6472891236a7591ec5086e88654e52aa2/lib/omniauth/strategies/basecamp.rb

:redirect_uri => callback_url,

From these projects which didn't redefined build_access_token, the most usual case is that they used callback_url in same rudimentary way:
https://github.com/tkengo/omniauth-dropbox-oauth2/blob/eb7d19cff04b4046f8bba44903674e9340684e1e/lib/omniauth/strategies/dropbox_oauth2.rb

  def callback_url
    if @authorization_code_from_signed_request
      ''
    else
      options[:callback_url] || super
  end

and few of the projects use @authorization_code_from_signed_request or options[:callback_url]. So it won't change their work. But still some of the projects use these params...

@erpe
Copy link

@erpe erpe commented on 2615267 Nov 2, 2015

Choose a reason for hiding this comment

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

omniauth-gplus doesn't like this change... -> samdunne/omniauth-gplus#25

@wschenk
Copy link

@wschenk wschenk commented on 2615267 Nov 3, 2015

Choose a reason for hiding this comment

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

This also breaks omniauth-instagram

@chussenot
Copy link

Choose a reason for hiding this comment

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

breaks linkedin and facebook callbacks for us...

  'omniauth-facebook',            '~> 2.0.1'
  'omniauth-linkedin-oauth2',     '~> 0.1.5'

@ananova
Copy link

@ananova ananova commented on 2615267 Dec 2, 2015

Choose a reason for hiding this comment

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

Just noting that this also broke our integration with lightspeed; the omniauth-lightspeed gem.

@RochesterinNYC
Copy link

Choose a reason for hiding this comment

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

Noting that this broke integration with the cf-uaa-lib gem.

@jerryluk
Copy link

Choose a reason for hiding this comment

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

This breaks our OAuth 2 gem as well

'omniauth-edmodo'

@mechiland
Copy link

Choose a reason for hiding this comment

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

This break out OAuth 2 gem as well

omniauth-jinshuju

@mcmire
Copy link

@mcmire mcmire commented on 2615267 Jan 25, 2016

Choose a reason for hiding this comment

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

@sferik Can you or one of the other maintainers here elaborate on why this change was necessary? Is there a better way to fix #28 without doing this? Can you place something in the README about this? A lot of people are clearly getting hit by this and it's causing a lot of time wasted.

@cconstantine
Copy link

Choose a reason for hiding this comment

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

Bump. This broke login for my app. Are there any active maintainers left on this project?

@knu
Copy link

@knu knu commented on 2615267 Nov 14, 2016

Choose a reason for hiding this comment

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

D'oh, just figured out how our OAuth2 consumer got broken, by this.

@knu
Copy link

@knu knu commented on 2615267 Nov 14, 2016

Choose a reason for hiding this comment

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

Workaround 1:

gem 'omniauth-oauth2', '1.3.1'

Workaround 2:
Put the following in your strategy class:

      def callback_url
        full_host + script_name + callback_path
      end

The example in README no longer works without this.

@knu
Copy link

@knu knu commented on 2615267 Dec 16, 2016

Choose a reason for hiding this comment

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

I submitted #100 last month as a fix for this, which also covers the original intention in #28.

@ryenski
Copy link

Choose a reason for hiding this comment

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

This release breaks a bunch of integrations and should be yanked.

@cconstantine
Copy link

Choose a reason for hiding this comment

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

Please don't yank. If you want to revert please release a new version with a minor version bump. That way people who don't have broken integrations (or those who can't update right this instant) can continue to install the gem.

@ryenski
Copy link

@ryenski ryenski commented on 2615267 Jan 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

What is the path for fixing this? Do the individual integrations need to be fixed? If so, what is the fix? I don't want to be stuck on an old version if a future update is released and this is still broken.

@pboling
Copy link
Member

Choose a reason for hiding this comment

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

@sferik This change breaks omniauth-greenhouse, and does't follow the oauth 2.0 spec.

redirect_uri
REQUIRED, if the "redirect_uri" parameter was included in the
authorization request as described in Section 4.1.1, and their
values MUST be identical.

@bnorton
Copy link

@bnorton bnorton commented on 2615267 Jun 4, 2020

Choose a reason for hiding this comment

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

this also broke omniauth-hubspot but was "fixed" with this commit

@pboling
Copy link
Member

@pboling pboling commented on 2615267 Apr 1, 2021

Choose a reason for hiding this comment

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

Ping @sferik Is there a way we can change this again to comply with the OAuth 2.0 Spec?

redirect_uri
REQUIRED, if the "redirect_uri" parameter was included in the
authorization request as described in Section 4.1.1, and their
values MUST be identical.

Please sign in to comment.