From f9a31c173d0263faa526ac2092d7c4f66151fe0e Mon Sep 17 00:00:00 2001 From: Mihaly Lengyel Date: Tue, 18 Jun 2024 22:52:24 +0200 Subject: [PATCH 1/7] feat: update email and pw change logic and add more security checks --- lib/build/recipe/accountlinking/recipe.js | 35 ++++++-- .../emailpassword/api/implementation.js | 74 +++++++---------- .../emailpassword/recipeImplementation.js | 31 +++++++ .../recipe/thirdparty/recipeImplementation.js | 25 ++++++ lib/ts/recipe/accountlinking/recipe.ts | 36 +++++++-- .../emailpassword/api/implementation.ts | 80 ++++++++----------- .../emailpassword/recipeImplementation.ts | 34 ++++++++ .../recipe/thirdparty/recipeImplementation.ts | 28 ++++++- test/emailpassword/updateEmailPass.test.js | 4 +- 9 files changed, 239 insertions(+), 108 deletions(-) diff --git a/lib/build/recipe/accountlinking/recipe.js b/lib/build/recipe/accountlinking/recipe.js index ac9e83d6e..3d8319a61 100644 --- a/lib/build/recipe/accountlinking/recipe.js +++ b/lib/build/recipe/accountlinking/recipe.js @@ -397,18 +397,41 @@ class Recipe extends recipeModule_1.default { doUnionOfAccountInfo: false, userContext: input.userContext, }); - let primaryUserForNewEmail = existingUsersWithNewEmail.filter((u) => u.isPrimaryUser); - if (primaryUserForNewEmail.length > 1) { + let otherUsersWithNewEmail = existingUsersWithNewEmail.filter((u) => u.id !== user.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) { // 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; } + if ( + !input.isVerified && // TODO: verify + !user.loginMethods.some((lm) => lm.hasSameEmailAs(input.newEmail) && lm.verified) && + otherUsersWithNewEmail.length !== 0 + ) { + let shouldDoAccountLinking = await this.config.shouldDoAutomaticAccountLinking( + otherUsersWithNewEmail[0].loginMethods[0], + user, + input.session, + tenantId, + input.userContext + ); + if ( + shouldDoAccountLinking.shouldAutomaticallyLink && + shouldDoAccountLinking.shouldRequireVerification + ) { + logger_1.logDebugMessage( + `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` + ); + return false; + } + } logger_1.logDebugMessage( `isEmailChangeAllowed: can change on ${tenantId} cause input user is primary and new email doesn't belong to any other primary user` ); @@ -426,10 +449,10 @@ class Recipe extends recipeModule_1.default { ); continue; } - if (primaryUserForNewEmail.length === 1) { + if (otherPrimaryUserForNewEmail.length === 1) { let shouldDoAccountLinking = await this.config.shouldDoAutomaticAccountLinking( user.loginMethods[0], - primaryUserForNewEmail[0], + otherPrimaryUserForNewEmail[0], input.session, tenantId, input.userContext 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..1b2f001fd 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,36 @@ 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) { + return { + status: "EMAIL_CHANGE_NOT_ALLOWED_ERROR", + // TODO: error code? + reason: "New email cannot be applied to existing account because of account takeover risks.", + }; + } + } if (input.applyPasswordPolicy || input.applyPasswordPolicy === undefined) { let formFields = getEmailPasswordConfig().signUpFeature.formFields; if (input.password !== undefined) { diff --git a/lib/build/recipe/thirdparty/recipeImplementation.js b/lib/build/recipe/thirdparty/recipeImplementation.js index 90f1b2dbf..c5e854675 100644 --- a/lib/build/recipe/thirdparty/recipeImplementation.js +++ b/lib/build/recipe/thirdparty/recipeImplementation.js @@ -24,6 +24,31 @@ 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) { + return { + status: "EMAIL_CHANGE_NOT_ALLOWED_ERROR", + // TODO: error code? In this case reason is overwritten by ERR_CODE_005 by the wrapping function + // if called by our build-in APIs + reason: "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`), { diff --git a/lib/ts/recipe/accountlinking/recipe.ts b/lib/ts/recipe/accountlinking/recipe.ts index 2b40199e7..47ecb180e 100644 --- a/lib/ts/recipe/accountlinking/recipe.ts +++ b/lib/ts/recipe/accountlinking/recipe.ts @@ -560,19 +560,43 @@ 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 !== user!.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) { // 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; } + if ( + !input.isVerified && // TODO: verify + !user.loginMethods.some((lm) => lm.hasSameEmailAs(input.newEmail) && lm.verified) && + otherUsersWithNewEmail.length !== 0 + ) { + let shouldDoAccountLinking = await this.config.shouldDoAutomaticAccountLinking( + otherUsersWithNewEmail[0].loginMethods[0], + user, + input.session, + tenantId, + input.userContext + ); + if ( + shouldDoAccountLinking.shouldAutomaticallyLink && + shouldDoAccountLinking.shouldRequireVerification + ) { + logDebugMessage( + `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` + ); + return false; + } + } logDebugMessage( `isEmailChangeAllowed: can change on ${tenantId} cause input user is primary and new email doesn't belong to any other primary user` ); @@ -592,10 +616,10 @@ export default class Recipe extends RecipeModule { continue; } - if (primaryUserForNewEmail.length === 1) { + if (otherPrimaryUserForNewEmail.length === 1) { let shouldDoAccountLinking = await this.config.shouldDoAutomaticAccountLinking( user.loginMethods[0], - primaryUserForNewEmail[0], + otherPrimaryUserForNewEmail[0], input.session, tenantId, input.userContext 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..c3f85ac05 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,39 @@ 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, // TODO: should this also get session? + userContext: input.userContext, + }); + if (!isEmailChangeAllowed) { + return { + status: "EMAIL_CHANGE_NOT_ALLOWED_ERROR", + // TODO: error code? + reason: "New email cannot be applied to existing account because of account takeover risks.", + }; + } + } if (input.applyPasswordPolicy || input.applyPasswordPolicy === undefined) { let formFields = getEmailPasswordConfig().signUpFeature.formFields; if (input.password !== undefined) { diff --git a/lib/ts/recipe/thirdparty/recipeImplementation.ts b/lib/ts/recipe/thirdparty/recipeImplementation.ts index 58dbbc248..0eb994403 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,32 @@ 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) { + return { + status: "EMAIL_CHANGE_NOT_ALLOWED_ERROR", + // TODO: error code? In this case reason is overwritten by ERR_CODE_005 by the wrapping function + // if called by our build-in APIs + reason: "New email cannot be applied to existing account because of account takeover risks.", + }; + } + } let response = await querier.sendPostRequest( new NormalisedURLPath(`/${tenantId}/recipe/signinup`), { 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", }); From b3a5cac87eb7231a00e92b763e265b981004e37c Mon Sep 17 00:00:00 2001 From: Mihaly Lengyel Date: Tue, 25 Jun 2024 13:08:16 +0200 Subject: [PATCH 2/7] feat: update error messages --- lib/build/recipe/accountlinking/index.js | 3 +- lib/build/recipe/accountlinking/recipe.d.ts | 10 +++- lib/build/recipe/accountlinking/recipe.js | 45 +++++++++++------- .../emailpassword/recipeImplementation.js | 8 ++-- .../recipe/thirdparty/api/implementation.js | 6 ++- .../recipe/thirdparty/recipeImplementation.js | 13 +++-- lib/ts/recipe/accountlinking/index.ts | 3 +- lib/ts/recipe/accountlinking/recipe.ts | 47 ++++++++++++------- .../emailpassword/recipeImplementation.ts | 10 ++-- .../recipe/thirdparty/api/implementation.ts | 6 ++- .../recipe/thirdparty/recipeImplementation.ts | 13 +++-- 11 files changed, 105 insertions(+), 59 deletions(-) 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 3d8319a61..c3f01ab22 100644 --- a/lib/build/recipe/accountlinking/recipe.js +++ b/lib/build/recipe/accountlinking/recipe.js @@ -384,40 +384,40 @@ 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 otherUsersWithNewEmail = existingUsersWithNewEmail.filter((u) => u.id !== user.id); + 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 (otherPrimaryUserForNewEmail.length !== 0) { logger_1.logDebugMessage( `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 && // TODO: verify - !user.loginMethods.some((lm) => lm.hasSameEmailAs(input.newEmail) && lm.verified) && + !input.isVerified && + !inputUser.loginMethods.some((lm) => lm.hasSameEmailAs(input.newEmail) && lm.verified) && otherUsersWithNewEmail.length !== 0 ) { let shouldDoAccountLinking = await this.config.shouldDoAutomaticAccountLinking( otherUsersWithNewEmail[0].loginMethods[0], - user, + inputUser, input.session, tenantId, input.userContext @@ -429,7 +429,7 @@ class Recipe extends recipeModule_1.default { logger_1.logDebugMessage( `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` ); - return false; + return { allowed: false, reason: "ACCOUNT_TAKEOVER_RISK" }; } } logger_1.logDebugMessage( @@ -443,7 +443,7 @@ 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` ); @@ -451,7 +451,7 @@ class Recipe extends recipeModule_1.default { } if (otherPrimaryUserForNewEmail.length === 1) { let shouldDoAccountLinking = await this.config.shouldDoAutomaticAccountLinking( - user.loginMethods[0], + inputUser.loginMethods[0], otherPrimaryUserForNewEmail[0], input.session, tenantId, @@ -472,7 +472,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` @@ -483,7 +483,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 { @@ -674,9 +674,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/recipeImplementation.js b/lib/build/recipe/emailpassword/recipeImplementation.js index 1b2f001fd..8c09609c1 100644 --- a/lib/build/recipe/emailpassword/recipeImplementation.js +++ b/lib/build/recipe/emailpassword/recipeImplementation.js @@ -183,11 +183,13 @@ function getRecipeInterface(querier, getEmailPasswordConfig) { session: undefined, userContext: input.userContext, }); - if (!isEmailChangeAllowed) { + if (!isEmailChangeAllowed.allowed) { return { status: "EMAIL_CHANGE_NOT_ALLOWED_ERROR", - // TODO: error code? - reason: "New email cannot be applied to existing account because of account takeover risks.", + 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.", }; } } diff --git a/lib/build/recipe/thirdparty/api/implementation.js b/lib/build/recipe/thirdparty/api/implementation.js index 860c226d2..571ce19da 100644 --- a/lib/build/recipe/thirdparty/api/implementation.js +++ b/lib/build/recipe/thirdparty/api/implementation.js @@ -163,11 +163,13 @@ function getAPIInterface() { session, userContext, }); - if (!isEmailChangeAllowed) { + if (!isEmailChangeAllowed.allowed) { 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)", + isEmailChangeAllowed.reason === "PRIMARY_USER_CONFLICT" + ? "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/lib/build/recipe/thirdparty/recipeImplementation.js b/lib/build/recipe/thirdparty/recipeImplementation.js index c5e854675..df6887fc9 100644 --- a/lib/build/recipe/thirdparty/recipeImplementation.js +++ b/lib/build/recipe/thirdparty/recipeImplementation.js @@ -40,12 +40,13 @@ function getRecipeImplementation(querier, providers) { session, userContext: userContext, }); - if (!isEmailChangeAllowed) { + if (!isEmailChangeAllowed.allowed) { return { status: "EMAIL_CHANGE_NOT_ALLOWED_ERROR", - // TODO: error code? In this case reason is overwritten by ERR_CODE_005 by the wrapping function - // if called by our build-in APIs - reason: "New email cannot be applied to existing account because of account takeover risks.", + 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.", }; } } @@ -114,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 47ecb180e..092ae3e34 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,29 +560,29 @@ export default class Recipe extends RecipeModule { userContext: input.userContext, }); - let otherUsersWithNewEmail = existingUsersWithNewEmail.filter((u) => u.id !== user!.id); + 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 (otherPrimaryUserForNewEmail.length !== 0) { logDebugMessage( `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 && // TODO: verify - !user.loginMethods.some((lm) => lm.hasSameEmailAs(input.newEmail) && lm.verified) && + !input.isVerified && + !inputUser.loginMethods.some((lm) => lm.hasSameEmailAs(input.newEmail) && lm.verified) && otherUsersWithNewEmail.length !== 0 ) { let shouldDoAccountLinking = await this.config.shouldDoAutomaticAccountLinking( otherUsersWithNewEmail[0].loginMethods[0], - user, + inputUser, input.session, tenantId, input.userContext @@ -594,7 +594,7 @@ export default class Recipe extends RecipeModule { logDebugMessage( `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` ); - return false; + return { allowed: false, reason: "ACCOUNT_TAKEOVER_RISK" }; } } logDebugMessage( @@ -609,7 +609,7 @@ 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` ); @@ -618,7 +618,7 @@ export default class Recipe extends RecipeModule { if (otherPrimaryUserForNewEmail.length === 1) { let shouldDoAccountLinking = await this.config.shouldDoAutomaticAccountLinking( - user.loginMethods[0], + inputUser.loginMethods[0], otherPrimaryUserForNewEmail[0], input.session, tenantId, @@ -642,7 +642,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( @@ -654,7 +654,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: { @@ -804,9 +804,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/recipeImplementation.ts b/lib/ts/recipe/emailpassword/recipeImplementation.ts index c3f85ac05..4c2e39d70 100644 --- a/lib/ts/recipe/emailpassword/recipeImplementation.ts +++ b/lib/ts/recipe/emailpassword/recipeImplementation.ts @@ -275,14 +275,16 @@ export default function getRecipeInterface( user, isVerified: isEmailVerified, newEmail: input.email, - session: undefined, // TODO: should this also get session? + session: undefined, userContext: input.userContext, }); - if (!isEmailChangeAllowed) { + if (!isEmailChangeAllowed.allowed) { return { status: "EMAIL_CHANGE_NOT_ALLOWED_ERROR", - // TODO: error code? - reason: "New email cannot be applied to existing account because of account takeover risks.", + 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.", }; } } diff --git a/lib/ts/recipe/thirdparty/api/implementation.ts b/lib/ts/recipe/thirdparty/api/implementation.ts index f9ea8884c..1da7a639a 100644 --- a/lib/ts/recipe/thirdparty/api/implementation.ts +++ b/lib/ts/recipe/thirdparty/api/implementation.ts @@ -175,11 +175,13 @@ export default function getAPIInterface(): APIInterface { userContext, }); - if (!isEmailChangeAllowed) { + if (!isEmailChangeAllowed.allowed) { 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)", + isEmailChangeAllowed.reason === "PRIMARY_USER_CONFLICT" + ? "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/lib/ts/recipe/thirdparty/recipeImplementation.ts b/lib/ts/recipe/thirdparty/recipeImplementation.ts index 0eb994403..f730537a3 100644 --- a/lib/ts/recipe/thirdparty/recipeImplementation.ts +++ b/lib/ts/recipe/thirdparty/recipeImplementation.ts @@ -33,12 +33,13 @@ export default function getRecipeImplementation(querier: Querier, providers: Pro session, userContext: userContext, }); - if (!isEmailChangeAllowed) { + if (!isEmailChangeAllowed.allowed) { return { status: "EMAIL_CHANGE_NOT_ALLOWED_ERROR", - // TODO: error code? In this case reason is overwritten by ERR_CODE_005 by the wrapping function - // if called by our build-in APIs - reason: "New email cannot be applied to existing account because of account takeover risks.", + 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.", }; } } @@ -141,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)", }; } From 1a9bd398450bc066f1750be5b00d2f21c5e9281a Mon Sep 17 00:00:00 2001 From: Mihaly Lengyel Date: Tue, 25 Jun 2024 23:42:27 +0200 Subject: [PATCH 3/7] refactor: improve debug logs and clarify conditions --- lib/build/recipe/accountlinking/recipe.js | 59 ++++++++++++++--------- lib/ts/recipe/accountlinking/recipe.ts | 59 ++++++++++++++--------- 2 files changed, 74 insertions(+), 44 deletions(-) diff --git a/lib/build/recipe/accountlinking/recipe.js b/lib/build/recipe/accountlinking/recipe.js index c3f01ab22..656686bc6 100644 --- a/lib/build/recipe/accountlinking/recipe.js +++ b/lib/build/recipe/accountlinking/recipe.js @@ -410,32 +410,47 @@ class Recipe extends recipeModule_1.default { ); return { allowed: false, reason: "PRIMARY_USER_CONFLICT" }; } - if ( - !input.isVerified && - !inputUser.loginMethods.some((lm) => lm.hasSameEmailAs(input.newEmail) && lm.verified) && - otherUsersWithNewEmail.length !== 0 - ) { - let shouldDoAccountLinking = await this.config.shouldDoAutomaticAccountLinking( - otherUsersWithNewEmail[0].loginMethods[0], - inputUser, - input.session, - tenantId, - input.userContext + 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` ); - if ( - shouldDoAccountLinking.shouldAutomaticallyLink && - shouldDoAccountLinking.shouldRequireVerification - ) { - logger_1.logDebugMessage( - `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` - ); - return { allowed: false, reason: "ACCOUNT_TAKEOVER_RISK" }; - } + 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( diff --git a/lib/ts/recipe/accountlinking/recipe.ts b/lib/ts/recipe/accountlinking/recipe.ts index 092ae3e34..5f7ae50c4 100644 --- a/lib/ts/recipe/accountlinking/recipe.ts +++ b/lib/ts/recipe/accountlinking/recipe.ts @@ -575,32 +575,47 @@ export default class Recipe extends RecipeModule { ); return { allowed: false, reason: "PRIMARY_USER_CONFLICT" }; } - if ( - !input.isVerified && - !inputUser.loginMethods.some((lm) => lm.hasSameEmailAs(input.newEmail) && lm.verified) && - otherUsersWithNewEmail.length !== 0 - ) { - let shouldDoAccountLinking = await this.config.shouldDoAutomaticAccountLinking( - otherUsersWithNewEmail[0].loginMethods[0], - inputUser, - input.session, - tenantId, - input.userContext + 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` ); - if ( - shouldDoAccountLinking.shouldAutomaticallyLink && - shouldDoAccountLinking.shouldRequireVerification - ) { - logDebugMessage( - `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` - ); - return { allowed: false, reason: "ACCOUNT_TAKEOVER_RISK" }; - } + 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( From 8099c687227c9554862ca388e3c93c99d420c1ae Mon Sep 17 00:00:00 2001 From: Mihaly Lengyel Date: Tue, 25 Jun 2024 23:45:31 +0200 Subject: [PATCH 4/7] chore: update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e8aaa543..fffe0d4ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ 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 +- 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. ### Changes From 7d93633867b83bcc216280803dad60ec88defb99 Mon Sep 17 00:00:00 2001 From: Mihaly Lengyel Date: Wed, 26 Jun 2024 10:18:11 +0200 Subject: [PATCH 5/7] chore: empty line from changelog --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a28d33c5..490a32857 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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 - 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 From 54486913a53566a27bdc8dd11d0bbca267f40375 Mon Sep 17 00:00:00 2001 From: Mihaly Lengyel Date: Fri, 28 Jun 2024 11:19:09 +0200 Subject: [PATCH 6/7] refactor: remove duplicated check and bypass mapping for already mapped errcodes --- .../recipe/thirdparty/api/implementation.js | 84 +++---------------- .../recipe/thirdparty/api/implementation.ts | 84 ++----------------- 2 files changed, 19 insertions(+), 149 deletions(-) diff --git a/lib/build/recipe/thirdparty/api/implementation.js b/lib/build/recipe/thirdparty/api/implementation.js index 571ce19da..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,86 +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.allowed) { - return { - status: "SIGN_IN_UP_NOT_ALLOWED", - reason: - isEmailChangeAllowed.reason === "PRIMARY_USER_CONFLICT" - ? "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)", - }; - } } const preAuthChecks = await authUtils_1.AuthUtils.preAuthChecks({ authenticatingAccountInfo: { @@ -220,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/ts/recipe/thirdparty/api/implementation.ts b/lib/ts/recipe/thirdparty/api/implementation.ts index 1da7a639a..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,76 +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.allowed) { - return { - status: "SIGN_IN_UP_NOT_ALLOWED", - reason: - isEmailChangeAllowed.reason === "PRIMARY_USER_CONFLICT" - ? "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)", - }; - } } const preAuthChecks = await AuthUtils.preAuthChecks({ @@ -234,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"); From 32feab5bdad515153b2ed2f899313a8dde3e252c Mon Sep 17 00:00:00 2001 From: Mihaly Lengyel Date: Fri, 28 Jun 2024 11:28:18 +0200 Subject: [PATCH 7/7] chore: update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 490a32857..bbbeaad9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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.