Allow verified access token - Githubissues

simi / omniauth-facebook

Facebook OAuth2 Strategy for OmniAuth

https://simi.github.io/omniauth-facebook/

1.26k stars 403 forks source link

Allow verified access token #315

Closed enewbury closed 2 years ago

simi commented 5 years ago

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 commented 5 years ago

@enewbury rebased

simi commented 5 years ago

@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 :+1:

PS: Feel free to add changelog entry as well.

enewbury commented 5 years ago

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

enewbury commented 5 years ago

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

simi commented 5 years ago

Looks good to me. Can you please rebase?

@homakov is there any chance to take a look at this final change (related to https://github.com/mkdynamic/omniauth-facebook/pull/174#issuecomment-110944435).

enewbury commented 5 years ago

@homakov any update on this?

homakov commented 5 years ago

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

enewbury commented 5 years ago

@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 commented 5 years ago

@enewbury not sure what's unclear on Egor's response at https://github.com/mkdynamic/omniauth-facebook/pull/315#issuecomment-472911170. He's not going to review this.

enewbury commented 5 years ago

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

simi commented 5 years ago

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

enewbury commented 5 years ago

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 commented 4 years ago

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 commented 4 years ago

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

simi commented 4 years ago

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

LesterKim commented 4 years ago

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

fbalarini commented 4 years ago

@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 commented 4 years ago

Feel free to point to this branch and test.

dvkch commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 3 years ago

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

enewbury commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@enewbury thanks for taking a care of this.