Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
May 16, 2024

Conversation

chriscborg
Copy link
Contributor

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

  • Add changes to documentation (guides, repository pages, code comments)

Copy link

Thanks for opening this pull request!

Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this change alone does not break any existing apps that currently send tokens?

@chriscborg
Copy link
Contributor Author

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
Copy link
Member

mtrezza commented May 7, 2024

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
Copy link
Contributor Author

chriscborg commented May 8, 2024

@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
Copy link
Member

mtrezza commented May 12, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

SebC99 commented May 14, 2024

@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
Copy link
Contributor Author

@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
Copy link
Contributor

SebC99 commented May 14, 2024

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
Copy link
Member

mtrezza commented May 15, 2024

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 #9122; otherwise we can just go ahead and merge as is.

@chriscborg
Copy link
Contributor Author

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

@mtrezza
Copy link
Member

mtrezza commented May 16, 2024

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

@mtrezza mtrezza changed the title fix: Changes facebook auth host to resolve JWT validation issue on version 6.x.x fix: Facebook Limited Login not workind due to incorrect domain in JWT validation May 16, 2024
@mtrezza mtrezza merged commit 0e92f76 into parse-community:release-6.x.x May 16, 2024
25 of 26 checks passed
parseplatformorg pushed a commit that referenced this pull request May 16, 2024
## [6.5.6](6.5.5...6.5.6) (2024-05-16)

### Bug Fixes

* Facebook Limited Login not workind due to incorrect domain in JWT validation ([#9120](#9120)) ([0e92f76](0e92f76))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.5.6

@mtrezza
Copy link
Member

mtrezza commented Jun 17, 2024

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 #9120 (comment), since it seems to be working as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants