supertokens / supertokens-node

Node SDK for SuperTokens core
https://supertokens.com
Other
281 stars 72 forks source link

feat(facebook): added missing user scope data #763

Closed kriskw1999 closed 7 months ago

kriskw1999 commented 7 months ago

Summary of change

Description:

This pull request introduces enhancements to the Facebook authentication process within the supertoken framework, aligning it with the functionality already present in the Google authentication implementation. The update ensures that user data fields are fetched automatically based on the provided scopes. A map is utilized to associate scopes with the data fields they correspond to, simplifying the process of requesting additional user information during authentication.

The change is designed to be backward compatible, ensuring that existing implementations are not disrupted. The current implementation considers the primary scopes and automatically fetches relevant user data fields such as name, email, and birthday. Additional scopes and corresponding data fields can be mapped and included in a follow-up update to further extend functionality.

This enhancement streamlines the integration process for developers and provides a more comprehensive and configurable user authentication experience.

Note: The pull request also aims to open a discussion for incorporating more scopes and their associated data fields to cover a broader range of user data that can be fetched seamlessly.

Checklist for important updates

Remaining TODOs for this PR

netlify[bot] commented 7 months ago

Deploy Preview for precious-marshmallow-968a81 canceled.

Name Link
Latest commit 14f69371ae374c0d470d298191037f8551df0257
Latest deploy log https://app.netlify.com/sites/precious-marshmallow-968a81/deploys/657f04b800a04900084f3320
netlify[bot] commented 7 months ago

Deploy Preview for astounding-pegasus-21c111 canceled.

Name Link
Latest commit 14f69371ae374c0d470d298191037f8551df0257
Latest deploy log https://app.netlify.com/sites/astounding-pegasus-21c111/deploys/657f04b8f36216000899e41a
kriskw1999 commented 7 months ago

Sorry, but the error in pipeline seems to be related to a Github temporary api issue, can you can retry to run the job manually?

rishabhpoddar commented 7 months ago

Thanks for the PR @kriskw1999. A quick question:

We already allow users to configure userInfoEndpointQueryParams in the provider config, using which devs can specify the value for fields. Devs may need to specify this anyway if the fields they want are not in the PR you created (are there any extra fields?)

As per adding additional scopes by default, we intentionally do not do that because we want to keep things minimal. All that SuperTokens requires for auth is the email and the third party ID. Adding more scopes might result in extra warnings during the consent screen for the third party provider, which may not be desirable by default.

kriskw1999 commented 7 months ago

Yes sure got your point. If you take a look at the code by default I am keeping just the minimal scope as is already done, the additional scopes are used only if provided inside the config (if the user does not specify the scopes just Id and email are requested in the query as is already done).

For the 3 scopes provided these are all the possible queries params that can be asked.

By the way there are many additional scopes that are missing inside the map, so this can be enriched in order to support the other missing scopes if this is a wanted behavior.

This is just a kind of proposal because the Google part already works by default retrieving the user data when the specified scope allows it, and within the doc is not so clear that for some providers like Facebook it does not depend on the scope.

In this way the api in general will become much simpler and clear. Do you see any drawbacks in this case?

@rishabhpoddar

rishabhpoddar commented 7 months ago

That's fair. I don't see any issues with this, other than that we would have to keep this list up to date, which is OK. Could you add all the scope -> field mappings in the PR? Or if not, we can add it when we get time!

Thanks.

FYI: My point about additional scopes was targeted towards your first comment -The pull request also aims to open a discussion for incorporating more scopes :)

kriskw1999 commented 7 months ago

@rishabhpoddar Yes sorry the description was not so clear I was talking about the missing scopes inside the map, I totally agree in setting by default the minimum scope.

BTW I have updated the version accordingly and added the missing scopes as requested, these should be all the possible scopes for the login api, with their associated fields inside the map. Let me know if it is ok, meanwhile thank you for the time spent on reviewing it!

rishabhpoddar commented 7 months ago

Thats awesome! Thank you. I'll get this merged after we release the current version :)