diff --git a/.changeset/mfa-setup-reverification.md b/.changeset/mfa-setup-reverification.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/mfa-setup-reverification.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa-interactions.feature.test.tsx b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa-interactions.feature.test.tsx index 52460d42121..52c3128f777 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa-interactions.feature.test.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa-interactions.feature.test.tsx @@ -92,22 +92,23 @@ describe('User profile MFA interaction boundaries', () => { expect(await screen.findByRole('img', { name: /QR code/i })).toBeVisible(); }); - it('keeps the dialog closed when dismissed while authenticator preparation is pending', async () => { + it('keeps the dialog open until authenticator preparation responds', async () => { const fapi = await renderMfa(); const held = holdRequests('post', '/v1/me/totp'); const user = userEvent.setup(); await user.click(screen.getByRole('button', { name: 'Add verification method' })); await user.click(screen.getByRole('button', { name: /Authenticator app/ })); await waitFor(() => expect(held.requests).toHaveLength(1)); - await user.keyboard('{Escape}'); - await waitFor(() => expect(screen.queryByRole('dialog')).toBeNull()); - held.release(); - await waitFor(() => expect(fapi.mfa.totpCreations).toBe(1)); - expect(screen.queryByRole('dialog')).toBeNull(); - await user.click(screen.getByRole('button', { name: 'Add verification method' })); - expect(await screen.findByRole('dialog', { name: 'Add 2-step verification' })).toHaveAccessibleDescription( - 'Choose a verification method', - ); + const dialog = screen.getByRole('dialog'); + try { + expect(within(dialog).queryByRole('button', { name: 'Close' })).toBeNull(); + await user.keyboard('{Escape}'); + expect(dialog).toBeVisible(); + } finally { + held.release(); + } + expect(await screen.findByRole('img', { name: /QR code/i })).toBeVisible(); + expect(fapi.mfa.totpCreations).toBe(1); }); it('shows backup codes when dismissed while authenticator verification is pending', async () => { diff --git a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa-management.feature.test.tsx b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa-management.feature.test.tsx index c90e2e9d762..059eecc91c5 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa-management.feature.test.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa-management.feature.test.tsx @@ -9,6 +9,7 @@ import { fapiUrl, holdRequests, serveFapi, + VERIFICATION_CODE, worker, } from '../../../../__tests__/feature/fake-fapi'; import { fapiClient, fapiPhoneNumber, fapiSession, fapiUser } from '../../../../__tests__/feature/fapi'; @@ -370,9 +371,10 @@ describe('User profile MFA management', () => { } expect(screen.getByRole('button', { name: 'Copy and close' })).toBeVisible(); expect(screen.getByRole('button', { name: 'Copy and close' })).toHaveAttribute('aria-disabled', 'true'); - await waitFor(() => expect(screen.getByRole('button', { name: 'Close' })).toHaveFocus()); + expect(screen.queryByRole('button', { name: 'Close' })).toBeNull(); held.release(); await waitFor(() => expect(screen.getByText('CODE0100')).toBeVisible()); + expect(screen.getByRole('button', { name: 'Close' })).toBeVisible(); for (const name of ['Print', 'Download', 'Copy and close']) { expect(screen.getByRole('button', { name })).toBeEnabled(); } @@ -481,9 +483,13 @@ describe('User profile MFA management', () => { await regenerate(user); await waitFor(() => expect(first.requests).toHaveLength(1)); const dialog = screen.getByRole('dialog'); - await user.keyboard('{Escape}'); - expect(dialog).toBeVisible(); - first.release(); + try { + expect(within(dialog).queryByRole('button', { name: 'Close' })).toBeNull(); + await user.keyboard('{Escape}'); + expect(dialog).toBeVisible(); + } finally { + first.release(); + } expect(await within(dialog).findByText('Enter your current password to continue')).toBeVisible(); await user.type(screen.getByLabelText('Password'), 'hunter2{Enter}'); @@ -491,6 +497,130 @@ describe('User profile MFA management', () => { expect(fapi.mfa.backupCodeCreations).toBe(1); }); + const startSms = async (user: ReturnType) => { + await user.click(screen.getByRole('button', { name: 'Add verification method' })); + await user.click(screen.getByRole('button', { name: /SMS verification/ })); + }; + + it('verifies the session before reserving an existing phone for SMS MFA', async () => { + const fapi = await renderMfa(undefined, mfaEnvironment(), { verification: { secrets: { password: 'hunter2' } } }); + demandReverificationOnce('/v1/me/phone_numbers/phone_1'); + const user = userEvent.setup(); + + await startSms(user); + await user.click(screen.getByRole('button', { name: 'Continue' })); + expect(await screen.findByText('Enter your current password to continue')).toBeVisible(); + await waitFor(() => expect(screen.getByLabelText('Password')).toHaveFocus()); + await user.keyboard('hunter2{Enter}'); + + expect(await screen.findByText('CODE0001')).toBeVisible(); + expect(fapi.mfa.phoneUpdates).toContainEqual({ id: 'phone_1', reserved: true, default: undefined }); + }); + + it('keeps SMS code verification open until its reservation challenge can be cancelled', async () => { + const unverified = fapiPhoneNumber({ ...phone, verification: { ...phone.verification, status: 'unverified' } }); + const fapi = await renderMfa(fapiUser({ id: 'user_1', phone_numbers: [unverified] }), mfaEnvironment(), { + verification: { secrets: { password: 'hunter2' } }, + }); + demandReverificationOnce('/v1/me/phone_numbers/phone_1'); + const user = userEvent.setup(); + + await startSms(user); + await user.click(screen.getByRole('button', { name: 'Continue' })); + const code = await screen.findByRole('textbox', { name: 'Verification code' }); + const attempt = holdRequests('post', '/v1/me/phone_numbers/phone_1/attempt_verification'); + await user.type(code, VERIFICATION_CODE); + await waitFor(() => expect(attempt.requests).toHaveLength(1)); + const dialog = screen.getByRole('dialog'); + try { + expect(within(dialog).queryByRole('button', { name: 'Close' })).toBeNull(); + await user.keyboard('{Escape}'); + expect(dialog).toBeVisible(); + } finally { + attempt.release(); + } + + expect(await within(dialog).findByText('Enter your current password to continue')).toBeVisible(); + await user.click(within(dialog).getByRole('button', { name: 'Close' })); + await waitFor(() => expect(screen.queryByRole('dialog')).toBeNull()); + expect(fapi.mfa.phoneAttempts).toEqual([{ id: 'phone_1', code: VERIFICATION_CODE }]); + expect(fapi.mfa.phoneUpdates).toEqual([]); + expect(screen.queryByRole('alert')).toBeNull(); + }); + + it('verifies the session before creating a phone for SMS MFA, then sends its code', async () => { + const fapi = await renderMfa(fapiUser({ id: 'user_1' }), mfaEnvironment(), { + verification: { secrets: { password: 'hunter2' } }, + }); + demandReverificationOnce('/v1/me/phone_numbers'); + const user = userEvent.setup(); + + await startSms(user); + await user.type(screen.getByRole('textbox', { name: 'Phone' }), '5555550303'); + await user.click(screen.getByRole('button', { name: 'Send code' })); + expect(await screen.findByText('Enter your current password to continue')).toBeVisible(); + await waitFor(() => expect(screen.getByLabelText('Password')).toHaveFocus()); + await user.keyboard('hunter2{Enter}'); + + await waitFor(() => expect(fapi.mfa.phonePreparations).toHaveLength(1)); + expect(fapi.mfa.phoneCreations).toHaveLength(1); + expect(await screen.findByRole('button', { name: 'Verify' })).toBeVisible(); + }); + + it('lets the user cancel SMS MFA verification quietly and add the method again', async () => { + const fapi = await renderMfa(undefined, mfaEnvironment(), { verification: { secrets: { password: 'hunter2' } } }); + demandReverificationOnce('/v1/me/phone_numbers/phone_1'); + const user = userEvent.setup(); + + await startSms(user); + await user.click(screen.getByRole('button', { name: 'Continue' })); + await screen.findByText('Enter your current password to continue'); + await user.click(within(screen.getByRole('dialog')).getByRole('button', { name: 'Close' })); + + await waitFor(() => expect(screen.queryByRole('dialog')).toBeNull()); + expect(fapi.mfa.phoneUpdates).toEqual([]); + expect(screen.queryByRole('alert')).toBeNull(); + + await startSms(user); + await user.click(screen.getByRole('button', { name: 'Continue' })); + expect(await screen.findByText('CODE0001')).toBeVisible(); + }); + + it('verifies the session before starting authenticator setup', async () => { + const fapi = await renderMfa(undefined, mfaEnvironment(), { verification: { secrets: { password: 'hunter2' } } }); + demandReverificationOnce('/v1/me/totp'); + const user = userEvent.setup(); + + await user.click(screen.getByRole('button', { name: 'Add verification method' })); + await user.click(screen.getByRole('button', { name: /Authenticator app/ })); + expect(await screen.findByText('Enter your current password to continue')).toBeVisible(); + await waitFor(() => expect(screen.getByLabelText('Password')).toHaveFocus()); + await user.keyboard('hunter2{Enter}'); + + expect(await screen.findByRole('img', { name: /QR code/i })).toBeVisible(); + expect(fapi.mfa.totpCreations).toBe(1); + }); + + it('lets the user cancel authenticator verification quietly and start setup again', async () => { + const fapi = await renderMfa(undefined, mfaEnvironment(), { verification: { secrets: { password: 'hunter2' } } }); + demandReverificationOnce('/v1/me/totp'); + const user = userEvent.setup(); + + await user.click(screen.getByRole('button', { name: 'Add verification method' })); + await user.click(screen.getByRole('button', { name: /Authenticator app/ })); + await screen.findByText('Enter your current password to continue'); + await user.click(within(screen.getByRole('dialog')).getByRole('button', { name: 'Close' })); + + await waitFor(() => expect(screen.queryByRole('dialog')).toBeNull()); + expect(fapi.mfa.totpCreations).toBe(0); + expect(screen.queryByRole('alert')).toBeNull(); + + await user.click(screen.getByRole('button', { name: 'Add verification method' })); + await user.click(screen.getByRole('button', { name: /Authenticator app/ })); + expect(await screen.findByRole('img', { name: /QR code/i })).toBeVisible(); + expect(fapi.mfa.totpCreations).toBe(1); + }); + it('copies server-issued backup codes and closes their one-time surface', async () => { const fapi = await renderMfa(); const write = vi.spyOn(navigator.clipboard, 'writeText').mockResolvedValue(); diff --git a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.controller.ts b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.controller.ts index 0c0f90bbc16..62286ba4c01 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.controller.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.controller.ts @@ -6,6 +6,7 @@ import { setup } from '../../../machine/setup'; import type { DoneInvokeEvent } from '../../../machine/types'; import { useMachine } from '../../../machine/use-machine'; import { toLocalizableError } from '../../../utils/errors'; +import type { ReverificationController } from '../../reverification'; import type { UserProfileMfaAddableMethod, UserProfileMfaModel } from './user-profile-mfa-section.types'; import type { UserProfileMfaSectionViewProps } from './user-profile-mfa-section.view'; import type { UserProfileMfaSetupViewProps } from './user-profile-mfa-setup.view'; @@ -61,6 +62,7 @@ type MfaEvent = const { createMachine, assign, fromPromise } = setup(); const RESEND_COOLDOWN_SECONDS = 30; +const REVERIFIABLE_ACTIONS: readonly MfaAction[] = ['generate', 'sms', 'prepareAuthenticator']; const tick = { actions: assign(context => ({ resendSeconds: Math.max(0, context.resendSeconds - 1) })) }; const mfaMachine = createMachine({ @@ -413,21 +415,27 @@ function createSetupProps( send: MfaSend, actions: MfaSetupActions, errorText: ReturnType, + reverification: ReverificationController, ): UserProfileMfaSetupViewProps { const flow = context.flow; const sms = flow.kind === 'sms' ? flow : undefined; const authenticator = flow.kind === 'authenticator' ? flow : undefined; const backup = flow.kind === 'backup' ? flow : undefined; + const isReverifying = + context.pending !== undefined && REVERIFIABLE_ACTIONS.includes(context.pending) && reverification.visible; + let step: UserProfileMfaSetupViewProps['step']; + if (isReverifying) { + step = 'verify'; + } else if (flow.kind === 'closed') { + step = 'select'; + } else if (flow.kind === 'backup') { + step = 'backup-codes'; + } else { + step = flow.kind; + } return { - step: - flow.kind === 'closed' - ? 'select' - : flow.kind === 'backup' - ? context.pending === 'generate' && model.backupCodesReverification.visible - ? 'verify' - : 'backup-codes' - : flow.kind, - reverification: model.backupCodesReverification, + step, + reverification, methods: model.addableMethods, onSelect: actions.select, onBack: actions.back, @@ -521,6 +529,11 @@ export function useUserProfileMfaController(model: ReadyModel) { } }; + // The machine guarantees that only one is running/visible at a time + const reverification = + [model.createPhoneReverification, model.enrollSmsReverification, model.authenticatorReverification].find( + controller => controller.visible, + ) ?? model.backupCodesReverification; const setupProps = createSetupProps( model, context, @@ -539,6 +552,7 @@ export function useUserProfileMfaController(model: ReadyModel) { print, }, errorText, + reverification, ); const sectionProps: UserProfileMfaSectionViewProps = { @@ -552,8 +566,9 @@ export function useUserProfileMfaController(model: ReadyModel) { }; const showAddTrigger = model.addableMethods.length > 0; - const reverification = model.backupCodesReverification; const reverifying = setupProps.step === 'verify'; + const hasPendingReverification = context.pending !== undefined && REVERIFIABLE_ACTIONS.includes(context.pending); + const canClose = !hasPendingReverification || Boolean(reverification.onCancel); return { dialogOpen, @@ -561,15 +576,15 @@ export function useUserProfileMfaController(model: ReadyModel) { showAddControl: showAddTrigger || dialogOpen, reverifying, onDialogOpenChange: (open: boolean) => { - if (!open && context.pending === 'generate') { - if (!reverification.onCancel) { - return; - } - reverification.onCancel(); + if (!open && !canClose) { + return; + } + if (!open && hasPendingReverification) { + reverification.onCancel?.(); } send({ type: open ? 'OPEN' : 'CLOSE' }); }, - showCloseButton: !reverifying || Boolean(reverification.onCancel), + showCloseButton: canClose, sectionProps, setupProps, }; diff --git a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.model.ts b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.model.ts index 888aaf68166..8c712130fb0 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.model.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.model.ts @@ -112,7 +112,11 @@ function createSmsActions( userId: string, sessionId: string, refresh: () => Promise, - makeDefault: (phone: PhoneNumberResource) => Promise, + calls: { + makeDefault: (phone: PhoneNumberResource) => Promise; + createPhone: (current: UserResource, phoneNumber: string) => Promise; + reservePhone: (phone: PhoneNumberResource) => Promise; + }, ): Pick { const currentPhone = (phoneId: string) => { const current = requireIdentity(clerk, userId, sessionId); @@ -129,7 +133,7 @@ function createSmsActions( const existing = current.phoneNumbers.find( phone => !phone.reservedForSecondFactor && phone.phoneNumber === phoneNumber, ); - return summarizePhone(existing ?? (await current.createPhoneNumber({ phoneNumber }))); + return summarizePhone(existing ?? (await calls.createPhone(current, phoneNumber))); }, enrollSms: async (phoneId, code): Promise => { let phone = currentPhone(phoneId); @@ -144,7 +148,7 @@ function createSmsActions( if (phone.reservedForSecondFactor || !configuredFactors(environment).includes('phone_code')) { throw phoneUnavailable(); } - const result = await phone.setReservedForSecondFactor({ reserved: true }); + const result = await calls.reservePhone(phone); await refresh().catch(() => undefined); return { status: 'complete', backupCodes: result.backupCodes ?? [] }; }, @@ -156,7 +160,7 @@ function createSmsActions( if (phone.verification.status !== 'verified' || !phone.reservedForSecondFactor) { throw phoneUnavailable(); } - await makeDefault(phone); + await calls.makeDefault(phone); await refresh().catch(() => undefined); }, }; @@ -196,6 +200,15 @@ export function useUserProfileMfaModel(): UserProfileMfaModel { const [createBackupCode, backupCodesReverification] = useReverificationFlow((current: UserResource) => current.createBackupCode(), ); + const [createPhone, createPhoneReverification] = useReverificationFlow((current: UserResource, phoneNumber: string) => + current.createPhoneNumber({ phoneNumber }), + ); + const [reservePhone, enrollSmsReverification] = useReverificationFlow((phone: PhoneNumberResource) => + phone.setReservedForSecondFactor({ reserved: true }), + ); + const [createTotp, authenticatorReverification] = useReverificationFlow((current: UserResource) => + current.createTOTP(), + ); if (!userLoaded || !sessionLoaded || !environment) { return { status: 'loading' }; @@ -221,7 +234,9 @@ export function useUserProfileMfaModel(): UserProfileMfaModel { await current.reload(); }; - const smsActions = smsEnabled ? createSmsActions(clerk, environment, userId, sessionId, refresh, makeDefault) : {}; + const smsActions = smsEnabled + ? createSmsActions(clerk, environment, userId, sessionId, refresh, { makeDefault, createPhone, reservePhone }) + : {}; return { status: 'ready', @@ -239,7 +254,7 @@ export function useUserProfileMfaModel(): UserProfileMfaModel { if (current.totpEnabled || !configuredFactors(environment).includes('totp')) { throw new ClerkRuntimeError('Authenticator is unavailable.', { code: 'mfa_authenticator_unavailable' }); } - const result = await current.createTOTP(); + const result = await createTotp(current); return { secret: result.secret ?? '', uri: result.uri ?? '' }; } : undefined, @@ -268,5 +283,8 @@ export function useUserProfileMfaModel(): UserProfileMfaModel { removeReverification, setDefaultReverification, backupCodesReverification, + createPhoneReverification, + enrollSmsReverification, + authenticatorReverification, }; } diff --git a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.types.ts b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.types.ts index 2d87c6a0196..3581282f5c2 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.types.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.types.ts @@ -49,4 +49,7 @@ export type UserProfileMfaModel = removeReverification: ReverificationController; setDefaultReverification: ReverificationController; backupCodesReverification: ReverificationController; + createPhoneReverification: ReverificationController; + enrollSmsReverification: ReverificationController; + authenticatorReverification: ReverificationController; }; diff --git a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-setup.view.tsx b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-setup.view.tsx index 363cb957392..12541dbb03b 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-setup.view.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-setup.view.tsx @@ -1,4 +1,4 @@ -import { Flow } from '../../../components/flow'; +import { Flow, useFlowAutoFocus } from '../../../components/flow'; import type { ReverificationController } from '../../reverification'; import { Reverification } from '../../reverification'; import type { UserProfileAddAuthenticatorViewProps } from './user-profile-add-authenticator.view'; @@ -60,10 +60,21 @@ export function UserProfileMfaSetupView(props: UserProfileMfaSetupViewProps) { /> - + )} ); } + +function ReverifyStep(props: { controller: ReverificationController }) { + const initialFocusRef = useFlowAutoFocus(); + + return ( + + ); +}