Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Making sure access_token doesn't get built twice #96

Closed
wants to merge 4 commits into from

7 participants

@etagwerker

Hi!

I decided to add the condition after running into issue #73.

The solution turned out to be this one:

http://stackoverflow.com/a/14127277/615138

  • Ernesto
@mariusbutuc

@etagwerker ensure the tests pass too ;)

@felipecsl

I tested with this version, still got same problem

@etagwerker

@mariusbutuc good point. Tests pass now.

@etagwerker

@felipecsl are you sure that you aren't adding the middleware twice? That was my original problem.

@etagwerker

@mkdynamic tests pass now. Please let me know what you think!

@felipecsl

@etagwerker sorry my bad, I had a silly mistake, was using the wrong fb app secret key, once I figured it out and changed, everything worked!

.gitignore
@@ -6,3 +6,4 @@ pkg/*
.powenv
tmp
bin
+.rvmrc
@mkdynamic Owner

Can you remove this removal? Don't want to couple removing this file with this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mkdynamic mkdynamic commented on the diff
lib/omniauth/strategies/facebook.rb
@@ -230,6 +210,34 @@ def image_url uid, options
url.to_s
end
+
+ protected
@mkdynamic Owner

Just checking, why change this method to be protected?

@simi Collaborator
simi added a note

@mkdynamic @simi Sorry for the delay, I changed this method to be protected because of this:
https://github.com/intridea/omniauth-oauth2/blob/8a7a97ecbdb71c04b96bfa47dbc465d5d931f694/lib/omniauth/strategies/oauth2.rb#L89

As you can see OmniAuth::Strategies::OAuth2 defines #build_access_token as a protected method.

On the other hand, OmniAuth::Strategies::Facebook changes the visibility of the method.

Do you guys have a good reason to do that?

@mkdynamic Owner

Ah, no that is unintentional. We should (separately) change that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@simi
Collaborator

@mkdynamic do you think this will improve something? I'll rebase if you're ok with this. I understand that change, but I've never run into this issue by myself.

@mkdynamic
Owner

Appreciate the work on this, but I'm not convinced to merge for a few of reasons:

  • It changes the semantics of #build_token from those of the omniauth-oauth2 implementation (which does build the token every time)
  • I am fairly certain it does not fix #73, which is related to the automatic CSRF protection not working with the client-side flow.
  • The stackoverflow solution linked seems unrelated (perhaps I am missing something).
  • I haven't seen a cohesive description of the problem this fixes and why it fixes it.

So, I'm inclined to close this for now. Chime in if I am missing something obvious.

@mkdynamic mkdynamic closed this
@mcnute

I used this railscast to build a client login flow for a facebook canvas app http://railscasts.com/episodes/360-facebook-authentication
My App is build on top of rails 4.0.2. The omniauth-facbook gem is the latest 1.6.0

I had the same csrf detection error problem with it. Although I downloaded the source of the railscast and got it working with the same exact gem versions (omniauth-facebook, omniauth, oauth2, omniauth-oauth2) of my project. I got it working locally and in the facebook canvas environment with tunnelss (ssl for local development sites) and pow.

This behavior is not due to the gem version nor the facebook api.
This must be related how rails has changed from version 3.2.x to 4.0.x. Probably how CSRF handling has changed.
I will post an update if the culprit emerges on the search.

@rajsinghtut

@mcnute I'm having the same issue. Did you fine the culprit?

@mcnute
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 2, 2013
  1. @etagwerker

    Moved build_access_token to protected area. Added a condition to avoi…

    etagwerker authored
    …d building access token twice.
Commits on Jan 22, 2013
  1. @etagwerker
  2. @etagwerker
Commits on Dec 2, 2013
  1. @etagwerker

    Revert "Added .rvmrc to .gitignore."

    etagwerker authored
    This reverts commit a780a1b.
This page is out of date. Refresh to see the latest.
Showing with 31 additions and 23 deletions.
  1. +28 −20 lib/omniauth/strategies/facebook.rb
  2. +3 −3 test/test.rb
View
48 lib/omniauth/strategies/facebook.rb
@@ -56,26 +56,6 @@ def raw_info
@raw_info ||= access_token.get('/me').parsed || {}
end
- def build_access_token
- if access_token = request.params["access_token"]
- ::OAuth2::AccessToken.from_hash(
- client,
- {"access_token" => access_token}.update(access_token_options)
- )
- elsif signed_request_contains_access_token?
- hash = signed_request.clone
- ::OAuth2::AccessToken.new(
- client,
- hash.delete('oauth_token'),
- hash.merge!(access_token_options.merge(:expires_at => hash.delete('expires')))
- )
- else
- with_authorization_code! { super }.tap do |token|
- token.options.merge!(access_token_options)
- end
- end
- end
-
def request_phase
if signed_request_contains_access_token?
# if we already have an access token, we can just hit the
@@ -230,6 +210,34 @@ def image_url uid, options
url.to_s
end
+
+ protected
@mkdynamic Owner

Just checking, why change this method to be protected?

@simi Collaborator
simi added a note

@mkdynamic @simi Sorry for the delay, I changed this method to be protected because of this:
https://github.com/intridea/omniauth-oauth2/blob/8a7a97ecbdb71c04b96bfa47dbc465d5d931f694/lib/omniauth/strategies/oauth2.rb#L89

As you can see OmniAuth::Strategies::OAuth2 defines #build_access_token as a protected method.

On the other hand, OmniAuth::Strategies::Facebook changes the visibility of the method.

Do you guys have a good reason to do that?

@mkdynamic Owner

Ah, no that is unintentional. We should (separately) change that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ # Builds access_token only if it's necessary
+ # and it hasn't been built already
+ def build_access_token
+ if self.access_token
+ self.access_token
+ else
+ if access_token = request.params["access_token"]
+ ::OAuth2::AccessToken.from_hash(
+ client,
+ {"access_token" => access_token}.update(access_token_options)
+ )
+ elsif signed_request_contains_access_token?
+ hash = signed_request.clone
+ ::OAuth2::AccessToken.new(
+ client,
+ hash.delete('oauth_token'),
+ hash.merge!(access_token_options.merge(:expires_at => hash.delete('expires')))
+ )
+ else
+ with_authorization_code! { super }.tap do |token|
+ token.options.merge!(access_token_options)
+ end
+ end
+ end
+ end
end
end
end
View
6 test/test.rb
@@ -478,13 +478,13 @@ def setup
end
test 'returns a new access token from the signed request' do
- result = strategy.build_access_token
+ result = strategy.send(:build_access_token)
assert_kind_of ::OAuth2::AccessToken, result
assert_equal @payload['oauth_token'], result.token
end
test 'returns an access token with the correct expiry time' do
- result = strategy.build_access_token
+ result = strategy.send(:build_access_token)
assert_equal @payload['expires'], result.expires_at
end
end
@@ -499,7 +499,7 @@ def setup
end
test 'returns a new access token' do
- result = strategy.build_access_token
+ result = strategy.send(:build_access_token)
assert_kind_of ::OAuth2::AccessToken, result
assert_equal 'm4c0d3z', result.token
end
Something went wrong with that request. Please try again.