feat(recovery-key): Make inline recovery key flow functional + navigate desktop Sync signins #17629

LZoog · 2024-09-18T00:10:28Z

Because:

We want to encourage Sync signin users to create an account recovery key

This commit:

Moves and refactors part of FlowRecoveryKeyHint into new component for sharing
Sets ups the create recovery key and hint handlers in container component, call where needed
Queries if recovery key exists after signin if sync, stores PW-related data into sensitiveDataClient
Navigates signing in desktop Sync flow users without a recovery key to this new flow before taking them to CAD, if the feature flag is on

Create an account, and then through the Sync flow / fxa-dev-launcher, sign in. If the account doesn't have a recovery key, you'll be taken to the new flow after 2FA/signin token code. Double check me that sync sign in is as expected for all cases, it seems to be from my testing.

At the end of the flow users are then taken to CAD without the "You're signed into Firefox" success messaging. We are making modifications to CAD in another ticket this sprint, so we may need a follow up issue after that to display "Recovery key hint saved" or some similar success messaging on the pair choice screen, but that's TBD and Eduardo gave the thumbs up on this for now.

packages/fxa-settings/src/pages/InlineRecoveryKeySetup/container.tsx

LZoog · 2024-09-18T00:14:13Z

packages/fxa-settings/src/pages/InlineRecoveryKeySetup/container.tsx

+    return <></>;
+  }
+
+  // Curry already checked values


There's a few places in other container components I think we could benefit from doing this in. This avoids having to pass these values into InlineRecoveryKeySetup and avoids having to tell typescript with ! that these are not undefined.

Very nice 🙌 Also simplifies a lot of the mocking for tests and stories

LZoog · 2024-09-18T21:07:58Z

packages/fxa-settings/src/pages/Signin/container.tsx

