[WIP] Add verified access_token parameter authentication.

[simi]

Allow access_token authorization. This time secured and verified by /debug_token Facebook API call (APP_ID and scopes are compared).

TODO

• ensure this is the right way (please confirm)
  • @asmatameem Authentication fails when using it from Android or iOS SDK on client side #167
  • @felipekk Authentication fails when using it from Android or iOS SDK on client side #167
  • @damianorenfer access token in callback url #160
  • @fredkelly Auth hash from access_token? #90
• handle case when token is expired (it raises app_id doesn't match error)
• raise custom error when scopes are not met
• add tests
• refactor code

[damianorenfer]

Exactly what I was looking for, thanks!

[felipekk]

Works great!!!

[allaire]

@simi From my understanding, we need this PR to be able to use iOS/Android facebook login and authenticate with our API that uses omniauth-facebook, correct?

[mkdynamic]

@simi How does this avoid the attack described in https://github.com/mkdynamic/omniauth-facebook/wiki/Access-token-vulnerability:-CVE-2013-4593? Seems like it'd be vulnerable in the same way...

[allaire]

@mkdynamic it uses the /debug_token endpoint to validate

[mkdynamic]

@allaire But doesn't that only guarantees it's issued for the OAuth app in question? I don't see how that avoids the against the attack described in https://github.com/mkdynamic/omniauth-facebook/wiki/Access-token-vulnerability:-CVE-2013-4593, where somebody tricks somebody to click on a constructed callback URL which includes a valid access_token (but for the attacker). Could you elaborate on how that's avoided with this approach? Apologies if I'm missing something.

[allaire]

@mkdynamic The issue was that it was possible to use an access token from another app (the old way didn't validate what app the access_token was associated with). The /debug_token check this.

See https://developers.facebook.com/docs/facebook-login/access-tokens#debug

[mkdynamic]

@allaire Ah, I mis-remembered the attack in that CVE. You're correct in you assertion that checking the token via /debug_token avoids that one specifically. Thanks for the explanation.

The thing I was actually thinking of was exploiting the unsafe way some apps will link an access token passed during the callback phase to that application's current account/login. That enables an attacker to link their own (issued and valid for that same FB OAuth client) access token to a victim's account on that application. Currently this is prevented via the CSRF mechanism, but in this PR that is disabled so I believe it opens that vulnerability up. Perhaps it's enough to explicitly explain that this practice is unsafe, but just want to be explicit about it.

[mkdynamic]

There are so many ways to open vulnerabilities in OAuth 2.0, that I sometimes loose track :)

[simi]

Thanks @allaire, you're right! @mkdynamic this was consulted with @homakov also, but I'm not sure if I mentioned that CSRF protection will be disabled for this. I'll try to rethink this again.

@allaire can you confirm this works for you meanwhile?

[allaire]

@simi doing this today! Would it be wise to rebase from master?

[simi]

@allaire I just force pushed sync with master.

[allaire]

@simi Got it to work by changing this line to:

missing_scopes = authorize_params.scope.split(',').collect(&:strip) - token_info.parsed.fetch("data", {}).fetch("scopes", [])

for some reason, there's a space before the "email" scope:

>> authorize_params.scope.split(',')
=> ["public_profile", " email"]```

[allaire]

So, the way to fix it is to add collect(&:strip) or split with the space split(', ')

[simi]

that's weird, but it is ok to use strip here

[allaire]

@simi Anyway you can add the strip to that PR?

[simi]

done

[allaire]

@simi Everything is running smooth! I'm available for additional testing to merge this 👍

[allaire]

@simi Any ETA when we can get this merged? Can I help with something in particular?

Thanks!

[amrnt]

I'm adding my voice to @allaire! I tested the branch and its smoothly working! Any plans to merge it to the master soon?

[simi]

Sure, there's plan to merge this, but I'm still not sure if this approach is secure enough to recommend it.

[allaire]

maybe @homakov could chime in?

[homakov]

If CSRF protection is still enabled, having this with /debug_token is fine. @allaire

[allaire]

Thanks for your input @homakov!

[allaire]

@simi So homakov just confirmed it's fine, what can we do to help? :)

[hugobast]

@allaire @simi I can take over this PR and finish these outstanding items:

• handle case when token is expired (it raises app_id doesn't match error)
• raise custom error when scopes are not met
• add tests
• refactor code

I really want this to be part of a release and not have to specify a branch in my Gemfile. Really motivated over here so let me know if it's worthwhile to do it and how to go about it, should I fork and redo a PR from master?

[allaire]

@hugobast Would love to see this merged too! I'll let @simi answer the PR question.

[simi]

@hugobast @allaire hello! I totally forgot about this. Feel free to continue on this. First. can you ensure that CSRF is enabled for that request as was mentioned in #174 (comment)?

[hugobast]

I will. I can't commit to this branch so a fork and then a second PR sounds good to you guys?

[simi]

Feel free to open feature branch in your fork and point us there here. Let's keep this PR opened with all history. I'll merge it here when it will be ready.

[allaire]

Could this be rebased from master @simi?

[hugobast]

I never made good on what I said. Got caught up with other things, I'll try and get something going.

[killerham]

Is this gong to be merged in any time soon?

[augustosamame]

+1

[carlows]

Any updates of this PR?

[allaire]

If anyone is wondering, we are using this branch in production for a while now. We had to do some tweaks with the settings to make it works with latest FB API, here's our Devise config:

  config.omniauth :facebook, ENV["FACEBOOK_APP_ID"], ENV["FACEBOOK_APP_SECRET"],
                  client_options: {
                    site: "https://graph.facebook.com/v2.6",
                    authorize_url: "https://www.facebook.com/v2.6/dialog/oauth",
                    token_url: "oauth/access_token"
                  },
                  token_params: { parse: :json },
                  scope: "public_profile, email",
                  secure_image_url: true,
                  info_fields: "first_name,last_name,picture,name,email"

the token_params: { parse: :json } part is important

[spaghetticode]

I'd love to use this feature! Hope to see it merged soon

[ersinakinci]

Any progress?

[danieloprado]

How to use it?

[danieloprado]

Error:
NoMethodError: undefined method `underscore' for nil:NilClass

stack

…ers/devise_token_auth/omniauth_callbacks_controller.rb: 149:in `devise_mapping'
…gems/devise-4.2.0/app/controllers/devise_controller.rb:  26:in `_prefixes'
….0/gems/actionview-4.2.9/lib/action_view/view_paths.rb:  60:in `lookup_context'
…3.0/gems/actionview-4.2.9/lib/action_view/rendering.rb:  29:in `process'
…/gems/skylight-1.4.0/lib/skylight/probes/middleware.rb:  31:in `call'
…uby/2.3.0/gems/omniauth-1.3.1/lib/omniauth/strategy.rb: 408:in `call_app!'
…uby/2.3.0/gems/omniauth-1.3.1/lib/omniauth/strategy.rb: 362:in `callback_phase'
…mniauth-oauth2-1.4.0/lib/omniauth/strategies/oauth2.rb:  75:in `callback_phase'
…ebook-f4d0627604d2/lib/omniauth/strategies/facebook.rb:  68:in `callback_phase'
…uby/2.3.0/gems/omniauth-1.3.1/lib/omniauth/strategy.rb: 227:in `callback_call'
…uby/2.3.0/gems/omniauth-1.3.1/lib/omniauth/strategy.rb: 184:in `call!'

[enewbury]

@mkdynamic I made a PR to address the items left in the checklist, and tested it working on my project.
#315

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.