Merge pull request #19501 from mozilla/FXA-12406-v2

fix(sessions): Add env var for unverified session state + bandaid FE fix

Author: LZoog

packages/functional-tests/tests/signin/signinCached.spec.ts

@@ -4,7 +4,6 @@
 
 import { Page, expect, test } from '../../lib/fixtures/standard';
 import { BaseTarget, Credentials } from '../../lib/targets/base';
-import { TestAccountTracker } from '../../lib/testAccountTracker';
 import { SettingsPage } from '../../pages/settings';
 import { SigninPage } from '../../pages/signin';
 
@@ -15,12 +14,13 @@ test.describe('severity-2 #smoke', () => {
       syncBrowserPages: { page, settings, signin },
       testAccountTracker,
     }) => {
-      const { email } = await signInSyncAccount(
+      const credentials = await testAccountTracker.signUp();
+      const { email } = await signInAccount(
         target,
         page,
         settings,
         signin,
-        testAccountTracker
+        credentials
       );
 
       await signin.clearSessionStorage();
@@ -40,12 +40,13 @@ test.describe('severity-2 #smoke', () => {
       syncBrowserPages: { page, settings, signin },
       testAccountTracker,
     }) => {
-      const { email } = await signInSyncAccount(
+      const credentials = await testAccountTracker.signUp();
+      const { email } = await signInAccount(
         target,
         page,
         settings,
         signin,
-        testAccountTracker
+        credentials
       );
 
       await signin.clearSessionStorage();
@@ -72,13 +73,8 @@ test.describe('severity-2 #smoke', () => {
       syncBrowserPages: { page, settings, signin },
       testAccountTracker,
     }) => {
-      const credentials = await signInSyncAccount(
-        target,
-        page,
-        settings,
-        signin,
-        testAccountTracker
-      );
+      const credentials = await testAccountTracker.signUp();
+      await signInAccount(target, page, settings, signin, credentials);
       await signin.destroySession(credentials.email);
       await page.goto(target.contentServerUrl);
 
@@ -101,13 +97,8 @@ test.describe('severity-2 #smoke', () => {
       syncBrowserPages: { page, settings, signin },
       testAccountTracker,
     }) => {
-      const credentials = await signInSyncAccount(
-        target,
-        page,
-        settings,
-        signin,
-        testAccountTracker
-      );
+      const credentials = await testAccountTracker.signUp();
+      await signInAccount(target, page, settings, signin, credentials);
 
       await page.goto(target.contentServerUrl);
 
@@ -171,45 +162,40 @@ test.describe('severity-2 #smoke', () => {
       syncBrowserPages: { page, settings, signin },
       testAccountTracker,
     }) => {
-      const credentials = await testAccountTracker.signUp();
-      const syncCredentials = await signInSyncAccount(
-        target,
-        page,
-        settings,
-        signin,
-        testAccountTracker
-      );
+      const credentials1 = await testAccountTracker.signUp();
+      const credentials2 = await testAccountTracker.signUp();
+
+      await signInAccount(target, page, settings, signin, credentials2);
 
       await page.goto(target.contentServerUrl);
 
       //Check prefilled email
-      await expect(page.getByText(syncCredentials.email)).toBeVisible();
+      await expect(page.getByText(credentials2.email)).toBeVisible();
 
       await signin.useDifferentAccountLink.click();
-      await signin.fillOutEmailFirstForm(credentials.email);
-      await signin.fillOutPasswordForm(credentials.password);
+      await signin.fillOutEmailFirstForm(credentials1.email);
+      await signin.fillOutPasswordForm(credentials1.password);
 
       //Verify logged in on Settings page
       await expect(settings.settingsHeading).toBeVisible();
-      await expect(settings.primaryEmail.status).toHaveText(credentials.email);
+      await expect(settings.primaryEmail.status).toHaveText(credentials1.email);
       await settings.signOut();
 
       await expect(signin.cachedSigninHeading).toBeVisible();
 
       // Check that suggested cached account is the sync account
-      await expect(page.getByText(syncCredentials.email)).toBeVisible();
+      await expect(page.getByText(credentials2.email)).toBeVisible();
     });
   });
 });
 
-async function signInSyncAccount(
+async function signInAccount(
   target: BaseTarget,
   page: Page,
   settings: SettingsPage,
   signin: SigninPage,
-  testAccountTracker: TestAccountTracker
+  credentials: Credentials
 ): Promise<Credentials> {
-  const credentials = await testAccountTracker.signUpSync();
   await page.goto(target.contentServerUrl);
   await page.waitForURL(target.contentServerUrl);
   await signin.fillOutEmailFirstForm(credentials.email);

packages/fxa-auth-server/config/index.ts

@@ -1600,11 +1600,17 @@ const convictConf = convict({
       },
     },
     forceGlobally: {
-      doc: 'Force sign-in confirmation for all accounts',
+      doc: 'Force sign-in confirmation for all accounts. Sets "mustVerify: 1" on created session tokens and creates an entry in unverifiedTokens, simulating a suspicious request or requesting scoped keys',
       format: Boolean,
       default: false,
       env: 'SIGNIN_CONFIRMATION_FORCE_GLOBALLY',
     },
+    tokenVerification: {
+      doc: 'If set to false, force sign-in confirmation for logins that do not request scoped keys. Sets "mustVerify: 0" on created session tokens but creates an entry in unverifiedTokens, simulating an unverified session state',
+      format: Boolean,
+      default: true,
+      env: 'SIGNIN_CONFIRMATION_TOKEN_VERIFICATION',
+    },
   },
   forcePasswordChange: {
     forcedEmailAddresses: {

packages/fxa-auth-server/lib/routes/account.ts

@@ -1075,6 +1075,10 @@ export class AccountHandler {
     };
 
     const skipTokenVerification = (request: AuthRequest, account: any) => {
+      // Skip all checks to simulate an unverified session token state
+      if (this.config.signinConfirmation.tokenVerification === false) {
+        return false;
+      }
       // If they're logging in from an IP address on which they recently did
       // another, successfully-verified login, then we can consider this one
       // verified as well without going through the loop again.

packages/fxa-auth-server/lib/routes/attached-clients.js

@@ -122,8 +122,8 @@ module.exports = (log, db, devices, clientUtils) => {
       options: {
         ...DEVICES_AND_SESSIONS_DOC.ACCOUNT_ATTACHED_CLIENT_DESTROY_POST,
         auth: {
-          strategy: 'sessionToken',
-          payload: 'required',
+          strategy: 'verifiedSessionToken',
+          payload: 'false',
         },
         validate: {
           payload: isA

packages/fxa-auth-server/test/local/routes/attached-clients.js

@@ -419,7 +419,7 @@ describe('/account/attached_clients', () => {
 });
 
 describe('/account/attached_client/destroy', () => {
-  let config, uid, log, db, devices, request, route;
+  let config, uid, log, db, devices, request, route, accountRoutes;
 
   beforeEach(() => {
     config = {};
@@ -434,7 +434,7 @@ describe('/account/attached_client/destroy', () => {
       },
       payload: {},
     });
-    const accountRoutes = makeRoutes({
+    accountRoutes = makeRoutes({
       config,
       log,
       db,
@@ -443,6 +443,14 @@ describe('/account/attached_client/destroy', () => {
     route = getRoute(accountRoutes, '/account/attached_client/destroy').handler;
   });
 
+  it('requires verifiedSessionToken auth strategy', () => {
+    const routeConfig = getRoute(
+      accountRoutes,
+      '/account/attached_client/destroy'
+    );
+    assert.equal(routeConfig.options.auth.strategy, 'verifiedSessionToken');
+  });
+
   it('can destroy by deviceId', async () => {
     const deviceId = newId();
     request.payload = {

packages/fxa-graphql-api/src/gql/session.resolver.ts

@@ -53,7 +53,7 @@ export class SessionResolver {
   }
 
   @Query((returns) => SessionType)
-  @UseGuards(GqlAuthGuard)
+  @UseGuards(UnverifiedSessionGuard)
   session(@GqlUserId() uid: string, @GqlUserState() state: string) {
     this.log.info('session', { uid });
     return {

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

@@ -100,7 +100,9 @@ const AuthorizationContainer = ({
         const navigationOptions = {
           email: account?.email!,
           signinData: {
-            verified: data.verified,
+            // TODO, address signIn.verified vs session.verified discrepancy
+            // we're currently using 'sessionVerified' from recovery_email/status
+            verified: data.sessionVerified,
             verificationMethod: data.verificationMethod,
             verificationReason: data.verificationReason,
             uid: data.uid,

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

@@ -62,6 +62,7 @@ import {
   OAuthQueryParams,
   SigninQueryParams,
 } from '../../models/pages/signin';
+import { storeAccountData } from '../../lib/storage-utils';
 
 jest.mock('../../lib/channels/firefox', () => ({
   ...jest.requireActual('../../lib/channels/firefox'),
@@ -70,6 +71,11 @@ jest.mock('../../lib/channels/firefox', () => ({
   },
 }));
 
+jest.mock('../../lib/storage-utils', () => ({
+  ...jest.requireActual('../../lib/storage-utils'),
+  storeAccountData: jest.fn(),
+}));
+
 let integration: Integration;
 
 function mockSyncDesktopV3Integration() {
@@ -181,6 +187,7 @@ const mockSensitiveDataClient = createMockSensitiveDataClient();
 mockSensitiveDataClient.setDataType = jest.fn();
 
 const mockSession = {
+  isSessionVerified: jest.fn().mockResolvedValue(true),
   sendVerificationCode: jest.fn().mockResolvedValue(undefined),
   verified: false,
   token: MOCK_SESSION_TOKEN,
@@ -215,6 +222,8 @@ function mockModelsModule() {
     },
   }));
   (ModelsModule.useSession as jest.Mock).mockImplementation(() => mockSession);
+  mockSession.isSessionVerified = jest.fn().mockResolvedValue(true);
+  mockSession.sendVerificationCode = jest.fn().mockResolvedValue(undefined);
 }
 
 // Call this when testing local storage
@@ -569,6 +578,7 @@ describe('signin container', () => {
         expect(handlerResult?.data?.signIn?.verificationReason).toEqual(
           MOCK_VERIFICATION.verificationReason
         );
+        expect(storeAccountData).toHaveBeenCalled();
       });
     });
 
@@ -1035,6 +1045,10 @@ describe('signin container', () => {
         // Note, we cannot automatically upgrade unverified accounts, because the
         // reset password mechanism won't work in this case, so we want to fallback
         // to using V1 credentials in this scenario.
+        (ModelsModule.useSession as jest.Mock).mockImplementation(() => ({
+          ...mockSession,
+          isSessionVerified: jest.fn().mockResolvedValue(false),
+        }));
         render([
           mockGqlAvatarUseQuery(),
           mockGqlCredentialStatusMutation({

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

@@ -75,7 +75,11 @@ import {
   isUnsupportedContext,
 } from '../../models/integrations/utils';
 import { GqlKeyStretchUpgrade } from '../../lib/gql-key-stretch-upgrade';
-import { setCurrentAccount, StoredAccountData } from '../../lib/storage-utils';
+import {
+  setCurrentAccount,
+  storeAccountData,
+  StoredAccountData,
+} from '../../lib/storage-utils';
 import { cachedSignIn } from './utils';
 import OAuthDataError from '../../components/OAuthDataError';
 
@@ -419,15 +423,39 @@ const SigninContainer = ({
       // In this case, we should stash the credentials so we can try at a later
       // point in then flow after verification is complete.
       //
+      let isVerified = false;
+      if ('data' in result && result.data && 'signIn' in result.data) {
+        const accountData: StoredAccountData = {
+          email,
+          uid: result.data.signIn.uid,
+          lastLogin: Date.now(),
+          sessionToken: result.data.signIn.sessionToken,
+          // TODO, address signIn.verified vs session.verified discrepancy
+          verified: result.data.signIn.verified,
+          metricsEnabled: result.data.signIn.metricsEnabled,
+        };
+
+        storeAccountData(accountData);
+
+        // chicken and egg scenario where isSessionVerified GQL call needs
+        // the updated session token in local storage that we set with storeAccountData
+        // but we should update 'verified' once we know the real status
+        // this will be taken care of in the clean up ticket FXA-12454
+        isVerified = await session.isSessionVerified();
+        console.log('HELLO isVerified', isVerified);
+        storeAccountData({
+          ...accountData,
+          verified: isVerified,
+        });
+      }
+
       if (
         credentials.credentialStatus?.upgradeNeeded === true &&
         credentials.v2Credentials
       ) {
         let upgraded = false;
         if ('data' in result) {
-          const isVerified = result.data?.signIn.verified;
           const sessionToken = result.data?.signIn.sessionToken;
-
           if (sessionToken && isVerified) {
             upgraded = await upgradeClient.upgrade(
               email,
@@ -449,7 +477,19 @@ const SigninContainer = ({
         }
       }
 
-      return result;
+      // TODO, address signIn.verified vs session.verified discrepancy
+      // This currently overrides data.signIn.verified with session.isSessionVerified
+      return {
+        ...result,
+        ...('data' in result &&
+          result.data && {
+            data: {
+              ...result.data,
+              // Temporarily override signIn data with session token verification state
+              signIn: { ...result.data.signIn, verified: isVerified },
+            },
+          }),
+      };
     },
     [
       beginSignin,
@@ -465,6 +505,7 @@ const SigninContainer = ({
       authClient,
       sensitiveDataClient,
       originFromEmailFirst,
+      session,
     ]
   );
 

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

@@ -36,7 +36,6 @@ import {
 } from '../mocks';
 import { MozServices } from '../../lib/types';
 import * as utils from 'fxa-react/lib/utils';
-import { storeAccountData } from '../../lib/storage-utils';
 import VerificationMethods from '../../constants/verification-methods';
 import VerificationReasons from '../../constants/verification-reasons';
 import { SigninProps } from './interfaces';
@@ -371,7 +370,6 @@ describe('Signin component', () => {
             });
             expect(GleanMetrics.login.submit).toHaveBeenCalledTimes(1);
             expect(GleanMetrics.login.success).toHaveBeenCalledTimes(1);
-            expect(storeAccountData).toHaveBeenCalled();
           });
 
           it('navigates to /signin_totp_code when TOTP verification requested', async () => {