supertokens-core icon indicating copy to clipboard operation
supertokens-core copied to clipboard

Implement account linking

Open rishabhpoddar opened this issue 3 years ago • 18 comments

INOGRE THIS COMMENT. SEE THE OTHER COMMENTS IN THIS ISSUE

Right now, all the recipes form a user object in isolation - with its own user ID. For more complex scenarios for logging in, we need to enable user account linking across recipe, and even across auth providers (for smooth migration).

  • This requires the "primary" user ID mapped to an array of (user IDs, and "login method").

    type SuperTokensUser = {
       isSuperTokensUser: true,
       recipeUserId: string,
       recipeId: "emailpassword" | "thirdparty" | "passwordless"
    }
    
    type ExternalUser = {
       isSuperTokensUser: false,
       externalUserId: string,
    }
    
    // primary user ID (string) is mapped to an array of SuperTokensUser or External User 
    [primaryUserId: (SuperTokensUser | ExternalUser)[]]
    
  • It also requires a way to associate users to a primary user ID or create a new primary userId in case that user doesn't match any existing primary user:

    getAssociatedPrimaryUserId: (user: SuperTokensUser | ExternalUser) => Promise<string | undefined>
    
    // this will create a new primary userID if no existing can fit the input user.
    associateWithPrimaryUserId: (user: SuperTokensUser | ExternalUser) => Promise<string>
    
    getAllAccountsLinkedForPrimaryUser: (userId: string) => Promise<((SuperTokensUser | ExternalUser)[]) | undefined>
    
  • To aid in migration, we need to have a way to allow users to define what the primary user ID should be when creating it:

    // called inside associateWithPrimaryUserId 
    createPrimaryUserId: (user: SuperTokensUser | ExternalUser) => string
    

    This will allow them to, for example keep the external user IDs the same and not have to change the userIds in their tables during migration. By default, if the user is an external user, then the returned user ID will be equal to the external user, else if will be a new randomly generated user ID.

  • We also need a way to decide how accounts are linked. In theory, this can be any criteria that the developer wants to use, but we can provide a default one: Matching emails or matching phone number. If the user wants another criteria, they can probably implement their own via override.

  • We also should allow users to opt in / out for account account linking on a per user / per recipe basis. This can be done by overriding the getAssociatedPrimaryUserId function to always return a new user ID for the input user for whom account linking should not be done.

  • User account delinking should not ever happen automatically. This is because it may cause unintentional loss of data. For example, if a user's passwordless and social login accounts are linked via email, and then a user removes the email and adds a phone number to the account, then those accounts should not get delinked.

  • Changes to email in one linked acccount should propogate to all accounts.

Table structure:

TODO

Recipe implementation changes

TODO

How will this help in migration

TODO

rishabhpoddar avatar May 15 '22 14:05 rishabhpoddar

Extra notes:

  • Only new and verified accounts can be linked automatically (whilst they are unverified, they are not linked)
  • Account delinking is not something that happens automatically.
  • Automatic account linking is disabled by default
  • If the user has disabled account linking and then enables it, then existing accounts that could have been linked will not get linked. In the opposite situation, we do not unlink accounts.

rishabhpoddar avatar May 16 '22 12:05 rishabhpoddar

Data structure to handle account linking info:

userId -> [{
   recipeId: string,
   recipeUserId: string,
   verifiedIdentifyingIds: set<>, // NOTE: these should be dynamically calculated based on email verification table
   unverifiedIdentifyingIds: set<>,  // NOTE: these should be dynamically calculated based on email verification table
}]
  • combination of (recipeId, recipeUserId) is unique per primary user.
  • verifiedIdentifyingId and unverifiedIdentifyingId can either be an email or phone number.
  • The reason that these IDs are arrays is that some recipe like passwordless can have two identifying info (email and phone number).

Email verification

  • Email verification cause linking of accounts or creation of a primary user ID

Sign up changes

  • If the email / phone number is already verified during sign up, then we find the associated primary user ID and add this new recipe to that ID's array. If there is no primary user that exists for this ID, you make a new primary user (totally different user ID)
  • If it's not verified and the user has NOT enabled email verification globally, then you DO NOT create a primary user ID for this sign up at all.
  • Sign up is disallowed if the input email === to email of another primary user such that ALL linked accounts of that primary user containing that email is unverified. This is done because:
    • If we allow sign up i this situation, it will result in the new account to be linked to the unverified emails' primary account. In this case, a legit user's account (who just tried to sign up), will get linked to an attacker's account.

Sign in changes

  • We disallow sign in for thirdparty is the email has changed on the thirdparty provider's side such that it also belongs to another primary account.

Fetching user ID

  • If the current login method used is not linked to anything, then return the recipe ID in getUserId function
  • If the current login method used is linked, then we return the primary user ID, else we return the recipe level ID.

Post sign up / in callback

  • These remain the same.
  • Additionally, we provide onAccountLinked and onAccountUnlinked

