diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a677b2d7..7005e44b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [unreleased] +## [20.1.5] - 2024-10-09 + +- Fixes an issue where users were not able to reset their password if a user with the same email address was created before account linking was enabled. +- Fixes and re-works some security checks connected to password reset. + ## [20.1.4] - 2024-10-07 - Fixes an issue where revoking sessions for a specific tenant didn't work well diff --git a/lib/build/recipe/accountlinking/recipe.d.ts b/lib/build/recipe/accountlinking/recipe.d.ts index 5a850c2a0..261a2faa8 100644 --- a/lib/build/recipe/accountlinking/recipe.d.ts +++ b/lib/build/recipe/accountlinking/recipe.d.ts @@ -118,7 +118,12 @@ export default class Recipe extends RecipeModule { recipeUserId: RecipeUserId; userContext: UserContext; }) => Promise; - private shouldBecomePrimaryUser; + shouldBecomePrimaryUser( + user: User, + tenantId: string, + session: SessionContainerInterface | undefined, + userContext: UserContext + ): Promise; tryLinkingByAccountInfoOrCreatePrimaryUser({ inputUser, session, diff --git a/lib/build/recipe/emailpassword/api/implementation.js b/lib/build/recipe/emailpassword/api/implementation.js index 703c9462a..9b7e222f8 100644 --- a/lib/build/recipe/emailpassword/api/implementation.js +++ b/lib/build/recipe/emailpassword/api/implementation.js @@ -107,10 +107,51 @@ function getAPIImplementation() { } } // we find the primary user ID from the user's list for later use. - let primaryUserAssociatedWithEmail = users.find((u) => u.isPrimaryUser); - // first we check if there even exists a primary user that has the input email + let linkingCandidate = users.find((u) => u.isPrimaryUser); + logger_1.logDebugMessage( + "generatePasswordResetTokenPOST: primary linking candidate: " + + (linkingCandidate === null || linkingCandidate === void 0 ? void 0 : linkingCandidate.id) + ); + logger_1.logDebugMessage("generatePasswordResetTokenPOST: linking candidate count " + users.length); + // If there is no existing primary user and there is a single option to link + // we see if that user can become primary (and a candidate for linking) + if (linkingCandidate === undefined && users.length > 0) { + // If the only user that exists with this email is a non-primary emailpassword user, then we can just let them reset their password, because: + // we are not going to link anything and there is no risk of account takeover. + if ( + users.length === 1 && + users[0].loginMethods[0].recipeUserId.getAsString() === + (emailPasswordAccount === null || emailPasswordAccount === void 0 + ? void 0 + : emailPasswordAccount.recipeUserId.getAsString()) + ) { + return await generateAndSendPasswordResetToken( + emailPasswordAccount.recipeUserId.getAsString(), + emailPasswordAccount.recipeUserId + ); + } + const oldestUser = users.sort((a, b) => a.timeJoined - b.timeJoined)[0]; + logger_1.logDebugMessage( + `generatePasswordResetTokenPOST: oldest recipe level-linking candidate: ${oldestUser.id} (w/ ${ + oldestUser.loginMethods[0].verified ? "verified" : "unverified" + } email)` + ); + // Otherwise, we check if the user can become primary. + const shouldBecomePrimaryUser = await recipe_1.default + .getInstance() + .shouldBecomePrimaryUser(oldestUser, tenantId, undefined, userContext); + logger_1.logDebugMessage( + `generatePasswordResetTokenPOST: recipe level-linking candidate ${ + shouldBecomePrimaryUser ? "can" : "can not" + } become primary` + ); + if (shouldBecomePrimaryUser) { + linkingCandidate = oldestUser; + } + } + // first we check if there even exists a candidate user that has the input email // if not, then we do the regular flow for password reset. - if (primaryUserAssociatedWithEmail === undefined) { + if (linkingCandidate === undefined) { if (emailPasswordAccount === undefined) { logger_1.logDebugMessage(`Password reset email not sent, unknown user email: ${email}`); return { @@ -122,24 +163,45 @@ function getAPIImplementation() { emailPasswordAccount.recipeUserId ); } - // Next we check if there is any login method in which the input email is verified. + /* + This security measure helps prevent the following attack: + An attacker has email A and they create an account using TP and it doesn't matter if A is verified or not. Now they create another + account using EP with email A and verifies it. Both these accounts are linked. Now the attacker changes the email for EP recipe to + B which makes the EP account unverified, but it's still linked. + + If the real owner of B tries to signup using EP, it will say that the account already exists so they may try to reset password which should be denied, + because then they will end up getting access to attacker's account and verify the EP account. + + The problem with this situation is if the EP account is verified, it will allow further sign-ups with email B which will also be linked to this primary account, + that the attacker had created with email A. + + It is important to realize that the attacker had created another account with A because if they hadn't done that, then they wouldn't + have access to this account after the real user resets the password which is why it is important to check there is another non-EP account + linked to the primary such that the email is not the same as B. + + Exception to the above is that, if there is a third recipe account linked to the above two accounts and has B as verified, then we should + allow reset password token generation because user has already proven that the owns the email B + */ + // First we check if there is any login method in which the input email is verified. // If that is the case, then it's proven that the user owns the email and we can // trust linking of the email password account. let emailVerified = - primaryUserAssociatedWithEmail.loginMethods.find((lm) => { + linkingCandidate.loginMethods.find((lm) => { return lm.hasSameEmailAs(email) && lm.verified; }) !== undefined; - // finally, we check if the primary user has any other email / phone number + // then, we check if the primary user has any other email / phone number // associated with this account - and if it does, then it means that // there is a risk of account takeover, so we do not allow the token to be generated let hasOtherEmailOrPhone = - primaryUserAssociatedWithEmail.loginMethods.find((lm) => { + linkingCandidate.loginMethods.find((lm) => { // we do the extra undefined check below cause // hasSameEmailAs returns false if the lm.email is undefined, and // we want to check that the email is different as opposed to email // not existing in lm. return (lm.email !== undefined && !lm.hasSameEmailAs(email)) || lm.phoneNumber !== undefined; }) !== undefined; + // If we allow this to pass, then: + // 1. the if (!emailVerified && hasOtherEmailOrPhone) { return { status: "PASSWORD_RESET_NOT_ALLOWED", @@ -147,6 +209,25 @@ function getAPIImplementation() { "Reset password link was not created because of account take over risk. Please contact support. (ERR_CODE_001)", }; } + if (linkingCandidate.isPrimaryUser && emailPasswordAccount !== undefined) { + // If a primary user has the input email as verified or has no other emails then it is always allowed to reset their own password: + // - there is no risk of account takeover, because they have verified this email or haven't linked it to anything else (checked above this block) + // - there will be no linking as a result of this action, so we do not need to check for linking (checked here by seeing that the two accounts are already linked) + let areTheTwoAccountsLinked = + linkingCandidate.loginMethods.find((lm) => { + return lm.recipeUserId.getAsString() === emailPasswordAccount.recipeUserId.getAsString(); + }) !== undefined; + if (areTheTwoAccountsLinked) { + return await generateAndSendPasswordResetToken( + linkingCandidate.id, + emailPasswordAccount.recipeUserId + ); + } + } + // Here we know that the two accounts are NOT linked. We now need to check for an + // extra security measure here to make sure that the input email in the primary user + // is verified, and if not, we need to make sure that there is no other email / phone number + // associated with the primary user account. If there is, then we do not proceed. let shouldDoAccountLinkingResponse = await recipe_1.default .getInstance() .config.shouldDoAutomaticAccountLinking( @@ -156,7 +237,7 @@ function getAPIImplementation() { recipeId: "emailpassword", email, }, - primaryUserAssociatedWithEmail, + linkingCandidate, undefined, tenantId, userContext @@ -195,7 +276,7 @@ function getAPIImplementation() { // notice that we pass in the primary user ID here. This means that // we will be creating a new email password account when the token // is consumed and linking it to this primary user. - return await generateAndSendPasswordResetToken(primaryUserAssociatedWithEmail.id, undefined); + return await generateAndSendPasswordResetToken(linkingCandidate.id, undefined); } else { logger_1.logDebugMessage( `Password reset email not sent, isSignUpAllowed returned false for email: ${email}` @@ -205,36 +286,6 @@ function getAPIImplementation() { }; } } - // At this point, we know that some email password user exists with this email - // and also some primary user ID exist. We now need to find out if they are linked - // together or not. If they are linked together, then we can just generate the token - // else we check for more security conditions (since we will be linking them post token generation) - let areTheTwoAccountsLinked = - primaryUserAssociatedWithEmail.loginMethods.find((lm) => { - return lm.recipeUserId.getAsString() === emailPasswordAccount.recipeUserId.getAsString(); - }) !== undefined; - if (areTheTwoAccountsLinked) { - return await generateAndSendPasswordResetToken( - primaryUserAssociatedWithEmail.id, - emailPasswordAccount.recipeUserId - ); - } - // Here we know that the two accounts are NOT linked. We now need to check for an - // extra security measure here to make sure that the input email in the primary user - // is verified, and if not, we need to make sure that there is no other email / phone number - // associated with the primary user account. If there is, then we do not proceed. - /* - This security measure helps prevent the following attack: - An attacker has email A and they create an account using TP and it doesn't matter if A is verified or not. Now they create another account using EP with email A and verifies it. Both these accounts are linked. Now the attacker changes the email for EP recipe to B which makes the EP account unverified, but it's still linked. - - If the real owner of B tries to signup using EP, it will say that the account already exists so they may try to reset password which should be denied because then they will end up getting access to attacker's account and verify the EP account. - - The problem with this situation is if the EP account is verified, it will allow further sign-ups with email B which will also be linked to this primary account (that the attacker had created with email A). - - It is important to realize that the attacker had created another account with A because if they hadn't done that, then they wouldn't have access to this account after the real user resets the password which is why it is important to check there is another non-EP account linked to the primary such that the email is not the same as B. - - Exception to the above is that, if there is a third recipe account linked to the above two accounts and has B as verified, then we should allow reset password token generation because user has already proven that the owns the email B - */ // But first, this only matters it the user cares about checking for email verification status.. if (!shouldDoAccountLinkingResponse.shouldAutomaticallyLink) { // here we will go ahead with the token generation cause @@ -245,18 +296,8 @@ function getAPIImplementation() { emailPasswordAccount.recipeUserId ); } - if (!shouldDoAccountLinkingResponse.shouldRequireVerification) { - // the checks below are related to email verification, and if the user - // does not care about that, then we should just continue with token generation - return await generateAndSendPasswordResetToken( - primaryUserAssociatedWithEmail.id, - emailPasswordAccount.recipeUserId - ); - } - return await generateAndSendPasswordResetToken( - primaryUserAssociatedWithEmail.id, - emailPasswordAccount.recipeUserId - ); + // Here we accounted for both `shouldRequireVerification` by the above checks (where we return ERR_CODE_001) + return await generateAndSendPasswordResetToken(linkingCandidate.id, emailPasswordAccount.recipeUserId); }, passwordResetPOST: async function ({ formFields, token, tenantId, options, userContext }) { async function markEmailAsVerified(recipeUserId, email) { @@ -339,7 +380,6 @@ function getAPIImplementation() { // The function below will try and also create a primary user of the new account, this can happen if: // 1. the user was unverified and linking requires verification // We do not take try linking by session here, since this is supposed to be called without a session - // Still, the session object is passed around because it is a required input for shouldDoAutomaticAccountLinking const linkRes = await recipe_1.default.getInstance().tryLinkingByAccountInfoOrCreatePrimaryUser({ tenantId, inputUser: updatedUserAfterEmailVerification, @@ -366,7 +406,7 @@ function getAPIImplementation() { } let userIdForWhomTokenWasGenerated = tokenConsumptionResponse.userId; let emailForWhomTokenWasGenerated = tokenConsumptionResponse.email; - let existingUser = await __1.getUser(tokenConsumptionResponse.userId, userContext); + let existingUser = await __1.getUser(userIdForWhomTokenWasGenerated, userContext); if (existingUser === undefined) { // This should happen only cause of a race condition where the user // might be deleted before token creation and consumption. @@ -377,109 +417,126 @@ function getAPIImplementation() { status: "RESET_PASSWORD_INVALID_TOKEN_ERROR", }; } - // We start by checking if the existingUser is a primary user or not. If it is, - // then we will try and create a new email password user and link it to the primary user (if required) - if (existingUser.isPrimaryUser) { - // If this user contains an email password account for whom the token was generated, - // then we update that user's password. - let emailPasswordUserIsLinkedToExistingUser = - existingUser.loginMethods.find((lm) => { - // we check based on user ID and not email because the only time - // the primary user ID is used for token generation is if the email password - // user did not exist - in which case the value of emailPasswordUserExists will - // resolve to false anyway, and that's what we want. - // there is an edge case where if the email password recipe user was created - // after the password reset token generation, and it was linked to the - // primary user id (userIdForWhomTokenWasGenerated), in this case, - // we still don't allow password update, cause the user should try again - // and the token should be regenerated for the right recipe user. - return ( - lm.recipeUserId.getAsString() === userIdForWhomTokenWasGenerated && - lm.recipeId === "emailpassword" - ); - }) !== undefined; - if (emailPasswordUserIsLinkedToExistingUser) { + let tokenGeneratedForEmailPasswordUser = existingUser.loginMethods.some((lm) => { + // we check based on user ID and not email because the only time the user ID of another login method + // is used for token generation is if the email password user did not exist - in which case the + // value of emailPasswordUserIsLinkedToExistingUser will resolve to false anyway, and that's what we want. + // there is an edge case where if the email password recipe user was created + // after the password reset token generation, and it was linked to the + // primary user id (userIdForWhomTokenWasGenerated), in this case, + // we still don't allow password update, cause the user should try again + // and the token should be regenerated for the right recipe user. + return ( + lm.recipeUserId.getAsString() === userIdForWhomTokenWasGenerated && lm.recipeId === "emailpassword" + ); + }); + if (tokenGeneratedForEmailPasswordUser) { + if (!existingUser.isPrimaryUser) { + // If this is a recipe level emailpassword user, we can always allow them to reset their password. return doUpdatePasswordAndVerifyEmailAndTryLinkIfNotPrimary( new recipeUserId_1.default(userIdForWhomTokenWasGenerated) ); - } else { - // this means that the existingUser does not have an emailpassword user associated - // with it. It could now mean that no emailpassword user exists, or it could mean that - // the the ep user exists, but it's not linked to the current account. - // If no ep user doesn't exists, we will create one, and link it to the existing account. - // If ep user exists, then it means there is some race condition cause - // then the token should have been generated for that user instead of the primary user, - // and it shouldn't have come into this branch. So we can simply send a password reset - // invalid error and the user can try again. - // NOTE: We do not ask the dev if we should do account linking or not here - // cause we already have asked them this when generating an password reset token. - // In the edge case that the dev changes account linking allowance from true to false - // when it comes here, only a new recipe user id will be created and not linked - // cause createPrimaryUserIdOrLinkAccounts will disallow linking. This doesn't - // really cause any security issue. - let createUserResponse = await options.recipeImplementation.createNewRecipeUser({ - tenantId, - email: tokenConsumptionResponse.email, - password: newPassword, - userContext, - }); - if (createUserResponse.status === "EMAIL_ALREADY_EXISTS_ERROR") { - // this means that the user already existed and we can just return an invalid - // token (see the above comment) - return { - status: "RESET_PASSWORD_INVALID_TOKEN_ERROR", - }; - } else { - // we mark the email as verified because password reset also requires - // access to the email to work.. This has a good side effect that - // any other login method with the same email in existingAccount will also get marked - // as verified. - await markEmailAsVerified( - createUserResponse.user.loginMethods[0].recipeUserId, - tokenConsumptionResponse.email + } + // If the user is a primary user resetting the password of an emailpassword user linked to it + // we need to check for account takeover risk (similar to what we do when generating the token) + // We check if there is any login method in which the input email is verified. + // If that is the case, then it's proven that the user owns the email and we can + // trust linking of the email password account. + let emailVerified = + existingUser.loginMethods.find((lm) => { + return lm.hasSameEmailAs(emailForWhomTokenWasGenerated) && lm.verified; + }) !== undefined; + // finally, we check if the primary user has any other email / phone number + // associated with this account - and if it does, then it means that + // there is a risk of account takeover, so we do not allow the token to be generated + let hasOtherEmailOrPhone = + existingUser.loginMethods.find((lm) => { + // we do the extra undefined check below cause + // hasSameEmailAs returns false if the lm.email is undefined, and + // we want to check that the email is different as opposed to email + // not existing in lm. + return ( + (lm.email !== undefined && !lm.hasSameEmailAs(emailForWhomTokenWasGenerated)) || + lm.phoneNumber !== undefined ); - const updatedUser = await __1.getUser(createUserResponse.user.id, userContext); - if (updatedUser === undefined) { - throw new Error("Should never happen - user deleted after during password reset"); - } - createUserResponse.user = updatedUser; - // Now we try and link the accounts. The function below will try and also - // create a primary user of the new account, and if it does that, it's OK.. - // But in most cases, it will end up linking to existing account since the - // email is shared. - // We do not take try linking by session here, since this is supposed to be called without a session - // Still, the session object is passed around because it is a required input for shouldDoAutomaticAccountLinking - const linkRes = await recipe_1.default - .getInstance() - .tryLinkingByAccountInfoOrCreatePrimaryUser({ - tenantId, - inputUser: createUserResponse.user, - session: undefined, - userContext, - }); - const userAfterLinking = linkRes.status === "OK" ? linkRes.user : createUserResponse.user; - if (linkRes.status === "OK" && linkRes.user.id !== existingUser.id) { - // this means that the account we just linked to - // was not the one we had expected to link it to. This can happen - // due to some race condition or the other.. Either way, this - // is not an issue and we can just return OK - } - return { - status: "OK", - email: tokenConsumptionResponse.email, - user: userAfterLinking, - }; - } + }) !== undefined; + if (!emailVerified && hasOtherEmailOrPhone) { + // We can return an invalid token error, because in this case the token should not have been created + // whenever they try to re-create it they'll see the appropriate error message + return { + status: "RESET_PASSWORD_INVALID_TOKEN_ERROR", + }; } - } else { - // This means that the existing user is not a primary account, which implies that - // it must be a non linked email password account. In this case, we simply update the password. - // Linking to an existing account will be done after the user goes through the email - // verification flow once they log in (if applicable). + // since this doesn't result in linking and there is no risk of account takeover, we can allow the password reset to proceed return doUpdatePasswordAndVerifyEmailAndTryLinkIfNotPrimary( new recipeUserId_1.default(userIdForWhomTokenWasGenerated) ); } + // this means that the existingUser is primary but does not have an emailpassword user associated + // with it. It could now mean that no emailpassword user exists, or it could mean that + // the the ep user exists, but it's not linked to the current account. + // If no ep user doesn't exists, we will create one, and link it to the existing account. + // If ep user exists, then it means there is some race condition cause + // then the token should have been generated for that user instead of the primary user, + // and it shouldn't have come into this branch. So we can simply send a password reset + // invalid error and the user can try again. + // NOTE: We do not ask the dev if we should do account linking or not here + // cause we already have asked them this when generating an password reset token. + // In the edge case that the dev changes account linking allowance from true to false + // when it comes here, only a new recipe user id will be created and not linked + // cause createPrimaryUserIdOrLinkAccounts will disallow linking. This doesn't + // really cause any security issue. + let createUserResponse = await options.recipeImplementation.createNewRecipeUser({ + tenantId, + email: tokenConsumptionResponse.email, + password: newPassword, + userContext, + }); + if (createUserResponse.status === "EMAIL_ALREADY_EXISTS_ERROR") { + // this means that the user already existed and we can just return an invalid + // token (see the above comment) + return { + status: "RESET_PASSWORD_INVALID_TOKEN_ERROR", + }; + } else { + // we mark the email as verified because password reset also requires + // access to the email to work.. This has a good side effect that + // any other login method with the same email in existingAccount will also get marked + // as verified. + await markEmailAsVerified( + createUserResponse.user.loginMethods[0].recipeUserId, + tokenConsumptionResponse.email + ); + const updatedUser = await __1.getUser(createUserResponse.user.id, userContext); + if (updatedUser === undefined) { + throw new Error("Should never happen - user deleted after during password reset"); + } + createUserResponse.user = updatedUser; + // Now we try and link the accounts. The function below will try and also + // create a primary user of the new account, and if it does that, it's OK.. + // But in most cases, it will end up linking to existing account since the + // email is shared. + // We do not take try linking by session here, since this is supposed to be called without a session + // Still, the session object is passed around because it is a required input for shouldDoAutomaticAccountLinking + const linkRes = await recipe_1.default.getInstance().tryLinkingByAccountInfoOrCreatePrimaryUser({ + tenantId, + inputUser: createUserResponse.user, + session: undefined, + userContext, + }); + const userAfterLinking = linkRes.status === "OK" ? linkRes.user : createUserResponse.user; + if (linkRes.status === "OK" && linkRes.user.id !== existingUser.id) { + // this means that the account we just linked to + // was not the one we had expected to link it to. This can happen + // due to some race condition or the other.. Either way, this + // is not an issue and we can just return OK + } + return { + status: "OK", + email: tokenConsumptionResponse.email, + user: userAfterLinking, + }; + } }, signInPOST: async function ({ formFields, tenantId, session, options, userContext }) { const errorCodeMap = { diff --git a/lib/build/version.d.ts b/lib/build/version.d.ts index 94b306c8a..d00dc1c93 100644 --- a/lib/build/version.d.ts +++ b/lib/build/version.d.ts @@ -1,4 +1,4 @@ // @ts-nocheck -export declare const version = "20.1.4"; +export declare const version = "20.1.5"; export declare const cdiSupported: string[]; export declare const dashboardVersion = "0.13"; diff --git a/lib/build/version.js b/lib/build/version.js index 045a8d8dd..02ed3001a 100644 --- a/lib/build/version.js +++ b/lib/build/version.js @@ -15,7 +15,7 @@ exports.dashboardVersion = exports.cdiSupported = exports.version = void 0; * License for the specific language governing permissions and limitations * under the License. */ -exports.version = "20.1.4"; +exports.version = "20.1.5"; exports.cdiSupported = ["5.1"]; // Note: The actual script import for dashboard uses v{DASHBOARD_VERSION} exports.dashboardVersion = "0.13"; diff --git a/lib/ts/recipe/accountlinking/recipe.ts b/lib/ts/recipe/accountlinking/recipe.ts index 177d61def..a29c6bc7d 100644 --- a/lib/ts/recipe/accountlinking/recipe.ts +++ b/lib/ts/recipe/accountlinking/recipe.ts @@ -740,7 +740,7 @@ export default class Recipe extends RecipeModule { } }; - private async shouldBecomePrimaryUser( + public async shouldBecomePrimaryUser( user: User, tenantId: string, session: SessionContainerInterface | undefined, diff --git a/lib/ts/recipe/emailpassword/api/implementation.ts b/lib/ts/recipe/emailpassword/api/implementation.ts index 7467c2957..e81d66631 100644 --- a/lib/ts/recipe/emailpassword/api/implementation.ts +++ b/lib/ts/recipe/emailpassword/api/implementation.ts @@ -148,11 +148,54 @@ export default function getAPIImplementation(): APIInterface { } // we find the primary user ID from the user's list for later use. - let primaryUserAssociatedWithEmail = users.find((u) => u.isPrimaryUser); + let linkingCandidate = users.find((u) => u.isPrimaryUser); - // first we check if there even exists a primary user that has the input email + logDebugMessage("generatePasswordResetTokenPOST: primary linking candidate: " + linkingCandidate?.id); + logDebugMessage("generatePasswordResetTokenPOST: linking candidate count " + users.length); + + // If there is no existing primary user and there is a single option to link + // we see if that user can become primary (and a candidate for linking) + if (linkingCandidate === undefined && users.length > 0) { + // If the only user that exists with this email is a non-primary emailpassword user, then we can just let them reset their password, because: + // we are not going to link anything and there is no risk of account takeover. + if ( + users.length === 1 && + users[0].loginMethods[0].recipeUserId.getAsString() === + emailPasswordAccount?.recipeUserId.getAsString() + ) { + return await generateAndSendPasswordResetToken( + emailPasswordAccount.recipeUserId.getAsString(), + emailPasswordAccount.recipeUserId + ); + } + + const oldestUser = users.sort((a, b) => a.timeJoined - b.timeJoined)[0]; + logDebugMessage( + `generatePasswordResetTokenPOST: oldest recipe level-linking candidate: ${oldestUser.id} (w/ ${ + oldestUser.loginMethods[0].verified ? "verified" : "unverified" + } email)` + ); + // Otherwise, we check if the user can become primary. + const shouldBecomePrimaryUser = await AccountLinking.getInstance().shouldBecomePrimaryUser( + oldestUser, + tenantId, + undefined, + userContext + ); + + logDebugMessage( + `generatePasswordResetTokenPOST: recipe level-linking candidate ${ + shouldBecomePrimaryUser ? "can" : "can not" + } become primary` + ); + if (shouldBecomePrimaryUser) { + linkingCandidate = oldestUser; + } + } + + // first we check if there even exists a candidate user that has the input email // if not, then we do the regular flow for password reset. - if (primaryUserAssociatedWithEmail === undefined) { + if (linkingCandidate === undefined) { if (emailPasswordAccount === undefined) { logDebugMessage(`Password reset email not sent, unknown user email: ${email}`); return { @@ -165,19 +208,39 @@ export default function getAPIImplementation(): APIInterface { ); } - // Next we check if there is any login method in which the input email is verified. + /* + This security measure helps prevent the following attack: + An attacker has email A and they create an account using TP and it doesn't matter if A is verified or not. Now they create another + account using EP with email A and verifies it. Both these accounts are linked. Now the attacker changes the email for EP recipe to + B which makes the EP account unverified, but it's still linked. + + If the real owner of B tries to signup using EP, it will say that the account already exists so they may try to reset password which should be denied, + because then they will end up getting access to attacker's account and verify the EP account. + + The problem with this situation is if the EP account is verified, it will allow further sign-ups with email B which will also be linked to this primary account, + that the attacker had created with email A. + + It is important to realize that the attacker had created another account with A because if they hadn't done that, then they wouldn't + have access to this account after the real user resets the password which is why it is important to check there is another non-EP account + linked to the primary such that the email is not the same as B. + + Exception to the above is that, if there is a third recipe account linked to the above two accounts and has B as verified, then we should + allow reset password token generation because user has already proven that the owns the email B + */ + + // First we check if there is any login method in which the input email is verified. // If that is the case, then it's proven that the user owns the email and we can // trust linking of the email password account. let emailVerified = - primaryUserAssociatedWithEmail.loginMethods.find((lm) => { + linkingCandidate.loginMethods.find((lm) => { return lm.hasSameEmailAs(email) && lm.verified; }) !== undefined; - // finally, we check if the primary user has any other email / phone number + // then, we check if the primary user has any other email / phone number // associated with this account - and if it does, then it means that // there is a risk of account takeover, so we do not allow the token to be generated let hasOtherEmailOrPhone = - primaryUserAssociatedWithEmail.loginMethods.find((lm) => { + linkingCandidate.loginMethods.find((lm) => { // we do the extra undefined check below cause // hasSameEmailAs returns false if the lm.email is undefined, and // we want to check that the email is different as opposed to email @@ -185,6 +248,8 @@ export default function getAPIImplementation(): APIInterface { return (lm.email !== undefined && !lm.hasSameEmailAs(email)) || lm.phoneNumber !== undefined; }) !== undefined; + // If we allow this to pass, then: + // 1. the if (!emailVerified && hasOtherEmailOrPhone) { return { status: "PASSWORD_RESET_NOT_ALLOWED", @@ -193,6 +258,28 @@ export default function getAPIImplementation(): APIInterface { }; } + if (linkingCandidate.isPrimaryUser && emailPasswordAccount !== undefined) { + // If a primary user has the input email as verified or has no other emails then it is always allowed to reset their own password: + // - there is no risk of account takeover, because they have verified this email or haven't linked it to anything else (checked above this block) + // - there will be no linking as a result of this action, so we do not need to check for linking (checked here by seeing that the two accounts are already linked) + let areTheTwoAccountsLinked = + linkingCandidate.loginMethods.find((lm) => { + return lm.recipeUserId.getAsString() === emailPasswordAccount!.recipeUserId.getAsString(); + }) !== undefined; + + if (areTheTwoAccountsLinked) { + return await generateAndSendPasswordResetToken( + linkingCandidate.id, + emailPasswordAccount.recipeUserId + ); + } + } + + // Here we know that the two accounts are NOT linked. We now need to check for an + // extra security measure here to make sure that the input email in the primary user + // is verified, and if not, we need to make sure that there is no other email / phone number + // associated with the primary user account. If there is, then we do not proceed. + let shouldDoAccountLinkingResponse = await AccountLinking.getInstance().config.shouldDoAutomaticAccountLinking( emailPasswordAccount !== undefined ? emailPasswordAccount @@ -200,7 +287,7 @@ export default function getAPIImplementation(): APIInterface { recipeId: "emailpassword", email, }, - primaryUserAssociatedWithEmail, + linkingCandidate, undefined, tenantId, userContext @@ -242,7 +329,7 @@ export default function getAPIImplementation(): APIInterface { // notice that we pass in the primary user ID here. This means that // we will be creating a new email password account when the token // is consumed and linking it to this primary user. - return await generateAndSendPasswordResetToken(primaryUserAssociatedWithEmail.id, undefined); + return await generateAndSendPasswordResetToken(linkingCandidate.id, undefined); } else { logDebugMessage( `Password reset email not sent, isSignUpAllowed returned false for email: ${email}` @@ -253,40 +340,6 @@ export default function getAPIImplementation(): APIInterface { } } - // At this point, we know that some email password user exists with this email - // and also some primary user ID exist. We now need to find out if they are linked - // together or not. If they are linked together, then we can just generate the token - // else we check for more security conditions (since we will be linking them post token generation) - let areTheTwoAccountsLinked = - primaryUserAssociatedWithEmail.loginMethods.find((lm) => { - return lm.recipeUserId.getAsString() === emailPasswordAccount!.recipeUserId.getAsString(); - }) !== undefined; - - if (areTheTwoAccountsLinked) { - return await generateAndSendPasswordResetToken( - primaryUserAssociatedWithEmail.id, - emailPasswordAccount.recipeUserId - ); - } - - // Here we know that the two accounts are NOT linked. We now need to check for an - // extra security measure here to make sure that the input email in the primary user - // is verified, and if not, we need to make sure that there is no other email / phone number - // associated with the primary user account. If there is, then we do not proceed. - - /* - This security measure helps prevent the following attack: - An attacker has email A and they create an account using TP and it doesn't matter if A is verified or not. Now they create another account using EP with email A and verifies it. Both these accounts are linked. Now the attacker changes the email for EP recipe to B which makes the EP account unverified, but it's still linked. - - If the real owner of B tries to signup using EP, it will say that the account already exists so they may try to reset password which should be denied because then they will end up getting access to attacker's account and verify the EP account. - - The problem with this situation is if the EP account is verified, it will allow further sign-ups with email B which will also be linked to this primary account (that the attacker had created with email A). - - It is important to realize that the attacker had created another account with A because if they hadn't done that, then they wouldn't have access to this account after the real user resets the password which is why it is important to check there is another non-EP account linked to the primary such that the email is not the same as B. - - Exception to the above is that, if there is a third recipe account linked to the above two accounts and has B as verified, then we should allow reset password token generation because user has already proven that the owns the email B - */ - // But first, this only matters it the user cares about checking for email verification status.. if (!shouldDoAccountLinkingResponse.shouldAutomaticallyLink) { @@ -299,19 +352,8 @@ export default function getAPIImplementation(): APIInterface { ); } - if (!shouldDoAccountLinkingResponse.shouldRequireVerification) { - // the checks below are related to email verification, and if the user - // does not care about that, then we should just continue with token generation - return await generateAndSendPasswordResetToken( - primaryUserAssociatedWithEmail.id, - emailPasswordAccount.recipeUserId - ); - } - - return await generateAndSendPasswordResetToken( - primaryUserAssociatedWithEmail.id, - emailPasswordAccount.recipeUserId - ); + // Here we accounted for both `shouldRequireVerification` by the above checks (where we return ERR_CODE_001) + return await generateAndSendPasswordResetToken(linkingCandidate.id, emailPasswordAccount.recipeUserId); }, passwordResetPOST: async function ({ formFields, @@ -433,7 +475,6 @@ export default function getAPIImplementation(): APIInterface { // The function below will try and also create a primary user of the new account, this can happen if: // 1. the user was unverified and linking requires verification // We do not take try linking by session here, since this is supposed to be called without a session - // Still, the session object is passed around because it is a required input for shouldDoAutomaticAccountLinking const linkRes = await AccountLinking.getInstance().tryLinkingByAccountInfoOrCreatePrimaryUser({ tenantId, inputUser: updatedUserAfterEmailVerification, @@ -466,7 +507,7 @@ export default function getAPIImplementation(): APIInterface { let userIdForWhomTokenWasGenerated = tokenConsumptionResponse.userId; let emailForWhomTokenWasGenerated = tokenConsumptionResponse.email; - let existingUser = await getUser(tokenConsumptionResponse.userId, userContext); + let existingUser = await getUser(userIdForWhomTokenWasGenerated, userContext); if (existingUser === undefined) { // This should happen only cause of a race condition where the user @@ -479,112 +520,136 @@ export default function getAPIImplementation(): APIInterface { }; } - // We start by checking if the existingUser is a primary user or not. If it is, - // then we will try and create a new email password user and link it to the primary user (if required) + let tokenGeneratedForEmailPasswordUser = existingUser.loginMethods.some((lm) => { + // we check based on user ID and not email because the only time the user ID of another login method + // is used for token generation is if the email password user did not exist - in which case the + // value of emailPasswordUserIsLinkedToExistingUser will resolve to false anyway, and that's what we want. - if (existingUser.isPrimaryUser) { - // If this user contains an email password account for whom the token was generated, - // then we update that user's password. - let emailPasswordUserIsLinkedToExistingUser = - existingUser.loginMethods.find((lm) => { - // we check based on user ID and not email because the only time - // the primary user ID is used for token generation is if the email password - // user did not exist - in which case the value of emailPasswordUserExists will - // resolve to false anyway, and that's what we want. - - // there is an edge case where if the email password recipe user was created - // after the password reset token generation, and it was linked to the - // primary user id (userIdForWhomTokenWasGenerated), in this case, - // we still don't allow password update, cause the user should try again - // and the token should be regenerated for the right recipe user. - return ( - lm.recipeUserId.getAsString() === userIdForWhomTokenWasGenerated && - lm.recipeId === "emailpassword" - ); - }) !== undefined; + // there is an edge case where if the email password recipe user was created + // after the password reset token generation, and it was linked to the + // primary user id (userIdForWhomTokenWasGenerated), in this case, + // we still don't allow password update, cause the user should try again + // and the token should be regenerated for the right recipe user. + return ( + lm.recipeUserId.getAsString() === userIdForWhomTokenWasGenerated && lm.recipeId === "emailpassword" + ); + }); - if (emailPasswordUserIsLinkedToExistingUser) { + if (tokenGeneratedForEmailPasswordUser) { + if (!existingUser.isPrimaryUser) { + // If this is a recipe level emailpassword user, we can always allow them to reset their password. return doUpdatePasswordAndVerifyEmailAndTryLinkIfNotPrimary( new RecipeUserId(userIdForWhomTokenWasGenerated) ); - } else { - // this means that the existingUser does not have an emailpassword user associated - // with it. It could now mean that no emailpassword user exists, or it could mean that - // the the ep user exists, but it's not linked to the current account. - // If no ep user doesn't exists, we will create one, and link it to the existing account. - // If ep user exists, then it means there is some race condition cause - // then the token should have been generated for that user instead of the primary user, - // and it shouldn't have come into this branch. So we can simply send a password reset - // invalid error and the user can try again. - - // NOTE: We do not ask the dev if we should do account linking or not here - // cause we already have asked them this when generating an password reset token. - // In the edge case that the dev changes account linking allowance from true to false - // when it comes here, only a new recipe user id will be created and not linked - // cause createPrimaryUserIdOrLinkAccounts will disallow linking. This doesn't - // really cause any security issue. - - let createUserResponse = await options.recipeImplementation.createNewRecipeUser({ - tenantId, - email: tokenConsumptionResponse.email, - password: newPassword, - userContext, - }); - if (createUserResponse.status === "EMAIL_ALREADY_EXISTS_ERROR") { - // this means that the user already existed and we can just return an invalid - // token (see the above comment) - return { - status: "RESET_PASSWORD_INVALID_TOKEN_ERROR", - }; - } else { - // we mark the email as verified because password reset also requires - // access to the email to work.. This has a good side effect that - // any other login method with the same email in existingAccount will also get marked - // as verified. - await markEmailAsVerified( - createUserResponse.user.loginMethods[0].recipeUserId, - tokenConsumptionResponse.email + } + + // If the user is a primary user resetting the password of an emailpassword user linked to it + // we need to check for account takeover risk (similar to what we do when generating the token) + + // We check if there is any login method in which the input email is verified. + // If that is the case, then it's proven that the user owns the email and we can + // trust linking of the email password account. + let emailVerified = + existingUser.loginMethods.find((lm) => { + return lm.hasSameEmailAs(emailForWhomTokenWasGenerated) && lm.verified; + }) !== undefined; + + // finally, we check if the primary user has any other email / phone number + // associated with this account - and if it does, then it means that + // there is a risk of account takeover, so we do not allow the token to be generated + let hasOtherEmailOrPhone = + existingUser.loginMethods.find((lm) => { + // we do the extra undefined check below cause + // hasSameEmailAs returns false if the lm.email is undefined, and + // we want to check that the email is different as opposed to email + // not existing in lm. + return ( + (lm.email !== undefined && !lm.hasSameEmailAs(emailForWhomTokenWasGenerated)) || + lm.phoneNumber !== undefined ); - const updatedUser = await getUser(createUserResponse.user.id, userContext); - if (updatedUser === undefined) { - throw new Error("Should never happen - user deleted after during password reset"); - } - createUserResponse.user = updatedUser; - // Now we try and link the accounts. The function below will try and also - // create a primary user of the new account, and if it does that, it's OK.. - // But in most cases, it will end up linking to existing account since the - // email is shared. - // We do not take try linking by session here, since this is supposed to be called without a session - // Still, the session object is passed around because it is a required input for shouldDoAutomaticAccountLinking - const linkRes = await AccountLinking.getInstance().tryLinkingByAccountInfoOrCreatePrimaryUser({ - tenantId, - inputUser: createUserResponse.user, - session: undefined, - userContext, - }); - const userAfterLinking = linkRes.status === "OK" ? linkRes.user : createUserResponse.user; - if (linkRes.status === "OK" && linkRes.user.id !== existingUser.id) { - // this means that the account we just linked to - // was not the one we had expected to link it to. This can happen - // due to some race condition or the other.. Either way, this - // is not an issue and we can just return OK - } - return { - status: "OK", - email: tokenConsumptionResponse.email, - user: userAfterLinking, - }; - } + }) !== undefined; + + if (!emailVerified && hasOtherEmailOrPhone) { + // We can return an invalid token error, because in this case the token should not have been created + // whenever they try to re-create it they'll see the appropriate error message + return { + status: "RESET_PASSWORD_INVALID_TOKEN_ERROR", + }; } - } else { - // This means that the existing user is not a primary account, which implies that - // it must be a non linked email password account. In this case, we simply update the password. - // Linking to an existing account will be done after the user goes through the email - // verification flow once they log in (if applicable). + + // since this doesn't result in linking and there is no risk of account takeover, we can allow the password reset to proceed return doUpdatePasswordAndVerifyEmailAndTryLinkIfNotPrimary( new RecipeUserId(userIdForWhomTokenWasGenerated) ); } + + // this means that the existingUser is primary but does not have an emailpassword user associated + // with it. It could now mean that no emailpassword user exists, or it could mean that + // the the ep user exists, but it's not linked to the current account. + // If no ep user doesn't exists, we will create one, and link it to the existing account. + // If ep user exists, then it means there is some race condition cause + // then the token should have been generated for that user instead of the primary user, + // and it shouldn't have come into this branch. So we can simply send a password reset + // invalid error and the user can try again. + + // NOTE: We do not ask the dev if we should do account linking or not here + // cause we already have asked them this when generating an password reset token. + // In the edge case that the dev changes account linking allowance from true to false + // when it comes here, only a new recipe user id will be created and not linked + // cause createPrimaryUserIdOrLinkAccounts will disallow linking. This doesn't + // really cause any security issue. + + let createUserResponse = await options.recipeImplementation.createNewRecipeUser({ + tenantId, + email: tokenConsumptionResponse.email, + password: newPassword, + userContext, + }); + if (createUserResponse.status === "EMAIL_ALREADY_EXISTS_ERROR") { + // this means that the user already existed and we can just return an invalid + // token (see the above comment) + return { + status: "RESET_PASSWORD_INVALID_TOKEN_ERROR", + }; + } else { + // we mark the email as verified because password reset also requires + // access to the email to work.. This has a good side effect that + // any other login method with the same email in existingAccount will also get marked + // as verified. + await markEmailAsVerified( + createUserResponse.user.loginMethods[0].recipeUserId, + tokenConsumptionResponse.email + ); + const updatedUser = await getUser(createUserResponse.user.id, userContext); + if (updatedUser === undefined) { + throw new Error("Should never happen - user deleted after during password reset"); + } + createUserResponse.user = updatedUser; + // Now we try and link the accounts. The function below will try and also + // create a primary user of the new account, and if it does that, it's OK.. + // But in most cases, it will end up linking to existing account since the + // email is shared. + // We do not take try linking by session here, since this is supposed to be called without a session + // Still, the session object is passed around because it is a required input for shouldDoAutomaticAccountLinking + const linkRes = await AccountLinking.getInstance().tryLinkingByAccountInfoOrCreatePrimaryUser({ + tenantId, + inputUser: createUserResponse.user, + session: undefined, + userContext, + }); + const userAfterLinking = linkRes.status === "OK" ? linkRes.user : createUserResponse.user; + if (linkRes.status === "OK" && linkRes.user.id !== existingUser.id) { + // this means that the account we just linked to + // was not the one we had expected to link it to. This can happen + // due to some race condition or the other.. Either way, this + // is not an issue and we can just return OK + } + return { + status: "OK", + email: tokenConsumptionResponse.email, + user: userAfterLinking, + }; + } }, signInPOST: async function ({ diff --git a/lib/ts/version.ts b/lib/ts/version.ts index 019a9144c..eeb048f94 100644 --- a/lib/ts/version.ts +++ b/lib/ts/version.ts @@ -12,7 +12,7 @@ * License for the specific language governing permissions and limitations * under the License. */ -export const version = "20.1.4"; +export const version = "20.1.5"; export const cdiSupported = ["5.1"]; diff --git a/package-lock.json b/package-lock.json index 0b5e24635..1793d8619 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "supertokens-node", - "version": "20.1.4", + "version": "20.1.5", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "supertokens-node", - "version": "20.1.4", + "version": "20.1.5", "license": "Apache-2.0", "dependencies": { "buffer": "^6.0.3", diff --git a/package.json b/package.json index ed1cec641..723c30c41 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "supertokens-node", - "version": "20.1.4", + "version": "20.1.5", "description": "NodeJS driver for SuperTokens core", "main": "index.js", "scripts": {