diff --git a/CHANGELOG.md b/CHANGELOG.md index 660dbd83b..bbbeaad9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Breaking changes - Defined the entry points of the library using the "exports" field in package.json to make ESM imports more comfortable. This can cause some issues for applications using directory imports from the `lib/build` directory. In those cases we recommend adding `index.js` to the import path. - +- `isEmailChangeAllowed` now returns false for unverified addresses if input user is a primary user and there exists another user with the same email address and linking requires verification +- Generating a password reset token is now denied if all of the following is true: + - a linked email password user exists + - the email address is not verified + - the user has another email address or phone number associated with it +- Account linking based on emails now require the email to be verified in both users if `shouldRequireVerification` is set to `true` instead of only requiring it for the recipe user. - The access token cookie expiry has been changed from 100 years to 1 year due to some browsers capping the maximum expiry at 400 days. No action is needed on your part. ### Changes diff --git a/lib/build/recipe/accountlinking/index.js b/lib/build/recipe/accountlinking/index.js index ad00a94fb..68e7fbbb5 100644 --- a/lib/build/recipe/accountlinking/index.js +++ b/lib/build/recipe/accountlinking/index.js @@ -129,13 +129,14 @@ class Wrapper { } static async isEmailChangeAllowed(recipeUserId, newEmail, isVerified, session, userContext) { const user = await __1.getUser(recipeUserId.getAsString(), userContext); - return await recipe_1.default.getInstance().isEmailChangeAllowed({ + const res = await recipe_1.default.getInstance().isEmailChangeAllowed({ user, newEmail, isVerified, session, userContext: utils_1.getUserContext(userContext), }); + return res.allowed; } } exports.default = Wrapper; diff --git a/lib/build/recipe/accountlinking/recipe.d.ts b/lib/build/recipe/accountlinking/recipe.d.ts index 48970b36d..f6050bce1 100644 --- a/lib/build/recipe/accountlinking/recipe.d.ts +++ b/lib/build/recipe/accountlinking/recipe.d.ts @@ -104,7 +104,15 @@ export default class Recipe extends RecipeModule { isVerified: boolean; session: SessionContainerInterface | undefined; userContext: UserContext; - }) => Promise; + }) => Promise< + | { + allowed: true; + } + | { + allowed: false; + reason: "PRIMARY_USER_CONFLICT" | "ACCOUNT_TAKEOVER_RISK"; + } + >; verifyEmailForRecipeUserIfLinkedAccountsAreVerified: (input: { user: User; recipeUserId: RecipeUserId; diff --git a/lib/build/recipe/accountlinking/recipe.js b/lib/build/recipe/accountlinking/recipe.js index ac9e83d6e..656686bc6 100644 --- a/lib/build/recipe/accountlinking/recipe.js +++ b/lib/build/recipe/accountlinking/recipe.js @@ -384,35 +384,73 @@ class Recipe extends recipeModule_1.default { * on the link by mistake, causing account linking to happen which can result * in account take over if this recipe user is malicious. */ - let user = input.user; - if (user === undefined) { + let inputUser = input.user; + if (inputUser === undefined) { throw new Error("Passed in recipe user id does not exist"); } - for (const tenantId of user.tenantIds) { + for (const tenantId of inputUser.tenantIds) { let existingUsersWithNewEmail = await this.recipeInterfaceImpl.listUsersByAccountInfo({ - tenantId: user.tenantIds[0], + tenantId: inputUser.tenantIds[0], accountInfo: { email: input.newEmail, }, doUnionOfAccountInfo: false, userContext: input.userContext, }); - let primaryUserForNewEmail = existingUsersWithNewEmail.filter((u) => u.isPrimaryUser); - if (primaryUserForNewEmail.length > 1) { + let otherUsersWithNewEmail = existingUsersWithNewEmail.filter((u) => u.id !== inputUser.id); + let otherPrimaryUserForNewEmail = otherUsersWithNewEmail.filter((u) => u.isPrimaryUser); + if (otherPrimaryUserForNewEmail.length > 1) { throw new Error("You found a bug. Please report it on github.com/supertokens/supertokens-node"); } - if (user.isPrimaryUser) { + if (inputUser.isPrimaryUser) { // this is condition one from the above comment. - if (primaryUserForNewEmail.length === 1 && primaryUserForNewEmail[0].id !== user.id) { + if (otherPrimaryUserForNewEmail.length !== 0) { logger_1.logDebugMessage( - "isEmailChangeAllowed: returning false cause email change will lead to two primary users having same email" + `isEmailChangeAllowed: returning false cause email change will lead to two primary users having same email on ${tenantId}` ); - return false; + return { allowed: false, reason: "PRIMARY_USER_CONFLICT" }; + } + if (input.isVerified) { + logger_1.logDebugMessage( + `isEmailChangeAllowed: can change on ${tenantId} cause input user is primary, new email is verified and doesn't belong to any other primary user` + ); + continue; + } + if (inputUser.loginMethods.some((lm) => lm.hasSameEmailAs(input.newEmail) && lm.verified)) { + logger_1.logDebugMessage( + `isEmailChangeAllowed: can change on ${tenantId} cause input user is primary, new email is verified in another login method and doesn't belong to any other primary user` + ); + continue; + } + if (otherUsersWithNewEmail.length === 0) { + logger_1.logDebugMessage( + `isEmailChangeAllowed: can change on ${tenantId} cause input user is primary and the new email doesn't belong to any other user (primary or non-primary)` + ); + continue; + } + const shouldDoAccountLinking = await this.config.shouldDoAutomaticAccountLinking( + otherUsersWithNewEmail[0].loginMethods[0], + inputUser, + input.session, + tenantId, + input.userContext + ); + if (!shouldDoAccountLinking.shouldAutomaticallyLink) { + logger_1.logDebugMessage( + `isEmailChangeAllowed: can change on ${tenantId} cause linking is disabled` + ); + continue; + } + if (!shouldDoAccountLinking.shouldRequireVerification) { + logger_1.logDebugMessage( + `isEmailChangeAllowed: can change on ${tenantId} cause linking is doesn't require email verification` + ); + continue; } logger_1.logDebugMessage( - `isEmailChangeAllowed: can change on ${tenantId} cause input user is primary and new email doesn't belong to any other primary user` + `isEmailChangeAllowed: returning false because the user hasn't verified the new email address and there exists another user with it on ${tenantId} and linking requires verification` ); - continue; + return { allowed: false, reason: "ACCOUNT_TAKEOVER_RISK" }; } else { if (input.isVerified) { logger_1.logDebugMessage( @@ -420,16 +458,16 @@ class Recipe extends recipeModule_1.default { ); continue; } - if (user.loginMethods[0].hasSameEmailAs(input.newEmail)) { + if (inputUser.loginMethods[0].hasSameEmailAs(input.newEmail)) { logger_1.logDebugMessage( `isEmailChangeAllowed: can change on ${tenantId} cause input user is not a primary and new email is same as the older one` ); continue; } - if (primaryUserForNewEmail.length === 1) { + if (otherPrimaryUserForNewEmail.length === 1) { let shouldDoAccountLinking = await this.config.shouldDoAutomaticAccountLinking( - user.loginMethods[0], - primaryUserForNewEmail[0], + inputUser.loginMethods[0], + otherPrimaryUserForNewEmail[0], input.session, tenantId, input.userContext @@ -449,7 +487,7 @@ class Recipe extends recipeModule_1.default { logger_1.logDebugMessage( "isEmailChangeAllowed: returning false cause input user is not a primary there exists a primary user exists with the new email." ); - return false; + return { allowed: false, reason: "ACCOUNT_TAKEOVER_RISK" }; } logger_1.logDebugMessage( `isEmailChangeAllowed: can change on ${tenantId} cause input user is not a primary no primary user exists with the new email` @@ -460,7 +498,7 @@ class Recipe extends recipeModule_1.default { logger_1.logDebugMessage( "isEmailChangeAllowed: returning true cause email change can happen on all tenants the user is part of" ); - return true; + return { allowed: true }; }; this.verifyEmailForRecipeUserIfLinkedAccountsAreVerified = async (input) => { try { @@ -651,9 +689,20 @@ class Recipe extends recipeModule_1.default { ); return { status: "NO_LINK" }; } - if (shouldDoAccountLinking.shouldRequireVerification && !inputUser.loginMethods[0].verified) { + const accountInfoVerifiedInPrimUser = primaryUserThatCanBeLinkedToTheInputUser.loginMethods.some( + (lm) => + (inputUser.loginMethods[0].email !== undefined && + lm.hasSameEmailAs(inputUser.loginMethods[0].email)) || + (inputUser.loginMethods[0].phoneNumber !== undefined && + lm.hasSamePhoneNumberAs(inputUser.loginMethods[0].phoneNumber) && + lm.verified) + ); + if ( + shouldDoAccountLinking.shouldRequireVerification && + (!inputUser.loginMethods[0].verified || !accountInfoVerifiedInPrimUser) + ) { logger_1.logDebugMessage( - "tryLinkingByAccountInfoOrCreatePrimaryUser: not linking because shouldRequireVerification is true but the login method is not verified" + "tryLinkingByAccountInfoOrCreatePrimaryUser: not linking because shouldRequireVerification is true but the login method is not verified in the new or the primary user" ); return { status: "NO_LINK" }; } diff --git a/lib/build/recipe/emailpassword/api/implementation.js b/lib/build/recipe/emailpassword/api/implementation.js index 4e8045c98..6ed81771b 100644 --- a/lib/build/recipe/emailpassword/api/implementation.js +++ b/lib/build/recipe/emailpassword/api/implementation.js @@ -122,6 +122,31 @@ function getAPIImplementation() { emailPasswordAccount.recipeUserId ); } + // Next 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) => { + return lm.hasSameEmailAs(email) && 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 = + primaryUserAssociatedWithEmail.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 (!emailVerified && hasOtherEmailOrPhone) { + return { + status: "PASSWORD_RESET_NOT_ALLOWED", + reason: + "Reset password link was not created because of account take over risk. Please contact support. (ERR_CODE_001)", + }; + } let shouldDoAccountLinkingResponse = await recipe_1.default .getInstance() .config.shouldDoAutomaticAccountLinking( @@ -228,51 +253,10 @@ function getAPIImplementation() { emailPasswordAccount.recipeUserId ); } - // Now we start the required security checks. First we check if the primary user - // it has just one linked account. And if that's true, then we continue - // cause then there is no scope for account takeover - if (primaryUserAssociatedWithEmail.loginMethods.length === 1) { - return await generateAndSendPasswordResetToken( - primaryUserAssociatedWithEmail.id, - emailPasswordAccount.recipeUserId - ); - } - // Next 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) => { - return lm.hasSameEmailAs(email) && lm.verified; - }) !== undefined; - if (emailVerified) { - return await generateAndSendPasswordResetToken( - primaryUserAssociatedWithEmail.id, - emailPasswordAccount.recipeUserId - ); - } - // 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 = - primaryUserAssociatedWithEmail.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 (hasOtherEmailOrPhone) { - return { - status: "PASSWORD_RESET_NOT_ALLOWED", - reason: - "Reset password link was not created because of account take over risk. Please contact support. (ERR_CODE_001)", - }; - } else { - return await generateAndSendPasswordResetToken( - primaryUserAssociatedWithEmail.id, - emailPasswordAccount.recipeUserId - ); - } + return await generateAndSendPasswordResetToken( + primaryUserAssociatedWithEmail.id, + emailPasswordAccount.recipeUserId + ); }, passwordResetPOST: async function ({ formFields, token, tenantId, options, userContext }) { async function markEmailAsVerified(recipeUserId, email) { diff --git a/lib/build/recipe/emailpassword/recipeImplementation.js b/lib/build/recipe/emailpassword/recipeImplementation.js index 937dca20e..8c09609c1 100644 --- a/lib/build/recipe/emailpassword/recipeImplementation.js +++ b/lib/build/recipe/emailpassword/recipeImplementation.js @@ -6,6 +6,7 @@ var __importDefault = }; Object.defineProperty(exports, "__esModule", { value: true }); const recipe_1 = __importDefault(require("../accountlinking/recipe")); +const recipe_2 = __importDefault(require("../emailverification/recipe")); const normalisedURLPath_1 = __importDefault(require("../../normalisedURLPath")); const __1 = require("../.."); const constants_1 = require("./constants"); @@ -160,6 +161,38 @@ function getRecipeInterface(querier, getEmailPasswordConfig) { ); }, updateEmailOrPassword: async function (input) { + const accountLinking = recipe_1.default.getInstance(); + if (input.email) { + const user = await __1.getUser(input.recipeUserId.getAsString(), input.userContext); + if (user === undefined) { + return { status: "UNKNOWN_USER_ID_ERROR" }; + } + const evInstance = recipe_2.default.getInstance(); + let isEmailVerified = false; + if (evInstance) { + isEmailVerified = await evInstance.recipeInterfaceImpl.isEmailVerified({ + recipeUserId: input.recipeUserId, + email: input.email, + userContext: input.userContext, + }); + } + const isEmailChangeAllowed = await accountLinking.isEmailChangeAllowed({ + user, + isVerified: isEmailVerified, + newEmail: input.email, + session: undefined, + userContext: input.userContext, + }); + if (!isEmailChangeAllowed.allowed) { + return { + status: "EMAIL_CHANGE_NOT_ALLOWED_ERROR", + reason: + isEmailChangeAllowed.reason === "ACCOUNT_TAKEOVER_RISK" + ? "New email cannot be applied to existing account because of account takeover risks." + : "New email cannot be applied to existing account because of there is another primary user with the same email address.", + }; + } + } if (input.applyPasswordPolicy || input.applyPasswordPolicy === undefined) { let formFields = getEmailPasswordConfig().signUpFeature.formFields; if (input.password !== undefined) { diff --git a/lib/build/recipe/thirdparty/api/implementation.js b/lib/build/recipe/thirdparty/api/implementation.js index 860c226d2..e688b95ae 100644 --- a/lib/build/recipe/thirdparty/api/implementation.js +++ b/lib/build/recipe/thirdparty/api/implementation.js @@ -5,9 +5,8 @@ var __importDefault = return mod && mod.__esModule ? mod : { default: mod }; }; Object.defineProperty(exports, "__esModule", { value: true }); -const recipe_1 = __importDefault(require("../../accountlinking/recipe")); const emailverification_1 = __importDefault(require("../../emailverification")); -const recipe_2 = __importDefault(require("../../emailverification/recipe")); +const recipe_1 = __importDefault(require("../../emailverification/recipe")); const authUtils_1 = require("../../../authUtils"); const logger_1 = require("../../../logger"); function getAPIInterface() { @@ -36,7 +35,7 @@ function getAPIInterface() { "Cannot sign in / up due to security reasons. Please contact support. (ERR_CODE_023)", }, }; - const { provider, tenantId, options, session, userContext } = input; + const { provider, tenantId, options, userContext } = input; let oAuthTokensToUse = {}; if ("redirectURIInfo" in input && input.redirectURIInfo !== undefined) { oAuthTokensToUse = await provider.exchangeAuthCodeForOAuthTokens({ @@ -92,84 +91,19 @@ function getAPIInterface() { // We do this check here and not in the recipe function cause we want to keep the // recipe function checks to a minimum so that the dev has complete control of // what they can do. - // The isEmailChangeAllowed function takes in a isVerified boolean. Now, even though - // we already have that from the input, that's just what the provider says. If the - // provider says that the email is NOT verified, it could have been that the email + // The isEmailChangeAllowed and preAuthChecks functions take an isVerified boolean. + // Now, even though we already have that from the input, that's just what the provider says. + // If the provider says that the email is NOT verified, it could have been that the email // is verified on the user's account via supertokens on a previous sign in / up. // So we just check that as well before calling isEmailChangeAllowed const recipeUserId = authenticatingUser.loginMethod.recipeUserId; - if (!emailInfo.isVerified && recipe_2.default.getInstance() !== undefined) { + if (!emailInfo.isVerified && recipe_1.default.getInstance() !== undefined) { emailInfo.isVerified = await emailverification_1.default.isEmailVerified( recipeUserId, emailInfo.id, userContext ); } - /** - * In this API, during only a sign in, we check for isEmailChangeAllowed first, then - * change the email by calling the recipe function, then check if is sign in allowed. - * This may result in a few states where email change is allowed, but still, sign in - * is not allowed: - * - * Various outcomes of isSignInAllowed vs isEmailChangeAllowed - * isSignInAllowed result: - * - is primary user -> TRUE - * - is recipe user - * - other recipe user exists - * - no -> TRUE - * - yes - * - email verified -> TRUE - * - email unverified -> FALSE - * - other primary user exists - * - no -> TRUE - * - yes - * - email verification status - * - this && primary -> TRUE - * - !this && !primary -> FALSE - * - this && !primary -> FALSE - * - !this && primary -> FALSE - * - * isEmailChangeAllowed result: - * - is primary user -> TRUE - * - is recipe user - * - other recipe user exists - * - no -> TRUE - * - yes - * - email verified -> TRUE - * - email unverified -> TRUE - * - other primary user exists - * - no -> TRUE - * - yes - * - email verification status - * - this && primary -> TRUE - * - !this && !primary -> FALSE - * - this && !primary -> TRUE - * - !this && primary -> FALSE - * - * Based on the above, isEmailChangeAllowed can return true, but isSignInAllowed will return false - * in the following situations: - * - If a recipe user is signing in with a new email, other recipe users with the same email exist, - * and one of them is unverfied. In this case, the email change will happen in the social login - * recipe, but the user will not be able to login anyway. - * - * - If the recipe user is signing in with a new email, there exists a primary user with the same - * email, but this new email is verified for the recipe user already, but the primary user's email - * is not verified. - */ - let isEmailChangeAllowed = await recipe_1.default.getInstance().isEmailChangeAllowed({ - user: authenticatingUser.user, - isVerified: emailInfo.isVerified, - newEmail: emailInfo.id, - session, - userContext, - }); - if (!isEmailChangeAllowed) { - return { - status: "SIGN_IN_UP_NOT_ALLOWED", - reason: - "Cannot sign in / up because new email cannot be applied to existing account. Please contact support. (ERR_CODE_005)", - }; - } } const preAuthChecks = await authUtils_1.AuthUtils.preAuthChecks({ authenticatingAccountInfo: { @@ -218,6 +152,10 @@ function getAPIInterface() { tenantId, userContext, }); + if (response.status === "SIGN_IN_UP_NOT_ALLOWED") { + // In this case we do not need to do mapping, since the recipe function already has the right response shape. + return response; + } if (response.status !== "OK") { logger_1.logDebugMessage("signInUpPOST: erroring out because signInUp returned " + response.status); return authUtils_1.AuthUtils.getErrorStatusResponseWithReason( diff --git a/lib/build/recipe/thirdparty/recipeImplementation.js b/lib/build/recipe/thirdparty/recipeImplementation.js index 90f1b2dbf..df6887fc9 100644 --- a/lib/build/recipe/thirdparty/recipeImplementation.js +++ b/lib/build/recipe/thirdparty/recipeImplementation.js @@ -24,6 +24,32 @@ function getRecipeImplementation(querier, providers) { session, userContext, }) { + const accountLinking = recipe_1.default.getInstance(); + const users = await __1.listUsersByAccountInfo( + tenantId, + { thirdParty: { id: thirdPartyId, userId: thirdPartyUserId } }, + false, + userContext + ); + const user = users[0]; + if (user !== undefined) { + const isEmailChangeAllowed = await accountLinking.isEmailChangeAllowed({ + user, + isVerified: isVerified, + newEmail: email, + session, + userContext: userContext, + }); + if (!isEmailChangeAllowed.allowed) { + return { + status: "EMAIL_CHANGE_NOT_ALLOWED_ERROR", + reason: + isEmailChangeAllowed.reason === "PRIMARY_USER_CONFLICT" + ? "Email already associated with another primary user." + : "New email cannot be applied to existing account because of account takeover risks.", + }; + } + } let response = await querier.sendPostRequest( new normalisedURLPath_1.default(`/${tenantId}/recipe/signinup`), { @@ -89,7 +115,9 @@ function getRecipeImplementation(querier, providers) { return { status: "SIGN_IN_UP_NOT_ALLOWED", reason: - "Cannot sign in / up because new email cannot be applied to existing account. Please contact support. (ERR_CODE_005)", + response.reason === "Email already associated with another primary user." + ? "Cannot sign in / up because new email cannot be applied to existing account. Please contact support. (ERR_CODE_005)" + : "Cannot sign in / up because new email cannot be applied to existing account. Please contact support. (ERR_CODE_024)", }; } if (response.status === "OK") { diff --git a/lib/ts/recipe/accountlinking/index.ts b/lib/ts/recipe/accountlinking/index.ts index fb82f3138..995a341e0 100644 --- a/lib/ts/recipe/accountlinking/index.ts +++ b/lib/ts/recipe/accountlinking/index.ts @@ -165,13 +165,14 @@ export default class Wrapper { ) { const user = await getUser(recipeUserId.getAsString(), userContext); - return await Recipe.getInstance().isEmailChangeAllowed({ + const res = await Recipe.getInstance().isEmailChangeAllowed({ user, newEmail, isVerified, session, userContext: getUserContext(userContext), }); + return res.allowed; } } diff --git a/lib/ts/recipe/accountlinking/recipe.ts b/lib/ts/recipe/accountlinking/recipe.ts index 2b40199e7..5f7ae50c4 100644 --- a/lib/ts/recipe/accountlinking/recipe.ts +++ b/lib/ts/recipe/accountlinking/recipe.ts @@ -528,7 +528,7 @@ export default class Recipe extends RecipeModule { isVerified: boolean; session: SessionContainerInterface | undefined; userContext: UserContext; - }): Promise => { + }): Promise<{ allowed: true } | { allowed: false; reason: "PRIMARY_USER_CONFLICT" | "ACCOUNT_TAKEOVER_RISK" }> => { /** * The purpose of this function is to check that if a recipe user ID's email * can be changed or not. There are two conditions for when it can't be changed: @@ -544,15 +544,15 @@ export default class Recipe extends RecipeModule { * in account take over if this recipe user is malicious. */ - let user = input.user; + let inputUser = input.user; - if (user === undefined) { + if (inputUser === undefined) { throw new Error("Passed in recipe user id does not exist"); } - for (const tenantId of user.tenantIds) { + for (const tenantId of inputUser.tenantIds) { let existingUsersWithNewEmail = await this.recipeInterfaceImpl.listUsersByAccountInfo({ - tenantId: user.tenantIds[0], + tenantId: inputUser.tenantIds[0], accountInfo: { email: input.newEmail, }, @@ -560,23 +560,62 @@ export default class Recipe extends RecipeModule { userContext: input.userContext, }); - let primaryUserForNewEmail = existingUsersWithNewEmail.filter((u) => u.isPrimaryUser); - if (primaryUserForNewEmail.length > 1) { + let otherUsersWithNewEmail = existingUsersWithNewEmail.filter((u) => u.id !== inputUser!.id); + + let otherPrimaryUserForNewEmail = otherUsersWithNewEmail.filter((u) => u.isPrimaryUser); + if (otherPrimaryUserForNewEmail.length > 1) { throw new Error("You found a bug. Please report it on github.com/supertokens/supertokens-node"); } - if (user.isPrimaryUser) { + if (inputUser.isPrimaryUser) { // this is condition one from the above comment. - if (primaryUserForNewEmail.length === 1 && primaryUserForNewEmail[0].id !== user.id) { + if (otherPrimaryUserForNewEmail.length !== 0) { logDebugMessage( - "isEmailChangeAllowed: returning false cause email change will lead to two primary users having same email" + `isEmailChangeAllowed: returning false cause email change will lead to two primary users having same email on ${tenantId}` ); - return false; + return { allowed: false, reason: "PRIMARY_USER_CONFLICT" }; + } + if (input.isVerified) { + logDebugMessage( + `isEmailChangeAllowed: can change on ${tenantId} cause input user is primary, new email is verified and doesn't belong to any other primary user` + ); + continue; + } + if (inputUser.loginMethods.some((lm) => lm.hasSameEmailAs(input.newEmail) && lm.verified)) { + logDebugMessage( + `isEmailChangeAllowed: can change on ${tenantId} cause input user is primary, new email is verified in another login method and doesn't belong to any other primary user` + ); + continue; + } + if (otherUsersWithNewEmail.length === 0) { + logDebugMessage( + `isEmailChangeAllowed: can change on ${tenantId} cause input user is primary and the new email doesn't belong to any other user (primary or non-primary)` + ); + continue; + } + + const shouldDoAccountLinking = await this.config.shouldDoAutomaticAccountLinking( + otherUsersWithNewEmail[0].loginMethods[0], + inputUser, + input.session, + tenantId, + input.userContext + ); + if (!shouldDoAccountLinking.shouldAutomaticallyLink) { + logDebugMessage(`isEmailChangeAllowed: can change on ${tenantId} cause linking is disabled`); + continue; + } + + if (!shouldDoAccountLinking.shouldRequireVerification) { + logDebugMessage( + `isEmailChangeAllowed: can change on ${tenantId} cause linking is doesn't require email verification` + ); + continue; } logDebugMessage( - `isEmailChangeAllowed: can change on ${tenantId} cause input user is primary and new email doesn't belong to any other primary user` + `isEmailChangeAllowed: returning false because the user hasn't verified the new email address and there exists another user with it on ${tenantId} and linking requires verification` ); - continue; + return { allowed: false, reason: "ACCOUNT_TAKEOVER_RISK" }; } else { if (input.isVerified) { logDebugMessage( @@ -585,17 +624,17 @@ export default class Recipe extends RecipeModule { continue; } - if (user.loginMethods[0].hasSameEmailAs(input.newEmail)) { + if (inputUser.loginMethods[0].hasSameEmailAs(input.newEmail)) { logDebugMessage( `isEmailChangeAllowed: can change on ${tenantId} cause input user is not a primary and new email is same as the older one` ); continue; } - if (primaryUserForNewEmail.length === 1) { + if (otherPrimaryUserForNewEmail.length === 1) { let shouldDoAccountLinking = await this.config.shouldDoAutomaticAccountLinking( - user.loginMethods[0], - primaryUserForNewEmail[0], + inputUser.loginMethods[0], + otherPrimaryUserForNewEmail[0], input.session, tenantId, input.userContext @@ -618,7 +657,7 @@ export default class Recipe extends RecipeModule { logDebugMessage( "isEmailChangeAllowed: returning false cause input user is not a primary there exists a primary user exists with the new email." ); - return false; + return { allowed: false, reason: "ACCOUNT_TAKEOVER_RISK" }; } logDebugMessage( @@ -630,7 +669,7 @@ export default class Recipe extends RecipeModule { logDebugMessage( "isEmailChangeAllowed: returning true cause email change can happen on all tenants the user is part of" ); - return true; + return { allowed: true }; }; verifyEmailForRecipeUserIfLinkedAccountsAreVerified = async (input: { @@ -780,9 +819,20 @@ export default class Recipe extends RecipeModule { return { status: "NO_LINK" }; } - if (shouldDoAccountLinking.shouldRequireVerification && !inputUser.loginMethods[0].verified) { + const accountInfoVerifiedInPrimUser = primaryUserThatCanBeLinkedToTheInputUser.loginMethods.some( + (lm) => + (inputUser.loginMethods[0].email !== undefined && + lm.hasSameEmailAs(inputUser.loginMethods[0].email)) || + (inputUser.loginMethods[0].phoneNumber !== undefined && + lm.hasSamePhoneNumberAs(inputUser.loginMethods[0].phoneNumber) && + lm.verified) + ); + if ( + shouldDoAccountLinking.shouldRequireVerification && + (!inputUser.loginMethods[0].verified || !accountInfoVerifiedInPrimUser) + ) { logDebugMessage( - "tryLinkingByAccountInfoOrCreatePrimaryUser: not linking because shouldRequireVerification is true but the login method is not verified" + "tryLinkingByAccountInfoOrCreatePrimaryUser: not linking because shouldRequireVerification is true but the login method is not verified in the new or the primary user" ); return { status: "NO_LINK" }; } diff --git a/lib/ts/recipe/emailpassword/api/implementation.ts b/lib/ts/recipe/emailpassword/api/implementation.ts index b17c2cb2c..843e270cd 100644 --- a/lib/ts/recipe/emailpassword/api/implementation.ts +++ b/lib/ts/recipe/emailpassword/api/implementation.ts @@ -165,6 +165,34 @@ export default function getAPIImplementation(): APIInterface { ); } + // Next 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) => { + return lm.hasSameEmailAs(email) && 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 = + primaryUserAssociatedWithEmail.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 (!emailVerified && hasOtherEmailOrPhone) { + return { + status: "PASSWORD_RESET_NOT_ALLOWED", + reason: + "Reset password link was not created because of account take over risk. Please contact support. (ERR_CODE_001)", + }; + } + let shouldDoAccountLinkingResponse = await AccountLinking.getInstance().config.shouldDoAutomaticAccountLinking( emailPasswordAccount !== undefined ? emailPasswordAccount @@ -280,54 +308,10 @@ export default function getAPIImplementation(): APIInterface { ); } - // Now we start the required security checks. First we check if the primary user - // it has just one linked account. And if that's true, then we continue - // cause then there is no scope for account takeover - if (primaryUserAssociatedWithEmail.loginMethods.length === 1) { - return await generateAndSendPasswordResetToken( - primaryUserAssociatedWithEmail.id, - emailPasswordAccount.recipeUserId - ); - } - - // Next 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) => { - return lm.hasSameEmailAs(email) && lm.verified; - }) !== undefined; - - if (emailVerified) { - return await generateAndSendPasswordResetToken( - primaryUserAssociatedWithEmail.id, - emailPasswordAccount.recipeUserId - ); - } - - // 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 = - primaryUserAssociatedWithEmail.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 (hasOtherEmailOrPhone) { - return { - status: "PASSWORD_RESET_NOT_ALLOWED", - reason: - "Reset password link was not created because of account take over risk. Please contact support. (ERR_CODE_001)", - }; - } else { - return await generateAndSendPasswordResetToken( - primaryUserAssociatedWithEmail.id, - emailPasswordAccount.recipeUserId - ); - } + return await generateAndSendPasswordResetToken( + primaryUserAssociatedWithEmail.id, + emailPasswordAccount.recipeUserId + ); }, passwordResetPOST: async function ({ formFields, diff --git a/lib/ts/recipe/emailpassword/recipeImplementation.ts b/lib/ts/recipe/emailpassword/recipeImplementation.ts index e1f44b073..4c2e39d70 100644 --- a/lib/ts/recipe/emailpassword/recipeImplementation.ts +++ b/lib/ts/recipe/emailpassword/recipeImplementation.ts @@ -1,5 +1,6 @@ import { RecipeInterface, TypeNormalisedInput } from "./types"; import AccountLinking from "../accountlinking/recipe"; +import EmailVerification from "../emailverification/recipe"; import { Querier } from "../../querier"; import NormalisedURLPath from "../../normalisedURLPath"; import { getUser } from "../.."; @@ -252,6 +253,41 @@ export default function getRecipeInterface( } | { status: "PASSWORD_POLICY_VIOLATED_ERROR"; failureReason: string } > { + const accountLinking = AccountLinking.getInstance(); + if (input.email) { + const user = await getUser(input.recipeUserId.getAsString(), input.userContext); + + if (user === undefined) { + return { status: "UNKNOWN_USER_ID_ERROR" }; + } + + const evInstance = EmailVerification.getInstance(); + + let isEmailVerified = false; + if (evInstance) { + isEmailVerified = await evInstance.recipeInterfaceImpl.isEmailVerified({ + recipeUserId: input.recipeUserId, + email: input.email, + userContext: input.userContext, + }); + } + const isEmailChangeAllowed = await accountLinking.isEmailChangeAllowed({ + user, + isVerified: isEmailVerified, + newEmail: input.email, + session: undefined, + userContext: input.userContext, + }); + if (!isEmailChangeAllowed.allowed) { + return { + status: "EMAIL_CHANGE_NOT_ALLOWED_ERROR", + reason: + isEmailChangeAllowed.reason === "ACCOUNT_TAKEOVER_RISK" + ? "New email cannot be applied to existing account because of account takeover risks." + : "New email cannot be applied to existing account because of there is another primary user with the same email address.", + }; + } + } if (input.applyPasswordPolicy || input.applyPasswordPolicy === undefined) { let formFields = getEmailPasswordConfig().signUpFeature.formFields; if (input.password !== undefined) { diff --git a/lib/ts/recipe/thirdparty/api/implementation.ts b/lib/ts/recipe/thirdparty/api/implementation.ts index f9ea8884c..e91bb2dc3 100644 --- a/lib/ts/recipe/thirdparty/api/implementation.ts +++ b/lib/ts/recipe/thirdparty/api/implementation.ts @@ -1,5 +1,4 @@ import { APIInterface } from "../"; -import AccountLinking from "../../accountlinking/recipe"; import EmailVerification from "../../emailverification"; import EmailVerificationRecipe from "../../emailverification/recipe"; import { AuthUtils } from "../../../authUtils"; @@ -35,7 +34,7 @@ export default function getAPIInterface(): APIInterface { "Cannot sign in / up due to security reasons. Please contact support. (ERR_CODE_023)", }, }; - const { provider, tenantId, options, session, userContext } = input; + const { provider, tenantId, options, userContext } = input; let oAuthTokensToUse: any = {}; @@ -99,9 +98,9 @@ export default function getAPIInterface(): APIInterface { // recipe function checks to a minimum so that the dev has complete control of // what they can do. - // The isEmailChangeAllowed function takes in a isVerified boolean. Now, even though - // we already have that from the input, that's just what the provider says. If the - // provider says that the email is NOT verified, it could have been that the email + // The isEmailChangeAllowed and preAuthChecks functions take an isVerified boolean. + // Now, even though we already have that from the input, that's just what the provider says. + // If the provider says that the email is NOT verified, it could have been that the email // is verified on the user's account via supertokens on a previous sign in / up. // So we just check that as well before calling isEmailChangeAllowed @@ -114,74 +113,6 @@ export default function getAPIInterface(): APIInterface { userContext ); } - - /** - * In this API, during only a sign in, we check for isEmailChangeAllowed first, then - * change the email by calling the recipe function, then check if is sign in allowed. - * This may result in a few states where email change is allowed, but still, sign in - * is not allowed: - * - * Various outcomes of isSignInAllowed vs isEmailChangeAllowed - * isSignInAllowed result: - * - is primary user -> TRUE - * - is recipe user - * - other recipe user exists - * - no -> TRUE - * - yes - * - email verified -> TRUE - * - email unverified -> FALSE - * - other primary user exists - * - no -> TRUE - * - yes - * - email verification status - * - this && primary -> TRUE - * - !this && !primary -> FALSE - * - this && !primary -> FALSE - * - !this && primary -> FALSE - * - * isEmailChangeAllowed result: - * - is primary user -> TRUE - * - is recipe user - * - other recipe user exists - * - no -> TRUE - * - yes - * - email verified -> TRUE - * - email unverified -> TRUE - * - other primary user exists - * - no -> TRUE - * - yes - * - email verification status - * - this && primary -> TRUE - * - !this && !primary -> FALSE - * - this && !primary -> TRUE - * - !this && primary -> FALSE - * - * Based on the above, isEmailChangeAllowed can return true, but isSignInAllowed will return false - * in the following situations: - * - If a recipe user is signing in with a new email, other recipe users with the same email exist, - * and one of them is unverfied. In this case, the email change will happen in the social login - * recipe, but the user will not be able to login anyway. - * - * - If the recipe user is signing in with a new email, there exists a primary user with the same - * email, but this new email is verified for the recipe user already, but the primary user's email - * is not verified. - */ - - let isEmailChangeAllowed = await AccountLinking.getInstance().isEmailChangeAllowed({ - user: authenticatingUser.user, - isVerified: emailInfo.isVerified, - newEmail: emailInfo.id, - session, - userContext, - }); - - if (!isEmailChangeAllowed) { - return { - status: "SIGN_IN_UP_NOT_ALLOWED", - reason: - "Cannot sign in / up because new email cannot be applied to existing account. Please contact support. (ERR_CODE_005)", - }; - } } const preAuthChecks = await AuthUtils.preAuthChecks({ @@ -232,6 +163,11 @@ export default function getAPIInterface(): APIInterface { userContext, }); + if (response.status === "SIGN_IN_UP_NOT_ALLOWED") { + // In this case we do not need to do mapping, since the recipe function already has the right response shape. + return response; + } + if (response.status !== "OK") { logDebugMessage("signInUpPOST: erroring out because signInUp returned " + response.status); return AuthUtils.getErrorStatusResponseWithReason(response, errorCodeMap, "SIGN_IN_UP_NOT_ALLOWED"); diff --git a/lib/ts/recipe/thirdparty/recipeImplementation.ts b/lib/ts/recipe/thirdparty/recipeImplementation.ts index 58dbbc248..f730537a3 100644 --- a/lib/ts/recipe/thirdparty/recipeImplementation.ts +++ b/lib/ts/recipe/thirdparty/recipeImplementation.ts @@ -5,7 +5,7 @@ import { findAndCreateProviderInstance, mergeProvidersFromCoreAndStatic } from " import AccountLinking from "../accountlinking/recipe"; import MultitenancyRecipe from "../multitenancy/recipe"; import RecipeUserId from "../../recipeUserId"; -import { getUser } from "../.."; +import { getUser, listUsersByAccountInfo } from "../.."; import { User as UserType } from "../../types"; import { User } from "../../user"; import { AuthUtils } from "../../authUtils"; @@ -16,6 +16,33 @@ export default function getRecipeImplementation(querier: Querier, providers: Pro this: RecipeInterface, { thirdPartyId, thirdPartyUserId, email, isVerified, tenantId, session, userContext } ) { + const accountLinking = AccountLinking.getInstance(); + const users = await listUsersByAccountInfo( + tenantId, + { thirdParty: { id: thirdPartyId, userId: thirdPartyUserId } }, + false, + userContext + ); + + const user = users[0]; + if (user !== undefined) { + const isEmailChangeAllowed = await accountLinking.isEmailChangeAllowed({ + user, + isVerified: isVerified, + newEmail: email, + session, + userContext: userContext, + }); + if (!isEmailChangeAllowed.allowed) { + return { + status: "EMAIL_CHANGE_NOT_ALLOWED_ERROR", + reason: + isEmailChangeAllowed.reason === "PRIMARY_USER_CONFLICT" + ? "Email already associated with another primary user." + : "New email cannot be applied to existing account because of account takeover risks.", + }; + } + } let response = await querier.sendPostRequest( new NormalisedURLPath(`/${tenantId}/recipe/signinup`), { @@ -115,7 +142,9 @@ export default function getRecipeImplementation(querier: Querier, providers: Pro return { status: "SIGN_IN_UP_NOT_ALLOWED", reason: - "Cannot sign in / up because new email cannot be applied to existing account. Please contact support. (ERR_CODE_005)", + response.reason === "Email already associated with another primary user." + ? "Cannot sign in / up because new email cannot be applied to existing account. Please contact support. (ERR_CODE_005)" + : "Cannot sign in / up because new email cannot be applied to existing account. Please contact support. (ERR_CODE_024)", }; } diff --git a/test/emailpassword/updateEmailPass.test.js b/test/emailpassword/updateEmailPass.test.js index 55c0ac765..11b350187 100644 --- a/test/emailpassword/updateEmailPass.test.js +++ b/test/emailpassword/updateEmailPass.test.js @@ -127,7 +127,7 @@ describe(`updateEmailPassTest: ${printPath("[test/emailpassword/updateEmailPass. let res = await signIn("public", "test@gmail.com", "testPass123"); const res2 = await updateEmailOrPassword({ - userId: STExpress.convertToRecipeUserId(res.user.id), + recipeUserId: STExpress.convertToRecipeUserId(res.user.id), email: "test2@gmail.com", password: "test", }); @@ -224,7 +224,7 @@ describe(`updateEmailPassTest: ${printPath("[test/emailpassword/updateEmailPass. let res = await signIn("public", "test@gmail.com", "testPass123"); const res2 = await updateEmailOrPassword({ - userId: STExpress.convertToRecipeUserId(res.user.id), + recipeUserId: STExpress.convertToRecipeUserId(res.user.id), email: "test2@gmail.com", password: "1", });