-
Notifications
You must be signed in to change notification settings - Fork 514
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
Implement account linking #447
Comments
Extra notes:
|
Data structure to handle account linking info:
Email verification
Sign up changes
Sign in changes
Fetching user ID
Post sign up / in callback
Changing user info
Disabling / enabling account linking
Migration
Internal vs external usersOld - 1 (for reference)When someone already has existing users and they want to migrate those users into supertokens, they have to call the signUp function for that existing user. This will create a new user in supertokens with its own userId. To facilitate easier migration, we want to make sure that this user's ID is the same as the external user ID. For this, we should allow the developer to create a primary user ID (with their own userID = external userId) that is associated with the new account. The email / phone number of the new account will go in the Note that this account linking is done manually and therefore can be done even if the account linking feature is disabled. The catch to this is that if there already exists a primary user account with the same identifying info, then this new account will be linked to that. In this way, the user's ID cannot be set to the existing external user ID - but this is OK since it means that this external user already somehow had an account with supertokens via a different login method.. which should be impossible? Old - 2If account linking is enabledWe have a table already for user ID mapping. When a user is being migrated, we will consider their identifiers as verified immediately, and so their getUserId function will return the primary user ID. This means, that we can map the primary User ID to the external userID in the user ID mapping table. Now a problem is that there might already be a primary user ID mapping in that table. This can happen if there existed a user in the older system who logged in with a different method but with the same email as the user being migrated now (but account linking was not enabled in their older system). In this case, the user ID mapping table will not allow the same primary user ID to be inserted again (unique constraint on the supertokens user ID column). So here, we can either:
NewLet take a scenario where two external user accounts are linked to the same externalUserId. Which means an externalUserId has multiple login methods for the user. Now when importing such user, we'll end up creating two recipe users (linked to the same primaryUserId), with primaryUserId having external userId mapping. E.g.
When importing EU1 with L1, recipe user R1 is created with primaryUserId P1 and external userId mapping with EU1. If there is an email password user sign up, we create a recipe user ID R1 with no primary user ID. Now if the user maps R1 to E1 (external user ID), and then verifies the account (which creates a primary user ID, P1 === R1), then it will still work since now the primary user ID will be mapped to the E1. One problem here is that if R1 needs to be deleted, but P1 is also linked to another account, then we still want to keep the user ID mapping. So for this, we need to make the user ID mapping reference the primary user ID OR recipe user ID. TODO: db testing Deleting a userIf the user ID belongs to a primary account, all the linked accounts are also deleted. If the user ID belongs to an individual recipe account, only that account is removed and delinked. Note that we need to delete from reset password table as well explicitly even if email password user does not exist - cause password reset tokens may exists for a primary account. Relation to
|
Change required to session and functions that take userIDThe session object's
In case an account is not linked, the primary user ID would be equal to the recipe user ID, so these functions would all still work. EDIT: OLD -> see next next section in this comment If the primary user id === recipe user ID <--> the userId will just be that ID and nothing else. When making a JWT, we will always just use the primary userID for it (just like it's happening now anyway). EDIT: (this is what we decided to do):
|
Change to verify email function interfaceDuring email verification, we want to upgrade the user's session in the following case:
Change to refreshing flowNothing changes here because when we link accounts, we revoke all sessions belonging to the recipeUser, if the recipe user id != primary user ID. In this case, a session refresh with those sessions will automatically log out the user Change to session container
The creation of the new session happens in the API, so it may work:
|
Affect on delete userThe existing function in supertokens backend SDK would be good enough for this. If that function is given a primary user ID, it would delete all the linked accounts and the primary account (all in one transaction) If the input is a recipe user ID, then it would only delete that account and remove it from linked accounts. If that is the only account in the linked accounts, then we delete the primary user ID as well We should explicitly also delete password reset tokens for given userId because it may have password reset tokens for the primary account if emailpassword account didn't existed when creating the token. Incase we are only deleting recipeUserId then we will delete password reset token only if the recipe is email password |
Additional functions created
|
Post sign up / in callback (Ignore this comment)Even if there is a recipe level sign up, it may not mean that post email verification, their user ID might change (as is in the case of email password login). We want the dev to run their post sign in / up logic only after account linking has been fully finished. This calls for different post sign up callback. We want these to be used even if account linking is disabled. Something like:
|
Enabling / disabling automatic account linkingBy default, we want to keep it disabled (due to the complexity it presents). However, we allow users to enable it on a per recipe level that allows them to configure if an account should be linked to another or not. This would give the flexibility to the user to enable / disable account linking on a per user basis. Accounts that were linked in the past will remain linked even after the user has disabled automatic account linking. The function on a per recipe level can look like this:
Note that this function only governs if automatic account linking should happen or not. If this function returns false and the user does manual account linking, the account linking will succeed. |
Fetching info about user from non auth recipes
We have added an extra Tying into session claimsSince the order of the failure of the claims depends on the order in which the user gives the claims, if email verification is not first always, then it may cause a situation where other claims fail even if they are not supposed to, just cause the user ID of this user is not yet the primary user ID. In order to solve this, we can reorder the claims to always have email verification first. |
Affect on user pagination and count functionsThe So the return type of the pagination functions change to:
SQL Queries (For pagination and count)
SELECT
*
FROM (
SELECT
MIN(recipe_user_id) as recipe_user_id, primary_user_id, timejoined
FROM supertokens.all_auth_recipe_users
WHERE
primary_user_id is not null
GROUP BY primary_user_id
UNION
SELECT
*
FROM supertokens.all_auth_recipe_users
WHERE
primary_user_id is null
) as result
ORDER BY timejoined, recipe_user_id
LIMIT X;
SELECT
*
FROM (
SELECT
MIN(recipe_user_id) as recipe_user_id, primary_user_id, timejoined
FROM supertokens.all_auth_recipe_users
WHERE
primary_user_id is not null
and
(
(
primary_user_id > 1
and
timejoined = 1
) OR
(
timejoined > 1
)
)
group by primary_user_id
UNION
SELECT
*
FROM supertokens.all_auth_recipe_users
WHERE
primary_user_id is null
and
(
(
recipe_user_id > 3
and
timejoined = 1
) OR
(
timejoined > 1
)
)
) as result
ORDER BY timejoined, recipe_user_id
LIMIT X;
SELECT
COUNT(*) as total
FROM (
SELECT
MIN(recipe_user_id) as recipe_user_id, primary_user_id, timejoined
FROM supertokens.all_auth_recipe_users
WHERE
primary_user_id is not null
GROUP BY primary_user_id
UNION
SELECT
*
FROM supertokens.all_auth_recipe_users
WHERE
primary_user_id is null
) as result; |
Known issues:
|
Change to reset password flowIf attacker signs up with social provider with email This has now been accounted for in our flows |
New emailsOn account linking, we should send an email to the user saying that they just logged in via XYZ, and if it wasn't them, then they should contact support for help or visit . In that URL, we should tell the dev to give the option of unlinking accounts. Decided: We can do this at a later time. For now, it's not needed |
Additional considerations:
|
If the account to be linked has existing non auth recipe info (like in metadata), that info is kept as is and nothing is done with it. We don't even check if such info exists since 99% of the time, no info will exist anyway. |
What if the user is calling isEmailVerified with a primaryUserId and email (which belongs to non primary account)? This will return false even if the email is verified (cause the entry wont exist in the email verification db). This can happen if the user makes the mistake of passing primaryUserId instead of recipeUserId. Should we account for this? We should just ignore this kind of mistake for now. |
Account linked callback:
This is called whenever our SDK calls SuperTokens.linkAccounts(...) and that returns success and an account has actually been linked (vs just a primary user ID has been created). This function is called by our SDK in:
Account unlinking callback:
This is called in the |
Table structure changes
Table structure for
|
To Discuss
|
Discussion notes
What to implementOn sign up without user explicit consent: -> automatic (not doing now, but just architecting for it) By user on post sign up (link github to my existing account) -> user driven manual By developer using linkAccount themselves: -> developer driven manual Account deduplication -> Preventing account duplication (i.e. if email ID with another recipe already exists, prevent signup with same email ID with another recipe or prompt the user to try signing up with original recipe method) - similar to what atlasian does. |
Changes to API interface and recipe interface
|
createdNewUser boolean
We can introduce a new boolean like
|
Core API changes:TODO -> For the APIs that returns
|
Functions name change discussion (node SDK)function name 1:
|
List of TODO (ignore this checklist and see the one at the top of this issue)These are the list of TODOs that came out of initial implementation discussions (original notes can be found here: https://jamboard.google.com/d/1uWMgs1rnw3Z-IDV7fkQ3J4rnZW8XMdRLYXBK2HKUdZk/edit): Points to note
Discussion points
Node SDK
Core changes
|
What is the status on this feature and when can we expect this to ship? |
It's being actively worked on - but as you can see, the first comment on this PR, the checklist is HUGE. So it may take a while unfortunately. In the timeline of a few months i'd say. |
Hello, how far are we with account linking? |
Will be released for node SDK in the coming week |
About creation of primary user and account linking during sign inConsider the following situation:
Now the github account will become the primary user and the google one will be linked to it. This will cause a change in the user ID from To prevent this, users can:
An alternative would be that instead of making the github user the primary one, we make the google one the primary one. This however, is assuming that the first account was the one in which the user has the most data. What if the following happens?
In this case, if we make google the primary user, then there will also be data loss since github is the actual preferred method of the user. |
This issue is about account linking
Flow diagrams: https://lucid.app/lucidchart/82064b11-858b-4f97-b2ee-3d6e6ed604a6/edit?viewport_loc=-91%2C-655%2C2048%2C1196%2C0_0&invitationId=inv_92259a69-24b4-472f-bbc6-0af69bf835a5#
TODO:
linkAccountToExistingAccountPOST
API on backend SDKlinkAccountsWithUserFromSession
should we throw email verification claim failure in case the existing account is not verified? How will this be handled on the frontend (with respect to claim validation order for mfa: chat reference: https://supertokens.slack.com/archives/D02B887A2MQ/p1683003076081709)?Due to howlinkAccountsWithUserFromSession
works, in MFA, we are forcing email verification to happen right after the first factor (cause email verification is required for account linking by default)I don't think that this is true - cause this happens only if the first factor is not a primary user. But in our case, we are making the first factor a primary user.linkAccountsWithUserFromSession
SignUpPost can return an accountLinkingStatus property instead of retuning createdNewUser: boolean to make the interface less confusingDuring normal sign up (recipe level) store the recipe user id in some table (this should be in the same transaction as the one that creates the recipe user id). If shouldDoAccountLinking returns false or linkAccounts succeeds remove the recpe user id from the table. During sign in if the user id is present in the table, do account linking for that recipe user id.This is to counteract the issue that sign up of recipe level user can succeed, but account linking of this user might fail. If we do not do this, then this user will never be account linked automatically.Same situation holds true if the user's email is verified but then after that, account linking fails for whatever reasonlinkAccountsWithUserFromSession
, there is a part where we returnNEW_ACCOUNT_NEEDS_TO_BE_VERIFIED_ERROR
. Right before that, we have a todo for this should never happen.. solve itlinkAccountToExistingAccountPOST
, we verify the credentials properly if the user already exists.linkAccountToExistingAccountPOST
wherein we want to link an existing user to the session, but with wrong credentials of the existing user.linkAccountToExistingAccountPOST
where the current session's email is not verified, then it results in email verification claim failurelinkAccountToExistingAccountPOST
should it matter if the session's account is verified or not (if it's a primary user already)?linkAccountToExistingAccountPOST
before creating a recipe user, should we check the same conditions as in isSignUpAllowed - that, is if there is another primary user which doesn't have the same email as unverified? Maybe this is not needed cause if there was another primary user with the same email, we prevent that anyway (regardless of if their email is verified or not)..linkAccountToExistingAccountPOST
, if the account linking claim is not to be added and if it exists, it should be removed - just for cleanup (basically you want to remove this claim after linking is done)linkAccountToExistingAccountPOST
if the new account is a primary user, then we should not allow linking.linkAccountToExistingAccountPOST
when we are checking if the email of the new and the existing account is same, we need to check for ALL identifying infos of the existing account and NOT just the currently logged in account. For example, if the new user is emailA and the logged in user is emailB, but the logged in user has another linked account with emailA, it means that even if emailA is not verified (of the new or existing account), we do the linking.shouldRequireVerification
is true. But where should this happen?updateEmailOrPassword
, we should check for password policy thing as well (even in dashboard password change API)purgeSessionOfAccountLinkingClaimIfRequired
everytime before getting the value from the claim on the backend (or better, add it to how we get the value in the first place as part of the claim definition)verifyEmailPOST
, we remove the account linking claim if required.We need to remove from accounts to link table when we make a user a primary user or link an account.Make sure to add a version oflinkEmailPasswordAccountsWithUserFromSession
to all auth recipes.Test that emailpassword.linkEmailPasswordAccountsWithUserFromSession works fine (along with other recipe functions).purgeSessionOfAccountLinkingClaimIfRequired
in account linking index.ts?userEmailVerifyGet
takes a recipeUserId instead of userIduserEmailVerifyPut
takes a recipeUserId instead of userIduserEmailVerifyTokenPost
takes a recipeUserId instead of userIdIf the AccountLinkingClaim is kept in the session even after linking, is that OK?Check that calling the email verification APIs should remove this claimDoes this have any effect on the frontend or backend otherwise?Should we have something to remove this claim is not needed from time to time? Maybe we can add a check in the session refresh API?linkAccountsWithUserFromSession
is of the right structure and that it sends back a 403createAndSendCustomEmail
from all recipes (all deprecated ways of sending emails)normalizedInputMap
in the User type the best idea? How can we detect if that is not used as to prevent bugs (specifically, how can we enforce thathasSameEmailAs
type functions are used when comparing emails etc for users?validateAccessTokenStructure
recipeUserId
instead ofuserId
orid
. Related to this, should we have specific types for recipe user id and primary user id so that users and we don't make a mistake with this? Similar to how we have string vs NormalisedURLDomain (so we can have RecipeUserId, PrimaryUserId)MOCK=true
from the.github/workflows/tests.yml
fileImplementgetPasswordResetTokenInfo
in the coreImplementgetEmailVerificationTokenInfo
in the corerevokeSessionsForLinkedAccounts
in implementation ofrevokeAllSessionsForUser
in core.fetchSessionsForAllLinkedAccounts
in implementation ofgetAllSessionHandlesForUser
in core.allowLinking: false,
when callingisSignUpAllowed
. In this case, we also want to make sure that we end up creating the social login ID - cause otherwise how will support team actually link the account?Add test for "change in account to link does not cause account linking post email verification with the older primary user id (with and without session)"If a user is shared across tenants, and then they use the same email on another tenant, they should have their account linked automatically.removeAllLinkedAccounts
should be false when deleting recipe level users, and true when deleting the main user.userId
torecipeUserId
RECIPE_NOT_INITIALISED
will go away. Type of status OK, will not have recipeId and also, the user type will change to be a primary user with first name and last name.EMAIL_CHANGE_NOT_ALLOWED_ERROR
status (whoseerror
needs to be displayed on the frontend). Input will take recipeUserId. If first name or last name is to change, then we need to make that change on a primary user id (which can be fetched from the input recipe user id)Test that when linking post login, and verification flow is happening, the user still has access to the rest of the app on the frontend etc.user.thirdParty.length > 0
. Need to fixlistUsersByAccountInfo
gets multiple inputs - like email and third party id or email and phone number?disable account linking in email verification api by checking if email verification is optional or notonAccountLinked
sign in up callback / code consume callback pages should call the post account linking API in case a session exists.These screens need to also take into account that email verification would be required.New event types need to be made for this as well.Can we eliminate need for linkAccountPostSession alltogether for now and to link table as well? This depends on how MFA will do linking.Should we get rid of combination recipes on the backend? If yes, this will require a change in the routing logic based on rid.createEmailVerificationToken
function does not normalise the user context when passing it togetEmailForRecipeUserId
session
arg fromshouldDoAutomaticAccountLinking
if we have totally commented out the linkAccountWithSession functions.Need to do account linking even if a user is being associated with a new tenant and that tenant already has a user (with a different ID) for the same login method and account info.Change createdNewUser boolean in thirdparty and passwordless to be true iff there is a new primary user created or a new recipe user created (without linking).createdNewRecipeUser
Please check across all APIs that accept only recipe user id that you are doing such a check.
The text was updated successfully, but these errors were encountered: