Skip to content

Commit fedd5bf

Browse files
austincalvelageaustincalvelage
committed
refactor(mosaic): simplify MFA composition and cover recovery
1 parent 23a357f commit fedd5bf

5 files changed

Lines changed: 148 additions & 54 deletions

File tree

‎.changeset/mfa-section-feedback.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
---
2+
---

‎packages/mosaic/src/features/user-profile/__tests__/user-profile-mfa-management.feature.test.tsx‎

Lines changed: 93 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,45 @@ import { describe, expect, it, vi } from 'vitest';
66
import { fapiUrl, holdRequests, serveFapi, worker } from '../../../__tests__/feature/fake-fapi';
77
import { fapiClient, fapiPhoneNumber, fapiSession, fapiUser } from '../../../__tests__/feature/fapi';
88
import { renderWithClerk } from '../../../__tests__/feature/render';
9-
import { UserProfileMfaSection, useUserProfileMfaSlot } from '../user-profile-mfa-section/user-profile-mfa-section';
9+
import { mfaSectionNode, UserProfileMfaSection } from '../user-profile-mfa-section/user-profile-mfa-section';
10+
import { useUserProfileMfaModel } from '../user-profile-mfa-section/user-profile-mfa-section.model';
1011
import { useUserProfilePasswordSlot } from '../user-profile-password-section/user-profile-password-section';
1112
import { UserProfileSecurityPanelView } from '../user-profile-security-panel.view';
1213
import { mfaEnvironment, phone, renderMfa } from './mfa-feature-setup';
1314

