Allow verified access token

[enewbury]

No description provided.

[simi]

Hello. Thanks for this pull request.

I'll try to rebase the base branch to get rid of unrelated commits and take a look for you changes.

[simi]

@enewbury rebased

[simi]

@enewbury I think this is really nice follow-up of my initial PoC branch. I have left you few minor comments. If you can keep API backwards compatible, we don't need to release major version and it will be easier for everyone to update.

Anyway thanks for this nice pull request 👍

PS: Feel free to add changelog entry as well.

[enewbury]

Sorry, I was traveling overseas, I'll make your requested changes.

[enewbury]

@simi just checking in to make sure this doesn't fall through the cracks

[simi]

Looks good to me. Can you please rebase?

@homakov is there any chance to take a look at this final change (related to #174 (comment)).

[enewbury]

@homakov any update on this?

[homakov]

@enewbury Sorry, been away from oauth space and don't want to give a bad advice.

[enewbury]

@homakov Sorry to keep bugging you, but we'd prefer to be depending on the official version of this gem, rather than my fork. If you get a chance, we'd love to have this merged in.

[simi]

@enewbury not sure what's unclear on Egor's response at #315 (comment). He's not going to review this.

[enewbury]

@simi Oh gotcha, I thought he was saying, "sorry for the delay, give me a second to re-familiarize myself"

[simi]

We need to find out some way to get proper security review. Any idea how to proceed @enewbury?

[enewbury]

Well, I'm definitely no security expert. I get the gist of how oauth works, but I don't have a lot of connections in the security space. Are there other maintainers of this repo that are still active?

[dvkch]

Hi,

I'm trying out your PR as I'm currently creating an Android app associated to a Rails API that uses devise and omniauth-facebook to provide login support. Up until now it used the fbsr_XXX cookie to log a user in, and since mobile FB SDK don't provide an equivalent this looks promising.

I've hit a dead end on CSRF detection in oauth2.rb > def callback_phase from the omniauth-oauth2 lib, resulting in all logins hitting Csrf detected.

After some tests I have noted the following :

• frontend login using fbsr_XXX works
• provider_ignores_state: true fixes the issue with access_token login
• logging request.params["state"] shows it is empty, except if I pass a state parameter to my callback request from the Android app
• logging session["omniauth.state"] show it is always empty
• configuring MyApp::Application.config.session_store using cookie_store or cache_store, with my domain, domain: :all or no domain config doesn't solve it
• session["omniauth.state"] seems to only be set in oauth2.rb > def authorize_params in omniauth-oauth2, never in omniauth-facebook

This may not be the proper place to report this, but since your PR adds Be sure to leave CSRF protection on for this method of authentication to the new Android/iOS section in Readme.md I thought you would have a clue on what's going on @enewbury ? It seems the Android app is supposed to send the same state value as session["omniauth.state"], but I don't know how to define the latter.

Thanks for your input!

[LesterKim]

@simi Any updates here on getting this branch reviewed/approved for merge? Ideally, we could have this merged into master.

[simi]

I'm still a little concerned about security of this :/ Any idea how to move this @LesterKim?

[LesterKim]

@simi To move forward, how about merging the allow-verified-access-token branch in this repository into master?

[fbalarini]

@simi I'm trying to build a social login flow using a React Native app with a Rails API. I want to use this gem to verify the access token the app gets with the Facebook SDK. Is there any news regarding this PR? I think it will solve my issue...

[simi]

Feel free to point to this branch and test.

[dvkch]

The original maintainer of the fork being non responding for the moment I created another fork with the same changes but based on the latest omniauth-facebook 8.0 to handle the new avatar changes. Here it is : https://github.com/dvkch/omniauth-facebook/tree/allow-verified-access-token

I don't think I will maintain it a lot, as the work was originally done by @enewbury and approved by @simi, but I thought it could help some people while this PR is being rebased and merged

[simi]

@dvkch This is still on my radar, but I'm not still 100% sure this is secure enough and nobody proved that it is. Any experience from running this in production or any shared security research on this would make it easier to finally merge.

Next to this we will need documentation and example app updated.

[dvkch]

@simi good to know. we have been running it in prod for some time now, the only bump we encountered is the case where a user would login on the phone while deselecting some scopes, resulting in a "missing scope" error. Since our Rails app is API only we solved it by putting the strictly required scopes in our device omniauth config, and the list of scopes to hopefully request in the web, iOS and Android apps.

On the security side I do not know enough to give you a complete answer, all I can say is the debug_token endpoint used to check the access token does its job as advertised and seems to fix the case of anyone trying to send an invalid token.

The only other issue I can imagine is some MITM attack obtaining your access token when you're logging in and using it for themselves, but I would think there is the same risk when using a cookie.

[matiassalles99]

Im having the same issues with mobile apps, is this going to be merged any time soon? Or shall I use another fork?

[enewbury]

Hey folks, sorry I've been MIA. The truth is I did this for an old job, and I have almost no memory of what was happening with the Be sure to leave CSRF protection on for this method of authentication... did you figure it out @dvkch?
As far as security, yeah, i'm definitely no expert there, but this is just an implementation of what facebook recommended in their docs, so i would hope they aren't recommending something insecure?

[dvkch]

@enewbury I've just checked and the two projects I am using it in uses provider_ignores_state: true in its devise configuration. I can't say much more as I've not had my head in this for a while too :/

[enewbury]

I'm going to close this PR in favor of #377 so that's it a bit easier to review having all the changes together, and I've also pinged some folks at my consultancy to do a security review and maybe they can help determine if this is safe to include in main.

[simi]

@enewbury thanks for taking a care of this.