Changing user info

  • Users cannot change their email / phone number to another one if that email / phone number belongs to another primary user ID.
  • Change to a user's info in one recipe DO NOT propogate to another recipe automatically. This in turn means that someone could, over time, have multiple verified identifiers linked to one account. So if there is a new sign up with ANY one of those identifiers, that account will be linked to this existing one)

Disabling / enabling account linking

  • Automatic account linking only applies to new accounts after account linking has been enabled. So if there was an old account with email a, and then account linking was enabled, and then a new account is created with email a, that new account will NOT be linked to the older account, but if another new account is created with a, then it will be linked to the previous one.
  • If account linking is disabled, then any new account will not be linked automatically even if it could have been linked.
  • We should also have a function which the user can override to decide if a user should be considered for account linking during sign up. By default, this function will just be equal to the global config for if account linking is enabled or not.

Migration

  • Existing accounts won't have an associated primary user - so will not automatically be linked anyway. So there should be no data migration needed -> just creation of new tables and logical updates

Internal vs external users

Old - 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 verifiedIdentifyingId array of this entry.

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 - 2

If account linking is enabled

We 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:

  • Link the new account with the older one and ignore the new external user ID; OR
  • Create a new primary user ID for this new account and map this new primary user ID to the external user ID. This way, there will be two primary user IDs with the same email, yet they will be delinked (just like in the older system). This situation should arise only during migration. In this case the problem is that if there is a new sign up with the same email, which one should it be associated with (so we cannot allow this situation at all)?; OR
  • Even if account linking is enabled, we totally ignore it and not create primary users for migrated users at all. This means that any future signs ups with the same email will not be linked to the existing user account.; OR
  • We associate the new user with the existing primary account, but mark them isFullyLinked as false for them. This means their account is not linked, so we will map their recipe user ID to the external user ID. This implies that any new user sign up with the same email will get associated with the same external user ID that was migrated first (I think i prefer this one).

New

Let 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.

  • external user 1:
    • id: EU1
    • login info: L1
  • external user 2:
    • id: EU1
    • login info: L2

When importing EU1 with L1, recipe user R1 is created with primaryUserId P1 and external userId mapping with EU1. When importing EU1 with L2, recipe user R2 is created. Here, because EU1 is also associated with P1, we link R2 to P1.

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 user

If 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 all_auth_recipe_users table

  • When a user signs up, we add them to the all_auth_recipe_users account with their recipe ID
  • If account linking is not done for this user, then nothing changes.
  • New column in this table for primary user ID.

rishabhpoddar avatar May 17 '22 06:05 rishabhpoddar

Change required to session and functions that take userID

The session object's getUserId function will return the primary user ID function. There will be an additional function called getRecipeUserId which will return the recipe user ID of used for the current method of login.

~~All functions that currently take a userID (that need a recipe userID) should check if the input userID is a primary user id or not. If it is a primary user ID, it would thrown an error asking the user to give a recipe user ID.~~ Reason: The recipeUserId will be equal to the primaryUserId

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 The way this will be stored in the session is that the userId field in it will have <recipe user id>|<primary user id>. The delimiter will be used to separate out the two user IDs. When we try to split based on the delimiter, we should be careful to only consider the last element of the array as the recipe user ID. If the size of the array is more than 2, then we should join back everything except for the last element with a |. This is because we we will allow users to pass their own primary user ID.

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):

  • We should add a recipeUserId field which will always be the recipe user ID
  • The sub can be primary user ID (if linked) or recipe user ID (if not linked)
  • revokeSession needs to change to allow work with either recipeUserId or primaryUserId. This means that we do a query like delete from session_info where primaryUserId = ".." OR recipeUserId = "..."

rishabhpoddar avatar May 22 '22 11:05 rishabhpoddar

Change to verify email function interface

During email verification, we want to upgrade the user's session in the following case:

  • Checking if an email is verified API - this should upgrade the user's current session if it does not reflect their new primary user ID
  • After email verification is successful

~Right now, these recipe functions take the email / token directly. Just having this, there is no way to create a new session with the new userID. So instead, we should also make them taken an optional session object which it can use to upgrade the session if needed. The reason it is optional is because if the user is calling these function offline, they won't have the session object.~ -> The session change happens in the API level and not recipe function level.

Change to refreshing flow

Nothing 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

~We now also need to be able to create a new session from an existing session. This requires that we get access to all the info that is required to create a new session from an existing session container object. In some cases, this is already there, but in some frameworks (like in python), we do not store all the info - for example, the request object is not stored in the session container.~

~the above point makes no sense.. what was that about? -> for functions like refresh session, if primary user id has changed, we still have access to the request object. But for functions like email verification, we may not have the request object in python sdk which would be required to create a new session once the email is verified. This is what the above comment meant.~

~In both the cases above, we want to create a new session as opposed to modify the userID of the older session cause modifying the userID of the older session will immediately invalidate the refresh token stored on the frontend. And if the new refresh token doesn't reach the frontend (cause maybe of some network failure), then the user will be logged out on refresh.~

