feat(react): Send fxaLogin webchannel message conditionally after password reset #15412

LZoog · 2023-06-06T00:33:15Z

Because:

The browser should be be notified when a PW reset occurs on a verified account through the Sync flow, as this logs the user in

This commit:

Conditionally runs the fxaLogin command with required account data after a password reset with or without recovery key, when the flow is sync
Receives other values off the account reset call needed for webchannel account data
Temporarily checks for SyncBasic and SyncDesktop flows to run the check because we don't need to port over context in local storage logic (which would account for SyncDesktop only), since we are switching to codes soon
Sends up service with the email request so we can append the param and check when link is received (causing the SyncBasic integration)
Removes resendResetPassword and calls resetPassword where needed since the code was duplicated

LZoog · 2023-06-06T00:38:39Z

packages/fxa-settings/src/lib/channels/helpers.ts

-//     Object.entries(loginData).filter(([key, value]) => value !== undefined)
-//   );
-// }
+const REQUIRED_LOGIN_FIELDS: Array<keyof FxALoginRequest> = [


There's a link to what params are used on the FF side above FxALoginRequest. This is different than the previous "allowed" and "required" fields which were commented out, please see what we're doing in this file and double check me. I figure if there's more we need later for the "confirm" pages we can add them then.

Looks right. One comment, is that although authAt is listed in the FF source as a field, but I don't see it being used, and it looks like the content-server auth broker model doesn't have it listed as a required field. It seems like what you have is technically more correct though, so it is probably fine.

LZoog · 2023-06-06T00:40:43Z

packages/fxa-settings/src/models/Account.ts

-  async resetPassword(email: string): Promise<PasswordForgotSendCodePayload> {
+  async resetPassword(
+    email: string,
+    service?: string


I'm not sure if this would have been preferred anyway but I tried using CreateRelier here, and ran into issues. There was nothing in the console, so I worked my way up and saw this was struggling to pull off of Context here, maybe because this isn't a React component? Didn't dive too deep, since we're replacing links with codes soon anyway, and just called CreateRelier in the component using it instead.

I think that's good. I don't think we should be accessing context here. Better to pass it in.

LZoog · 2023-06-07T14:57:20Z

packages/fxa-settings/src/lib/channels/helpers.ts

-// for (const key of ALLOWED_LOGIN_FIELDS) {
-//   loginData[key] = account[key];
-// }
-//   // TODO: account for multiservice when we combine reliers


I'm going to add this comment back since we do need this. I think this has to do with the signup or confirm view since in content-server, _formatForMultiServiceBrowser will send offeredSyncEngines and declinedSyncEngines. It can be looked at in https://mozilla-hub.atlassian.net/browse/FXA-7308.

dschom · 2023-06-07T15:36:37Z

packages/fxa-settings/src/models/Account.ts

          },
        },
      });
      currentAccount(getOldSettingsData(accountReset));
      sessionToken(accountReset.sessionToken);
+      return accountReset;


Probably a follow up item, but it'd be great to return something other than 'any' here.

LZoog · 2023-06-07T16:43:05Z

Going to go ahead and open this as ready - there are 2 failing tests I'm still looking into but should be g2g otherwise.

(edit: not sure why GH is showing no changes since I just pushed, will push again in a sec...)

LZoog · 2023-06-07T18:01:48Z

I just pushed again with everything passing locally for me but looks like I'm afflicted by the Github outage so it hasn't shown up yet. The push from 1512f88 to 6ca824c contains everything except the last two failures.

One thing to note, everything passes but I'm seeing a few warnings around An update to [component] inside a test was not wrapped in act(...).: At least one of them was here before this branch, so I'm just going to file a follow up. edit: follow up is here

vbudhram · 2023-06-07T18:42:13Z

packages/fxa-settings/src/models/Account.ts

@@ -614,51 +629,6 @@ export class Account implements AccountData {
    }
  }

