From 161f012bc0452982ada1307f6d19ca4ba1f3be35 Mon Sep 17 00:00:00 2001 From: wangsijie Date: Tue, 31 Oct 2023 09:32:20 +0800 Subject: [PATCH] fix(core): prompt to bind mfa for register (#4783) --- .../interaction/actions/submit-interaction.ts | 10 ++++- .../src/routes/interaction/types/guard.ts | 2 + .../src/routes/interaction/types/index.ts | 1 + .../verifications/mfa-verification.test.ts | 31 ++++++++++++-- .../verifications/mfa-verification.ts | 42 +++++++++++++------ 5 files changed, 69 insertions(+), 17 deletions(-) diff --git a/packages/core/src/routes/interaction/actions/submit-interaction.ts b/packages/core/src/routes/interaction/actions/submit-interaction.ts index 2e642f720..cc0c86c65 100644 --- a/packages/core/src/routes/interaction/actions/submit-interaction.ts +++ b/packages/core/src/routes/interaction/actions/submit-interaction.ts @@ -30,6 +30,7 @@ import type { VerifiedRegisterInteractionResult, } from '../types/index.js'; import { clearInteractionStorage } from '../utils/interaction.js'; +import { userMfaDataKey } from '../verifications/mfa-verification.js'; import { postAffiliateLogs, parseUserProfile } from './helpers.js'; @@ -96,7 +97,7 @@ export default async function submitInteraction( const { event, profile } = interaction; if (event === InteractionEvent.Register) { - const { pendingAccountId } = interaction; + const { pendingAccountId, mfaSkipped } = interaction; const id = pendingAccountId ?? (await generateUserId()); const userProfile = await parseUserProfile(tenantContext, interaction); const mfaVerifications = parseBindMfas(interaction); @@ -119,6 +120,13 @@ export default async function submitInteraction( mfaVerifications, } ), + ...conditional( + mfaSkipped && { + [userMfaDataKey]: { + skipped: true, + }, + } + ), }, getInitialUserRoles(isInAdminTenant, isCreatingFirstAdminUser, isCloud) ); diff --git a/packages/core/src/routes/interaction/types/guard.ts b/packages/core/src/routes/interaction/types/guard.ts index 0cf834b28..052298e99 100644 --- a/packages/core/src/routes/interaction/types/guard.ts +++ b/packages/core/src/routes/interaction/types/guard.ts @@ -59,6 +59,8 @@ export const anonymousInteractionResultGuard = z.object({ // The user id to be used for register, if not provided, a new one will be generated // WebAuthn requires a user id to be provided, so we have to generate and know it before submit interaction pendingAccountId: z.string().optional(), + // The marks that the user has skip binding new MFA (for new users, they don't have database records yet) + mfaSkipped: z.boolean().optional(), }); export const forgotPasswordProfileGuard = z.object({ diff --git a/packages/core/src/routes/interaction/types/index.ts b/packages/core/src/routes/interaction/types/index.ts index 35b163ead..78ff368a1 100644 --- a/packages/core/src/routes/interaction/types/index.ts +++ b/packages/core/src/routes/interaction/types/index.ts @@ -80,6 +80,7 @@ export type VerifiedRegisterInteractionResult = { identifiers?: Identifier[]; bindMfas?: BindMfa[]; pendingAccountId?: string; + mfaSkipped?: boolean; }; export type VerifiedSignInInteractionResult = { diff --git a/packages/core/src/routes/interaction/verifications/mfa-verification.test.ts b/packages/core/src/routes/interaction/verifications/mfa-verification.test.ts index 10bbc3620..27fcf3261 100644 --- a/packages/core/src/routes/interaction/verifications/mfa-verification.test.ts +++ b/packages/core/src/routes/interaction/verifications/mfa-verification.test.ts @@ -12,7 +12,6 @@ import { mockUserWithMfaVerifications, } from '#src/__mocks__/user.js'; import RequestError from '#src/errors/RequestError/index.js'; -import { createMockProvider } from '#src/test-utils/oidc-provider.js'; import { MockTenant } from '#src/test-utils/tenant.js'; import { createContextWithRouteParameters } from '#src/utils/test-utils.js'; @@ -38,6 +37,9 @@ const mockBackupCodes = ['foo']; await mockEsmWithActual('../utils/backup-code-validation.js', () => ({ generateBackupCodes: jest.fn().mockReturnValue(mockBackupCodes), })); +const { storeInteractionResult } = await mockEsmWithActual('../utils/interaction.js', () => ({ + storeInteractionResult: jest.fn(), +})); const { validateMandatoryBindMfa, verifyBindMfa, verifyMfa } = await import( './mfa-verification.js' @@ -104,8 +106,6 @@ const signInInteraction: AccountVerifiedInteractionResult = { accountId: 'foo', }; -const provider = createMockProvider(); - describe('validateMandatoryBindMfa', () => { afterEach(() => { findUserById.mockReset(); @@ -140,9 +140,32 @@ describe('validateMandatoryBindMfa', () => { ).resolves.not.toThrow(); }); - it('bindMfa missing and not required should pass', async () => { + it('bindMfa missing and not required should throw (for skip)', async () => { await expect( validateMandatoryBindMfa(tenantContext, baseCtx, interaction) + ).rejects.toMatchError( + new RequestError( + { + code: 'user.missing_mfa', + status: 422, + }, + { availableFactors: [MfaFactor.TOTP], skippable: true } + ) + ); + expect(storeInteractionResult).toHaveBeenCalledWith( + { mfaSkipped: true }, + expect.anything(), + expect.anything(), + expect.anything() + ); + }); + + it('bindMfa missing and not required, marked as skipped should pass', async () => { + await expect( + validateMandatoryBindMfa(tenantContext, baseCtx, { + ...interaction, + mfaSkipped: true, + }) ).resolves.not.toThrow(); }); }); diff --git a/packages/core/src/routes/interaction/verifications/mfa-verification.ts b/packages/core/src/routes/interaction/verifications/mfa-verification.ts index b928f0040..4ea9dee2c 100644 --- a/packages/core/src/routes/interaction/verifications/mfa-verification.ts +++ b/packages/core/src/routes/interaction/verifications/mfa-verification.ts @@ -82,7 +82,7 @@ export const verifyMfa = async ( return interaction; }; -const userMfaDataKey = 'mfa'; +export const userMfaDataKey = 'mfa'; /** * Check if the user has skipped MFA binding */ @@ -170,7 +170,7 @@ const validateMandatoryBindMfaForSignIn = async ( export const validateMandatoryBindMfa = async ( tenant: TenantContext, - ctx: WithInteractionSieContext & WithInteractionDetailsContext, + ctx: Context & WithInteractionSieContext & WithInteractionDetailsContext, interaction: VerifiedSignInInteractionResult | VerifiedRegisterInteractionResult ): Promise => { const { @@ -190,27 +190,45 @@ export const validateMandatoryBindMfa = async ( ); if (event === InteractionEvent.Register) { - if (policy !== MfaPolicy.Mandatory) { + if (hasFactorInBind) { return interaction; } - assertThat( - hasFactorInBind, - new RequestError( + if (policy === MfaPolicy.Mandatory) { + throw new RequestError( { code: 'user.missing_mfa', status: 422, }, { availableFactors } - ) + ); + } + + const { mfaSkipped } = interaction; + if (mfaSkipped) { + return interaction; + } + + // Auto mark MFA skipped for new users, will change to manual mark in the future + await storeInteractionResult( + { + mfaSkipped: true, + }, + ctx, + tenant.provider, + true + ); + + throw new RequestError( + { + code: 'user.missing_mfa', + status: 422, + }, + { availableFactors, skippable: true } ); } - if (event === InteractionEvent.SignIn) { - return validateMandatoryBindMfaForSignIn(tenant, ctx, interaction); - } - - return interaction; + return validateMandatoryBindMfaForSignIn(tenant, ctx, interaction); }; /**