The creation of the new session happens in the API, so it may work:

  • Existing access token payload should not be used when creating the new session. If the user wants to transfer the content of the existing session as well, they can override the email verification GET and POST APIs to manually update the new session with the contents of the older session.

rishabhpoddar avatar May 24 '22 07:05 rishabhpoddar

Affect on delete user

The 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

rishabhpoddar avatar May 27 '22 10:05 rishabhpoddar

Additional functions created

// if isPrimaryUser object is false, loginMethods will always contain
// one item in the array which would corresponds to the recipe user
type User = {
    id: string; // primaryUserId or recipeUserId
    timeJoined: number; // minimum timeJoined value from linkedRecipes
    isPrimaryUser: boolean; //  something that we use and the user should not really care about
    emails: string[],
    phoneNumbers: string[],
    loginMethods: {
        recipeId: string,
        recipeUserId: string,
        timeJoined: number,
        verified: boolean,
        email?: string,
        phoneNumber?: string,
        thirdParty?: {
            id: string;
            userId: string;
        }
    }[]
}

type RecipeLevelUser = {
   id: "", // this will always be the recipe user ID
   timeJoined: ...,
   primaryUserId?: "",
   ... // email or phone number or thirdPartyInfo
}

type AccountInfo = {
    email: string
} | {
    thirdpartyId: string,
    thirdpartyUserId: string
} | {
    phoneNumber: string
}

type AccountInfoWithAuthType = {
    authType: "emailpassword" | "passwordless",
    email: string
} | {
    authType: "thirdparty",
    thirdpartyId: string,
    thirdpartyUserId: string
} | {
    authType: "passwordless",
    phoneNumber: string
}

// this is there cause we use this in the shouldDoAccountLinking callback and that
// function takes in an input user. In case of thirdparty, if the input user doesn't have email,
// it will be strange for the developer, so we add an email to the "thirdparty" type as well. 
type AccountInfoAndEmailWithAuthType = {
    authType: "emailpassword" | "passwordless",
    email: string
} | {
    authType: "thirdparty",
    thirdpartyId: string,
    thirdpartyUserId: string,
    email: string
} | {
    authType: "passwordless",
    phoneNumber: string
}

SuperTokens.getUser(userId: string) => User | undefined // userId can be primary or recipe
SuperTokens.listUsersByAccountInfo(info: AccountInfo) => User[] | undefined
SuperTokens.getUserByAccountInfo(info: AccountInfoWithAuthType) => User | undefined

/*
- Both recipeUserId and primaryUserId must exist.
- recipeUserId should not be linked to any other account.
*/
AccountLinking.linkAccounts(recipeUserId: string, primaryUserId: string) => Promise<{status: "OK", user: User} | {status: "ACCOUNTS_CANNOT_BE_LINKED_ERROR", reason: string}>

/*
- If recipeUserId is equal to it's primary user ID, we should delete the recipe user ID.
*/
AccountLinking.unlinkAccount(recipeUserId: string) => Promise<{status: "OK" | "PRIMARY_USER_ID_NOT_FOUND_ERROR"}>

/*
- Creates a new primary user ID for the input ID such that the primary user ID === recipe user ID
- Input recipe user ID must not be associated with any primary ID already.
*/
AccountLinking.createPrimaryUser(recipeUserId: string) => Promise<{status: "OK", user: User} | {status: "PRIMARY_USER_ALREADY_EXISTS_ERROR", reason: string}>

AccountLinking.canLinkAccounts(recipeUserId: string, primaryUserId: string) => Promise<{canLink: false, reason: string} | { canLink: true }>
  • For getUser, if the input is a recipe user ID, and if that input has an associated primary user ID, the function will return the whole user object where the id is the primary user ID. This is true even if the input recipe user ID != primary id

rishabhpoddar avatar May 27 '22 12:05 rishabhpoddar

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:

EmailPassword.init({
   callbacks: {
      postSignUp: (user, session, formFields, userContext) => Promise<void>,
      postSignIn: (user, session, userContext) => Promise<void>,
   }
})

ThirdParty.init({
   callbacks: {
      postSignUp: (user, session, authCodeResponse, userContext) => Promise<void>,
      postSignIn: (user, session, authCodeResponse, userContext) => Promise<void>
   }
})

Passwordless.init({
   callbacks: {   
      postSignUp: (user, session, preAuthSessionId, userContext) => Promise<void>,
      postSignIn: (user, session, preAuthSessionId, userContext) => Promise<void>
   }
})

ThirdPartyEmailPassword.init({
   callbacks: {
      postEmailPasswordSignUp: (user, session, formFields, userContext) => Promise<void>,
      postEmailPasswordSignIn: (user, session, userContext) => Promise<void>,
      postThirdPartySignUp: (user, session, authCodeResponse, userContext) => Promise<void>,
      postThirdPartySignIn: (user, session, authCodeResponse, userContext) => Promise<void>
   }
})

