From 9e1aac9bca8b3e97f7a5e5e5e5f0aa225216fca3 Mon Sep 17 00:00:00 2001 From: MagentaManifold <17zhaomingyuan@gmail.com> Date: Fri, 24 Oct 2025 18:54:40 -0400 Subject: [PATCH] fix(settings): redirect user on AAL mismatch in mfa guard Because: * General Application Error when interacting with site after 2FA enabled This commit: * redirects user on AAL mismatch in mfa guard Closes #FXA-12576 --- .../Settings/MfaGuard/index.test.tsx | 91 +++++++++++++++---- .../components/Settings/MfaGuard/index.tsx | 75 +++++++++------ .../PageChangePassword/index.test.tsx | 16 +++- .../PageSecondaryEmailAdd/index.test.tsx | 49 +++++++--- .../PageSecondaryEmailVerify/index.test.tsx | 59 +++++++++--- .../UnitRowRecoveryKey/index.test.tsx | 8 +- .../UnitRowSecondaryEmail/index.test.tsx | 3 + .../UnitRowTwoStepAuth/index.test.tsx | 3 + 8 files changed, 227 insertions(+), 77 deletions(-) diff --git a/packages/fxa-settings/src/components/Settings/MfaGuard/index.test.tsx b/packages/fxa-settings/src/components/Settings/MfaGuard/index.test.tsx index a2cccfa48c2..0348941c3c5 100644 --- a/packages/fxa-settings/src/components/Settings/MfaGuard/index.test.tsx +++ b/packages/fxa-settings/src/components/Settings/MfaGuard/index.test.tsx @@ -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'); @@ -43,6 +47,10 @@ jest.mock('../../../models', () => ({ useAlertBar: () => mockAlertBar, })); +jest.mock('../../../lib/hooks/useNavigateWithQuery', () => ({ + useNavigateWithQuery: () => mockNavigateWithQuery, +})); + jest.mock('@reach/router', () => ({ ...jest.requireActual('@reach/router'), useNavigate: () => mockNavigate, @@ -50,7 +58,7 @@ jest.mock('@reach/router', () => ({ 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' })); @@ -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 () => { @@ -72,10 +84,12 @@ describe('MfaGuard', () => { ); - expect(mockAuthClient.mfaRequestOtp).toHaveBeenCalledWith( - mockSessionToken, - mockScope - ); + await waitFor(() => { + expect(mockAuthClient.mfaRequestOtp).toHaveBeenCalledWith( + mockSessionToken, + mockScope + ); + }); expect( await screen.findByText('Enter confirmation code') @@ -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( @@ -121,7 +135,7 @@ describe('MfaGuard', () => { ); - expect(screen.getByText('secured')).toBeInTheDocument(); + expect(await screen.findByText('secured')).toBeInTheDocument(); expect( screen.queryByText('Enter confirmation code') ).not.toBeInTheDocument(); @@ -160,7 +174,9 @@ describe('MfaGuard', () => { ); - expect(screen.queryByText('Enter confirmation code')).toBeInTheDocument(); + expect( + await screen.findByText('Enter confirmation code') + ).toBeInTheDocument(); await submitCode('654321'); expect( @@ -181,7 +197,9 @@ describe('MfaGuard', () => { ); - 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') @@ -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' })); @@ -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.') @@ -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.') @@ -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( + + +
secured
+ + + ); + + 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 @@ -297,7 +350,9 @@ describe('MfaGuard', () => { ); - await userEvent.click(screen.getByRole('button', { name: 'Cancel' })); + await userEvent.click( + await screen.findByRole('button', { name: 'Cancel' }) + ); expect(mockOnDismiss).toHaveBeenCalledTimes(1); }); @@ -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); diff --git a/packages/fxa-settings/src/components/Settings/MfaGuard/index.tsx b/packages/fxa-settings/src/components/Settings/MfaGuard/index.tsx index 60c15b0210b..32961338f66 100644 --- a/packages/fxa-settings/src/components/Settings/MfaGuard/index.tsx +++ b/packages/fxa-settings/src/components/Settings/MfaGuard/index.tsx @@ -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 @@ -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 @@ -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(); @@ -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) { + console.warn('2FA must be entered to access /settings!'); + navigateWithQuery('/signin_totp_code', { + state: { + email: account.email, + sessionToken: sessionToken, + uid: account.uid, + verified: false, + isSessionAALUpgrade: true, + }, + }); + return; + } + setModalLoading(false); + if (JwtTokenCache.hasToken(sessionToken, requiredScope)) { return; } @@ -122,10 +146,6 @@ export const MfaGuard = ({ await authClient.mfaRequestOtp(sessionToken, requiredScope); } catch (err) { MfaOtpRequestCache.remove(sessionToken, requiredScope); - if (err.code === 401) { - handleError(err); - return; - } if (err.code === 429) { setShowResendSuccessBanner(false); setLocalizedErrorBannerMessage( @@ -150,6 +170,9 @@ export const MfaGuard = ({ onDismiss, config.mfa.otp.expiresInMinutes, debounce, + navigateWithQuery, + account.email, + account.uid, ]); const onSubmitOtp = async (code: string) => { @@ -191,11 +214,6 @@ export const MfaGuard = ({ } catch (err) { MfaOtpRequestCache.remove(sessionToken, requiredScope); setShowResendSuccessBanner(false); - if (err.code === 401) { - handleError(err); - return; - } - setShowResendSuccessBanner(false); setLocalizedErrorBannerMessage( getLocalizedErrorMessage(ftlMsgResolver, err) ); @@ -207,24 +225,25 @@ export const MfaGuard = ({ const email = account.email; const expirationTime = config.mfa.otp.expiresInMinutes; - const getModal = () => ( - setLocalizedErrorBannerMessage(undefined), - resendCodeLoading, - showResendSuccessBanner, - localizedErrorBannerMessage, - reason, - }} - > -

Re-verify Account!

- - ); + const getModal = () => + modalLoading ? ( + + ) : ( + 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 diff --git a/packages/fxa-settings/src/components/Settings/PageChangePassword/index.test.tsx b/packages/fxa-settings/src/components/Settings/PageChangePassword/index.test.tsx index 96830ee8296..bb02a158a7a 100644 --- a/packages/fxa-settings/src/components/Settings/PageChangePassword/index.test.tsx +++ b/packages/fxa-settings/src/components/Settings/PageChangePassword/index.test.tsx @@ -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 = { [`${mockSessionToken}-password`]: 'mock-jwt-token' }; +const mockJwtState: Record = { + [`${mockSessionToken}-password`]: 'mock-jwt-token', +}; jest.mock('../../../lib/cache', () => { const actual = jest.requireActual('../../../lib/cache'); return { @@ -102,7 +109,12 @@ const settingsContext = mockSettingsContext(); const render = async (mockAccount = account) => { renderWithRouter( - + diff --git a/packages/fxa-settings/src/components/Settings/PageSecondaryEmailAdd/index.test.tsx b/packages/fxa-settings/src/components/Settings/PageSecondaryEmailAdd/index.test.tsx index 1dbbdc01695..aaf5bab6ca4 100644 --- a/packages/fxa-settings/src/components/Settings/PageSecondaryEmailAdd/index.test.tsx +++ b/packages/fxa-settings/src/components/Settings/PageSecondaryEmailAdd/index.test.tsx @@ -190,21 +190,30 @@ describe('PageSecondaryEmailAdd', () => { }); }); - describe('MfaGuard', () => { + describe('MfaGuard', () => { const mockEmail = 'user@example.com'; const mockAuthClient = new AuthClient('http://localhost:9000'); let user: UserEvent; const setupMockAuthClient = () => { - mockAuthClient.mfaRequestOtp = jest.fn().mockResolvedValueOnce({ code: 200, errno: 0 }); - mockAuthClient.mfaOtpVerify = jest.fn().mockResolvedValueOnce({ accessToken: mockJwt }); - } + mockAuthClient.mfaRequestOtp = jest + .fn() + .mockResolvedValueOnce({ code: 200, errno: 0 }); + mockAuthClient.mfaOtpVerify = jest + .fn() + .mockResolvedValueOnce({ accessToken: mockJwt }); + mockAuthClient.sessionStatus = jest.fn().mockResolvedValue({ + details: { + sessionVerificationMeetsMinimumAAL: true, + }, + }); + }; const resetJwtCache = () => { if (JwtTokenCache.hasToken(mockSessionToken, requiredScope)) { JwtTokenCache.removeToken(mockSessionToken, requiredScope); } - } + }; beforeEach(() => { jest.clearAllMocks(); @@ -222,16 +231,20 @@ describe('PageSecondaryEmailAdd', () => { renderWithRouter( - + ); expect(screen.getByTestId('secondary-email-input')).toBeInTheDocument(); - expect(screen.queryByText('Enter confirmation code')).not.toBeInTheDocument(); + expect( + screen.queryByText('Enter confirmation code') + ).not.toBeInTheDocument(); expect(mockAuthClient.mfaRequestOtp).not.toHaveBeenCalled(); }); - it('renders MFA guard if JWT does not exist', () => { + it('renders MFA guard if JWT does not exist', async () => { const appCtx = mockAppContext({ authClient: mockAuthClient as any, account: { ...(account as any), email: mockEmail }, @@ -239,18 +252,24 @@ describe('PageSecondaryEmailAdd', () => { renderWithRouter( - + ); - expect(screen.getByText('Enter confirmation code')).toBeInTheDocument(); + expect( + await screen.findByText('Enter confirmation code') + ).toBeInTheDocument(); }); it('renders MFA guard if the JWT is invalid', async () => { // an invalid token should be picked up by the guard which will // "send" a new one and display the modal again. JwtTokenCache.setToken(mockSessionToken, requiredScope, 'invalid-jwt'); - account.createSecondaryEmail = jest.fn().mockRejectedValue({ code: 401, errno: 110 }); + account.createSecondaryEmail = jest + .fn() + .mockRejectedValue({ code: 401, errno: 110 }); const appCtx = mockAppContext({ authClient: mockAuthClient as any, account: { ...(account as any), email: mockEmail }, @@ -258,7 +277,9 @@ describe('PageSecondaryEmailAdd', () => { renderWithRouter( - + ); @@ -266,7 +287,9 @@ describe('PageSecondaryEmailAdd', () => { await user.click(screen.getByTestId('save-button')); expect(account.createSecondaryEmail).toHaveBeenCalledWith(mockEmail); - expect(await screen.findByText('Enter confirmation code')).toBeInTheDocument(); + expect( + await screen.findByText('Enter confirmation code') + ).toBeInTheDocument(); }); }); }); diff --git a/packages/fxa-settings/src/components/Settings/PageSecondaryEmailVerify/index.test.tsx b/packages/fxa-settings/src/components/Settings/PageSecondaryEmailVerify/index.test.tsx index 782357b68eb..ecbbce8ea32 100644 --- a/packages/fxa-settings/src/components/Settings/PageSecondaryEmailVerify/index.test.tsx +++ b/packages/fxa-settings/src/components/Settings/PageSecondaryEmailVerify/index.test.tsx @@ -122,15 +122,22 @@ describe('PageSecondaryEmailVerify', () => { let user: UserEvent; const setupMockAuthClient = () => { - mockAuthClient.mfaRequestOtp = jest.fn().mockResolvedValueOnce({ code: 200, errno: 0 }); - mockAuthClient.mfaOtpVerify = jest.fn().mockResolvedValueOnce({ accessToken: mockJwt }); - } + mockAuthClient.mfaRequestOtp = jest + .fn() + .mockResolvedValueOnce({ code: 200, errno: 0 }); + mockAuthClient.mfaOtpVerify = jest + .fn() + .mockResolvedValueOnce({ accessToken: mockJwt }); + mockAuthClient.sessionStatus = jest.fn().mockResolvedValue({ + details: { sessionVerificationMeetsMinimumAAL: true }, + }); + }; const resetJwtCache = () => { if (JwtTokenCache.hasToken(mockSessionToken, requiredScope)) { JwtTokenCache.removeToken(mockSessionToken, requiredScope); } - } + }; beforeEach(() => { jest.clearAllMocks(); @@ -148,16 +155,22 @@ describe('PageSecondaryEmailVerify', () => { renderWithRouter( - + ); - expect(screen.getByTestId('secondary-email-verify-form')).toBeInTheDocument(); - expect(screen.queryByText('Enter confirmation code')).not.toBeInTheDocument(); + expect( + screen.getByTestId('secondary-email-verify-form') + ).toBeInTheDocument(); + expect( + screen.queryByText('Enter confirmation code') + ).not.toBeInTheDocument(); expect(mockAuthClient.mfaRequestOtp).not.toHaveBeenCalled(); }); - it('renders MFA guard if JWT does not exist', () => { + it('renders MFA guard if JWT does not exist', async () => { const appCtx = mockAppContext({ authClient: mockAuthClient as any, account: { ...(account as any), email: mockEmail }, @@ -165,18 +178,24 @@ describe('PageSecondaryEmailVerify', () => { renderWithRouter( - + ); - expect(screen.getByText('Enter confirmation code')).toBeInTheDocument(); + expect( + await screen.findByText('Enter confirmation code') + ).toBeInTheDocument(); }); it('renders MFA guard if the JWT is invalid', async () => { // an invalid token should be picked up by the guard which will // "send" a new one and display the modal again. JwtTokenCache.setToken(mockSessionToken, requiredScope, 'invalid-jwt'); - account.verifySecondaryEmail = jest.fn().mockRejectedValue({ code: 401, errno: 110 }); + account.verifySecondaryEmail = jest + .fn() + .mockRejectedValue({ code: 401, errno: 110 }); const appCtx = mockAppContext({ authClient: mockAuthClient as any, account: { ...(account as any), email: mockEmail }, @@ -184,17 +203,27 @@ describe('PageSecondaryEmailVerify', () => { renderWithRouter( - + ); // mock the submit for the verification code, which should trigger the MFA guard // because the JWT is considered invalid server-side. - await user.type(screen.getByTestId('verification-code-input-field'), '123456'); + await user.type( + screen.getByTestId('verification-code-input-field'), + '123456' + ); await user.click(screen.getByTestId('secondary-email-verify-submit')); - expect(account.verifySecondaryEmail).toHaveBeenCalledWith(mockEmail, '123456'); - expect(await screen.findByText('Enter confirmation code')).toBeInTheDocument(); + expect(account.verifySecondaryEmail).toHaveBeenCalledWith( + mockEmail, + '123456' + ); + expect( + await screen.findByText('Enter confirmation code') + ).toBeInTheDocument(); }); }); }); diff --git a/packages/fxa-settings/src/components/Settings/UnitRowRecoveryKey/index.test.tsx b/packages/fxa-settings/src/components/Settings/UnitRowRecoveryKey/index.test.tsx index d440ac9b946..ab02350717d 100644 --- a/packages/fxa-settings/src/components/Settings/UnitRowRecoveryKey/index.test.tsx +++ b/packages/fxa-settings/src/components/Settings/UnitRowRecoveryKey/index.test.tsx @@ -40,7 +40,13 @@ const accountWithoutPassword = { recoveryKey: { exists: false }, } as unknown as Account; -const authClient = {} as unknown as AuthClient; +const authClient = { + sessionStatus: jest.fn().mockResolvedValue({ + details: { + sessionVerificationMeetsMinimumAAL: true, + }, + }), +} as unknown as AuthClient; const renderWithContext = ( account: Partial, diff --git a/packages/fxa-settings/src/components/Settings/UnitRowSecondaryEmail/index.test.tsx b/packages/fxa-settings/src/components/Settings/UnitRowSecondaryEmail/index.test.tsx index a458d6c45fe..1a2588ffa51 100644 --- a/packages/fxa-settings/src/components/Settings/UnitRowSecondaryEmail/index.test.tsx +++ b/packages/fxa-settings/src/components/Settings/UnitRowSecondaryEmail/index.test.tsx @@ -54,6 +54,9 @@ describe('UnitRowSecondaryEmail', () => { mockAuthClient.mfaOtpVerify = jest .fn() .mockResolvedValue({ accessToken: mockJwt }); + mockAuthClient.sessionStatus = jest.fn().mockResolvedValue({ + details: { sessionVerificationMeetsMinimumAAL: true }, + }); }; const resetJwtCache = () => { diff --git a/packages/fxa-settings/src/components/Settings/UnitRowTwoStepAuth/index.test.tsx b/packages/fxa-settings/src/components/Settings/UnitRowTwoStepAuth/index.test.tsx index 6f8cf9c4f89..37994aa326a 100644 --- a/packages/fxa-settings/src/components/Settings/UnitRowTwoStepAuth/index.test.tsx +++ b/packages/fxa-settings/src/components/Settings/UnitRowTwoStepAuth/index.test.tsx @@ -20,6 +20,9 @@ jest.mock('../../../models', () => ({ ...jest.requireActual('../../../models'), useAuthClient: () => ({ mfaRequestOtp: jest.fn(), + sessionStatus: jest.fn().mockResolvedValue({ + details: { sessionVerificationMeetsMinimumAAL: true }, + }), }), }));