Merge pull request #19546 from mozilla/FXA-12472

fix(settings): Redirect users from /settings if session is unverified

Author: LZoog

packages/functional-tests/pages/settings/components/connectedService.ts

@@ -2,31 +2,28 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-import { ElementHandle, Page } from '@playwright/test';
+import { Locator, Page } from '@playwright/test';
 
 export class ConnectedService {
   name = '';
   constructor(
-    readonly element: ElementHandle<Node>,
+    readonly element: Locator,
     readonly page: Page
   ) {}
 
-  static async create(
-    element: ElementHandle<Node>,
-    page: Page
-  ) {
+  static async create(element: Locator, page: Page) {
     const service = new ConnectedService(element, page);
     service.name = await service.getName();
     return service;
   }
 
   async getName() {
-    const p = await this.element.waitForSelector('[data-testid=service-name]');
+    const p = this.element.locator('[data-testid=service-name]');
     return p.innerText();
   }
 
   async signout() {
-    const button = await this.element.waitForSelector(
+    const button = this.element.locator(
       '[data-testid=connected-service-sign-out]'
     );
     return button.click();

packages/functional-tests/pages/settings/components/unitRow.ts

@@ -123,13 +123,17 @@ export class TotpRow extends UnitRow {
 
 export class ConnectedServicesRow extends UnitRow {
   async services() {
-    const elements = this.page.locator('[data-testid=settings-connected-service]');
+    const elements = this.page.locator(
+      '[data-testid=settings-connected-service]'
+    );
     // there should be multiple, but you cannot call `.waitFor()` on a locator
     // that returns multiple elements.
     await elements.first().waitFor({ state: 'attached' });
-    const elementHandles = await elements.elementHandles();
+    const count = await elements.count();
     return Promise.all(
-      elementHandles.map((el) => ConnectedService.create(el, this.page))
+      Array.from({ length: count }, (_, i) =>
+        ConnectedService.create(elements.nth(i), this.page)
+      )
     );
   }
 }

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

@@ -5,6 +5,7 @@
 import React, { ReactNode } from 'react';
 import { act, screen, waitFor } from '@testing-library/react';
 import { renderWithLocalizationProvider } from 'fxa-react/lib/test-utils/localizationProvider';
+import { navigate } from '@reach/router';
 import App from '.';
 import * as Metrics from '../../lib/metrics';
 import {
@@ -29,12 +30,18 @@ import { firefox } from '../../lib/channels/firefox';
 
 import GleanMetrics from '../../lib/glean';
 import config from '../../lib/config';
-import * as utils from 'fxa-react/lib/utils';
 import { currentAccount } from '../../lib/cache';
 import { MozServices } from '../../lib/types';
 import mockUseSyncEngines from '../../lib/hooks/useSyncEngines/mocks';
 import useSyncEngines from '../../lib/hooks/useSyncEngines';
 
+jest.mock('@reach/router', () => {
+  return {
+    ...jest.requireActual('@reach/router'),
+    navigate: jest.fn(),
+  };
+});
+
 jest.mock('../../lib/hooks/useSyncEngines', () => ({
   __esModule: true,
   default: jest.fn(),
@@ -269,6 +276,7 @@ describe('loading spinner states', () => {
     });
     (useSession as jest.Mock).mockReturnValue({
       isValid: () => true,
+      isSessionVerified: () => Promise.resolve(true),
     });
     (currentAccount as jest.Mock).mockReturnValueOnce({
       uid: 123,
@@ -368,21 +376,10 @@ describe('AuthAndAccountSetupRoutes', () => {
 });
 
 describe('SettingsRoutes', () => {
-  let hardNavigateSpy: jest.SpyInstance;
   const settingsPath = '/settings';
-  jest.mock('@reach/router', () => ({
-    ...jest.requireActual('@reach/router'),
-    useLocation: () => {
-      return {
-        pathname: settingsPath,
-      };
-    },
-  }));
 
   beforeEach(() => {
-    hardNavigateSpy = jest
-      .spyOn(utils, 'hardNavigate')
-      .mockImplementation(() => {});
+    (navigate as jest.Mock).mockClear();
     (useInitialMetricsQueryState as jest.Mock).mockReturnValue({
       loading: false,
     });
@@ -396,6 +393,7 @@ describe('SettingsRoutes', () => {
     });
     (useSession as jest.Mock).mockReturnValue({
       isValid: () => false,
+      isSessionVerified: () => Promise.resolve(true),
     });
     (useProductInfoState as jest.Mock).mockReturnValue({
       loading: false,
@@ -409,7 +407,7 @@ describe('SettingsRoutes', () => {
   });
 
   afterEach(() => {
-    hardNavigateSpy.mockRestore();
+    (navigate as jest.Mock).mockRestore();
     (useIntegration as jest.Mock).mockRestore();
     (useInitialMetricsQueryState as jest.Mock).mockRestore();
     (useLocalSignedInQueryState as jest.Mock).mockRestore();
@@ -449,7 +447,7 @@ describe('SettingsRoutes', () => {
     await act(() => navigateResult);
 
     await waitFor(() => {
-      expect(hardNavigateSpy).toHaveBeenCalledWith(
+      expect(navigate).toHaveBeenCalledWith(
         `/?redirect_to=${encodeURIComponent(settingsPath)}`
       );
     });
@@ -493,7 +491,7 @@ describe('SettingsRoutes', () => {
     await act(() => navigateResult);
 
     await waitFor(() => {
-      expect(hardNavigateSpy).not.toHaveBeenCalled();
+      expect(navigate).not.toHaveBeenCalled();
     });
 
     expect(screen.getByText('Session Expired')).toBeInTheDocument();
@@ -529,6 +527,7 @@ describe('SettingsRoutes', () => {
 
     (useSession as jest.Mock).mockReturnValue({
       isValid: () => true,
+      isSessionVerified: () => Promise.resolve(true),
     });
 
     let navigateResult: Promise<void>;
@@ -556,7 +555,7 @@ describe('SettingsRoutes', () => {
     await act(() => navigateResult);
 
     await waitFor(() => {
-      expect(hardNavigateSpy).not.toHaveBeenCalled();
+      expect(navigate).not.toHaveBeenCalled();
     });
     expect(screen.getByTestId('settings-profile')).toBeInTheDocument();
   });
@@ -588,7 +587,7 @@ describe('SettingsRoutes', () => {
     await act(() => navigateResult);
 
     await waitFor(() => {
-      expect(hardNavigateSpy).not.toHaveBeenCalled();
+      expect(navigate).not.toHaveBeenCalled();
     });
     expect(screen.getByTestId('settings-profile')).toBeInTheDocument();
   });

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

@@ -2,7 +2,12 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-import { RouteComponentProps, Router, useLocation } from '@reach/router';
+import {
+  navigate,
+  RouteComponentProps,
+  Router,
+  useLocation,
+} from '@reach/router';
 import {
   lazy,
   Suspense,
@@ -35,8 +40,6 @@ import {
   SettingsContext,
 } from '../../models/contexts/SettingsContext';
 
-import { hardNavigate } from 'fxa-react/lib/utils';
-
 import sentryMetrics from 'fxa-shared/sentry/browser';
 
 // Components
@@ -348,12 +351,6 @@ export const App = ({
     return <LoadingSpinner fullScreen />;
   }
 
-  // If we're on settings route but user is not signed in, redirect immediately
-  if (window.location.pathname?.includes('/settings') && !isSignedIn) {
-    hardNavigate('/');
-    return <LoadingSpinner fullScreen />;
-  }
-
   return (
     <Router basepath="/">
       <AuthAndAccountSetupRoutes
@@ -389,7 +386,7 @@ const SettingsRoutes = ({
       // For regular RP / web logins, maybe the session token expired. In this
       // case we just send them to the root.
       params.set('redirect_to', location.pathname);
-      hardNavigate(`/?${params.toString()}`);
+      navigate(`/?${params.toString()}`);
     }
 
     return <LoadingSpinner fullScreen />;

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

@@ -4,6 +4,7 @@
 
 import React, { ReactNode } from 'react';
 import { History } from '@reach/router';
+import { waitFor } from '@testing-library/react';
 import { Account, AppContext, useInitialSettingsState } from '../../models';
 import {
   mockAppContext,
@@ -82,8 +83,8 @@ describe('Settings App', () => {
     expect(getByLabelText('Loading…')).toBeInTheDocument();
   });
 
-  it('renders `AppErrorDialog` component when settings query errors', () => {
-    (useInitialSettingsState as jest.Mock).mockReturnValueOnce({
+  it('renders `AppErrorDialog` component when settings query errors', async () => {
+    (useInitialSettingsState as jest.Mock).mockReturnValue({
       error: { message: 'Error' },
     });
     const { getByRole } = renderWithRouter(
@@ -92,15 +93,19 @@ describe('Settings App', () => {
       </AppContext.Provider>
     );
 
-    expect(getByRole('heading', { level: 2 })).toHaveTextContent(
-      'General application error'
-    );
+    await waitFor(() => {
+      expect(getByRole('heading', { level: 2 })).toHaveTextContent(
+        'General application error'
+      );
+    });
   });
 
   it('redirects to root if account is not verified', async () => {
     // this warning is expected, so we don't want to see it in the test output
     const warnSpy = jest.spyOn(console, 'warn').mockImplementation((msg) => {
-      if (msg === 'Account verification is require to access /settings!')
+      if (
+        msg === 'Account or email verification is require to access /settings!'
+      )
         return;
     });
     const unverifiedAccount = {
@@ -123,8 +128,34 @@ describe('Settings App', () => {
       { route: SETTINGS_PATH }
     );
 
-    await Promise.resolve();
-    expect(mockNavigateWithQuery).toHaveBeenCalledWith('/');
+    await waitFor(() => {
+      expect(mockNavigateWithQuery).toHaveBeenCalledWith('/');
+    });
+    warnSpy.mockRestore();
+  });
+
+  it('redirects to root if session is not verified', async () => {
+    // this warning is expected, so we don't want to see it in the test output
+    const warnSpy = jest.spyOn(console, 'warn').mockImplementation((msg) => {
+      if (
+        msg === 'Account or email verification is require to access /settings!'
+      )
+        return;
+    });
+    const unverifiedSession = mockSession(false);
+
+    renderWithRouter(
+      <AppContext.Provider
+        value={mockAppContext({ session: unverifiedSession })}
+      >
+        <Subject />
+      </AppContext.Provider>,
+      { route: SETTINGS_PATH }
+    );
+
+    await waitFor(() => {
+      expect(mockNavigateWithQuery).toHaveBeenCalledWith('/');
+    });
     warnSpy.mockRestore();
   });
 

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

@@ -2,7 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-import React, { useEffect } from 'react';
+import React, { useEffect, useState } from 'react';
 import * as Sentry from '@sentry/browser';
 import SettingsLayout from './SettingsLayout';
 import LoadingSpinner from 'fxa-react/components/LoadingSpinner';
@@ -48,6 +48,7 @@ export const Settings = ({
   const account = useAccount();
   const location = useLocation();
   const navigateWithQuery = useNavigateWithQuery();
+  const [sessionVerified, setSessionVerified] = useState<boolean | undefined>();
 
   useEffect(() => {
     /**
@@ -121,7 +122,13 @@ export const Settings = ({
     gleanEnabled && GleanMetrics.pageLoad(location.pathname);
   }, [location.pathname, gleanEnabled]);
 
-  if (loading) {
+  useEffect(() => {
+    (async () => {
+      setSessionVerified(await session.isSessionVerified());
+    })();
+  }, [session]);
+
+  if (loading || sessionVerified === undefined) {
     return <LoadingSpinner fullScreen />;
   }
 
@@ -132,13 +139,14 @@ export const Settings = ({
     return <AppErrorDialog data-testid="error-dialog" />;
   }
 
-  // If the account email isn't verified, kick back to root to prompt for verification.
-  // This should only happen if the user tries to access /settings directly
-  // without entering a confirmation code on confirm_signup_code page.
-  // Session verification requirement and redirect is handled on initial app load
-  // (look for isUnverifiedSessionError in lib/gql.ts for that handling)
-  if (account.primaryEmail.verified === false) {
-    console.warn('Account verification is require to access /settings!');
+  // If the account email isn't verified or the user is an unverified session state,
+  // kick back to root to prompt for verification. This should only happen if the user
+  // tries to access /settings directly without entering a confirmation code on
+  // confirm_signup_code page or signin_token_code page.
+  if (account.primaryEmail.verified === false || sessionVerified === false) {
+    console.warn(
+      'Account or email verification is require to access /settings!'
+    );
     navigateWithQuery('/');
     return <LoadingSpinner fullScreen />;
   }

packages/fxa-settings/src/lib/gql.ts

@@ -45,6 +45,9 @@ const isUnauthorizedError = (error: GraphQLError | GraphQLFormattedError) => {
  * not verified the login via email/2FA. Note, the email can be verified but not the
  * session. Only certain queries/API calls require a verified session.
  *
+ * TODO: FXA-12454 do we still need this or at least can it be removed for the
+ * '/settings' check?
+ *
  * @param error
  */
 const isUnverifiedSessionError = (