fix(core): skip MFA suggestion when user has factors from non-reg (#7782)

fix(core): skip MFA suggestion when user has factors from non-registration

Author: wangsijie

packages/core/src/routes/experience/classes/mfa.ts

@@ -16,6 +16,7 @@ import {
   type Mfa as MfaSettings,
   OrganizationRequiredMfaPolicy,
   MfaFactor,
+  SignInIdentifier,
 } from '@logto/schemas';
 import { generateStandardId, maskEmail, maskPhone } from '@logto/shared';
 import { cond, condObject, deduplicate, pick } from '@silverhand/essentials';
@@ -298,6 +299,7 @@ export class Mfa {
    * when conditions are met.
    * The purpose is to suggest another MFA factor if the user has only one Email or Phone factor.
    */
+  // eslint-disable-next-line complexity
   async guardAdditionalBindingSuggestion(
     factorsInUser: MfaFactor[],
     availableFactors: MfaFactor[]
@@ -315,6 +317,22 @@ export class Mfa {
       return;
     }
 
+    const { signUp } = await this.signInExperienceValidator.getSignInExperienceData();
+    // If the user has email, but not registered by email, no suggestion
+    if (
+      factorsInUser.includes(MfaFactor.EmailVerificationCode) &&
+      !signUp.identifiers.includes(SignInIdentifier.Email)
+    ) {
+      return;
+    }
+    // If the user has phone, but not registered by phone, no suggestion
+    if (
+      factorsInUser.includes(MfaFactor.PhoneVerificationCode) &&
+      !signUp.identifiers.includes(SignInIdentifier.Phone)
+    ) {
+      return;
+    }
+
     const additionalFactors = availableFactors
       .filter((factor) => !factorsInUser.includes(factor))
       .slice()

packages/integration-tests/src/tests/api/experience-api/bind-mfa/mfa-suggestion.test.ts

@@ -2,21 +2,28 @@ import { ConnectorType } from '@logto/connector-kit';
 import { InteractionEvent, MfaFactor, MfaPolicy, SignInIdentifier } from '@logto/schemas';
 import { authenticator } from 'otplib';
 
+import { deleteUser } from '#src/api/admin-user.js';
 import { updateSignInExperience } from '#src/api/sign-in-experience.js';
 import { initExperienceClient, logoutClient, processSession } from '#src/helpers/client.js';
-import { clearConnectorsByTypes, setEmailConnector } from '#src/helpers/connector.js';
+import {
+  clearConnectorsByTypes,
+  setEmailConnector,
+  setSmsConnector,
+} from '#src/helpers/connector.js';
 import {
   successfullySendVerificationCode,
   successfullyVerifyVerificationCode,
 } from '#src/helpers/experience/verification-code.js';
 import { expectRejects } from '#src/helpers/index.js';
 import { resetMfaSettings } from '#src/helpers/sign-in-experience.js';
 import { generateNewUserProfile } from '#src/helpers/user.js';
+import { generatePhone } from '#src/utils.js';
 
 describe('Register interaction - optional additional MFA suggestion', () => {
   beforeAll(async () => {
-    await clearConnectorsByTypes([ConnectorType.Email]);
+    await clearConnectorsByTypes([ConnectorType.Email, ConnectorType.Sms]);
     await setEmailConnector();
+    await setSmsConnector();
     // Set up sign-in experience upfront (refer to email-with-signup.test.ts pattern)
     await updateSignInExperience({
       signUp: {
@@ -42,7 +49,7 @@ describe('Register interaction - optional additional MFA suggestion', () => {
   });
 
   afterAll(async () => {
-    await clearConnectorsByTypes([ConnectorType.Email]);
+    await clearConnectorsByTypes([ConnectorType.Email, ConnectorType.Sms]);
     await resetMfaSettings();
   });
 
@@ -91,8 +98,9 @@ describe('Register interaction - optional additional MFA suggestion', () => {
 
     // Submit again should succeed
     const { redirectTo } = await client.submitInteraction();
-    await processSession(client, redirectTo);
+    const userId = await processSession(client, redirectTo);
     await logoutClient(client);
+    await deleteUser(userId);
   });
 
   it('should allow binding TOTP instead of skipping and then complete', async () => {
@@ -128,7 +136,100 @@ describe('Register interaction - optional additional MFA suggestion', () => {
 
     // Now submit should succeed
     const { redirectTo } = await client.submitInteraction();
-    await processSession(client, redirectTo);
+    const userId = await processSession(client, redirectTo);
+    await logoutClient(client);
+    await deleteUser(userId);
+  });
+
+  it('should not suggest MFA after fulfilling phone verification when both email and SMS factors are enabled', async () => {
+    // Configure MFA with email, phone, and TOTP factors
+    await updateSignInExperience({
+      signUp: {
+        identifiers: [SignInIdentifier.Email],
+        password: true,
+        verify: true,
+      },
+      signIn: {
+        methods: [
+          {
+            identifier: SignInIdentifier.Email,
+            password: true,
+            verificationCode: false,
+            isPasswordPrimary: false,
+          },
+        ],
+      },
+      mfa: {
+        factors: [MfaFactor.EmailVerificationCode, MfaFactor.PhoneVerificationCode, MfaFactor.TOTP],
+        policy: MfaPolicy.Mandatory,
+      },
+    });
+
+    const { primaryEmail, password } = generateNewUserProfile({
+      primaryEmail: true,
+      password: true,
+    });
+    const phoneNumber = generatePhone();
+    const client = await initExperienceClient({ interactionEvent: InteractionEvent.Register });
+
+    // Register with email
+    const { verificationId, code } = await successfullySendVerificationCode(client, {
+      identifier: { type: SignInIdentifier.Email, value: primaryEmail },
+      interactionEvent: InteractionEvent.Register,
+    });
+
+    await successfullyVerifyVerificationCode(client, {
+      identifier: { type: SignInIdentifier.Email, value: primaryEmail },
+      verificationId,
+      code,
+    });
+
+    // Fulfill required password before identifying the user
+    await client.updateProfile({ type: 'password', value: password });
+    await client.identifyUser({ verificationId });
+
+    // Submit should trigger MFA suggestion
+    await expectRejects<{
+      availableFactors: MfaFactor[];
+      skippable: boolean;
+      maskedIdentifiers?: Record<string, string>;
+      suggestion?: boolean;
+    }>(client.submitInteraction(), {
+      code: 'session.mfa.suggest_additional_mfa',
+      status: 422,
+      expectData: (data) => {
+        // Should include Email, Phone and TOTP
+        expect(data.availableFactors).toEqual([
+          MfaFactor.EmailVerificationCode,
+          MfaFactor.PhoneVerificationCode,
+          MfaFactor.TOTP,
+        ]);
+        expect(data.maskedIdentifiers).toBeDefined();
+        expect(data.maskedIdentifiers?.[MfaFactor.EmailVerificationCode]).toMatch(/\*{4}/);
+        expect(data.skippable).toBe(true);
+        expect(data.suggestion).toBe(true);
+      },
+    });
+
+    // Fulfill phone verification instead of skipping
+    const { verificationId: phoneVerificationId, code: phoneCode } =
+      await successfullySendVerificationCode(client, {
+        identifier: { type: SignInIdentifier.Phone, value: phoneNumber },
+        interactionEvent: InteractionEvent.Register,
+      });
+
+    const finalPhoneVerificationId = await successfullyVerifyVerificationCode(client, {
+      identifier: { type: SignInIdentifier.Phone, value: phoneNumber },
+      verificationId: phoneVerificationId,
+      code: phoneCode,
+    });
+
+    await client.bindMfa(MfaFactor.PhoneVerificationCode, finalPhoneVerificationId);
+
+    // Now submit should succeed without MFA suggestion
+    const { redirectTo } = await client.submitInteraction();
+    const userId = await processSession(client, redirectTo);
     await logoutClient(client);
+    await deleteUser(userId);
   });
 });