ThirdPartyPasswordless.init({
   callbacks: {
      postPasswordlessSignUp: (user, session, preAuthSessionId, userContext) => Promise<void>,
      postPasswordlessSignIn: (user, session, preAuthSessionId, userContext) => Promise<void>
      postThirdPartySignUp: (user, session, authCodeResponse, userContext) => Promise<void>,
      postThirdPartySignIn: (user, session, authCodeResponse, userContext) => Promise<void>
   }
})
  • If account linking is disabled, these will be called from the API directly with the relevant input
  • If account linking is enabled:
    • If the account is fully linked already in sign in / up, then we can call these functions
    • If account is not linked, we need to store the "extra info" (like formFields, authCodeResponse...) in session data (in db against the session), and then when it does get fully linked, we can retrieve those and put them in the callback (then delete them from the session's data - or since we will be creating a new session anyway, we can just let that data be)
  • The problem with this is that from a dev's point of view, it may be difficult to know when this is called - they may expected the sign up stuff to be called when the user is created in supertokens. Maybe instead, we can have a postAccountLinked callback which they should use instead of post sign up (only applicable when account linking is enabled).

rishabhpoddar avatar May 28 '22 05:05 rishabhpoddar

Enabling / disabling automatic account linking

By 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:

AccountLinking.init({
   shouldDoAccountLinking: (newAccountInfo: AccountInfoAndEmailWithAuthType, primaryUser: User | undefined, session: SessionContainer | undefined, userContext: any) => Promise<{shouldAutomaticallyLink: false} | {shouldAutomaticallyLink: true, shouldRequireVerification?: boolean = true}>
})
  • primaryUserId is optional because during signup it may or may not be there.
  • session: session will be undefined during signup. But during account linking post login, it will be there.

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.

rishabhpoddar avatar May 28 '22 05:05 rishabhpoddar

Fetching info about user from non auth recipes

~Ideally, there should be no extra info associated with the recipe user ID, and all info should be associated with the primary user ID. So I think we can not do anything special in here.~

We have added an extra primaryUserId in the user object type of the recipe level functions.

Tying into session claims

Since 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.

rishabhpoddar avatar May 28 '22 06:05 rishabhpoddar

Affect on user pagination and count functions

The all_auth_recipe_users now contains users that are recipe user IDs (non linked) and primary user IDs.

So the return type of the pagination functions change to:

{
    users: User[];
    nextPaginationToken?: string;
}[]

SQL Queries (For pagination and count)

  • first query (without pagination token)
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;
  • sql query with pagination token info
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;
  • user count sql
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;

rishabhpoddar avatar May 29 '22 06:05 rishabhpoddar

Known issues:

  • (RESOLVED) If the user signs up with email password and their account is not yet verified, the user ID returned will != the one after their account is verified. This means if the developer saves any application data in the middle, it will be lost. But this is not such a big problem as this can only happen if the email verification mode is switched on for the app - so account email verification is bound to happen quickly.
    • This is not true if we are creating a new primary user id. Otherwise, the user will get a postAccountLink callback where they can do data migration
  • If I sign up with email a and then i change it to email b (which doesn't belong to me), and it's not verified (so it's in the unverifiedIdentifyingId array, then the actual person who has email b cannot sign up.
    • We are okay with this problem
  • (RESOLVED) Migrating users may not end up getting external ID if a primary account already exists with the a randomly generated ID.
    • primary user Id are no longer auto generated. So they can't clash as they are unique
  • (RESOLVED) Because of the possibility of a row being deleted / changed from all_auth_recipe_users, there is a chance that the total user count will be slightly higher than what is actually the case, and that pagination results may be inconsistent.
    • The query to get user count will change
  • (RESOLVED) We do not allow sign ups if the email is already registered from another sign up method and is in the unverified ids array of that method's primary user - should we instead allow sign up but not link the accounts? The new signed up account will just not be linked automatically ever in that case (unless manually linked)
    • this will lead to a bad UX because the actual user will not be able to link their new account with another account at all.
  • (RESOLVED) It is possible that during social sign in, a user's email may have changed than what it was when they signed up. Normally this would change to a unique one, but what if some other primary user already exists with this same email? That we cannot update this user's mapping to that email.. in which case it becomes an issue.
    • Right now, we solve this y disallowing sign in and sending an error message to contact support.
  • (RESOLVED) When a session's userId needs to be changed (post email verification), then it creates a whole new session and keeps the older session alive. Is that OK?
    • during linking accounts, we remove all older sessions.
  • (RESOLVED) If a user signs up and has not verified their email (so their account is not linked yet), and then if the dev switches off email verification, then this user will be able to use the app. However, the post sign up callback will never be called for this user.
    • No post signup callback anymore
  • (RESOLVED) Should we map external user IDs via account linking feature? Or a different feature?
    • This is already a different feature
  • (RESOLVED) If a user has not signed up with email password and they have done with social login, and if they try and reset their password, they will not get an email
    • Now they will based on our flows
  • Reordering of claims validators to have email verification first. Is that a good idea?
    • When the user adds their own claims (in verify session or in global function), we always tell them to add the recipe added global validator claims first, and then their claims. This means, that the email verification claim validator will be added first and then their claims validators (like user roles validators). So this would not be an issue unless people reorder that validators array or if in the future we have other recipes that add validators and the user initialises them first.
    • For now, we decided to not solve this and wait until people have issues with this before taking any action to build a priority system.

rishabhpoddar avatar May 29 '22 07:05 rishabhpoddar

Change to reset password flow

If attacker signs up with social provider with email a (which they have access to), and then creates another email password account with email a (and verifies it) that is linked. Then they change their email to email b (unverified). Then the real user who owns email b tries to sign up with email password (email b), it will tell them that the email already exists. In this case, if they go through the reset password flow and finish that, then they can login, and verify their email. In this case, the attacker will now be linked to the account in which b is verified.

This has now been accounted for in our flows

rishabhpoddar avatar Jun 03 '22 16:06 rishabhpoddar

New emails

On 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 <some URL>. 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

rishabhpoddar avatar Jun 03 '22 16:06 rishabhpoddar

Additional considerations:

  • Implicitly creating email password user (if they have an existing account already with the identifying info with another recipe) if reset password is being done -> this also implies that their email is verified cause they just reset their password -> which means we can link it immediately.
  • Allowing social login user to set a password and then create an email password user which can be linked.
  • Allow user to do account linking even if email not verified, regardless of security risks.
    • [x] sign up
    • [x] account linking post login
    • [x] reset password
    • [x] refreshing session
    • [x] delete user
    • [x] email verification.
  • Allow account linking on a set of accounts (for example social login and not email password login)

rishabhpoddar avatar Aug 11 '22 09:08 rishabhpoddar

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#

bhumilsarvaiya avatar Aug 16 '22 05:08 bhumilsarvaiya

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.

rishabhpoddar avatar Aug 16 '22 11:08 rishabhpoddar

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.

bhumilsarvaiya avatar Aug 25 '22 10:08 bhumilsarvaiya

Account linked callback:

onAccountLinked(primaryUser: User, newAccountInfo: RecipeLevelUser & {recipeId: "..."}) => Promise<void>

supertokens.init({
   recipeList: [
      AccountLinking.init({
          onAccountLinked,
      })
   ]
})

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:

  • sign up
    • normal sign up
    • account linking post login
  • email verification post
  • when user calls linkAccounts manually

Account unlinking callback:

onAccountUnlinked(primaryUser: User, unlinkedAccount: RecipeLevelUser & {recipeId: "..."}) => Promise<void>

This is called in the unlinkAccounts function

bhumilsarvaiya avatar Sep 02 '22 07:09 bhumilsarvaiya

Table structure changes

  • Password reset table (because we allow doing password reset flow even if there is no email password user but when the user has a different account):
    • Foreign Key constraint from reset password table to email password table needs to be dropped
    • Add an additional column for email and make it part of primary key
  • User ID mapping foreign key constraint for all_auth_users table should be on primary user id column OR recipe user id column -> TODO - how is this possible?
  • all_auth_recipe_users will need a new column for primary user ID
  • session_info table should have a recipe_user_id column as well now
  • A new table called recipe_account_to_link which will contain recipeId and primaryUserId (both primary key). We insert into this table during post login account linking if the account to link requires email verification.

Table structure for primary_user_to_recipe_user

CREATE TABLE IF NOT EXISTS primary_user_to_recipe_user(
    primary_user_id CHAR(128),
    recipe_user_id CHAR(128) NOT NULL,
    recipe_id VARCHAR(128) NOT NULL,
    time_joined BIGINT NOT NULL,
    PRIMARY KEY (recipe_user_id)
);

CREATE INDEX primary_user_to_recipe_user_primary_user_index ON primary_user_to_recipe_user(primary_user_id, time_joined);
  • Note that primary_user_id can be NULL.
  • We have the time_joined cause it helps with pagination.

bhumilsarvaiya avatar Sep 22 '22 06:09 bhumilsarvaiya

Documentation changes

  • Need to change documentation so all the changes related to session claims happen in create new session function. This implies that all post sign in / up stuff should happen on the function level overrides and not API level

bhumilsarvaiya avatar Sep 27 '22 09:09 bhumilsarvaiya

To Discuss

  • [x] concept of account linking and how it works
  • [x] changes to sign-up flow
  • [x] user object
  • [x] function interfaces for account linking recipe
  • [x] user pagination function change
  • [x] post account linking callback
  • [x] enabling account linking per recipe level
  • [ ] new api for post login account linking (POST /user/account/link -> requires a session + info about other account)
    • [ ] if the user wants to do something post sign-up, they will have to do in normal sign-up API as well as this one
  • [x] affect on delete user
  • [ ] changes for dashboard (Only with Nemi)
  • [x] changes for session claims (Only with Mihaly)
  • [x] createdNewUser boolean
  • [x] session userId format
  • [ ] userId mapping (Only with Joel)
  • [x] Should an email be sent when account is linked? -> we can add later if required..
  • [x] disabling signup if they have unverified identity linked to an existing primaryUserId

bhumilsarvaiya avatar Sep 27 '22 12:09 bhumilsarvaiya

Discussion notes

  • Make it very clear that the user ID will change.
  • Phone number with password -> phone number is not verified.
  • Do not directly tie to email verification recipe, but instead to the recipe specific way of knowing if the info is verified or not.
  • “Adaptive account linking” -> even if email verification is not done, then the user’s account can be linked cause maybe they used the same device etc..
  • (which option to pick) if b is linked and verified, and the user changes their email, we should:
    • option 1) not allow changing of email; OR
    • option 2) keep the email as b, but change it only verified. OR
    • option 3) unverify it, but delink it.
    • Decision: We picked none of these options and kept with disabling sign up cause this happens in a rare enough situation only.

