Merge pull request #20048 from mozilla/FXA-13022

fix(signin): Check if session is valid for isFirefoxNonSync before rendering cached signin

Author: LZoog

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

@@ -90,6 +90,7 @@ function mockSyncDesktopV3Integration() {
     data: { service: 'sync' },
     isDesktopSync: () => true,
     isFirefoxClientServiceRelay: () => false,
+    isFirefoxNonSync: () => false,
     getCmsInfo: () => undefined,
   } as Integration;
 }
@@ -105,6 +106,7 @@ function mockOAuthWebIntegration(
     data,
     isDesktopSync: () => false,
     isFirefoxClientServiceRelay: () => false,
+    isFirefoxNonSync: () => false,
     getCmsInfo: () => undefined,
   } as Integration;
 }
@@ -118,6 +120,7 @@ function mockOAuthNativeIntegration() {
     wantsKeys: () => true,
     isDesktopSync: () => true,
     isFirefoxClientServiceRelay: () => false,
+    isFirefoxNonSync: () => false,
     getCmsInfo: () => undefined,
     data: {
       service: 'sync',
@@ -126,6 +129,23 @@ function mockOAuthNativeIntegration() {
   } as Integration;
 }
 
+function mockFirefoxNonSyncIntegration() {
+  integration = {
+    type: IntegrationType.OAuthNative,
+    getService: () => ModelsModule.OAuthNativeServices.Relay,
+    getClientId: () => MOCK_CLIENT_ID,
+    isSync: () => false,
+    wantsKeys: () => false,
+    isDesktopSync: () => false,
+    isFirefoxClientServiceRelay: () => true,
+    isFirefoxNonSync: () => true,
+    getCmsInfo: () => undefined,
+    data: {
+      service: ModelsModule.OAuthNativeServices.Relay,
+    },
+  } as Integration;
+}
+
 function mockWebIntegration() {
   // Leaving for historical record. Remove once baked.
   // integration = {
@@ -157,7 +177,11 @@ function mockWebIntegration() {
 function mockFetchModule() {
   global.fetch = jest.fn().mockResolvedValue({
     ok: true,
-    json: () => Promise.resolve({ id: 'avatar-id', url: 'https://example.com/avatar.png' }),
+    json: () =>
+      Promise.resolve({
+        id: 'avatar-id',
+        url: 'https://example.com/avatar.png',
+      }),
   });
 }
 
@@ -201,6 +225,7 @@ mockSensitiveDataClient.setDataType = jest.fn();
 
 const mockSession = {
   isSessionVerified: jest.fn().mockResolvedValue(true),
+  isValid: jest.fn().mockResolvedValue(true),
   sendVerificationCode: jest.fn().mockResolvedValue(undefined),
   verified: false,
   token: MOCK_SESSION_TOKEN,
@@ -280,6 +305,7 @@ function mockModelsModule() {
   }));
   (ModelsModule.useSession as jest.Mock).mockImplementation(() => mockSession);
   mockSession.isSessionVerified = jest.fn().mockResolvedValue(true);
+  mockSession.isValid = jest.fn().mockResolvedValue(true);
   mockSession.sendVerificationCode = jest.fn().mockResolvedValue(undefined);
 }
 
@@ -410,9 +436,9 @@ function mockSentryModule() {
   mockSentryCaptureMessage = jest.spyOn(SentryModule, 'captureMessage');
 }
 
-function render(
-  options?: { useFxAStatusResult?: ReturnType<typeof mockUseFxAStatus> }
-) {
+function render(options?: {
+  useFxAStatusResult?: ReturnType<typeof mockUseFxAStatus>;
+}) {
   const useFxAStatusResult = options?.useFxAStatusResult || mockUseFxAStatus();
 
   return renderWithLocalizationProvider(
@@ -940,10 +966,12 @@ describe('signin container', () => {
           keyFetchToken: MOCK_KEY_FETCH_TOKEN,
         });
         // Mock passwordChangeStartWithAuthPW to succeed
-        mockAuthClient.passwordChangeStartWithAuthPW = jest.fn().mockResolvedValue({
-          keyFetchToken: MOCK_KEY_FETCH_TOKEN,
-          passwordChangeToken: 'mockPasswordChangeToken',
-        });
+        mockAuthClient.passwordChangeStartWithAuthPW = jest
+          .fn()
+          .mockResolvedValue({
+            keyFetchToken: MOCK_KEY_FETCH_TOKEN,
+            passwordChangeToken: 'mockPasswordChangeToken',
+          });
         // Mock wrappedAccountKeys to succeed
         mockAuthClient.wrappedAccountKeys = jest.fn().mockResolvedValue({
           kA: MOCK_KB,
@@ -1034,10 +1062,12 @@ describe('signin container', () => {
           keyFetchToken: MOCK_KEY_FETCH_TOKEN,
         });
         // Mock passwordChangeStartWithAuthPW to throw an error
-        mockAuthClient.passwordChangeStartWithAuthPW = jest.fn().mockRejectedValue({
-          errno: 999,
-          message: 'Test error',
-        });
+        mockAuthClient.passwordChangeStartWithAuthPW = jest
+          .fn()
+          .mockRejectedValue({
+            errno: 999,
+            message: 'Test error',
+          });
 
         render();
 
@@ -1075,10 +1105,12 @@ describe('signin container', () => {
           keyFetchToken: MOCK_KEY_FETCH_TOKEN,
         });
         // Mock passwordChangeStartWithAuthPW to succeed
-        mockAuthClient.passwordChangeStartWithAuthPW = jest.fn().mockResolvedValue({
-          keyFetchToken: MOCK_KEY_FETCH_TOKEN,
-          passwordChangeToken: 'mockPasswordChangeToken',
-        });
+        mockAuthClient.passwordChangeStartWithAuthPW = jest
+          .fn()
+          .mockResolvedValue({
+            keyFetchToken: MOCK_KEY_FETCH_TOKEN,
+            passwordChangeToken: 'mockPasswordChangeToken',
+          });
         // Mock wrappedAccountKeys to throw an error
         mockAuthClient.wrappedAccountKeys = jest.fn().mockRejectedValue({
           errno: 999,
@@ -1122,10 +1154,12 @@ describe('signin container', () => {
           keyFetchToken: MOCK_KEY_FETCH_TOKEN,
         });
         // Mock passwordChangeStartWithAuthPW to succeed
-        mockAuthClient.passwordChangeStartWithAuthPW = jest.fn().mockResolvedValue({
-          keyFetchToken: MOCK_KEY_FETCH_TOKEN,
-          passwordChangeToken: 'mockPasswordChangeToken',
-        });
+        mockAuthClient.passwordChangeStartWithAuthPW = jest
+          .fn()
+          .mockResolvedValue({
+            keyFetchToken: MOCK_KEY_FETCH_TOKEN,
+            passwordChangeToken: 'mockPasswordChangeToken',
+          });
         // Mock wrappedAccountKeys to succeed
         mockAuthClient.wrappedAccountKeys = jest.fn().mockResolvedValue({
           kA: MOCK_KB,
@@ -1385,6 +1419,98 @@ describe('signin container', () => {
     });
   });
 
+  describe('Firefox non-Sync session validation', () => {
+    beforeEach(() => {
+      mockFirefoxNonSyncIntegration();
+    });
+
+    it('calls session.isValid and renders signin when session is valid', async () => {
+      const storedAccount = {
+        ...MOCK_STORED_ACCOUNT,
+        email: MOCK_QUERY_PARAM_EMAIL,
+        sessionToken: MOCK_SESSION_TOKEN,
+      };
+      mockCurrentAccount(storedAccount);
+      mockUseValidateModule();
+      mockSession.isValid = jest.fn().mockResolvedValue(true);
+
+      render();
+
+      await waitFor(() => {
+        expect(mockSession.isValid).toHaveBeenCalledWith(MOCK_SESSION_TOKEN);
+      });
+      await waitFor(() => {
+        expect(currentSigninProps).toBeDefined();
+        expect(currentSigninProps?.sessionToken).toBe(MOCK_SESSION_TOKEN);
+      });
+      expect(CacheModule.discardSessionToken).not.toHaveBeenCalled();
+    });
+
+    it('calls session.isValid and discards token when session is invalid', async () => {
+      const storedAccount: { sessionToken?: string; [key: string]: any } = {
+        ...MOCK_STORED_ACCOUNT,
+        email: MOCK_QUERY_PARAM_EMAIL,
+        sessionToken: MOCK_SESSION_TOKEN,
+      };
+      mockCurrentAccount(storedAccount);
+      // Make the spy actually clear the token so re-render picks it up
+      jest.spyOn(CacheModule, 'discardSessionToken').mockImplementation(() => {
+        storedAccount.sessionToken = undefined;
+      });
+      mockUseValidateModule();
+      mockSession.isValid = jest.fn().mockResolvedValue(false);
+
+      render();
+
+      await waitFor(() => {
+        expect(mockSession.isValid).toHaveBeenCalledWith(MOCK_SESSION_TOKEN);
+      });
+      await waitFor(() => {
+        expect(CacheModule.discardSessionToken).toHaveBeenCalled();
+      });
+      await waitFor(() => {
+        expect(currentSigninProps).toBeDefined();
+        expect(currentSigninProps?.sessionToken).toBeUndefined();
+      });
+    });
+
+    it('does not call session.isValid for Sync integration', async () => {
+      mockOAuthNativeIntegration();
+      mockCurrentAccount({
+        ...MOCK_STORED_ACCOUNT,
+        email: MOCK_QUERY_PARAM_EMAIL,
+        sessionToken: MOCK_SESSION_TOKEN,
+      });
+      mockUseValidateModule();
+      mockLocationState = MOCK_LOCATION_STATE_CAN_LINK_ACCOUNT_OK;
+      (ensureCanLinkAcountOrRedirect as jest.Mock).mockResolvedValue(true);
+
+      render();
+
+      await waitFor(() => {
+        expect(currentSigninProps).toBeDefined();
+      });
+      expect(mockSession.isValid).not.toHaveBeenCalled();
+    });
+
+    it('does not call session.isValid for OAuth web integration', async () => {
+      mockOAuthWebIntegration();
+      mockCurrentAccount({
+        ...MOCK_STORED_ACCOUNT,
+        email: MOCK_QUERY_PARAM_EMAIL,
+        sessionToken: MOCK_SESSION_TOKEN,
+      });
+      mockUseValidateModule();
+
+      render();
+
+      await waitFor(() => {
+        expect(currentSigninProps).toBeDefined();
+      });
+      expect(mockSession.isValid).not.toHaveBeenCalled();
+    });
+  });
+
   /**
    * These tests trigger the `setAccountStatus` useEffect, and because of the IIFE
    * inside that, and because there are no `waitFor` calls, they need to explicitly await

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

@@ -24,11 +24,12 @@ import {
   OAuthNativeSyncQueryParameters,
   OAuthQueryParams,
 } from '../../models/pages/signin';
-import { useCallback, useEffect, useState } from 'react';
+import { useCallback, useEffect, useRef, useState } from 'react';
 import {
   currentAccount,
   lastStoredAccount,
   findAccountByEmail,
+  discardSessionToken,
 } from '../../lib/cache';
 import { hardNavigate } from 'fxa-react/lib/utils';
 import {
@@ -278,17 +279,24 @@ const SigninContainer = ({
   }, []);
 
   // Avatar state - fetched directly from profile server
-  const [avatarData, setAvatarData] = useState<{ account: { avatar: { id: string; url: string } } } | undefined>(undefined);
+  const [avatarData, setAvatarData] = useState<
+    { account: { avatar: { id: string; url: string } } } | undefined
+  >(undefined);
   const [avatarLoading, setAvatarLoading] = useState(true);
 
   // Fetch avatar on mount from profile server (requires OAuth token)
   useEffect(() => {
-    if (sessionToken && config?.servers?.profile?.url && config?.oauth?.clientId) {
+    if (
+      sessionToken &&
+      config?.servers?.profile?.url &&
+      config?.oauth?.clientId
+    ) {
       // Get OAuth token with profile:avatar scope (required by profile server)
-      authClient.createOAuthToken(sessionToken, config.oauth.clientId, {
-        scope: 'profile:avatar',
-        ttl: PROFILE_OAUTH_TOKEN_TTL_SECONDS,
-      })
+      authClient
+        .createOAuthToken(sessionToken, config.oauth.clientId, {
+          scope: 'profile:avatar',
+          ttl: PROFILE_OAUTH_TOKEN_TTL_SECONDS,
+        })
         .then(({ access_token }) => {
           return fetch(`${config.servers.profile.url}/v1/avatar`, {
             method: 'GET',
@@ -324,6 +332,39 @@ const SigninContainer = ({
     }
   }, [authClient, config, sessionToken]);
 
+  // For Firefox non-Sync flows, validate the cached session token before rendering.
+  // This is because if users "sign out" from the browser menu, FxA doesn't know
+  // about it (and can't, because even with a web channel message, FxA would need
+  // to be pulled up in a tab) and Fx non-Sync flows will see cached sign-in.
+  // This provides those users with slightly less friction since they'd see an error
+  // message and then be asked to enter their password. If the session token is
+  // invalid, we discard it so the user sees the password field right away.
+  const needsSessionValidation =
+    integration.isFirefoxNonSync() && !!sessionToken;
+  const [sessionValidationComplete, setSessionValidationComplete] = useState(
+    !needsSessionValidation
+  );
+
+  const hasValidated = useRef(false);
+  useEffect(() => {
+    // This ensures the useEffect is only ran once
+    if (hasValidated.current) return;
+    hasValidated.current = true;
+
+    if (!needsSessionValidation) {
+      setSessionValidationComplete(true);
+      return;
+    }
+
+    (async () => {
+      const isValid = await session.isValid(sessionToken!);
+      if (!isValid) {
+        discardSessionToken();
+      }
+      setSessionValidationComplete(true);
+    })();
+  }, [needsSessionValidation, session, sessionToken]);
+
   const beginSigninHandler: BeginSigninHandler = useCallback(
     async (email: string, password: string) => {
       // - If the user came from email-first WITHOUT a linked third‑party account, Index already showed
@@ -545,8 +586,12 @@ const SigninContainer = ({
     );
   }
 
-  // Wait for async call (if needed) to complete
-  if (hasLinkedAccount === undefined || hasPassword === undefined) {
+  // Wait for async calls (if needed) to complete
+  if (
+    hasLinkedAccount === undefined ||
+    hasPassword === undefined ||
+    !sessionValidationComplete
+  ) {
     return (
       <AppLayout
         {...{ cmsInfo, loading: true, splitLayout, setCurrentSplitLayout }}
@@ -664,8 +709,10 @@ export async function trySignIn(
           metricsEnabled: response.metricsEnabled ?? true,
           emailVerified: response.emailVerified ?? false,
           sessionVerified: response.sessionVerified ?? false,
-          verificationMethod: (response.verificationMethod || VerificationMethods.EMAIL_OTP) as VerificationMethods,
-          verificationReason: response.verificationReason as VerificationReasons,
+          verificationMethod: (response.verificationMethod ||
+            VerificationMethods.EMAIL_OTP) as VerificationMethods,
+          verificationReason:
+            response.verificationReason as VerificationReasons,
           keyFetchToken: response.keyFetchToken,
         },
         ...(options.keys && {