+        // taken to signin_token_code, else GQL responds with 'Invalid token'
+
+        // todo: add try/catch
+        const { exists } = await authClient.recoveryKeyExists(


I think Sync users always go to signin_token_code or the 2FA page if they have 2FA, but I’ve gotten myself into a state locally where I didn’t have to enter the code just the other day, so it was either this or pass the authClient function from the 3 container components down into the presentation component and into handleNavigation to call there.

It was easiest to just call here but with this approach the session token isn't verified yet. Alternatively I could have returned this value at the signIn mutation which I started to do at first since I could check if input.options.service === 'sync', but... it felt wrong returning a value like that in the mutation since I didn't see where we were doing something like that otherwise. (LMK if you disagree just so I know for the future.)

I think it is fine the way you have it here. New users who just connected a new device would not have to enter an email code and would then get prompted to setup recovery key.

LZoog

Did a little self-review, this is fully testable but the next push should have more unit tests and a rebase on latest main.

LZoog · 2024-09-18T21:32:15Z

packages/fxa-settings/src/components/DataBlock/index.tsx

@@ -88,7 +88,7 @@ export const DataBlock = ({
        ) : (
          <span
            className={classNames({
-              'flex flex-col self-center align-middle grow pe-4': isInline,
+              'flex flex-col self-center align-middle grow': isInline,


This fixes the recovery key dropping to two lines.

LZoog · 2024-09-18T21:32:55Z

packages/fxa-settings/src/components/RecoveryKeySetupDownload/index.tsx

 const spinner = (
-  <LoadingSpinner className="bg-grey-20 flex items-center flex-col justify-center h-screen select-none" />
+  <LoadingSpinner className="bg-grey-20 flex items-center flex-col justify-center select-none" />


This fixes:

LZoog · 2024-09-18T21:35:25Z

packages/fxa-settings/src/components/RecoveryKeySetupHint/index.tsx

+
+export const MAX_HINT_LENGTH = 255;
+
+export const RecoveryKeySetupHint = ({


A large chunk of this was moved from FlowRecoveryKeyHint and tweaked to work with both page components. I left the flow metrics/viewName stuff as-is.

I did modify the top/bottom padding of LightbulbImage though, it was too much space based on both Figmas.

packages/fxa-settings/src/pages/InlineRecoveryKeySetup/container.tsx

LZoog · 2024-09-18T21:38:43Z

packages/fxa-settings/src/pages/InlineRecoveryKeySetup/container.tsx

+  const sensitiveDataClient = useSensitiveDataClient();
+  const sensitiveData = sensitiveDataClient.getData(AUTH_DATA_KEY);
+  const { authPW, emailForAuth } =
+    (sensitiveData as unknown as { emailForAuth: string; authPW: string }) ||


I think we could make some solid improvements with this, and move other values to it. I'll file a follow up and put it in our React Cleanup epic if we don't have something filed already, or leave a note on the React state management spike.

LZoog · 2024-09-19T15:30:20Z

packages/fxa-settings/src/pages/Signin/container.tsx

+        'data' in result &&
+        result.data &&
+        options.service === 'sync'
+        // && localStorage.getItem('TODO')


Currently tweaking this. Instead of having this check in getSyncNavigate too, I'm just going to do it here only and rename result.data.recoveryKeyExists.

vbudhram

@LZoog This is great! I've done a first pass of the code and tested it locally, which all seems to be working as expected.

I haven't verified metrics yet but I think that might be ok as a quick follow up since the PR is pretty big as it is.

vbudhram · 2024-09-19T16:53:41Z

packages/fxa-settings/src/components/RecoveryKeySetupHint/index.test.tsx

+      expect(accountWithSuccess.updateRecoveryKeyHint).toBeCalledWith(
+        hintValue
+      );
+      expect(logViewEvent).toBeCalledWith(


I thought we stopped supporting the older metrics in new views?

Yes! But most of this component was re-used from previous implementation so I left these.

vbudhram · 2024-09-19T16:55:08Z

packages/fxa-settings/src/components/RecoveryKeySetupHint/index.tsx

+    } else if (!DISPLAY_SAFE_UNICODE.test(hint)) {
+      const localizedUnsafeUnicodeCharError = ftlMsgResolver.getMsg(
+        'flow-recovery-key-hint-unsafe-char-error',
+        'The hint cannot contain unsafe unicode characters. Only letters, numbers, punctuation marks and symbols are allowed.'


vbudhram · 2024-09-19T17:12:11Z

packages/fxa-settings/src/pages/Signin/container.tsx

+        // taken to signin_token_code, else GQL responds with 'Invalid token'
+
+        // todo: add try/catch
+        const { exists } = await authClient.recoveryKeyExists(


I think it is fine the way you have it here. New users who just connected a new device would not have to enter an email code and would then get prompted to setup recovery key.

…te desktop Sync signins Because: * We want to encourage Sync signin users to create an account recovery key This commit: * Moves and refactors part of FlowRecoveryKeyHint into new component for sharing * Sets ups the create recovery key and hint handlers in container component, call where needed * Queries if recovery key exists after signin if sync, stores PW-related data into sensitiveDataClient * Navigates signing in desktop Sync flow users without a recovery key to this new flow before taking them to CAD, if the feature flag is on closes FXA-10079

LZoog commented Sep 18, 2024

View reviewed changes

packages/fxa-settings/src/pages/InlineRecoveryKeySetup/container.tsx Outdated Show resolved Hide resolved

LZoog commented Sep 18, 2024

View reviewed changes

LZoog force-pushed the FXA-10079 branch from 56c60d7 to 5bffcf9 Compare September 18, 2024 21:31

LZoog commented Sep 18, 2024

View reviewed changes

LZoog force-pushed the FXA-10079 branch from 5bffcf9 to 0dfdf46 Compare September 18, 2024 23:52

LZoog commented Sep 19, 2024

View reviewed changes

vbudhram reviewed Sep 19, 2024

View reviewed changes

LZoog force-pushed the FXA-10079 branch from 0dfdf46 to 767f228 Compare September 19, 2024 20:13

LZoog marked this pull request as ready for review September 19, 2024 20:13

LZoog requested review from a team as code owners September 19, 2024 20:13

LZoog force-pushed the FXA-10079 branch 3 times, most recently from f9aae41 to 9a4196f Compare September 19, 2024 21:18

bcolsson approved these changes Sep 19, 2024

View reviewed changes

LZoog force-pushed the FXA-10079 branch from 9a4196f to 11dfa2d Compare September 19, 2024 22:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(recovery-key): Make inline recovery key flow functional + navigate desktop Sync signins #17629

feat(recovery-key): Make inline recovery key flow functional + navigate desktop Sync signins #17629

LZoog commented Sep 18, 2024 •

edited

Loading

LZoog Sep 18, 2024

vpomerleau Sep 19, 2024

LZoog Sep 18, 2024

vbudhram Sep 19, 2024

LZoog left a comment

LZoog Sep 18, 2024

LZoog Sep 18, 2024

LZoog Sep 18, 2024

LZoog Sep 18, 2024

LZoog Sep 19, 2024

vbudhram left a comment

vbudhram Sep 19, 2024

LZoog Sep 19, 2024

vbudhram Sep 19, 2024

vbudhram Sep 19, 2024


		export const MAX_HINT_LENGTH = 255;

		export const RecoveryKeySetupHint = ({

feat(recovery-key): Make inline recovery key flow functional + navigate desktop Sync signins #17629

Are you sure you want to change the base?

feat(recovery-key): Make inline recovery key flow functional + navigate desktop Sync signins #17629

Conversation

LZoog commented Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LZoog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbudhram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LZoog commented Sep 18, 2024 •

edited

Loading