~Why is soft linking done?~ ~- In email password (or anything that yields and unverified email) sign up, we want to save the linked status there and not during email verification because we want to prevent sign ups using other methods with the same email whilst this account is unverified and is a candidate for being linked.~ ~- TODO: Why else? Do we even require this, especially if we go with option 2 in the above point.~ We no longer have this concept of soft linking

What to implement

On 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.

rishabhpoddar avatar Sep 29 '22 13:09 rishabhpoddar

Changes to API interface and recipe interface

  • Email verification API interface:
    • isEmailVerifiedGET -> return a session as well (cause it might be a new account linked session)
    • verifyEmailPOST -> return an optional session as well (if the input had a session, this will return a session) (cause it might be a new account linked session)

rishabhpoddar avatar Oct 31 '22 08:10 rishabhpoddar

createdNewUser boolean

~This should be true if (account linking enabled && new primary user created) || (account linking disabled && sign up called).~

~This implies that even if the account is not fully linked yet (as is the case with emailpassword recipe on sign up), we still set createdNewUser boolean to false.~

We can introduce a new boolean like createdNewRecipeUser:

  • createdNewUser: false, createdNewRecipeUser: false -> sign in
  • createdNewUser: false, createdNewRecipeUser: true -> sign up post login || sign up and account linked (verified or not)
  • createdNewUser: true, createdNewRecipeUser: false -> not possible
  • createdNewUser: true, createdNewRecipeUser: true -> sign up with new primary user ID creation || sign up without account linking

