Skip to content

fix(settings): Nav to signin totp if already enabled #19620

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

Merged
merged 1 commit into from
Nov 28, 2025
+52 −4
Merged

fix(settings): Nav to signin totp if already enabled #19620

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
39 changes: 39 additions & 0 deletions packages/fxa-settings/src/pages/InlineTotpSetup/container.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,45 @@ describe('InlineTotpSetupContainer', () => {
);
});
});

it('does not call createTotp while TOTP status is loading', async () => {
mockTotpStatusQuery.mockImplementation(() => {
return {
data: null,
loading: true,
};
});
jest
.spyOn(ApolloClientModule, 'useQuery')
.mockReturnValue(mockTotpStatusQuery());

render();

await waitFor(() => {
expect(mockCreateTotpMutation).not.toHaveBeenCalled();
});
});

it('does not call createTotp when TOTP is already verified', async () => {
mockSessionHook.mockImplementationOnce(() => ({
isSessionVerified: async () => true,
}));
mockTotpStatusQuery.mockImplementation(() => {
return {
data: MOCK_TOTP_STATUS_VERIFIED,
loading: false,
};
});
jest
.spyOn(ApolloClientModule, 'useQuery')
.mockReturnValue(mockTotpStatusQuery());

render();

await waitFor(() => {
expect(mockCreateTotpMutation).not.toHaveBeenCalled();
});
});
});

describe('renders', () => {
Expand Down
7 changes: 4 additions & 3 deletions packages/fxa-settings/src/pages/InlineTotpSetup/container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,9 @@ export const InlineTotpSetupContainer = ({
useEffect(() => {
if (
totp !== undefined ||
totpStatus?.account?.totp.verified === true ||
isTotpCreating.current
totpStatus?.account?.totp.verified ||
isTotpCreating.current ||
totpStatusLoading
) {
return;
}
Expand Down Expand Up @@ -171,7 +172,7 @@ export const InlineTotpSetupContainer = ({
if (
!isSignedIn ||
!signinState ||
totpStatusLoading === true ||
totpStatusLoading ||
totp === undefined ||
sessionVerified === undefined
) {
Expand Down
3 changes: 3 additions & 0 deletions packages/fxa-settings/src/pages/ResetPassword/ResetPasswordConfirmed/container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ const ResetPasswordConfirmedContainer = ({
error.errno === AuthUiErrors.TOTP_REQUIRED.errno ||
error.errno === AuthUiErrors.INSUFFICIENT_ACR_VALUES.errno
Copy link
Contributor

@MagentaManifold MagentaManifold Nov 19, 2025

Choose a reason for hiding this comment

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

If my understanding is correct (lmk if I'm wrong): TOTP_REQUIRED means TOTP is not enabled (but should be), while INSUFFICIENT_ACR_VALUES means TOTP is enabled, but the user doesn't have a session with this ACR (i.e., AAL2). So we can distinguish the two cases just by handling these two errnos separately, saving us an API call. Ditto for Signin/utils.ts

) {
// in most cases, this error will be returned because the account doesn't have TOTP enabled
// in the case where the account has TOTP enabled, it will be redirected to signin_totp_code
// from inline_totp_setup
navigateWithQuery(`/inline_totp_setup`, {
state: {
email,
Expand Down
7 changes: 6 additions & 1 deletion packages/fxa-settings/src/pages/Signin/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,8 +525,13 @@ const getOAuthNavigationTarget = async (
error.errno === AuthUiErrors.INSUFFICIENT_ACR_VALUES.errno
) {
GleanMetrics.login.error({ event: { reason: error.message } });
// If user already has TOTP enabled, send them to enter their code instead of setup
const hasTotp =
locationState.verificationMethod === VerificationMethods.TOTP_2FA;
return {
to: `/inline_totp_setup${navigationOptions.queryParams || ''}`,
to: hasTotp
? `/signin_totp_code${navigationOptions.queryParams || ''}`
: `/inline_totp_setup${navigationOptions.queryParams || ''}`,
locationState,
};
}
Expand Down