search

parse-community / parse-server

Parse Server for Node.js / Express
https://parseplatform.org
Apache License 2.0
20.7k stars 4.76k forks source link

fix: Facebook Limited Login not workind due to incorrect domain in JWT validation #9120

Closed chriscborg closed 1 month ago

chriscborg commented 1 month ago

Pull Request

Issue

Add support for Facebook auth JWT token #9117

Closes: #9117

Approach

A JWT token validation implementation seems to be already in place, however the host needs to be changed from facebook.com to www.facebook.com as suggested by @SebC99, because the old host is returning error 301 which is not followed by the jwt-rsa package.

Tasks

parse-github-assistant[bot] commented 1 month ago

Thanks for opening this pull request!

chriscborg commented 1 month ago

This is my first time contributing to the project so please bear with me. I have testing logging in with Facebook using an older version of my app without installing the latest Parse SDKs and Facebook SDK. I'm actually testing with the older ParseFacebookUtils. The login seems to have worked anyway. However I'm not sure if there are other tests I should make. It would be great is someone else tests this out just in case.

mtrezza commented 1 month ago

Sure, if you look at the CI there are 7 tests failing with this change. Surprisingly, there are already tests for FB limited login, some of which are failing now. This PR is only for Parse Server, so this is independent of the Parse iOS SDK. Could you take a look? It seems that limited login was already supported, but I'm not sure about the implementation.

chriscborg commented 1 month ago

@mtrezza Yes seems like it, and we needed to change the host to www.facebook.com in the unit tests as well for them to pass. However I didn't have time to review the implementation of the unit tests.

mtrezza commented 1 month ago

we needed to change the host to www.facebook.com in the unit tests as well

What do the FB docs say? I assume the exact URL must be documented there? Any references?

chriscborg commented 1 month ago

I wasn't able to find documentation about this yet, it's just a fix that worked. Their documentation seems to suggest using limited.facebook.com but I still need to try this out and test it. If anyone else has time to try out and test it as well, we might get this done faster since it's urgent.

SebC99 commented 1 month ago

@chriscborg Yes there's no official documentation, but from what I can see both urls https://www.facebook.com/.well-known/oauth/openid/jwks/ and https://limited.facebook.com/.well-known/oauth/openid/jwks/ work without any redirection.

chriscborg commented 1 month ago

@SebC99 thank you for checking. Do you know if we should be using limited.facebook.com for a more accurate implementation? Not sure if functionality offered is different.

SebC99 commented 1 month ago

I currently haven't any JWT token from facebook to test if the validation of the token works with the limited subdomain. I know it works with www as it's what we have in production.

mtrezza commented 1 month ago

I would suggest the opposite: dare to use www.facebook.com also for limited tokens, rather than dare to use limited.facebook.com also for traditional tokens. Both are just guesses, but the latter sounds more daring. I have only found these docs that mention the endpoint limited.facebook.com.

We could of course change the adapter to use limited.facebook.com only when token is provided, and www.facebook.com when access_token is provided. That may be the "best guess". Not sure if it's necessary though, if we empirically know already that www.facebook.com supports both tokens?

If you think the suggestion of 2 separate domains make sense, please feel free to go ahead and change this PR and https://github.com/parse-community/parse-server/pull/9122; otherwise we can just go ahead and merge as is.

chriscborg commented 1 month ago

In our case, using www.facebook.com to validate a limited login JWT token seem to have worked.

mtrezza commented 1 month ago

So let's go ahead with the PR and merge as is. Thanks for testing this out.

parseplatformorg commented 1 month ago

🎉 This change has been released in version 6.5.6

mtrezza commented 2 weeks ago

For reference, FB published this related to "Limited Login" endpoints:

Update the Facebook Login SDK to the most recent version and update any Limited Login endpoint domains within your application to the new Limited Login endpoint (as shown here).

Not sure if that warrants another PR where we use a different URL depending on the token type, as mentioned in https://github.com/parse-community/parse-server/pull/9120#issuecomment-2113331232, since it seems to be working as is.