Skip to content

task(settings): Wrap disable 2FA with MFA guard #19444

Merged
dschom merged 1 commit intomainfrom
FXA-12230
Sep 23, 2025
Merged

task(settings): Wrap disable 2FA with MFA guard #19444
dschom merged 1 commit intomainfrom
FXA-12230

Conversation

@dschom
Copy link
Copy Markdown
Contributor

@dschom dschom commented Sep 12, 2025 •
edited
Loading

Because

  • We want to require MFA when disabling 2FA

This pull request

  • Wraps the 2FA row's disable button with an MfaGuard component
  • Updates functional tests to supply MfaGuard's OTP code when required

Issue that this pull request solves

Closes: FXA-12230

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)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

A lot of tests 'disable 2FA' as part of the test tear down.

@dschom dschom force-pushed the FXA-12230 branch 3 times, most recently from c90be14 to 5a50713 Compare September 15, 2025 16:14
dschom
dschom commented Sep 15, 2025
View reviewed changes
Comment thread packages/fxa-settings/src/components/Settings/UnitRowTwoStepAuth/index.tsx
)}
{disable2FAModalRevealed && (
<DisableTwoStepAuthModal {...{ hideDisable2FAModal }} />
<MfaGuard
Copy link
Copy Markdown
Contributor Author

@dschom dschom Sep 15, 2025

Choose a reason for hiding this comment

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

@nshirley This seems to be all that's needed to wrap a button. Note that in this scenario the onDismissCallback is needed. This was recently added in #19443.

@dschom dschom force-pushed the FXA-12230 branch 3 times, most recently from df95652 to a175bc1 Compare September 15, 2025 17:20
dschom
dschom commented Sep 15, 2025
View reviewed changes
Comment thread packages/fxa-auth-server/lib/routes/totp.js
},
handler: async function (request) {
return routes
.find(
Copy link
Copy Markdown
Contributor Author

@dschom dschom Sep 15, 2025

Choose a reason for hiding this comment

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

@MagentaManifold @nshirley @vpomerleau I like this approach for the simple reason that it doesn't corrupt the git history of the previous route handler and makes the diff nicer.

Copy link
Copy Markdown
Contributor

@nshirley nshirley Sep 15, 2025

Choose a reason for hiding this comment

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

Oh, neat! 🤯

Copy link
Copy Markdown
Contributor

@vbudhram vbudhram Sep 18, 2025

Choose a reason for hiding this comment

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

Nice!

nshirley
nshirley reviewed Sep 15, 2025
View reviewed changes
Comment thread packages/fxa-settings/src/components/Settings/UnitRowTwoStepAuth/index.test.tsx Outdated
@dschom dschom force-pushed the FXA-12230 branch 2 times, most recently from a95cc14 to 4b24f50 Compare September 15, 2025 18:39
@dschom dschom changed the title WIP task(settings): Wrap disable 2FA with MFA guard Sep 15, 2025
@dschom dschom marked this pull request as ready for review September 15, 2025 18:41
@dschom dschom requested review from a team as code owners September 15, 2025 18:41
bcolsson
bcolsson requested changes Sep 16, 2025
View reviewed changes
Comment thread packages/fxa-settings/src/components/Settings/UnitRowTwoStepAuth/en.ftl Outdated
@dschom dschom force-pushed the FXA-12230 branch 2 times, most recently from 8b462c8 to 0244424 Compare September 17, 2025 17:21
@dschom dschom requested a review from bcolsson September 17, 2025 17:21
@dschom dschom mentioned this pull request Sep 17, 2025
Merged
4 tasks
@dschom dschom requested a review from nshirley September 17, 2025 23:29
vbudhram
vbudhram approved these changes Sep 18, 2025
View reviewed changes
Comment thread packages/functional-tests/tests/oauth/totp.spec.ts Outdated
Comment thread packages/fxa-auth-server/docs/swagger/totp-api.ts Outdated
Comment thread packages/fxa-auth-server/lib/routes/totp.js
},
handler: async function (request) {
return routes
.find(
Copy link
Copy Markdown
Contributor

@vbudhram vbudhram Sep 18, 2025

Choose a reason for hiding this comment

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

Nice!

nshirley
nshirley previously requested changes Sep 18, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@nshirley nshirley left a comment •
edited
Loading

Choose a reason for hiding this comment

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

So, comments here are non-blocking.

But, I grabbed the branch and hit a but. The Change button is also prompting with the MfaGuard. And, if the token is expired (to where it should re-prompt) I just get an error 401. Looks like it's just not hitting the error boundary there. Maybe it's not intentional to have the change triggering this in this PR?

image

If it turns out to be a "me" issue then I'll update my review! Wanted to flag in case it is something actually broken and unintentional

Comment thread packages/functional-tests/pages/settings/index.ts
Comment thread packages/functional-tests/pages/settings/index.ts
Comment thread packages/functional-tests/tests/oauth/totp.spec.ts Outdated
Comment thread packages/fxa-settings/src/components/Settings/UnitRowTwoStepAuth/index.tsx Outdated
@nshirley nshirley dismissed their stale review September 18, 2025 21:03

I realized that the change was done in this pr so what I'm seeing is likely not from your pr

@dschom
task(settings): Wrap disable 2FA with MFA guard
746329d 
Because:
- We want to require MFA when disabling 2FA

This Commit:
- Wraps the 2FA row's disable button with an MfaGuard component
- Updates functional tests to supply MfaGuard's OTP code when required
@dschom dschom merged commit bae1e9c into main Sep 23, 2025
19 checks passed
@dschom dschom deleted the FXA-12230 branch September 23, 2025 16:08
@dschom dschom mentioned this pull request Sep 23, 2025
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vbudhram vbudhram vbudhram approved these changes

@bcolsson bcolsson bcolsson approved these changes

@nshirley nshirley nshirley left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dschom @vbudhram @nshirley @bcolsson