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

feat: add identity linking methods #814

Merged
merged 6 commits into from
Dec 6, 2023

Conversation

kangmingtay
Copy link
Member

@kangmingtay kangmingtay commented Nov 29, 2023

What kind of change does this PR introduce?

  • Adds 3 new methods: unlinkIdentity, linkIdentity, getUserIdentities to support the new identity linking endpoints
  • All methods require the user to be authenticated
  • This PR should only be merged once the gotrue release with the new identity linking endpoints are deployed

TODO

  • fix issue where user is logged out if linking fails

@kangmingtay kangmingtay requested review from hf and J0 November 29, 2023 03:34
@J0 J0 self-requested a review November 29, 2023 07:19
J0
J0 previously requested changes Nov 29, 2023
Copy link
Contributor

@J0 J0 left a comment

Choose a reason for hiding this comment

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

Looks good to me but leave requesting changes so this doesn't accidentally get merged - feel free to resolve whenever

src/GoTrueClient.ts Show resolved Hide resolved
src/GoTrueClient.ts Show resolved Hide resolved
@kangmingtay kangmingtay dismissed J0’s stale review December 6, 2023 22:18

ready to be merged

@kangmingtay kangmingtay changed the title [wip] feat: add identity linking methods feat: add identity linking methods Dec 6, 2023
@kangmingtay kangmingtay merged commit 46d0f87 into master Dec 6, 2023
2 checks passed
@kangmingtay kangmingtay deleted the km/add-identity-linking-methods branch December 6, 2023 23:51
Copy link
Contributor

github-actions bot commented Dec 6, 2023

🎉 This PR is included in version 2.60.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@chhuang
Copy link

chhuang commented Jan 11, 2024

I think I've found a few issues with this, specifically using twitter as the provider.

  • After doing unlinkIdentity for twitter, I can still sign back in with the same Twitter account and it gets added/linked back to the same old account automatically. The Twitter account is using a different email address, I'd assume if I unlink it, and then do a new sign in/sign up, it would create a new account for me based on the email from the Twitter account, but that is not the case.

  • I'm not sure if this is related, unlinkIdentity remove the record from the auth.identities table but it doesn't clean/delete the data in raw_user_meta_data in the auth.users table (which is where the twitter data is stored).

  • linkIdentity using twitter as the provider doesn't work as expected. It returns error code 500 not found and request token doesn't match token in callback. It works fine for google and discord as the provider. It also works fine when I use with signInWithOAuth, so this should not be a Twitter APP settings issue on my end. I can see the error in the URL after the redirect back from Twitter. Apart from the error, it also logs me out of the application.

/account?error=server_error&error_code=500&error_description=Request+token+doesn%27t+match+token+in+callback#error=server_error&error_code=500&error_description=Request+token+doesn%2527t+match+token+in+callback

@kangmingtay @J0 would you be able to help please?

@J0
Copy link
Contributor

J0 commented Jan 15, 2024

Hey @chhuang,

Thanks for filing the issue and for your patience with us while we work through the tickets. Might need to trouble you for a bit of information. For:

After doing unlinkIdentity for twitter, I can still sign back in with the same Twitter account and it gets added/linked back to the same old account automatically. The Twitter account is using a different email address, I'd assume if I unlink it, and then do a new sign in/sign up, it would create a new account for me based on the email from the Twitter account, but that is not the case.

As I understand the first steps would be:

  1. User signs in with Google OAuth ([email protected]) and then signs in with Twitter OAuth (primary_email: [email protected]). Let's call this UserA.
  2. Developer calls unlinkIdentity for twitter . UserA now has one identity.

Mind sharing more about the next step?

I can still sign back in with the same Twitter account and it gets added/linked back to the same old account automatically.

I am guessing here but would it be:

  • User signs in with OAuth via Twitter OAuth (primary_email: [email protected]) and Identity is again linked to UserA (?)

If you have a linked video that would be very helpful

I'm not sure if this is related, unlinkIdentity remove the record from the auth.identities table but it doesn't clean/delete the data in raw_user_meta_data in the auth.users table (which is where the twitter data is stored).

I think unlinkIdentity only updates the providers app_meta_data and does not touch raw_user_metadata. Do you mind sharing what fields remain in raw_user_metadata and what you'd like cleared?

