Skip to content

fix(settings): redirect user on AAL mismatch in mfa guard #19599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+227 −77
Draft

fix(settings): redirect user on AAL mismatch in mfa guard #19599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 73 additions & 18 deletions packages/fxa-settings/src/components/Settings/MfaGuard/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,13 @@ const mockAuthClient = {
}
return Promise.reject(AuthUiErrors.INVALID_EXPIRED_OTP_CODE);
}),
sessionStatus: jest.fn().mockResolvedValue({
details: { sessionVerificationMeetsMinimumAAL: true },
}),
};
const mockAlertBar = { error: jest.fn() };
const mockNavigate = jest.fn();
const mockNavigateWithQuery = jest.fn();

jest.mock('../../../lib/cache', () => {
const actual = jest.requireActual('../../../lib/cache');
Expand All @@ -43,14 +47,18 @@ jest.mock('../../../models', () => ({
useAlertBar: () => mockAlertBar,
}));

jest.mock('../../../lib/hooks/useNavigateWithQuery', () => ({
useNavigateWithQuery: () => mockNavigateWithQuery,
}));

jest.mock('@reach/router', () => ({
...jest.requireActual('@reach/router'),
useNavigate: () => mockNavigate,
}));

async function submitCode(otp: string = mockOtp) {
await userEvent.type(
screen.getByRole('textbox', { name: 'Enter 6-digit code' }),
await screen.findByRole('textbox', { name: 'Enter 6-digit code' }),
otp
);
await userEvent.click(screen.getByRole('button', { name: 'Confirm' }));
Expand All @@ -61,6 +69,10 @@ describe('MfaGuard', () => {
JwtTokenCache.removeToken(mockSessionToken, mockScope);
MfaOtpRequestCache.remove(mockSessionToken, mockScope);
jest.clearAllMocks();

mockAuthClient.sessionStatus.mockResolvedValue({
details: { sessionVerificationMeetsMinimumAAL: true },
});
});

it('requests OTP and shows modal when JWT missing', async () => {
Expand All @@ -72,10 +84,12 @@ describe('MfaGuard', () => {
</AppContext.Provider>
);

expect(mockAuthClient.mfaRequestOtp).toHaveBeenCalledWith(
mockSessionToken,
mockScope
);
await waitFor(() => {
expect(mockAuthClient.mfaRequestOtp).toHaveBeenCalledWith(
mockSessionToken,
mockScope
);
});

expect(
await screen.findByText('Enter confirmation code')
Expand Down Expand Up @@ -110,7 +124,7 @@ describe('MfaGuard', () => {
});
});

it('renders children when JWT exists', () => {
it('renders children when JWT exists', async () => {
JwtTokenCache.setToken(mockSessionToken, mockScope, 'jwt-present');

renderWithRouter(
Expand All @@ -121,7 +135,7 @@ describe('MfaGuard', () => {
</AppContext.Provider>
);

expect(screen.getByText('secured')).toBeInTheDocument();
expect(await screen.findByText('secured')).toBeInTheDocument();
expect(
screen.queryByText('Enter confirmation code')
).not.toBeInTheDocument();
Expand Down Expand Up @@ -160,7 +174,9 @@ describe('MfaGuard', () => {
</AppContext.Provider>
);

expect(screen.queryByText('Enter confirmation code')).toBeInTheDocument();
expect(
await screen.findByText('Enter confirmation code')
).toBeInTheDocument();
await submitCode('654321');

expect(
Expand All @@ -181,7 +197,9 @@ describe('MfaGuard', () => {
</AppContext.Provider>
);

expect(screen.getByText('Enter confirmation code')).toBeInTheDocument();
expect(
await screen.findByText('Enter confirmation code')
).toBeInTheDocument();
await submitCode('654321');
expect(
await screen.findByText('Invalid or expired confirmation code')
Expand Down Expand Up @@ -209,7 +227,7 @@ describe('MfaGuard', () => {

// Trigger an error first
await userEvent.type(
screen.getByRole('textbox', { name: 'Enter 6-digit code' }),
await screen.findByRole('textbox', { name: 'Enter 6-digit code' }),
'654321'
);
await userEvent.click(screen.getByRole('button', { name: 'Confirm' }));
Expand All @@ -218,7 +236,7 @@ describe('MfaGuard', () => {
).toBeInTheDocument();

await userEvent.click(
screen.getByRole('button', { name: 'Email new code.' })
await screen.findByRole('button', { name: 'Email new code.' })
);
expect(
await screen.findByText('A new code was sent to your email.')
Expand All @@ -242,7 +260,7 @@ describe('MfaGuard', () => {
);

await userEvent.click(
screen.getByRole('button', { name: 'Email new code.' })
await screen.findByRole('button', { name: 'Email new code.' })
);
expect(
await screen.findByText('A new code was sent to your email.')
Expand All @@ -253,11 +271,46 @@ describe('MfaGuard', () => {
);

await userEvent.click(
screen.getByRole('button', { name: 'Email new code.' })
await screen.findByRole('button', { name: 'Email new code.' })
);
expect(await screen.findByText('Unexpected error')).toBeInTheDocument();
});

it('navigates to signin_totp_code when session does not meet minimum AAL', async () => {
// Mock sessionStatus to return false for AAL check
mockAuthClient.sessionStatus.mockResolvedValue({
details: { sessionVerificationMeetsMinimumAAL: false },
});

const context = mockAppContext();

renderWithRouter(
<AppContext.Provider value={context}>
<MfaGuard
requiredScope={mockScope}
debounceIntervalMs={0}
reason={MfaReason.test}
>
<div>secured</div>
</MfaGuard>
</AppContext.Provider>
);

await waitFor(() => {
expect(mockNavigateWithQuery).toHaveBeenCalledWith('/signin_totp_code', {
state: {
email: context.account!.email,
sessionToken: mockSessionToken,
uid: context.account!.uid,
verified: false,
isSessionAALUpgrade: true,
},
});
});

expect(mockAuthClient.mfaRequestOtp).not.toHaveBeenCalled();
});

it('goes home and shows error alert bar if request for OTP fails', async () => {
mockAuthClient.mfaRequestOtp.mockRejectedValueOnce(
AuthUiErrors.UNEXPECTED_ERROR
Expand Down Expand Up @@ -297,7 +350,9 @@ describe('MfaGuard', () => {
</AppContext.Provider>
);

await userEvent.click(screen.getByRole('button', { name: 'Cancel' }));
await userEvent.click(
await screen.findByRole('button', { name: 'Cancel' })
);

expect(mockOnDismiss).toHaveBeenCalledTimes(1);
});
Expand All @@ -317,25 +372,25 @@ describe('MfaGuard', () => {

// Should be debounced! The dialog just rendered and a code went out...
await userEvent.click(
screen.getByRole('button', { name: 'Email new code.' })
await screen.findByRole('button', { name: 'Email new code.' })
);
await act(async () => {
await new Promise((r) => setTimeout(r, 101));
});

await userEvent.click(
screen.getByRole('button', { name: 'Email new code.' })
await screen.findByRole('button', { name: 'Email new code.' })
);
// Should be debounced! The resend request above was just clicked...
await userEvent.click(
screen.getByRole('button', { name: 'Email new code.' })
await screen.findByRole('button', { name: 'Email new code.' })
);

await act(async () => {
await new Promise((r) => setTimeout(r, 101));
});
await userEvent.click(
screen.getByRole('button', { name: 'Email new code.' })
await screen.findByRole('button', { name: 'Email new code.' })
);

expect(mockAuthClient.mfaRequestOtp).toHaveBeenCalledTimes(3);
Expand Down
75 changes: 47 additions & 28 deletions packages/fxa-settings/src/components/Settings/MfaGuard/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ import * as Sentry from '@sentry/react';
import { MfaErrorBoundary } from './error-boundary';
import { getLocalizedErrorMessage } from '../../../lib/error-utils';
import GleanMetrics from '../../../lib/glean';
import { useNavigateWithQuery } from '../../../lib/hooks/useNavigateWithQuery';
import LoadingSpinner from 'fxa-react/components/LoadingSpinner';

/**
* This is a guard component designed to wrap around components that perform
Expand Down Expand Up @@ -58,9 +60,12 @@ export const MfaGuard = ({
const [resendCodeLoading, setResendCodeLoading] = useState(false);
const [showResendSuccessBanner, setShowResendSuccessBanner] = useState(false);

const [modalLoading, setModalLoading] = useState(true);

const resetStates = useCallback(() => {
setLocalizedErrorBannerMessage(undefined);
setShowResendSuccessBanner(false);
setModalLoading(true);
}, []);

// Reactive state: if the store state changes, a re-render is triggered
Expand All @@ -71,6 +76,7 @@ export const MfaGuard = ({
const account = useAccount();
const authClient = useAuthClient();
const navigate = useNavigate();
const navigateWithQuery = useNavigateWithQuery();
const sessionToken = getSessionToken();

const ftlMsgResolver = useFtlMsgResolver();
Expand Down Expand Up @@ -105,7 +111,25 @@ export const MfaGuard = ({
// Modal Setup
useEffect(() => {
(async () => {
// To avoid requesting multiple OTPs on mount
// Ensure the session meets the minimum AAL required
const {
details: { sessionVerificationMeetsMinimumAAL },
} = await authClient.sessionStatus(sessionToken);
if (!sessionVerificationMeetsMinimumAAL) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this logic really belogns in an error boundary. If we inline it in this component, I feel like we will inevtiably start copying this logic into any component that could potentially hit that states. That's alot of extra checks and code to copy around!

I think the better approach is making sure the error response from the server for an insuffecient AAL is consistent (it should be since this is check is now down the auth-schemes), and then respond to it at the error boundary.

console.warn('2FA must be entered to access /settings!');
navigateWithQuery('/signin_totp_code', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When testing locally, I see a flash of the child component before navigation - we should render a LoadingSpinner until we've resolved AAL/JWT checks

state: {
email: account.email,
sessionToken: sessionToken,
uid: account.uid,
verified: false,
isSessionAALUpgrade: true,
},
});
return;
}
setModalLoading(false);

if (JwtTokenCache.hasToken(sessionToken, requiredScope)) {
return;
}
Expand All @@ -122,10 +146,6 @@ export const MfaGuard = ({
await authClient.mfaRequestOtp(sessionToken, requiredScope);
} catch (err) {
MfaOtpRequestCache.remove(sessionToken, requiredScope);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for removing the call to the error boundary? Assuming there's a good reason, we probably want to also clear the MfaOtpRequestCache request cache too so it's consistent with the error-boundaries logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make sense to call the error boundary here, since the MFA error boundary handles invalid jwt and does not handle other authorization errors. (In this case it is not even possible for MfaErrorBoundary to differentiate invalid jwt and AAL mismatch, since they use the same errno INVALID_TOKEN)

if (err.code === 401) {
handleError(err);
return;
}
if (err.code === 429) {
setShowResendSuccessBanner(false);
setLocalizedErrorBannerMessage(
Expand All @@ -150,6 +170,9 @@ export const MfaGuard = ({
onDismiss,
config.mfa.otp.expiresInMinutes,
debounce,
navigateWithQuery,
account.email,
account.uid,
]);

const onSubmitOtp = async (code: string) => {
Expand Down Expand Up @@ -191,11 +214,6 @@ export const MfaGuard = ({
} catch (err) {
MfaOtpRequestCache.remove(sessionToken, requiredScope);
Copy link
Contributor

@dschom dschom Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above about removing call to error boundary.

setShowResendSuccessBanner(false);
if (err.code === 401) {
handleError(err);
return;
}
setShowResendSuccessBanner(false);
setLocalizedErrorBannerMessage(
getLocalizedErrorMessage(ftlMsgResolver, err)
);
Expand All @@ -207,24 +225,25 @@ export const MfaGuard = ({
const email = account.email;
const expirationTime = config.mfa.otp.expiresInMinutes;

const getModal = () => (
<Modal
{...{
email,
expirationTime,
onSubmit: onSubmitOtp,
onDismiss,
handleResendCode,
clearErrorMessage: () => setLocalizedErrorBannerMessage(undefined),
resendCodeLoading,
showResendSuccessBanner,
localizedErrorBannerMessage,
reason,
}}
>
<p>Re-verify Account!</p>
</Modal>
);
const getModal = () =>
modalLoading ? (
<LoadingSpinner fullScreen />
) : (
<Modal
{...{
email,
expirationTime,
onSubmit: onSubmitOtp,
onDismiss,
handleResendCode,
clearErrorMessage: () => setLocalizedErrorBannerMessage(undefined),
resendCodeLoading,
showResendSuccessBanner,
localizedErrorBannerMessage,
reason,
}}
/>
);

// If we don't have a JWT, we need to open the modal to prompt for it.
// Note: I'm torn on whether we should render the child components or not. It seems
Expand Down
16 changes: 14 additions & 2 deletions packages/fxa-settings/src/components/Settings/PageChangePassword/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,18 @@ const mockAuthClient = {
kA: 'kA-key',
kB: 'kB-key',
}),
sessionStatus: jest.fn().mockResolvedValue({
details: {
sessionVerificationMeetsMinimumAAL: true,
},
}),
} as any; // Use 'as any' to avoid TypeScript strict typing for mock

// Mock the cache module to provide session token and JWT cache
const mockSessionToken = 'mock-session-token';
const mockJwtState: Record<string, string> = { [`${mockSessionToken}-password`]: 'mock-jwt-token' };
const mockJwtState: Record<string, string> = {
[`${mockSessionToken}-password`]: 'mock-jwt-token',
};
jest.mock('../../../lib/cache', () => {
const actual = jest.requireActual('../../../lib/cache');
return {
Expand Down Expand Up @@ -102,7 +109,12 @@ const settingsContext = mockSettingsContext();

const render = async (mockAccount = account) => {
renderWithRouter(
<AppContext.Provider value={mockAppContext({ account: mockAccount, authClient: mockAuthClient })}>
<AppContext.Provider
value={mockAppContext({
account: mockAccount,
authClient: mockAuthClient,
})}
>
<SettingsContext.Provider value={settingsContext}>
<PageChangePassword />
</SettingsContext.Provider>
Expand Down
Loading