feat(mfa): create new ModalMfaProtected component by MagentaManifold · Pull Request #19362 · mozilla/fxa

MagentaManifold · 2025-08-27T16:57:09Z

Closes #

Because

we need a modal for MFA confirmation code input for MfaProtectedActionGuard

This pull request

creates a new ModalMfaProtected component, with stories and unit tests.

Issue that this pull request solves

Closes: FXA-12302

Checklist

Put an x in the boxes that apply

My commit is GPG signed.
If applicable, I have modified or added tests which pass locally.
I have added necessary documentation (if appropriate).
I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Other information (Optional)

Any other information that is important to this pull request.

bcolsson · 2025-08-27T23:37:59Z

+modal-mfa-protected-title = Enter confirmation code
+modal-mfa-protected-subtitle = to change your { -product-mozilla-account } info


These two strings need to either be a single string (similar to confirm-signup-code-heading-2) or we should adjust the strings to make sure the title and subtitle stand alone (e.g. something like Enter confirmation code Enter code to change your { -product-mozilla-account } info).

While we've been doing the former, I think we should consider avoiding this design going forward.

Some languages end up inverting the two sections with the span element coming first (following their language word order) so you don't have consistency across languages.

This structure no longer works for strings in the CMS since they can't be combined (and thus become very difficult to translate naturally), so we've had to adjust those already.

Hi @bcolsson! I have updated the strings, now both the main title and subtitle stand alone. Our content designer Jeff wonder if it's necessary to update existing designs to avoid splitting sentences as well.

@MagentaManifold - Missed this question: I wouldn't call it a high priority for the strings currently localized but I'd consider updating them at some point since we're already changing them elsewhere (such as in the CMS strings).

MagentaManifold · 2025-08-28T15:05:18Z

The Figma design does not include error states, and my current assumptions on error messages are:

When clicking on an MFA protected button, if we fail to request a new email OTP code due to rate limiting or some server error, then the modal does not show up, and an alert bar describing the error is shown at the top of the page (will be handled by MFAProtectedActionGuard, not the modal UI component).
An error banner shows up inside the modal if we fail to resend code or fail to verify the code due to rate limiting or some server error.
An error tooltip shows up on the input if the code submitted is invalid or expired.
A success banner shows up inside the modal if the confirmation code is successfully resent.

It might also make sense to close the modal and show an alert bar when encountering a server error, and the success banner can also be an alert bar, so I want the reviewer's opinion on this!

