refactor(core,phrases): change interaction bind-mfa to array (#4680)

2025-03-03 22:15:32 -05:00 · 2023-10-20 13:48:36 +08:00 · 2023-10-20 13:48:36 +08:00 · b972397f80
commit b972397f80
parent 54fd29e41f
26 changed files with 144 additions and 82 deletions
--- a/packages/core/src/routes/interaction/actions/submit-interaction.mfa.test.ts
+++ b/packages/core/src/routes/interaction/actions/submit-interaction.mfa.test.ts
@ -116,13 +116,13 @@ describe('submit action', () => {
    jest.clearAllMocks();
  });

-  describe('register with bindMfa', () => {
+  describe('register with bindMfas', () => {
    it('should handle totp', async () => {
      const interaction: VerifiedRegisterInteractionResult = {
        event: InteractionEvent.Register,
        profile,
        identifiers,
-        bindMfa: { type: MfaFactor.TOTP, secret: 'secret' },
+        bindMfas: [{ type: MfaFactor.TOTP, secret: 'secret' }],
      };

      await submitInteraction(interaction, ctx, tenant);
@ -151,7 +151,7 @@ describe('submit action', () => {
        event: InteractionEvent.Register,
        profile,
        identifiers,
-        bindMfa: mockWebAuthnBind,
+        bindMfas: [mockWebAuthnBind],
        pendingAccountId: 'id',
      };

@ -186,10 +186,12 @@ describe('submit action', () => {
        event: InteractionEvent.SignIn,
        accountId: 'foo',
        identifiers,
-        bindMfa: {
-          type: MfaFactor.TOTP,
-          secret: 'secret',
-        },
+        bindMfas: [
+          {
+            type: MfaFactor.TOTP,
+            secret: 'secret',
+          },
+        ],
      };

      await submitInteraction(interaction, ctx, tenant);
@ -221,7 +223,7 @@ describe('submit action', () => {
        event: InteractionEvent.SignIn,
        accountId: 'foo',
        identifiers,
-        bindMfa: mockWebAuthnBind,
+        bindMfas: [mockWebAuthnBind],
      };

      await submitInteraction(interaction, ctx, tenant);
--- a/packages/core/src/routes/interaction/actions/submit-interaction.ts
+++ b/packages/core/src/routes/interaction/actions/submit-interaction.ts
@ -33,32 +33,37 @@ import { clearInteractionStorage } from '../utils/interaction.js';

 import { postAffiliateLogs, parseUserProfile } from './helpers.js';

-const parseBindMfa = ({
-  bindMfa,
-}: VerifiedSignInInteractionResult | VerifiedRegisterInteractionResult):
-  | User['mfaVerifications'][number]
-  | undefined => {
-  if (!bindMfa) {
-    return;
+const parseBindMfas = ({
+  bindMfas,
+}:
+  | VerifiedSignInInteractionResult
+  | VerifiedRegisterInteractionResult): User['mfaVerifications'] => {
+  if (!bindMfas) {
+    return [];
  }

-  if (bindMfa.type === MfaFactor.TOTP) {
-    return {
-      type: MfaFactor.TOTP,
-      key: bindMfa.secret,
-      id: generateStandardId(),
-      createdAt: new Date().toISOString(),
-    };
-  }
+  return bindMfas.map((bindMfa) => {
+    if (bindMfa.type === MfaFactor.TOTP) {
+      return {
+        type: MfaFactor.TOTP,
+        key: bindMfa.secret,
+        id: generateStandardId(),
+        createdAt: new Date().toISOString(),
+      };
+    }

-  // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
-  if (bindMfa.type === MfaFactor.WebAuthn) {
-    return {
-      ...bindMfa,
-      id: generateStandardId(),
-      createdAt: new Date().toISOString(),
-    };
-  }
+    // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
+    if (bindMfa.type === MfaFactor.WebAuthn) {
+      return {
+        ...bindMfa,
+        id: generateStandardId(),
+        createdAt: new Date().toISOString(),
+      };
+    }
+
+    // Not expected to happen, the above if statements should cover all cases
+    throw new Error('Unsupported MFA factor');
+  });
 };

 const getInitialUserRoles = (
@ -91,7 +96,7 @@ export default async function submitInteraction(
    const { pendingAccountId } = interaction;
    const id = pendingAccountId ?? (await generateUserId());
    const userProfile = await parseUserProfile(tenantContext, interaction);
-    const mfaVerification = parseBindMfa(interaction);
+    const mfaVerifications = parseBindMfas(interaction);

    const { client_id } = ctx.interactionDetails.params;

@ -106,7 +111,11 @@ export default async function submitInteraction(
      {
        id,
        ...userProfile,
-        ...conditional(mfaVerification && { mfaVerifications: [mfaVerification] }),
+        ...conditional(
+          mfaVerifications.length > 0 && {
+            mfaVerifications,
+          }
+        ),
      },
      getInitialUserRoles(isInAdminTenant, isCreatingFirstAdminUser, isCloud)
    );
@ -138,12 +147,14 @@ export default async function submitInteraction(
  if (event === InteractionEvent.SignIn) {
    const user = await findUserById(accountId);
    const updateUserProfile = await parseUserProfile(tenantContext, interaction, user);
-    const mfaVerification = parseBindMfa(interaction);
+    const mfaVerifications = parseBindMfas(interaction);

    await updateUserById(accountId, {
      ...updateUserProfile,
      ...conditional(
-        mfaVerification && { mfaVerifications: [...user.mfaVerifications, mfaVerification] }
+        mfaVerifications.length > 0 && {
+          mfaVerifications: [...user.mfaVerifications, ...mfaVerifications],
+        }
      ),
    });
    await assignInteractionResults(ctx, provider, { login: { accountId } });
--- a/packages/core/src/routes/interaction/mfa.test.ts
+++ b/packages/core/src/routes/interaction/mfa.test.ts
@ -93,7 +93,7 @@ describe('interaction routes (MFA verification)', () => {
    });
  });

-  describe('PUT /interaction/bind-mfa', () => {
+  describe('POST /interaction/bind-mfa', () => {
    const path = `${interactionPrefix}/bind-mfa`;

    it('should return 204 and store results in session', async () => {
@ -104,13 +104,13 @@ describe('interaction routes (MFA verification)', () => {
        code: '123456',
      };

-      const response = await sessionRequest.put(path).send(body);
+      const response = await sessionRequest.post(path).send(body);
      expect(response.status).toEqual(204);
      expect(getInteractionStorage).toBeCalled();
      expect(verifyMfaSettings).toBeCalled();
      expect(bindMfaPayloadVerification).toBeCalled();
      expect(storeInteractionResult).toBeCalledWith(
-        { bindMfa: mockTotpBind },
+        { bindMfas: [mockTotpBind] },
        expect.anything(),
        expect.anything(),
        expect.anything()
--- a/packages/core/src/routes/interaction/mfa.ts
+++ b/packages/core/src/routes/interaction/mfa.ts
@ -26,8 +26,8 @@ export default function mfaRoutes<T extends IRouterParamContext>(
 ) {
  const { provider, queries } = tenant;

-  // Update New MFA
-  router.put(
+  // Set New MFA
+  router.post(
    `${interactionPrefix}/bind-mfa`,
    koaGuard({
      body: bindMfaPayloadGuard,
@ -52,6 +52,11 @@ export default function mfaRoutes<T extends IRouterParamContext>(
        verifyMfaSettings(bindMfaPayload.type, signInExperience);
      }

+      const { bindMfas = [] } = interactionStorage;
+      // Only allow one factor for now,
+      // TODO @sijie: revisit when implementing backup code factor
+      assertThat(bindMfas.length === 0, 'session.mfa.bind_mfa_existed');
+
      const { hostname, origin } = EnvSet.values.endpoint;
      const bindMfa = await bindMfaPayloadVerification(ctx, bindMfaPayload, interactionStorage, {
        rpId: hostname,
@ -61,7 +66,7 @@ export default function mfaRoutes<T extends IRouterParamContext>(

      log.append({ bindMfa, interactionStorage });

-      await storeInteractionResult({ bindMfa }, ctx, provider, true);
+      await storeInteractionResult({ bindMfas: [...bindMfas, bindMfa] }, ctx, provider, true);

      ctx.status = 204;

--- a/packages/core/src/routes/interaction/types/guard.ts
+++ b/packages/core/src/routes/interaction/types/guard.ts
@ -51,7 +51,7 @@ export const anonymousInteractionResultGuard = z.object({
  accountId: z.string().optional(),
  identifiers: z.array(identifierGuard).optional(),
  // The new mfa to be bound to the account
-  bindMfa: bindMfaGuard.optional(),
+  bindMfas: bindMfaGuard.array().optional(),
  // The pending mfa info, such as secret of TOTP
  pendingMfa: pendingMfaGuard.optional(),
  // The verified mfa
--- a/packages/core/src/routes/interaction/types/index.ts
+++ b/packages/core/src/routes/interaction/types/index.ts
@ -78,7 +78,7 @@ export type VerifiedRegisterInteractionResult = {
  event: InteractionEvent.Register;
  profile?: Profile;
  identifiers?: Identifier[];
-  bindMfa?: BindMfa;
+  bindMfas?: BindMfa[];
  pendingAccountId?: string;
 };

@ -87,7 +87,7 @@ export type VerifiedSignInInteractionResult = {
  accountId: string;
  identifiers: Identifier[];
  profile?: Profile;
-  bindMfa?: BindMfa;
+  bindMfas?: BindMfa[];
  verifiedMfa?: VerifyMfaResult;
 };

--- a/packages/core/src/routes/interaction/verifications/mfa-verification.test.ts
+++ b/packages/core/src/routes/interaction/verifications/mfa-verification.test.ts
@ -88,14 +88,16 @@ describe('validateMandatoryBindMfa', () => {
      );
    });

-    it('bindMfa exists should pass', async () => {
+    it('bindMfas exists should pass', async () => {
      await expect(
        validateMandatoryBindMfa(tenantContext, mfaRequiredCtx, {
          ...interaction,
-          bindMfa: {
-            type: MfaFactor.TOTP,
-            secret: 'foo',
-          },
+          bindMfas: [
+            {
+              type: MfaFactor.TOTP,
+              secret: 'foo',
+            },
+          ],
        })
      ).resolves.not.toThrow();
    });
@ -130,15 +132,17 @@ describe('validateMandatoryBindMfa', () => {
      ).resolves.not.toThrow();
    });

-    it('user mfaVerifications missing, bindMfa existing and required should pass', async () => {
+    it('user mfaVerifications missing, bindMfas existing and required should pass', async () => {
      findUserById.mockResolvedValueOnce(mockUser);
      await expect(
        validateMandatoryBindMfa(tenantContext, mfaRequiredCtx, {
          ...signInInteraction,
-          bindMfa: {
-            type: MfaFactor.TOTP,
-            secret: 'foo',
-          },
+          bindMfas: [
+            {
+              type: MfaFactor.TOTP,
+              secret: 'foo',
+            },
+          ],
        })
      ).resolves.not.toThrow();
    });
@ -161,10 +165,12 @@ describe('verifyBindMfa', () => {
    await expect(
      verifyBindMfa(tenantContext, {
        ...interaction,
-        bindMfa: {
-          type: MfaFactor.TOTP,
-          secret: 'foo',
-        },
+        bindMfas: [
+          {
+            type: MfaFactor.TOTP,
+            secret: 'foo',
+          },
+        ],
      })
    ).resolves.not.toThrow();
  });
@ -174,10 +180,12 @@ describe('verifyBindMfa', () => {
    await expect(
      verifyBindMfa(tenantContext, {
        ...signInInteraction,
-        bindMfa: {
-          type: MfaFactor.TOTP,
-          secret: 'foo',
-        },
+        bindMfas: [
+          {
+            type: MfaFactor.TOTP,
+            secret: 'foo',
+          },
+        ],
      })
    ).resolves.not.toThrow();
  });
@ -187,10 +195,12 @@ describe('verifyBindMfa', () => {
    await expect(
      verifyBindMfa(tenantContext, {
        ...signInInteraction,
-        bindMfa: {
-          type: MfaFactor.TOTP,
-          secret: 'foo',
-        },
+        bindMfas: [
+          {
+            type: MfaFactor.TOTP,
+            secret: 'foo',
+          },
+        ],
      })
    ).rejects.toMatchError(new RequestError({ code: 'user.totp_already_in_use', status: 422 }));
  });
--- a/packages/core/src/routes/interaction/verifications/mfa-verification.ts
+++ b/packages/core/src/routes/interaction/verifications/mfa-verification.ts
@ -17,15 +17,15 @@ export const verifyBindMfa = async (
  tenant: TenantContext,
  interaction: VerifiedSignInInteractionResult | VerifiedRegisterInteractionResult
 ): Promise<VerifiedInteractionResult> => {
-  const { bindMfa, event } = interaction;
+  const { bindMfas = [], event } = interaction;

-  if (!bindMfa || event !== InteractionEvent.SignIn) {
+  if (bindMfas.length === 0 || event !== InteractionEvent.SignIn) {
    return interaction;
  }

-  const { type } = bindMfa;
+  const totp = bindMfas.find(({ type }) => type === MfaFactor.TOTP);

-  if (type === MfaFactor.TOTP) {
+  if (totp) {
    const { accountId } = interaction;
    const { mfaVerifications } = await tenant.queries.users.findUserById(accountId);

@ -76,21 +76,27 @@ export const validateMandatoryBindMfa = async (
  const {
    mfa: { policy, factors },
  } = ctx.signInExperience;
-  const { event, bindMfa } = interaction;
+  const { event, bindMfas } = interaction;
+  const availableFactors = factors.filter((factor) => factor !== MfaFactor.BackupCode);

  if (policy !== MfaPolicy.Mandatory) {
    return interaction;
  }

+  const hasFactorInBind = Boolean(
+    bindMfas &&
+      availableFactors.some((factor) => bindMfas.some((bindMfa) => bindMfa.type === factor))
+  );
+
  if (event === InteractionEvent.Register) {
    assertThat(
-      bindMfa && factors.includes(bindMfa.type),
+      hasFactorInBind,
      new RequestError(
        {
          code: 'user.missing_mfa',
          status: 422,
        },
-        { availableFactors: factors.map((factor) => factor) }
+        { availableFactors }
      )
    );
  }
@ -98,7 +104,6 @@ export const validateMandatoryBindMfa = async (
  if (event === InteractionEvent.SignIn) {
    const { accountId } = interaction;
    const { mfaVerifications } = await tenant.queries.users.findUserById(accountId);
-    const hasFactorInBind = Boolean(bindMfa && factors.includes(bindMfa.type));
    const hasFactorInUser = factors.some((factor) =>
      mfaVerifications.some(({ type }) => type === factor)
    );
@ -109,7 +114,7 @@ export const validateMandatoryBindMfa = async (
          code: 'user.missing_mfa',
          status: 422,
        },
-        { availableFactors: factors.map((factor) => factor) }
+        { availableFactors }
      )
    );
  }
--- a/packages/experience/src/apis/interaction.ts
+++ b/packages/experience/src/apis/interaction.ts
@ -243,7 +243,7 @@ export const generateWebAuthnAuthnOptions = async () =>
    .json<WebAuthnAuthenticationOptions>();

 export const bindMfa = async (payload: BindMfaPayload) => {
-  await api.put(`${interactionPrefix}/bind-mfa`, { json: payload });
+  await api.post(`${interactionPrefix}/bind-mfa`, { json: payload });

  return api.post(`${interactionPrefix}/submit`).json<Response>();
 };
--- a/packages/integration-tests/src/api/interaction.ts
+++ b/packages/integration-tests/src/api/interaction.ts
@ -69,9 +69,9 @@ export const putInteractionProfile = async (cookie: string, payload: Profile) =>
    })
    .json();

-export const putInteractionBindMfa = async (cookie: string, payload: BindMfaPayload) =>
+export const postInteractionBindMfa = async (cookie: string, payload: BindMfaPayload) =>
  api
-    .put('interaction/bind-mfa', {
+    .post('interaction/bind-mfa', {
      headers: { cookie },
      json: payload,
      followRedirect: false,
--- a/packages/integration-tests/src/tests/api/interaction/mfa/totp.test.ts
+++ b/packages/integration-tests/src/tests/api/interaction/mfa/totp.test.ts
@ -5,7 +5,7 @@ import {
  putInteraction,
  deleteUser,
  initTotp,
-  putInteractionBindMfa,
+  postInteractionBindMfa,
  putInteractionMfa,
 } from '#src/api/index.js';
 import { initClient, processSession, logoutClient } from '#src/helpers/client.js';
@ -31,7 +31,7 @@ const registerWithMfa = async () => {
  const { secret } = await client.send(initTotp);
  const code = authenticator.generate(secret);

-  await client.send(putInteractionBindMfa, {
+  await client.send(postInteractionBindMfa, {
    type: MfaFactor.TOTP,
    code,
  });
@ -86,7 +86,7 @@ describe('register with mfa (mandatory TOTP)', () => {

    await client.send(initTotp);
    await expectRejects(
-      client.send(putInteractionBindMfa, {
+      client.send(postInteractionBindMfa, {
        type: MfaFactor.TOTP,
        code: '123456',
      }),
@ -112,7 +112,7 @@ describe('register with mfa (mandatory TOTP)', () => {
    const { secret } = await client.send(initTotp);
    const code = authenticator.generate(secret);

-    await client.send(putInteractionBindMfa, {
+    await client.send(postInteractionBindMfa, {
      type: MfaFactor.TOTP,
      code,
    });
--- a/packages/phrases/src/locales/de/errors/session.ts
+++ b/packages/phrases/src/locales/de/errors/session.ts
@ -39,6 +39,8 @@ const session = {
    webauthn_verification_failed: 'WebAuthn verification failed.',
    /** UNTRANSLATED */
    webauthn_verification_not_found: 'WebAuthn verification not found.',
+    /** UNTRANSLATED */
+    bind_mfa_existed: 'MFA already exists.',
  },
 };

--- a/packages/phrases/src/locales/en/errors/session.ts
+++ b/packages/phrases/src/locales/en/errors/session.ts
@ -29,6 +29,7 @@ const session = {
    invalid_totp_code: 'Invalid TOTP code.',
    webauthn_verification_failed: 'WebAuthn verification failed.',
    webauthn_verification_not_found: 'WebAuthn verification not found.',
+    bind_mfa_existed: 'MFA already exists.',
  },
 };

--- a/packages/phrases/src/locales/es/errors/session.ts
+++ b/packages/phrases/src/locales/es/errors/session.ts
@ -40,6 +40,8 @@ const session = {
    webauthn_verification_failed: 'WebAuthn verification failed.',
    /** UNTRANSLATED */
    webauthn_verification_not_found: 'WebAuthn verification not found.',
+    /** UNTRANSLATED */
+    bind_mfa_existed: 'MFA already exists.',
  },
 };

--- a/packages/phrases/src/locales/fr/errors/session.ts
+++ b/packages/phrases/src/locales/fr/errors/session.ts
@ -41,6 +41,8 @@ const session = {
    webauthn_verification_failed: 'WebAuthn verification failed.',
    /** UNTRANSLATED */
    webauthn_verification_not_found: 'WebAuthn verification not found.',
+    /** UNTRANSLATED */
+    bind_mfa_existed: 'MFA already exists.',
  },
 };

--- a/packages/phrases/src/locales/it/errors/session.ts
+++ b/packages/phrases/src/locales/it/errors/session.ts
@ -39,6 +39,8 @@ const session = {
    webauthn_verification_failed: 'WebAuthn verification failed.',
    /** UNTRANSLATED */
    webauthn_verification_not_found: 'WebAuthn verification not found.',
+    /** UNTRANSLATED */
+    bind_mfa_existed: 'MFA already exists.',
  },
 };

--- a/packages/phrases/src/locales/ja/errors/session.ts
+++ b/packages/phrases/src/locales/ja/errors/session.ts
@ -37,6 +37,8 @@ const session = {
    webauthn_verification_failed: 'WebAuthn verification failed.',
    /** UNTRANSLATED */
    webauthn_verification_not_found: 'WebAuthn verification not found.',
+    /** UNTRANSLATED */
+    bind_mfa_existed: 'MFA already exists.',
  },
 };

--- a/packages/phrases/src/locales/ko/errors/session.ts
+++ b/packages/phrases/src/locales/ko/errors/session.ts
@ -36,6 +36,8 @@ const session = {
    webauthn_verification_failed: 'WebAuthn verification failed.',
    /** UNTRANSLATED */
    webauthn_verification_not_found: 'WebAuthn verification not found.',
+    /** UNTRANSLATED */
+    bind_mfa_existed: 'MFA already exists.',
  },
 };

--- a/packages/phrases/src/locales/pl-pl/errors/session.ts
+++ b/packages/phrases/src/locales/pl-pl/errors/session.ts
@ -38,6 +38,8 @@ const session = {
    webauthn_verification_failed: 'WebAuthn verification failed.',
    /** UNTRANSLATED */
    webauthn_verification_not_found: 'WebAuthn verification not found.',
+    /** UNTRANSLATED */
+    bind_mfa_existed: 'MFA already exists.',
  },
 };

--- a/packages/phrases/src/locales/pt-br/errors/session.ts
+++ b/packages/phrases/src/locales/pt-br/errors/session.ts
@ -39,6 +39,8 @@ const session = {
    webauthn_verification_failed: 'WebAuthn verification failed.',
    /** UNTRANSLATED */
    webauthn_verification_not_found: 'WebAuthn verification not found.',
+    /** UNTRANSLATED */
+    bind_mfa_existed: 'MFA already exists.',
  },
 };

--- a/packages/phrases/src/locales/pt-pt/errors/session.ts
+++ b/packages/phrases/src/locales/pt-pt/errors/session.ts
@ -41,6 +41,8 @@ const session = {
    webauthn_verification_failed: 'WebAuthn verification failed.',
    /** UNTRANSLATED */
    webauthn_verification_not_found: 'WebAuthn verification not found.',
+    /** UNTRANSLATED */
+    bind_mfa_existed: 'MFA already exists.',
  },
 };

--- a/packages/phrases/src/locales/ru/errors/session.ts
+++ b/packages/phrases/src/locales/ru/errors/session.ts
@ -37,6 +37,8 @@ const session = {
    webauthn_verification_failed: 'WebAuthn verification failed.',
    /** UNTRANSLATED */
    webauthn_verification_not_found: 'WebAuthn verification not found.',
+    /** UNTRANSLATED */
+    bind_mfa_existed: 'MFA already exists.',
  },
 };

--- a/packages/phrases/src/locales/tr-tr/errors/session.ts
+++ b/packages/phrases/src/locales/tr-tr/errors/session.ts
@ -38,6 +38,8 @@ const session = {
    webauthn_verification_failed: 'WebAuthn verification failed.',
    /** UNTRANSLATED */
    webauthn_verification_not_found: 'WebAuthn verification not found.',
+    /** UNTRANSLATED */
+    bind_mfa_existed: 'MFA already exists.',
  },
 };

--- a/packages/phrases/src/locales/zh-cn/errors/session.ts
+++ b/packages/phrases/src/locales/zh-cn/errors/session.ts
@ -33,6 +33,8 @@ const session = {
    webauthn_verification_failed: 'WebAuthn verification failed.',
    /** UNTRANSLATED */
    webauthn_verification_not_found: 'WebAuthn verification not found.',
+    /** UNTRANSLATED */
+    bind_mfa_existed: 'MFA already exists.',
  },
 };

--- a/packages/phrases/src/locales/zh-hk/errors/session.ts
+++ b/packages/phrases/src/locales/zh-hk/errors/session.ts
@ -33,6 +33,8 @@ const session = {
    webauthn_verification_failed: 'WebAuthn verification failed.',
    /** UNTRANSLATED */
    webauthn_verification_not_found: 'WebAuthn verification not found.',
+    /** UNTRANSLATED */
+    bind_mfa_existed: 'MFA already exists.',
  },
 };

--- a/packages/phrases/src/locales/zh-tw/errors/session.ts
+++ b/packages/phrases/src/locales/zh-tw/errors/session.ts
@ -33,6 +33,8 @@ const session = {
    webauthn_verification_failed: 'WebAuthn verification failed.',
    /** UNTRANSLATED */
    webauthn_verification_not_found: 'WebAuthn verification not found.',
+    /** UNTRANSLATED */
+    bind_mfa_existed: 'MFA already exists.',
  },
 };