feat(fxa-settings): Implement using recovery phone to sign in by vpomerleau · Pull Request #18255 · mozilla/fxa

vpomerleau · 2025-01-20T22:43:30Z

Because

New feature: recovery phone as recovery method for 2FA during sign in

This pull request

Hook up new pages to choose recovery method and use recovery phone during sign in

Issue that this pull request solves

Closes: #FXA-10374

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)

Notes for testing - options to manually test the flow until #18276 is merged: either manually add a recovery phone to an existing account with 2FA via mysql or cherry pick the latest commit for PR-18276 to use UI to add a phone number
generated codes can be retrieved with redis-commander (or cherry pick #18277 to use inbox logs)

dschom · 2025-01-23T22:07:04Z

 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

 import { ConfigService } from '@nestjs/config';
-import { SmsManagerConfig } from './sms.manger.config';


dschom · 2025-01-23T22:19:46Z

+});
+
+const mockAuthClient = new AuthClient('http://localhost:9000', {
+  keyStretchVersion: 1,


We should use version 2

Guilty of copy pasta from another test file - @dschom should this be updated to v2 in all auth client mocks?

vbudhram

@vpomerleau Took a first pass at this. Gonna do some manual testing now

vbudhram · 2025-01-27T21:15:37Z

+      user.type(screen.getByRole('textbox'), MOCK_RECOVERY_CODE)
+    );
+
+    expect(screen.getByRole('button', { name: /Confirm/i })).toHaveAttribute(


vbudhram · 2025-01-28T17:56:07Z

+      return;
+    }
+    try {
+      await authClient.recoveryPhoneSigninSendCode(signinState.sessionToken);


FWIW, it seems odd to me to send the code from this page. I wonder if the recovery phone page should handle sending codes instead. Don't need to block on this though.

I don't disagree, but we'd have to review UX for error handling in that scenario because the designs currently have us showing an error on the choice screen if send fails for the regular path to recovery phone.

I'll file a follow up ticket to review if we should send the code before or after navigation.

bcolsson · 2025-01-28T19:16:51Z

-signin-recovery-code-instruction-v2 = Enter one of the one-time use backup authentication codes you saved during two-step authentication setup.
-signin-recovery-code-input-label-v2 = Enter 10-character code
+# codes here refers to backup authentication codes
+signin-recovery-code-instruction-v3 = Enter one of the one-time-use codes you saved when you set up two-step authentication.


Suggested change

signin-recovery-code-instruction-v3 = Enter one of the one-time-use codes you saved when you set up two-step authentication.

signin-recovery-code-instruction-v3 = Enter one of the one-time use codes you saved when you set up two-step authentication.

Nit, but aligns with previous string in terms of hyphenation.

Content reviewed and recommended "one-time-use" codes, so this is an intentional update :)

vbudhram

@vpomerleau Manual testing looks good! I think this will be ok to merge, I can still do some fixes while working on #18287.

🚀

LZoog · 2025-01-28T22:22:24Z

        'force_auth',
        'signin_recovery_code',
+        'signin_recovery_choice',
+        'signin_recovery_phone',


You probably already know this but just to be clear / for others too: we only need to add routes to content-server this way if it's not behind /settings/* - content-server already points all settings routes to our React app if users hit them directly. Otherwise, without these content-server modifications, the server doesn't know what to do on a refresh of that page and it ends up blank, so this is probably preferred for routes not behind settings.

(I wanted to think that through myself too, so, 👍)

Thank you, I wasn't 💯 sure on this so appreciate the confirmation!

LZoog · 2025-01-28T22:29:10Z

+  }),
+  mockRecoveryPhoneGet = jest.fn().mockResolvedValue({
+    exists: true,
+    phoneNumber: '7890',


Right now this will return MOCK_MASKED_PHONE_NUMBER FWIW, and not just the last 4, if the session isn't verified. I need to fill out details for FXA-11044.

Thank you, I'll update to test that the masked number is handled properly!

LZoog · 2025-01-28T22:32:40Z

+  const [loading, setLoading] = useState(true);
+
+  useEffect(() => {
+    if (!signinState || !signinState.sessionToken) {


Is sessionToken already in local storage at this point? Probably not here since I don't want you to have to refactor this all but I think we might as well just pull it from local storage when we can. (I'm not sure we should have ever passed it as part of signin state heh)

LZoog · 2025-01-28T22:37:01Z

+          signinState.sessionToken
+        );
+        // TODO verify that recoveryPhoneGet returns a masked phone number (last four digits only)
+        phoneNumber && setLastFourPhoneDigits(phoneNumber.slice(-4));


You might could create a little helper function for this:

fxa/packages/fxa-settings/src/components/Settings/SubRow/index.tsx

Lines 240 to 241 in 9955924

{phoneNumber.includes('•')

? phoneNumber

See:

fxa/libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts

Lines 192 to 193 in 9955924

* e.g. If the phone number was +15005551234 and phoneNumberMask was 4, the result would be +*******1234.

*/

fxa/libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts

Lines 259 to 260 in 9955924

const maskedPhoneNumber = phoneNumber

.substring(phoneNumber.length - lastN)

Would be nice to be more consistent. Could we clean this up in the ticket you filled for the twilio national number format?

I added a note in FXA-11044

LZoog · 2025-01-28T22:40:22Z

+          return;
+        }
+
+        if (phoneNumber && (!count || count === 0)) {


Is the type of this null | number? Not super nice that any of these returned values from authClient are any 😅

LZoog · 2025-01-28T22:42:14Z

+  }, [authClient, lastFourPhoneDigits, signinState, navigateWithQuery]);
+
+  const handlePhoneChoice = async () => {
+    if (!signinState) {


If you want to use the session token from signin state, you could curry this function with it so you don't have to recheck if (!signinState) {, but maybe later we should just pull from local storage instead and curry that.

I added a note in FXA-9875 about reviewing what's included in signinState (vs local storage)

LZoog · 2025-01-28T22:45:05Z

+  }
+
+  if (!signinState || !lastFourPhoneDigits) {
+    return <LoadingSpinner fullScreen />;


Is this where the redirect should happen instead of useEffect?

LZoog · 2025-01-28T22:50:17Z

+      user.type(screen.getByRole('textbox'), MOCK_RECOVERY_CODE)
+    );
+
+    expect(screen.getByRole('button', { name: /Confirm/i })).toHaveAttribute(


LZoog · 2025-01-28T22:53:05Z

@@ -0,0 +1,99 @@
+import React from 'react';


nit: missing license

LZoog · 2025-01-28T22:55:03Z

+    React.useState(false);
+  const ftlMsgResolver = useFtlMsgResolver();
+
+  const maskedPhoneNumber = `+1-•••-${lastFourPhoneDigits}`;


You may want to update this to just the dots, for consistency across add/delete inside Settings. In that new ticket I referenced, I think we can iron this out with that national_format from Twilio if we want these formatted properly.

Because: * New feature: recovery phone as recovery method for 2FA during sign in This commit: * Hook up new pages to choose recovery method and use recovery phone during sign in Closes #FXA-10374

vpomerleau force-pushed the FXA-10374 branch from b04a003 to 57726f2 Compare January 20, 2025 23:26

vpomerleau commented Jan 21, 2025

View reviewed changes

Comment thread libs/accounts/recovery-phone/src/lib/sms.manager.ts Outdated

vpomerleau force-pushed the FXA-10374 branch from 57726f2 to e1f39d8 Compare January 21, 2025 22:11

vpomerleau commented Jan 21, 2025

View reviewed changes

Comment thread packages/fxa-auth-client/lib/client.ts

vpomerleau force-pushed the FXA-10374 branch 6 times, most recently from abe0538 to 406b221 Compare January 23, 2025 21:34

dschom reviewed Jan 23, 2025

View reviewed changes

vpomerleau force-pushed the FXA-10374 branch from 406b221 to fdf0343 Compare January 23, 2025 22:10

dschom reviewed Jan 23, 2025

View reviewed changes

Comment thread packages/fxa-content-server/app/scripts/lib/router.js Outdated

dschom reviewed Jan 23, 2025

View reviewed changes

Comment thread packages/fxa-settings/src/pages/Signin/SigninRecoveryChoice/container.test.tsx Outdated

dschom reviewed Jan 24, 2025

View reviewed changes

Comment thread packages/fxa-settings/src/pages/Signin/SigninRecoveryChoice/container.tsx Outdated

vpomerleau force-pushed the FXA-10374 branch 4 times, most recently from 1e644ab to a08013f Compare January 27, 2025 17:35

vpomerleau changed the title ~~WIP feat(fxa-settings): Implement using recovery phone to sign in~~ feat(fxa-settings): Implement using recovery phone to sign in Jan 27, 2025

vpomerleau marked this pull request as ready for review January 27, 2025 17:42

vpomerleau requested review from a team as code owners January 27, 2025 17:42

vpomerleau force-pushed the FXA-10374 branch from a08013f to 8d38583 Compare January 27, 2025 18:15

vbudhram reviewed Jan 28, 2025

View reviewed changes

bcolsson approved these changes Jan 28, 2025

View reviewed changes

bcolsson reviewed Jan 28, 2025

View reviewed changes

vpomerleau force-pushed the FXA-10374 branch from 8d38583 to f5f76b0 Compare January 28, 2025 20:22

vbudhram approved these changes Jan 28, 2025

View reviewed changes

LZoog reviewed Jan 28, 2025

View reviewed changes

feat(fxa-settings): Implement using recovery phone to sign in

9b0fbc1

Because: * New feature: recovery phone as recovery method for 2FA during sign in This commit: * Hook up new pages to choose recovery method and use recovery phone during sign in Closes #FXA-10374

vpomerleau force-pushed the FXA-10374 branch from dd238b0 to 9b0fbc1 Compare January 28, 2025 23:23

vpomerleau merged commit 153efdd into main Jan 28, 2025

vpomerleau deleted the FXA-10374 branch January 28, 2025 23:55

	signin-recovery-code-instruction-v3 = Enter one of the one-time-use codes you saved when you set up two-step authentication.
	signin-recovery-code-instruction-v3 = Enter one of the one-time use codes you saved when you set up two-step authentication.

	* e.g. If the phone number was +15005551234 and phoneNumberMask was 4, the result would be +*******1234.
	*/

	const maskedPhoneNumber = phoneNumber
	.substring(phoneNumber.length - lastN)

Conversation

vpomerleau commented Jan 20, 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dschom Jan 23, 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!

Uh oh!

vbudhram left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbudhram left a comment

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!

Uh oh!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vpomerleau commented Jan 20, 2025 •

edited

Loading

dschom Jan 23, 2025 •

edited

Loading