dschom · 2025-08-28T16:07:46Z

  const onClick = useCallback(
    (ev: React.MouseEvent) => {
-      ev.preventDefault();
+      ev.stopPropagation();


Did you mean to change this? If so let's put in a a separate 'polish' PR.

Yeah because preventDefault doesn't actually work. stopPropagation is a hack (used in our actual code as well) needed to prevent the modal from disappearing due to losing focus. And yeah I can put it in a separate PR

Thanks for fixing this. That makes sense.

dschom · 2025-08-28T16:19:46Z

+        >
+          <p id="modal-mfa-protected-desc" className="my-6">
+            Enter the code that was sent to{' '}
+            <span className="font-bold">{email}</span> within 5 minutes. This


We should make this configurable. Please file a follow up for this since it involves messing with the app's configuration. For this PR however, pass the time limit as a prop.

Yeah I should have thought about it, cuz I literally just worked that email template

dschom · 2025-08-28T16:59:45Z

The Figma design does not include error states, and my current assumptions on error messages are:

* When clicking on an MFA protected button, if we fail to request a new email OTP code due to rate limiting or some server error, then the modal does not show up, and an alert bar describing the error is shown at the top of the page (will be handled by `MFAProtectedActionGuard`, not the modal UI component).

I'd say this is correct assumption.

* An error **banner** shows up inside the modal if we fail to resend code or fail to verify the code due to rate limiting or some server error.

I think closing the modal, and just using the aforementioned error banner is fine. Basically if the error is unexpected / atypical then using the generic error banner seems fine to me.

* An error **tooltip** shows up on the input if the code submitted is invalid or expired.

Yep seems consistent with other code entry user experiences. This is an expected / typical error state, so I think handling it like this makes sense.

* A success banner shows up inside the modal if the confirmation code is successfully resent.
It might also make sense to close the modal and show an alert bar when encountering a server error, and the success banner can also be an alert bar, so I want the reviewer's opinion on this!

I think in the case of an unexpected server error, closing the modal and showing the error banner makes sense. In the case of the success state, it seems a bit trickier. Like you are saying, most of the flows in settings just use the success alert banner like this:

However, I have a feeling this would feel off since we basically are just continuing the flow... I couldn't find an existing example of what to do here. The verified session guard appears to just send the user on to the next step without any success message, so for now let's leave out the success message. I think it'll be obvious enough that user has provided the code when they are directed to the next step. At least that's my opinion. Once this is available on stage, it might be good to ping UX about it. These sorts of things are easier to decide upon when seeing them in action.

dschom · 2025-08-28T17:06:31Z

+
+    await userEvent.click(screen.getByTestId('modal-dismiss'));
+
+    expect(onDismiss).toHaveBeenCalled();


Nothing to do here, but just a call out. I think the component that invokes this model should display a banner alerting the user that in order to proceed they must provide verification. Again, maybe ping UX on what message to display on dismissal if unsure.

dschom

Thanks for the updates! R+WC

Make the time frame a prop and address the l10n feedback.
Address the l10n feedback

MagentaManifold · 2025-08-28T17:30:36Z

@dschom Thanks for the review! By "success banner" I mean for successfully resending the code, not for successful verification (I agree that we don't need a success message for verification since continuing is enough). I am using a ready-made resend success banner component (ResendCodeSuccessBanner, used in several other pages as well). I guess it makes sense to keep the consistency

bcolsson · 2025-09-02T16:37:47Z

+modal-mfa-protected-instruction =
+  Enter the code that was sent to <email>{ $email }</email> within { $expirationTime ->
+    [one] 1 minute
+    *[other] { $expirationTime } minutes
+  }.


Suggested change

modal-mfa-protected-instruction =

Enter the code that was sent to <email>{ $email }</email> within { $expirationTime ->

[one] 1 minute

*[other] { $expirationTime } minutes

}.

modal-mfa-protected-instruction =

{ $expirationTime ->

[one] Enter the code that was sent to <email>{ $email }</email> within { $expirationTime } minute.

*[other] Enter the code that was sent to <email>{ $email }</email> within { $expirationTime } minutes.

}

While technically valid syntax, it's best practice to have the whole string for each variant of the selector. (This is better supported by Pontoon and easier to localize.)

bcolsson · 2025-09-02T19:34:29Z

+#   email (String) - the user's email
+#   expirationTime (Number) - the expiration time in minutes
+modal-mfa-protected-instruction = { $expirationTime ->
+    [one] Enter the code that was sent to <email>{ $email }</email> within 1 minute.


Suggested change

[one] Enter the code that was sent to <email>{ $email }</email> within 1 minute.

[one] Enter the code that was sent to <email>{ $email }</email> within { $expirationTime } minute.

Minor nit. Feel free to merge after fixing.

Because: * we need a modal for MFA confirmation code input for `MfaProtectedActionGuard` This commit: * creates a new ModalMfaProtected, with stories and unit tests. Closes #FXA-12302

MagentaManifold force-pushed the FXA-12302 branch from 4be0a3f to e5ed905 Compare August 27, 2025 16:58

MagentaManifold marked this pull request as ready for review August 27, 2025 17:46

MagentaManifold requested review from a team as code owners August 27, 2025 17:46

bcolsson requested changes Aug 27, 2025

View reviewed changes

dschom reviewed Aug 28, 2025

View reviewed changes

Comment thread packages/fxa-settings/src/components/Settings/ModalMfaProtected/index.tsx

dschom reviewed Aug 28, 2025

View reviewed changes

dschom approved these changes Aug 28, 2025

View reviewed changes

MagentaManifold force-pushed the FXA-12302 branch 2 times, most recently from 27a50ce to dfc9720 Compare August 29, 2025 18:41

MagentaManifold requested a review from bcolsson August 29, 2025 19:10

dschom mentioned this pull request Sep 2, 2025

Create MFA Guard that Manages JWTs #19383

Merged

4 tasks

bcolsson requested changes Sep 2, 2025

View reviewed changes

MagentaManifold force-pushed the FXA-12302 branch from dfc9720 to 8fab2a9 Compare September 2, 2025 18:03

MagentaManifold requested a review from bcolsson September 2, 2025 18:05

bcolsson approved these changes Sep 2, 2025

View reviewed changes

feat(mfa): create new ModalMfaProtected component

2961ab2

Because: * we need a modal for MFA confirmation code input for `MfaProtectedActionGuard` This commit: * creates a new ModalMfaProtected, with stories and unit tests. Closes #FXA-12302

MagentaManifold force-pushed the FXA-12302 branch from 8fab2a9 to 2961ab2 Compare September 2, 2025 19:43

MagentaManifold merged commit 3356b39 into main Sep 2, 2025
19 checks passed

MagentaManifold deleted the FXA-12302 branch September 2, 2025 20:25

		modal-mfa-protected-title = Enter confirmation code
		modal-mfa-protected-subtitle = to change your { -product-mozilla-account } info


		await userEvent.click(screen.getByTestId('modal-dismiss'));

		expect(onDismiss).toHaveBeenCalled();

	[one] Enter the code that was sent to <email>{ $email }</email> within 1 minute.
	[one] Enter the code that was sent to <email>{ $email }</email> within { $expirationTime } minute.

Conversation

MagentaManifold commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Because

This pull request

Issue that this pull request solves

Checklist

Screenshots (Optional)

Other information (Optional)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MagentaManifold commented Aug 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dschom Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dschom commented Aug 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dschom left a comment

Choose a reason for hiding this comment

Uh oh!

MagentaManifold commented Aug 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MagentaManifold commented Aug 27, 2025 •

edited

Loading

dschom Aug 28, 2025 •

edited

Loading