linkIdentity using twitter as the provider doesn't work as expected. It returns error code 500 not found and request token doesn't match token in callback. It works fine for google and discord as the provider. It also works fine when I use with signInWithOAuth, so this should not be a Twitter APP settings issue on my end. I can see the error in the URL after the redirect back from Twitter. Apart from the error, it also logs me out of the application.

I'm spitballing and will need to check further but there might be a mismatch in the one of the fields of the JWT. If you see any differences that stand out let us know.

Thanks!

@chhuang
Copy link

chhuang commented Jan 16, 2024

Thanks for looking into this @J0

Scenario 1 A (not working):

  1. User A signs in with Google ([email protected])
  2. User A links Twitter ([email protected]), the linking fails. Important to note that this is a brand new Twitter account that it has never been used to create or sign in before in our system.
  3. User A is redirected back and logged out automatically: {"code":401,"msg":"invalid claim: missing sub claim"}

image

In the same scenario, User A can sign in with Twitter ([email protected]) with no problem. I only did this after I failed to link it to [email protected] in step 1. But now I have created 2 different accounts, one with Google and one with Twitter. Obviously this was not the initial intention.

Scenario 1 B (working):

  1. User A signs in with Google ([email protected])
  2. User A links Discord ([email protected])
  3. User A is redirected back and Discord is added and linked to the same account.

Scenario 2:

  1. User A signs in with Google ([email protected])
  2. User A adds/links Twitter to the account (different email, [email protected])
  3. User A signs out
  4. User B signs in with Google ([email protected])
  5. User B adds/links Twitter ([email protected]), the linking fails because email is already used and linked to User A
  6. User B is redirected back to the site and logged out automatically

I think the UX can be improved for step 6? I can see this is in the TODO of this PR and it's checked, can you confirm it's working as intended please?

image

Scenario 3:

  1. User A signs in with Twitter ([email protected])
  2. User A links Discord to the account (different email, [email protected])
  3. User A tries to unlink Twitter, the unlink fails: {"code":403,"msg":"Unable to unlink identity due to email conflict"}

Unlike the other scenarios, here the user is not logged out automatically. I can also unlink my Discord without problem.

What is the reason I can't unlink my Twitter here? Since I've also added Discord, I should be able to unlink Twitter right?

image

@@ -286,6 +286,15 @@ export default class GoTrueClient {
if (error) {
this._debug('#_initialize()', 'error detecting session from URL', error)

// hacky workaround to keep the existing session if there's an error returned from identity linking
// TODO: once error codes are ready, we should match against it instead of the message
if (
Copy link

Choose a reason for hiding this comment

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

Is this check working correctly? It's showing as info instead of error in the logs. Also should the message string be 400: Identity is already linked to another user instead of Identity is already linked to another user?

image

I'm also getting a 401 on /user:
image

@J0

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @chhuang,

Thanks for the detailed write up! Looking at this now

Copy link
Member Author

Choose a reason for hiding this comment

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

@chhuang this is because the error is being appended as a query parameter in the redirect url but the http status code returned is 3xx and not a 4xx or 5xx

@kangmingtay
Copy link
Member Author

Hi @chhuang, thanks for providing all these details.

Scenario 1 will be fixed by this PR.

Scenario 2 is expected but you should not be logged out in step 6 - i'll look into why that's happening in the client library

Scenario 3 happens because you already have another user / identity in your db that uses the email [email protected]. When you unlink the twitter identity from the user, we need to update the auth.users.email column which has a unique constraint on it. If there is another user that initially signed in with discord using [email protected], this email will already be used in the auth.users table. This is a known issue and we're working on relaxing the email constraint on oauth identities so that this error goes away.

@chhuang
Copy link

chhuang commented Jan 20, 2024

Hi @chhuang, thanks for providing all these details.

Scenario 1 will be fixed by this PR.

Scenario 2 is expected but you should not be logged out in step 6 - i'll look into why that's happening in the client library

Scenario 3 happens because you already have another user / identity in your db that uses the email [email protected]. When you unlink the twitter identity from the user, we need to update the auth.users.email column which has a unique constraint on it. If there is another user that initially signed in with discord using [email protected], this email will already be used in the auth.users table. This is a known issue and we're working on relaxing the email constraint on oauth identities so that this error goes away.

Thanks @kangmingtay for the fix!

Can you tell me how to get the fix? Which library should I update?

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

Successfully merging this pull request may close these issues.

4 participants