rishabhpoddar avatar Nov 01 '22 12:11 rishabhpoddar

Core API changes:

TODO

-> For the APIs that returns recipeUserId, it should default to the userId

API: /recipe/signup
Recipe: emailpassword
METHOD: POST
CHANGE:
    - the returned user object will have the same structure as the global user.

API: /recipe/user
Recipe: core
METHOD: GET
CHANGE:
    - the returned user object will have the same structure as the global user.
    - we will also remove recipe level /recipe/user GET APIs

API: /recipe/user
Recipe: emailpassword
METHOD: PUT
ASSERT:
    - the input userId must be a recipeUserId pointing to an email password recipe - else return UNKNOWN_USER_ID_ERROR
REVIEW:
    - the logic might need to account for not allowing user to change the associated email if another primary user has the same email.

API: /recipe/signin
Recipe: emailpassword
METHOD: POST
CHANGE:
    - the returned user object will have the same structure as the global user.

API: /recipe/user/password/reset/token
Recipe: emailpassword
METHOD: POST
ASSERT:
    - the input userId should be either recipeUserId or primaryUserId. If primaryUserId has only one associated recipe user, do password reset for that. Else throw 400 error
    - TODO -> What are the list of changes here?

API: /recipe/user/password/reset
Recipe: emailpassword
METHOD: POST
CHANGE:
    - This API will just go away, and be replaced with /recipe/user/password/reset/token/consume (see below)

API: /recipe/user/passwordhash/import
Recipe: emailpassword
METHOD: GET
CHANGE:
    - the returned user object will have the same structure as the global user.