1415
describe('User profile MFA management', () => {
16+
it('renders a plain MFA node in Authentication over legacy methods', async () => {
17+
serveFapi({
18+
environment: mfaEnvironment(),
19+
client: fapiClient([fapiSession({ id: 'sess_1', user: fapiUser({ id: 'user_1' }) })]),
20+
});
21+
await renderWithClerk(
22+
<UserProfileSecurityPanelView
23+
mfaSlot={<div>Connected MFA</div>}
24+
mfaMethods={[{ id: 'injected', type: 'authenticator' }]}
25+
/>,
26+
);
27+
28+
const authentication = screen.getByRole('region', { name: 'Authentication' });
29+
expect(within(authentication).getByText('Connected MFA')).toBeVisible();
30+
expect(within(authentication).queryByRole('heading', { name: '2-step verification' })).toBeNull();
31+
});
32+
33+
it('suppresses injected MFA when the slot is explicitly null', async () => {
34+
serveFapi({
35+
environment: mfaEnvironment(),
36+
client: fapiClient([fapiSession({ id: 'sess_1', user: fapiUser({ id: 'user_1' }) })]),
37+
});
38+
await renderWithClerk(
39+
<UserProfileSecurityPanelView
40+
mfaSlot={null}
41+
mfaMethods={[{ id: 'injected', type: 'authenticator' }]}
42+
/>,
43+
);
44+
45+
expect(screen.queryByRole('region', { name: 'Authentication' })).toBeNull();
46+
});
47+
1548
it('removes SMS MFA while retaining the phone number', async () => {
1649
const reserved = fapiPhoneNumber({ ...phone, reserved_for_second_factor: true, default_second_factor: true });
1750
const fapi = await renderMfa(fapiUser({ id: 'user_1', phone_numbers: [reserved], two_factor_enabled: true }));
@@ -28,6 +61,26 @@ describe('User profile MFA management', () => {
2861
expect(fapi.client.sessions[0]?.user.phone_numbers).toHaveLength(1);
2962
});
3063

64+
it('moves focus through remaining MFA rows and then to Add after sequential removals', async () => {
65+
const first = fapiPhoneNumber({ ...phone, reserved_for_second_factor: true, default_second_factor: true });
66+
const second = fapiPhoneNumber({ id: 'phone_2', phone_number: '+15555550202', reserved_for_second_factor: true });
67+
const fapi = await renderMfa(fapiUser({ id: 'user_1', phone_numbers: [first, second], two_factor_enabled: true }));
68+
const user = userEvent.setup();
69+
70+
await user.click(screen.getByRole('button', { name: 'Manage SMS verification +15555550101' }));
71+
await user.click(screen.getByRole('menuitem', { name: 'Remove method' }));
72+
await user.click(within(screen.getByRole('alertdialog')).getByRole('button', { name: 'Remove' }));
73+
const remaining = await screen.findByRole('button', { name: 'Manage SMS verification +15555550202' });
74+
await waitFor(() => expect(remaining).toHaveFocus());
75+
76+
await user.keyboard('{Enter}');
77+
await user.click(screen.getByRole('menuitem', { name: 'Remove method' }));
78+
await user.click(within(screen.getByRole('alertdialog')).getByRole('button', { name: 'Remove' }));
79+
await waitFor(() => expect(screen.getByRole('button', { name: 'Add verification method' })).toHaveFocus());
80+
expect(fapi.mfa.phoneUpdates).toContainEqual({ id: 'phone_1', reserved: false, default: undefined });
81+
expect(fapi.mfa.phoneUpdates).toContainEqual({ id: 'phone_2', reserved: false, default: undefined });
82+
});
83+
3184
it('shows second-factor reverification for removal with no addable methods', async () => {
3285
const environment = mfaEnvironment();
3386
environment.user_settings.attributes.phone_number.used_for_second_factor = false;
@@ -123,7 +176,7 @@ describe('User profile MFA management', () => {
123176

124177
it('places a connected MFA slot after password in the security panel', async () => {
125178
function Panel() {
126-
const mfaSlot = useUserProfileMfaSlot();
179+
const mfaSlot = mfaSectionNode(useUserProfileMfaModel());
127180
const passwordSlot = useUserProfilePasswordSlot();
128181
return (
129182
<UserProfileSecurityPanelView
@@ -147,7 +200,7 @@ describe('User profile MFA management', () => {
147200

148201
it('uses the connected slot instead of duplicate injected MFA props', async () => {
149202
function Panel() {
150-
const mfaSlot = useUserProfileMfaSlot();
203+
const mfaSlot = mfaSectionNode(useUserProfileMfaModel());
151204
return (
152205
<UserProfileSecurityPanelView
153206
mfaSlot={mfaSlot}
@@ -166,7 +219,7 @@ describe('User profile MFA management', () => {
166219

167220
it('suppresses injected MFA when the connected slot is hidden', async () => {
168221
function Panel() {
169-
const mfaSlot = useUserProfileMfaSlot();
222+
const mfaSlot = mfaSectionNode(useUserProfileMfaModel());
170223
return (
171224
<UserProfileSecurityPanelView
172225
mfaSlot={mfaSlot}
@@ -524,6 +577,42 @@ describe('User profile MFA management', () => {
524577
expect(screen.getAllByText('SMS verification')[0]?.closest('li')).toHaveTextContent('Default');
525578
});
526579

580+
it('shows a failed default change and clears its error on retry', async () => {
581+
const first = fapiPhoneNumber({ ...phone, reserved_for_second_factor: true, default_second_factor: true });
582+
const second = fapiPhoneNumber({ id: 'phone_2', phone_number: '+15555550202', reserved_for_second_factor: true });
583+
const fapi = await renderMfa(fapiUser({ id: 'user_1', phone_numbers: [first, second], two_factor_enabled: true }));
584+
let attempts = 0;
585+
worker.use(
586+
http.post(fapiUrl('/v1/me/phone_numbers/phone_2'), ({ request }) => {
587+
if (new URL(request.url).searchParams.get('_method') !== 'PATCH') {
588+
return undefined;
589+
}
590+
attempts += 1;
591+
return attempts === 1
592+
? HttpResponse.json(
593+
{ errors: [{ code: 'unexpected_error', message: 'Unable to update default method.' }] },
594+
{ status: 503 },
595+
)
596+
: undefined;
597+
}),
598+
);
599+
const user = userEvent.setup();
600+
const selected = screen.getByRole('button', { name: 'Manage SMS verification +15555550202' });
601+
602+
await user.click(selected);
603+
await user.click(screen.getByRole('menuitem', { name: 'Set as default' }));
604+
await waitFor(() => expect(screen.getByRole('alert')).toHaveTextContent('Unable to update default method.'));
605+
expect(screen.getAllByText('Default')).toHaveLength(1);
606+
607+
await user.click(selected);
608+
await user.click(screen.getByRole('menuitem', { name: 'Set as default' }));
609+
await waitFor(() => expect(screen.queryByRole('alert')).toBeNull());
610+
await waitFor(() =>
611+
expect(fapi.mfa.phoneUpdates).toContainEqual({ id: 'phone_2', default: true, reserved: undefined }),
612+
);
613+
expect(attempts).toBe(2);
614+
});
615+
527616
it('keeps method menus available but hides default changes while one is pending', async () => {
528617
const first = fapiPhoneNumber({ ...phone, reserved_for_second_factor: true, default_second_factor: true });
529618
const second = fapiPhoneNumber({ id: 'phone_2', phone_number: '+15555550202', reserved_for_second_factor: true });

‎packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.controller.ts‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ import { useEffect, useRef, useState } from 'react';
33
import { useMessages } from '../../../localization';
44
import { setup } from '../../../machine/setup';
55
import { useMachine } from '../../../machine/use-machine';
6+
import { useReverificationController } from '../../reverification/reverification.controller';
7+
import type { ReverificationModel } from '../../reverification/reverification.model';
68
import {
79
MfaCancelledError,
810
type UserProfileMfaAddableMethod,
@@ -212,8 +214,9 @@ const mfaMachine = createMachine({
212214
},
213215
});
214216

215-
export function useUserProfileMfaController(model: ReadyModel) {
217+
export function useUserProfileMfaController(model: ReadyModel, reverificationModel: ReverificationModel) {
216218
const m = useMessages('userProfileMfa');
219+
const reverificationProps = useReverificationController(reverificationModel, model.resetReverification);
217220
const [{ context, value }, send] = useMachine(mfaMachine);
218221
const locked = useRef(false);
219222
const [now, setNow] = useState(() => Date.now());
@@ -457,9 +460,22 @@ export function useUserProfileMfaController(model: ReadyModel) {
457460
onSetDefault: setDefault,
458461
};
459462

463+
const dialogOpen = flow.kind !== 'closed';
464+
const showAddTrigger = model.addableMethods.length > 0;
465+
const showReverification = model.reverification.phase === 'active' || model.reverification.phase === 'retrying';
466+
460467
return {
461-
dialogOpen: flow.kind !== 'closed',
462-
showAddTrigger: model.addableMethods.length > 0,
468+
dialogOpen,
469+
showAddTrigger,
470+
showAddControl: showAddTrigger || dialogOpen,
471+
showReverification,
472+
separateReverificationOpen: !dialogOpen && showReverification,
473+
onSeparateReverificationOpenChange: (open: boolean) => {
474+
if (!open && model.reverification.phase === 'active') {
475+
model.reverification.cancel();
476+
}
477+
},
478+
reverificationProps,
463479
onDialogOpenChange: (open: boolean) => {
464480
if (!open && model.reverification.phase === 'active') {
465481
model.reverification.cancel();

‎packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.tsx‎

Lines changed: 17 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import type { ReactNode } from 'react';
22

33
import { Reverification } from '../../reverification/reverification';
4-
import { useReverificationController } from '../../reverification/reverification.controller';
54
import { useReverificationModel } from '../../reverification/reverification.model';
65
import { UserProfileAddMfaDialog } from './user-profile-add-mfa.dialog';
76
import { useUserProfileMfaController } from './user-profile-mfa-section.controller';
@@ -14,52 +13,43 @@ export interface UserProfileMfaSectionProps {
1413
fallback?: ReactNode;
1514
}
1615

17-
export interface UserProfileMfaSlot {
18-
content: ReactNode;
19-
}
20-
2116
export function UserProfileMfaSection(props: UserProfileMfaSectionProps = {}) {
22-
return useUserProfileMfaSlot(props)?.content ?? null;
17+
const model = useUserProfileMfaModel();
18+
return mfaSectionNode(model, props.fallback);
2319
}
2420

25-
export function useUserProfileMfaSlot({ fallback = null }: UserProfileMfaSectionProps = {}): UserProfileMfaSlot | null {
26-
const model = useUserProfileMfaModel();
21+
export function mfaSectionNode(model: UserProfileMfaModel, fallback: ReactNode = null): ReactNode {
2722
if (model.status === 'loading') {
28-
return fallback ? { content: fallback } : null;
23+
return fallback || null;
2924
}
3025
if (model.status === 'hidden') {
3126
return null;
3227
}
33-
return {
34-
content: (
35-
<MfaEditor
36-
key={`${model.userId}:${model.sessionId}`}
37-
model={model}
38-
/>
39-
),
40-
};
28+
return (
29+
<MfaEditor
30+
key={`${model.userId}:${model.sessionId}`}
31+
model={model}
32+
/>
33+
);
4134
}
4235

4336
function MfaEditor({ model }: { model: Extract<UserProfileMfaModel, { status: 'ready' }> }) {
44-
const controller = useUserProfileMfaController(model);
4537
const reverificationModel = useReverificationModel(model.reverification);
46-
const reverificationController = useReverificationController(reverificationModel, model.resetReverification);
47-
const separateReverification =
48-
!controller.dialogOpen && (model.reverification.phase === 'active' || model.reverification.phase === 'retrying');
38+
const controller = useUserProfileMfaController(model, reverificationModel);
4939

5040
return (
5141
<>
5242
<UserProfileMfaSectionView
5343
{...controller.sectionProps}
5444
addControl={
55-
controller.showAddTrigger || controller.dialogOpen ? (
45+
controller.showAddControl ? (
5646
<UserProfileAddMfaDialog
5747
open={controller.dialogOpen}
5848
onOpenChange={controller.onDialogOpenChange}
5949
hideTrigger={!controller.showAddTrigger}
6050
>
61-
{model.reverification.phase === 'active' || model.reverification.phase === 'retrying' ? (
62-
<Reverification {...reverificationController} />
51+
{controller.showReverification ? (
52+
<Reverification {...controller.reverificationProps} />
6353
) : (
6454
<UserProfileMfaSetupView {...controller.setupProps} />
6555
)}
@@ -68,15 +58,11 @@ function MfaEditor({ model }: { model: Extract<UserProfileMfaModel, { status: 'r
6858
}
6959
/>
7060
<UserProfileAddMfaDialog
71-
open={separateReverification}
72-
onOpenChange={open => {
73-
if (!open && model.reverification.phase === 'active') {
74-
model.reverification.cancel();
75-
}
76-
}}
61+
open={controller.separateReverificationOpen}
62+
onOpenChange={controller.onSeparateReverificationOpenChange}
7763
hideTrigger
7864
>
79-
<Reverification {...reverificationController} />
65+
<Reverification {...controller.reverificationProps} />
8066
</UserProfileAddMfaDialog>
8167
</>
8268
);

‎packages/mosaic/src/features/user-profile/user-profile-security-panel.view.tsx‎

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import type {
88
UserProfileDevice,
99
} from './user-profile-active-devices-section.view';
1010
import { UserProfileActiveDevicesSectionView } from './user-profile-active-devices-section.view';
11-
import type { UserProfileMfaSlot } from './user-profile-mfa-section/user-profile-mfa-section';
1211
import type {
1312
UserProfileMfaAddableMethod,
1413
UserProfileMfaMethod,
@@ -22,7 +21,7 @@ export type { UserProfileDevice, UserProfileMfaAddableMethod, UserProfileMfaMeth
2221

2322
export interface UserProfileSecurityPanelViewProps extends Omit<UserProfileActiveDevicesSectionViewProps, 'devices'> {
2423
passwordSlot?: UserProfilePasswordSlot | null;
25-
mfaSlot?: UserProfileMfaSlot | null;
24+
mfaSlot?: ReactNode;
2625
passkeys?: UserProfilePasskey[];
2726
passkeysVisible?: boolean;
2827
mfaMethods?: UserProfileMfaMethod[];
@@ -64,8 +63,21 @@ export function UserProfileSecurityPanelView({
6463
}: UserProfileSecurityPanelViewProps): ReactElement {
6564
const showPassword = Boolean(passwordSlot);
6665
const showPasskeys = passkeys !== undefined && passkeysVisible;
67-
const hasAuthentication =
68-
showPassword || showPasskeys || (mfaSlot !== undefined ? Boolean(mfaSlot) : mfaMethods !== undefined);
66+
const selectedMfaNode =
67+
mfaSlot !== undefined ? (
68+
mfaSlot
69+
) : mfaMethods !== undefined ? (
70+
<UserProfileMfaSectionView
71+
methods={mfaMethods}
72+
addableMethods={addableMfaMethods}
73+
addControl={mfaAddControl}
74+
onAdd={onAddMfaMethod}
75+
onRegenerateBackupCodes={onRegenerateBackupCodes}
76+
onRemove={onRemoveMfaMethod}
77+
onSetDefault={onSetDefaultMfaMethod}
78+
/>
79+
) : null;
80+
const hasAuthentication = showPassword || showPasskeys || Boolean(selectedMfaNode);
6981

7082
return (
7183
<Panel.Root render={<div {...themeProps('user-profile-security-panel')} />}>
@@ -83,18 +95,7 @@ export function UserProfileSecurityPanelView({
8395
onRemove={onRemovePasskey}
8496
/>
8597
) : null}
86-
{mfaSlot?.content}
87-
{mfaSlot === undefined && mfaMethods !== undefined ? (
88-
<UserProfileMfaSectionView
89-
methods={mfaMethods}
90-
addableMethods={addableMfaMethods}
91-
addControl={mfaAddControl}
92-
onAdd={onAddMfaMethod}
93-
onRegenerateBackupCodes={onRegenerateBackupCodes}
94-
onRemove={onRemoveMfaMethod}
95-
onSetDefault={onSetDefaultMfaMethod}
96-
/>
97-
) : null}
98+
{selectedMfaNode}
9899
</Section.Root>
99100
) : null}
100101
{devices ? (

0 commit comments

Comments
 (0)