-  async resendResetPassword(


Suprised this was duplicated like this 😅

vbudhram

Running this locally and looks like storybook is failing

Users/vijaybudhram/Desktop/working/fxa/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/index.stories.tsx
TypeScript error in /Users/vijaybudhram/Desktop/working/fxa/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/index.stories.tsx(10,34):
Module '"./mocks"' has no exported member 'Subject'.  TS2305

     8 | import { Account } from '../../../models';
     9 | import { withLocalization } from '../../../../.storybook/decorators';
  > 10 | import { paramsWithMissingEmail, Subject } from './mocks';
       |                                  ^
    11 | 
    12 | export default {
    13 |   title: 'Pages/ResetPassword/CompleteResetPassword',

Looks like Subject is now renderSubject?

LZoog · 2023-06-09T23:50:34Z

@dschom @vbudhram I had to refactor some mocks/tests/storybook things due to this. I'd changed it so Subject would be renderSubject, but that does not work with our Storybook setup and since it's nice to share state/test set up like we have there I changed it to getSubject. Most changes were needed because when we need to call CreateRelier, we need to mock a lot more than without.

One problem I'm having now is something @dschom I think you and I have both ran into, where rendering a "damaged" link causes all other stories to show the damaged link (CompleteResetPassword, AccountRecoveryConfirmKey) because Storybook builds all stories at runtime and I think there's some global window state that's persisting across stories. It's almost like we need a resetState method on our ReachRouterWindow to run between each story with a decorator... but we haven't needed to do this yet. Do you see what's causing this? 🥹 I've spent more time on stories/test refactors here than I thought I would, (EDIT: fixed! 🎉) it's really starting to bother me how inconsistent we are doing various things heh, definitely something to call out and file issues for in that architecture epic...

Feel free to test locally now though (and comment out the DamagedLink stories to see the others), if you create an account and then run fxa-dev-launcher (see you're not signed in via the menu), make sure the feature flag is on, then reset through the Sync flow in that window by clicking "Looking for Sync?" > "Forgot password?" > append &showReactApp=true and follow the link with the param as well. On successful reset you'll see you're logged in.

vbudhram

@LZoog Works as described! LGTM 👍🏽

dschom

LGMT

dschom · 2023-06-21T15:02:19Z

packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/index.test.tsx

@@ -76,7 +76,8 @@ const renderSubject = ({
  account = accountWithValidResetToken,
  params = mockCompleteResetPasswordParams,
 } = {}) => {
-  render(<Subject {...{ account, params }} />);
+  const { Subject, history, appCtx } = getSubject(account, params);
+  return renderWithRouter(<Subject />, { history }, appCtx);


Note that the 'render' import above is no longer used!

dschom · 2023-06-21T15:03:10Z

packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/mocks.tsx

@@ -6,14 +6,21 @@ import React from 'react';
 import { MozServices } from '../../../lib/types';
 import { LocationProvider } from '@reach/router';
 import { Account, AppContext } from '../../../models';
-import { mockAppContext, MOCK_ACCOUNT } from '../../../models/mocks';
+import {


LocationProvider and AppContext on the lines above are both unnused now.

dschom · 2023-06-21T15:05:23Z

packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/index.test.tsx

  mockCompleteResetPasswordParams,
  paramsWithMissingCode,
  paramsWithMissingEmail,
  paramsWithMissingEmailToHashWith,
  paramsWithMissingToken,
  paramsWithSyncDesktop,
-  Subject,
 } from './mocks';


Subject isn't used

dschom · 2023-06-21T15:05:29Z

packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/index.test.tsx

@@ -3,26 +3,22 @@
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */



render isn't used

dschom · 2023-06-21T15:06:45Z

packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/mocks.tsx

-      </LocationProvider>
-    </AppContext.Provider>
-  );
+        storageData: new StorageDataMock(windowWrapper),


Accoding to the type account must always be defined. Is there a reason for the account &&

dschom · 2023-06-21T15:07:55Z

packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/mocks.tsx

@@ -9,7 +9,13 @@ import CompleteResetPassword from '.';
 import LinkValidator from '../../../components/LinkValidator';
 import { StorageData, UrlQueryData } from '../../../lib/model-data';
 import { Account, AppContext } from '../../../models';


AppContext unused and LocationProvider above is also unused.

dschom · 2023-06-21T15:09:00Z

packages/fxa-settings/src/pages/ResetPassword/index.stories.tsx

@@ -4,71 +4,53 @@

 import React from 'react';


AppContext & LocationProvider are not used.

dschom · 2023-06-21T15:15:22Z

packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/mocks.tsx

+  mockAppContext,
+  MOCK_ACCOUNT,
+  createHistoryWithQuery,
+  renderWithRouter,


renderWithRouter not used. Same with AppContext & LocationProvider.

renderWithRouter not used. Same with AppContex & LocationProvider.

dschom · 2023-06-21T15:16:41Z

packages/fxa-settings/src/lib/storybook-utils.ts

+  route: string,
+  account = MOCK_ACCOUNT as unknown as Account,
+  queryParams?: string,
+  appCtx?: AppContextValue


appCtx not used?

dschom · 2023-06-21T15:17:53Z

packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/mocks.tsx

+  mockAppContext,
+  MOCK_ACCOUNT,
+  createHistoryWithQuery,
+  renderWithRouter,


renderWithRouter not used. Same with ApPContext & LocationProvider above.

@dschom Thanks for all the callouts. I said this in some other issue but I thought unused imports would show up in CI or something now.

…sword reset Because: * The browser should be be notified when a PW reset occurs on a verified account through the Sync flow, as this logs the user in This commit: * Conditionally runs the fxaLogin command with required account data after a password reset with or without recovery key, when the flow is sync * Receives other values off the account reset call needed for webchannel account data * Temporarily checks for SyncBasic and SyncDesktop flows to run the check because we don't need to port over context in local storage logic (which would account for SyncDesktop only), since we are switching to codes soon * Sends up `service` with the email request so we can append the param and check when link is received (causing the SyncBasic integration) * Removes resendResetPassword and calls resetPassword where needed since the code was duplicated Fixes FXA-7172

LZoog · 2023-06-21T16:42:10Z

Just pushed an update removing unused imports. Going to go ahead and merge since CI was already happy and we are about to tag.

LZoog force-pushed the FXA-7172 branch from 2efa35b to d022d06 Compare June 6, 2023 00:34

LZoog commented Jun 6, 2023

View reviewed changes

LZoog commented Jun 7, 2023

View reviewed changes

dschom reviewed Jun 7, 2023

View reviewed changes

LZoog force-pushed the FXA-7172 branch from d022d06 to 1512f88 Compare June 7, 2023 16:00

LZoog marked this pull request as ready for review June 7, 2023 16:43

LZoog requested a review from a team as a code owner June 7, 2023 16:43

LZoog force-pushed the FXA-7172 branch from 1512f88 to 6ca824c Compare June 7, 2023 17:44

LZoog force-pushed the FXA-7172 branch from 6ca824c to e98cfd6 Compare June 7, 2023 18:18

vbudhram reviewed Jun 7, 2023

View reviewed changes

LZoog force-pushed the FXA-7172 branch 3 times, most recently from 45eb136 to de6c7ca Compare June 9, 2023 23:45

LZoog force-pushed the FXA-7172 branch from de6c7ca to bbb632f Compare June 15, 2023 20:40

vbudhram approved these changes Jun 21, 2023

View reviewed changes

dschom approved these changes Jun 21, 2023

View reviewed changes

LZoog force-pushed the FXA-7172 branch from bbb632f to 7a94d3f Compare June 21, 2023 16:33

LZoog merged commit 49816e8 into main Jun 21, 2023

LZoog deleted the FXA-7172 branch June 21, 2023 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(react): Send fxaLogin webchannel message conditionally after password reset #15412

feat(react): Send fxaLogin webchannel message conditionally after password reset #15412

LZoog commented Jun 6, 2023 •

edited

Loading

LZoog Jun 6, 2023 •

edited

Loading

dschom Jun 7, 2023

LZoog Jun 6, 2023

dschom Jun 7, 2023

LZoog Jun 7, 2023

dschom Jun 7, 2023

LZoog commented Jun 7, 2023 •

edited

Loading

LZoog commented Jun 7, 2023 •

edited

Loading

vbudhram Jun 7, 2023

vbudhram left a comment

LZoog commented Jun 9, 2023 •

edited

Loading

vbudhram left a comment

dschom left a comment

dschom Jun 21, 2023

dschom Jun 21, 2023

dschom Jun 21, 2023

dschom Jun 21, 2023

dschom Jun 21, 2023

dschom Jun 21, 2023

dschom Jun 21, 2023

dschom Jun 21, 2023

dschom Jun 21, 2023

dschom Jun 21, 2023

dschom Jun 21, 2023

LZoog Jun 21, 2023

LZoog commented Jun 21, 2023

		@@ -3,26 +3,22 @@
		* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

feat(react): Send fxaLogin webchannel message conditionally after password reset #15412

feat(react): Send fxaLogin webchannel message conditionally after password reset #15412

Conversation

LZoog commented Jun 6, 2023 • edited Loading

LZoog Jun 6, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LZoog commented Jun 7, 2023 • edited Loading

LZoog commented Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

vbudhram left a comment

Choose a reason for hiding this comment

LZoog commented Jun 9, 2023 • edited Loading

vbudhram left a comment

Choose a reason for hiding this comment

dschom 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

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

Choose a reason for hiding this comment

LZoog commented Jun 21, 2023

LZoog commented Jun 6, 2023 •

edited

Loading

LZoog Jun 6, 2023 •

edited

Loading

LZoog commented Jun 7, 2023 •

edited

Loading

LZoog commented Jun 7, 2023 •

edited

Loading

LZoog commented Jun 9, 2023 •

edited

Loading