API: /recipe/signinup
Recipe: thirdparty
METHOD: POST
CHANGE:
    - the returned user object will have the same structure as the global user.
    - If the input thirdPartyInfo is associated with an existing primary user, and the input email is also associated with another primary user, then we return {status: `SIGN_IN_NOT_ALLOWED`, description: "..."}`

API: /recipe/session
METHOD: POST
CHANGE:
    - recipeUserId in input
    - accessToken payload should contain recipeUserId

API: /recipe/session
METHOD: GET
CHANGE:
    - accessToken payload should contain recipeUserId

API: /recipe/session/verify
METHOD: POST
CHANGE:
    - recipeUserId in returned session object
    - accessToken payload should contain recipeUserId

API: /recipe/session/refresh
METHOD: POST
CHANGE:
    - recipeUserId in returned session object
    - accessToken payload should contain recipeUserId
    - During token theft detection error, the session object should also have recipeUserId

API: /recipe/session/user
METHOD: GET
CHANGE:
    - input userId can be either primaryUserId or recipeUserId

API: /recipe/session/regenerate
METHOD: POST
CHANGE:
    - recipeUserId in returned session object

API: /users
METHOD: GET
CHANGE:
    - user object is changed. check account linking PR

API: /user/remove
METHOD: POST
CHANGE:
    - input userId can be either primaryUserId or recipeUserId
    - new input boolean removeAllLinkedAccounts

API: /recipe/user/email/verify/token
METHOD: POST
CHANGE:
    - input userId must be either a recipeUserId
  • For API that links account, the core will NOT do checks that are done in the canLinkAccount function cause we want the user to be able to override the canLinkAccount function to define their own logic if they wish to, and having checks in the core will essentially make their override void.
route: /recipe/accountlinking/users
method: get
query: {
    primaryUserIds: string[]
} | {
    recipeUserIds: string[]
}
response: {
    status: OK,
    userIdMapping: {
        [recipeUserId: string]: string | null
    }
} | {
    status: OK,
    userIdMapping: {
        [primaryUserId: string]: string[]
    }
}
details:
    - if primaryUserIds is passed, the keys in userIdMapping will be primaryUserIds
    - if recipeUserIds is passed, the keys in userIdMapping will be recipeUserIds
    - if recipeUserIds is passed and there is no primaryUserId found for any recipeUserId, the value for that recipeUserId in userIdMapping would be null
    - if recipeUserIds is passed, for any recipeUserId that doesn't really exists, the recipeUserId will not be present in the userIdMapping
    - if primaryUserIds is passed and there is no recipeUserId found for any primaryUserId, the value for that primaryUserId in userIdMapping would be an empty array
    - if primaryUserIds is passed, for any primaryUserId that doesn't really exists, the primaryUserId will not be present in the userIdMapping

-----

route: /recipe/accountlinking/user
method: put
body: {
    recipeUserId: string
    recipeId: string
    timeJoined: number
}
response: {
    status: ok
    createdNewEntry: boolean
}
details:
    - insert into a new table which used to account linking purpose
    - if the recipeUserId already exists in the table, createdNewEntry will be false, else true

-----

route: /users
method: get
update:
    - type of user object returned

-----

route: /recipe/accountlinking/user/primary
method: post
body: {
    recipeUserId
}
response:  {
    status: "OK";
    user: User;
} | {
    status:
        | "RECIPE_USER_ID_ALREADY_LINKED_WITH_ANOTHER_PRIMARY_USER_ID_ERROR"
        | "ACCOUNT_INFO_ALREADY_LINKED_WITH_ANOTHER_PRIMARY_USER_ID_ERROR";
    primaryUserId: string;
    description: string;
}
details:
    - for a given recipeUserId, create a new primaryUserId (primaryUserId will be equal to recipeUserId)
    - if primaryUserId is not created due to any reason, return the recipeUser

-----

route: /recipe/accountlinking/user/link
method: post
body: {
    recipeUserId: string;
    primaryUserId: string
}
response:  {
    status: "OK" | string;
}
details:
    - for a given recipeUserId and primaryUserId, update the row in new account linking table (add primaryUserId for recipeUserId)
    - if recipeUserId | primaryUserId doesn't exist or the linking was not done, return a string status stating what went wrong

-----

route: /recipe/accountlinking/user/unlink
method: post
body: {
    recipeUserId: string;
    primaryUserId: string;
}
response:  {
    status: "OK"
}
details:
    - for a given recipeUserId and primaryUserId, update the row in new account linking table (remove primaryUserId for recipeUserId)
    - if recipeUserId | primaryUserId doesn't exist or the unlinking was not done, return a string status stating what went wrong

-----

route: /recipe/accountlinking/user
method: get
query: {
    userId: string
}
response: {
    status: "OK,
    user: User
}
details:
    - fetch user object for given userId
    - userId can be primaryUserId or recipeuserId. First treat it as primaryUserId. if no user is found, treat it as recipeUserId


route: /users/accountinfo
method: get
query: {
    recipeId?: "emailpassword" | "passwordless";
    email: string;
} | {
    recipeId?: "thirdparty";
    thirdpartyId: string;
    thirdpartyUserId: string;
} | {
    recipeId?: "passwordless";
    phoneNumber: string;
}
response: {
    status: "OK,
    users: User[]
}
details:
    - fetch user object for given info. If recipeId is passed, on get account for that recipeId

route: /user/remove
method: post
update:
    - if the recipe user is the only acount linked to a specific primaryUserId, remove all data related to the primaryUserId


route: /recipe/accountlinking/user/linked_or_linkable
method: get
query: {
    userId: string // recipeUserId
}
response: {
    status: "OK,
    user?: User
}
details:
    - for input recipeUserId, get primary user which is linked to the recipeUserId or which is supposed to be linked to recipeUserId or can be linked (because of the identifying info associated with the recipeUserId)
    - if no primaryUser found, return user will be undefined

route: /recipe/user/password/reset/token/consume
method: post
body: {
    token: string
} 
response: {
   status: "OK";
   userId: string;
   email: string;
} | { status: "RESET_PASSWORD_INVALID_TOKEN_ERROR" }
details:
    - this will only consume the token in the core and not change the password

bhumilsarvaiya avatar Nov 21 '22 12:11 bhumilsarvaiya

Functions name change discussion (node SDK)

function name 1:

  • [ ] getUserByAccountInfo
  • [ ] getUserByAccountInfoWithLoginMethod

bhumilsarvaiya avatar Jan 18 '23 20:01 bhumilsarvaiya

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

  • In the sign up with existing flow, link accounts step:
    • First link the accounts and then mark the email as verified (this is shown in the wrong order in the helm chart)

Discussion points

  • How will account linking work with multi-tenancy
  • What logic/functionality should exist in the backend SDKs vs the Core
  • Confirm how recursion in the api interface will work with overrides in the node SDK (discuss with porcellus)

Node SDK

  • [x] Search for TODO: Nemi and remove resolve all those todos
  • [x] Remove createPrimaryUserIdOrLinkAccountsAfterEmailVerification function entierly and replace with call to createPrimaryUserIdOrLinkAccounts
  • [x] The status in the return types of create and canCreate primaryUser should be thought about more to remove redundant and confusing status values. We could have an additional boolean in status:"OK" to indicate whether the account was already linked. This should also be changed in canLinkAccounts
  • [x] Think about all the places where post link accounts should get called
  • [x] Unlink accounts should return a status instead of throwing an error
  • [x] Can doPostSignUp... just take the recipe level as the input?
  • [x] linkAccounts should just mark emails as verified if required. Blocks that call linkAccounts should not do this in their logic
  • [x] doPostSignUp... should mark the current email as verified if other login methods have the same email and is verified
  • [x] The api that calls accountLinkPostSignInViaSession should return an invalid claims error for email verification if accountLinkPostSignInViaSession returns NEEDS_TO_BE_VERIFIED
  • [x] listAccountByInfo should just accept the user object and forward that to the core. Let the core decide what recipe login info to prioritise
  • [x] accountLinkPostSignUp... should not check for email verification when creating a recipe user. We also dont need addClaim when returning createRecipeUser: true
  • [x] doPostSignUp... should call getPrimaryThatCanBeLinked... instead of listUsersByInfo
  • [x] Whatever block is calling createPrimaryAfterVerification should handle adding the session claim and not createPrimaryAfterVerification itself
  • [x] createPrimaryAfterVerification should check for email verification
  • [x] ACCOUNT_INFO_ALREADY_LINKED... should be renamed to ACCOUNT_INFO_ALREADY_ASSOCIATED...
  • [x] Is AddNewRecipeUserIdWithoutPrimary required? Can it be removed from the recipe interface?
  • [ ] SignUpPost can return an accountLinkingStatus property instead of retuning createdNewUser: boolean to make the interface less confusing
  • [x] Does accountLinkingPost... need isNotVerified boolean?
  • [ ] Maybe we should create a function in all recipes to create a user without signing up so that if the developer has overridden signUp it wont get called during account linking
  • [x] AccountLinkingPost should never have an empty description in the returned response
  • [ ] SignUp should use getUser... instead of overwriting the id
  • [x] For all places in the flow where we throw errors, can we return status instead? This will improve the general DX of the flow
  • [x] The backend SDK does not need to mark emails as verified post account linking, that can happen in the core

Core changes

  • [ ] During normal sign up (before linking) 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 is the user id is present in the table, do account linking for that recipe user id.
  • [ ] changeEmailOrPassword should check if the email is already associated with a primary user id (This should not happen in the API in the backend SDK)
  • [ ]

nkshah2 avatar May 01 '23 04:05 nkshah2

What is the status on this feature and when can we expect this to ship?

LukasKnuth avatar May 31 '23 14:05 LukasKnuth

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.

rishabhpoddar avatar May 31 '23 16:05 rishabhpoddar

Hello, how far are we with account linking?

hiteshjoshi avatar Sep 15 '23 05:09 hiteshjoshi

Will be released for node SDK in the coming week

rishabhpoddar avatar Sep 15 '23 05:09 rishabhpoddar

About creation of primary user and account linking during sign in

Consider the following situation:

  • App does not use account linking
  • User signs up with google login with email e1 -> recipe user with user id u1
  • User uses the app and adds data to it.
  • App now enables account linking
  • User signs up with github with email e1 -> user id is u2

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 u1 to u2 which might cause loss of data.

To prevent this, users can:

  • Make the google user a primary user before switching on auto account linking.
  • Implement shouldDoAutomaticAccountLinking in such a way that when github is going to become the primary user, they should reject that cause there is already data associated with the google account that has the same email.

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?

  • account linking off
  • google sign in
  • github sign in
  • user uses github account
  • account linking on
  • github sign in

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.

rishabhpoddar avatar Dec 14 '23 12:12 rishabhpoddar