diff --git a/packages/fxa-auth-client/lib/client.ts b/packages/fxa-auth-client/lib/client.ts index a74f4e03567..90df07d123a 100644 --- a/packages/fxa-auth-client/lib/client.ts +++ b/packages/fxa-auth-client/lib/client.ts @@ -149,6 +149,7 @@ export type SessionStatus = { uid: string; details: { accountEmailVerified: boolean; + mustVerify: boolean; sessionVerificationMethod: string | null; sessionVerified: boolean; sessionVerificationMeetsMinimumAAL: boolean; diff --git a/packages/fxa-auth-server/lib/routes/session.js b/packages/fxa-auth-server/lib/routes/session.js index 55e12d83edb..9b084b5916a 100644 --- a/packages/fxa-auth-server/lib/routes/session.js +++ b/packages/fxa-auth-server/lib/routes/session.js @@ -316,6 +316,7 @@ module.exports = function ( uid: isA.string().regex(HEX_STRING).required(), details: isA.object({ accountEmailVerified: isA.boolean(), + mustVerify: isA.boolean(), sessionVerificationMethod: isA.string().allow(null), sessionVerified: isA.boolean(), sessionVerificationMeetsMinimumAAL: isA.boolean(), @@ -359,6 +360,7 @@ module.exports = function ( uid: sessionToken.uid, details: { accountEmailVerified, + mustVerify: !!sessionToken.mustVerify, sessionVerificationMethod, sessionVerified, sessionVerificationMeetsMinimumAAL, diff --git a/packages/fxa-auth-server/lib/routes/session.spec.ts b/packages/fxa-auth-server/lib/routes/session.spec.ts index 99940acde5d..7dc1dfb9b29 100644 --- a/packages/fxa-auth-server/lib/routes/session.spec.ts +++ b/packages/fxa-auth-server/lib/routes/session.spec.ts @@ -204,6 +204,7 @@ describe('/session/status', () => { state: 'unverified', details: { accountEmailVerified: false, + mustVerify: false, sessionVerificationMeetsMinimumAAL: true, sessionVerificationMethod: 'totp-2fa', sessionVerified: false, @@ -242,6 +243,7 @@ describe('/session/status', () => { state: 'unverified', details: { accountEmailVerified: false, + mustVerify: false, sessionVerificationMethod: 'email', sessionVerified: false, verified: false, @@ -278,6 +280,7 @@ describe('/session/status', () => { state: 'unverified', details: { accountEmailVerified: false, + mustVerify: false, sessionVerificationMethod: 'email', sessionVerified: false, verified: false, @@ -314,6 +317,7 @@ describe('/session/status', () => { state: 'unverified', details: { accountEmailVerified: true, + mustVerify: false, sessionVerificationMethod: 'email', sessionVerified: false, verified: false, @@ -350,6 +354,7 @@ describe('/session/status', () => { state: 'verified', details: { accountEmailVerified: true, + mustVerify: false, sessionVerificationMethod: 'email', sessionVerified: true, verified: true, @@ -385,6 +390,7 @@ describe('/session/status', () => { state: 'verified', details: { accountEmailVerified: true, + mustVerify: false, sessionVerificationMethod: 'email', sessionVerified: true, verified: true, @@ -421,6 +427,7 @@ describe('/session/status', () => { state: 'verified', details: { accountEmailVerified: true, + mustVerify: false, sessionVerificationMethod: 'totp-2fa', sessionVerified: true, verified: true, diff --git a/packages/fxa-auth-server/test/remote/session_tests.in.spec.ts b/packages/fxa-auth-server/test/remote/session_tests.in.spec.ts index 6922afa118d..cb8b9356720 100644 --- a/packages/fxa-auth-server/test/remote/session_tests.in.spec.ts +++ b/packages/fxa-auth-server/test/remote/session_tests.in.spec.ts @@ -2,13 +2,20 @@ * 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 { createTestServer, TestServerInstance } from '../support/helpers/test-server'; +import { + createTestServer, + TestServerInstance, +} from '../support/helpers/test-server'; const Client = require('../client')(); let server: TestServerInstance; beforeAll(async () => { + // Force sign-in confirmation so the suite is deterministic. With the + // default (skipForNewAccounts.enabled=true) brand-new accounts get a + // pre-verified session and we lose the `unverified` state the + // `reauth` and `status` tests rely on. server = await createTestServer({ configOverrides: { signinConfirmation: { @@ -37,7 +44,11 @@ describe.each(testVersions)( const email = server.uniqueEmail(); const password = 'foobar'; const client = await Client.createAndVerify( - server.publicUrl, email, password, server.mailbox, testOptions + server.publicUrl, + email, + password, + server.mailbox, + testOptions ); await client.sessionStatus(); @@ -57,7 +68,12 @@ describe.each(testVersions)( it('deletes a different custom token', async () => { const email = server.uniqueEmail(); const password = 'foobar'; - const client = await Client.create(server.publicUrl, email, password, testOptions); + const client = await Client.create( + server.publicUrl, + email, + password, + testOptions + ); const sessionTokenCreate = client.sessionToken; const sessions = await client.api.sessions(sessionTokenCreate); @@ -85,7 +101,12 @@ describe.each(testVersions)( it('fails with a bad custom token', async () => { const email = server.uniqueEmail(); const password = 'foobar'; - const client = await Client.create(server.publicUrl, email, password, testOptions); + const client = await Client.create( + server.publicUrl, + email, + password, + testOptions + ); const sessionTokenCreate = client.sessionToken; const c = await client.login(); @@ -107,7 +128,9 @@ describe.each(testVersions)( expect(err.code).toBe(401); expect(err.errno).toBe(110); expect(err.error).toBe('Unauthorized'); - expect(err.message).toBe('The authentication token could not be found'); + expect(err.message).toBe( + 'The authentication token could not be found' + ); } }); }); @@ -117,7 +140,11 @@ describe.each(testVersions)( const email = server.uniqueEmail(); const password = 'foobar'; const client1 = await Client.createAndVerify( - server.publicUrl, email, password, server.mailbox, testOptions + server.publicUrl, + email, + password, + server.mailbox, + testOptions ); const client2 = await client1.duplicate(); @@ -150,7 +177,12 @@ describe.each(testVersions)( it('creates independent verification state for the new token', async () => { const email = server.uniqueEmail(); const password = 'foobar'; - const client1 = await Client.create(server.publicUrl, email, password, testOptions); + const client1 = await Client.create( + server.publicUrl, + email, + password, + testOptions + ); const client2 = await client1.duplicate(); expect(client1.verified).toBeFalsy(); @@ -183,7 +215,10 @@ describe.each(testVersions)( const email = server.uniqueEmail(); const password = 'foobar'; const client = await Client.createAndVerify( - server.publicUrl, email, password, server.mailbox, + server.publicUrl, + email, + password, + server.mailbox, { ...testOptions, keys: true } ); @@ -205,7 +240,11 @@ describe.each(testVersions)( const email = server.uniqueEmail(); const password = 'foobar'; const client = await Client.createAndVerify( - server.publicUrl, email, password, server.mailbox, testOptions + server.publicUrl, + email, + password, + server.mailbox, + testOptions ); await client.setupCredentials(email, 'fiibar'); @@ -225,7 +264,12 @@ describe.each(testVersions)( it('has sane account-verification behaviour', async () => { const email = server.uniqueEmail(); const password = 'foobar'; - const client = await Client.create(server.publicUrl, email, password, testOptions); + const client = await Client.create( + server.publicUrl, + email, + password, + testOptions + ); expect(client.verified).toBeFalsy(); // Clear the verification email, without verifying. @@ -247,7 +291,10 @@ describe.each(testVersions)( const email = server.uniqueEmail(); const password = 'foobar'; await Client.createAndVerify( - server.publicUrl, email, password, server.mailbox, + server.publicUrl, + email, + password, + server.mailbox, { ...testOptions, keys: false } ); @@ -288,7 +335,10 @@ describe.each(testVersions)( const email = server.uniqueEmail(); const password = 'foobar'; const client = await Client.createAndVerify( - server.publicUrl, email, password, server.mailbox, + server.publicUrl, + email, + password, + server.mailbox, { ...testOptions, keys: true } ); @@ -307,7 +357,11 @@ describe.each(testVersions)( const email = server.uniqueEmail(); const password = 'testx'; const c = await Client.createAndVerify( - server.publicUrl, email, password, server.mailbox, testOptions + server.publicUrl, + email, + password, + server.mailbox, + testOptions ); const uid = c.uid; await c.login(); @@ -318,6 +372,10 @@ describe.each(testVersions)( uid, details: { accountEmailVerified: true, + // With signin confirmation forced (see configOverrides at top of file), + // every fresh login on a new account sets mustVerify=true because + // the server is going to require this session be verified. + mustVerify: true, sessionVerificationMeetsMinimumAAL: true, sessionVerificationMethod: null, sessionVerified: false, @@ -326,6 +384,21 @@ describe.each(testVersions)( }); }); + it('exposes mustVerify=true for a session that requires verification', async () => { + const email = server.uniqueEmail(); + const password = 'testx'; + const c = await Client.createAndVerify( + server.publicUrl, + email, + password, + server.mailbox, + testOptions + ); + await c.login({ keys: true }); + const status = await c.api.sessionStatus(c.sessionToken); + expect(status.details.mustVerify).toBe(true); + }); + it('errors with invalid token', async () => { const client = new Client(server.publicUrl, testOptions); try { diff --git a/packages/fxa-settings/src/models/mocks.tsx b/packages/fxa-settings/src/models/mocks.tsx index a87a1bfcccb..fd50e297d80 100644 --- a/packages/fxa-settings/src/models/mocks.tsx +++ b/packages/fxa-settings/src/models/mocks.tsx @@ -138,6 +138,7 @@ export function mockAuthClient() { details: { verified: true, accountEmailVerified: true, + mustVerify: false, sessionVerified: true, sessionVerificationMeetsMinimumAAL: true, sessionVerificationMethod: 'email', diff --git a/packages/fxa-settings/src/pages/Signin/container.test.tsx b/packages/fxa-settings/src/pages/Signin/container.test.tsx index e79f610c816..8a5a38d7aef 100644 --- a/packages/fxa-settings/src/pages/Signin/container.test.tsx +++ b/packages/fxa-settings/src/pages/Signin/container.test.tsx @@ -401,7 +401,7 @@ function mockSigninModule() { } function mockReactUtilsModule() { - jest.spyOn(ReactUtils, 'hardNavigate').mockImplementation(() => { }); + jest.spyOn(ReactUtils, 'hardNavigate').mockImplementation(() => {}); } let mockGetCredentials: jest.SpyInstance; @@ -1240,7 +1240,7 @@ describe('signin container', () => { // doesn't overwrite the initial hasPassword=false from locationState. mockAuthClient.accountStatusByEmail = jest .fn() - .mockReturnValue(new Promise(() => { })); + .mockReturnValue(new Promise(() => {})); mockLocationState = { email: MOCK_ROUTER_STATE_EMAIL, hasPassword: false, @@ -1410,6 +1410,7 @@ describe('signin container', () => { details: { accountEmailVerified: true, sessionVerified: false, + mustVerify: true, }, }); @@ -1489,6 +1490,7 @@ describe('signin container', () => { details: { accountEmailVerified: true, sessionVerified: false, + mustVerify: true, }, }); @@ -1511,6 +1513,41 @@ describe('signin container', () => { }); }); + it('skips verification when mustVerify=false even if session is unverified (FXA-12972 SUMO regression)', async () => { + // Mirrors the SUMO-style OAuth RP case: the session is unverified but + // the auth-server has decided it does not need verification + // (mustVerify=false). cachedSignIn must not send a verifyLoginCode + // email and must leave verificationMethod/verificationReason undefined + // so the user is not stranded on the confirm-code page. + mockSession.sendVerificationCode.mockClear(); + + mockAuthClient.accountProfile = jest.fn().mockResolvedValue({ + authenticationMethods: ['pwd', 'email'], + authenticatorAssuranceLevel: 1, + }); + mockAuthClient.sessionStatus = jest.fn().mockResolvedValue({ + details: { + accountEmailVerified: true, + sessionVerified: false, + mustVerify: false, + }, + }); + + mockUseValidateModule(); + render(); + + await waitFor(async () => { + const handlerResult = + await currentSigninProps?.cachedSigninHandler(MOCK_SESSION_TOKEN); + + expect(mockSession.sendVerificationCode).not.toHaveBeenCalled(); + expect(handlerResult?.data?.verificationMethod).toBeUndefined(); + expect(handlerResult?.data?.verificationReason).toBeUndefined(); + expect(handlerResult?.data?.sessionVerified).toEqual(false); + expect(handlerResult?.data?.emailVerified).toEqual(true); + }); + }); + it('does not return a verification reason with verified 2FA session', async () => { // Reset session mock for this test mockSession.sendVerificationCode.mockClear(); @@ -1608,7 +1645,7 @@ describe('signin container', () => { }); it('calls session.isValid and discards token when session is invalid', async () => { - const storedAccount: { sessionToken?: string;[key: string]: any } = { + const storedAccount: { sessionToken?: string; [key: string]: any } = { ...MOCK_STORED_ACCOUNT, email: MOCK_QUERY_PARAM_EMAIL, sessionToken: MOCK_SESSION_TOKEN, diff --git a/packages/fxa-settings/src/pages/Signin/utils.test.ts b/packages/fxa-settings/src/pages/Signin/utils.test.ts index c4f776d67f4..a24e4a1081c 100644 --- a/packages/fxa-settings/src/pages/Signin/utils.test.ts +++ b/packages/fxa-settings/src/pages/Signin/utils.test.ts @@ -4,11 +4,13 @@ import VerificationMethods from '../../constants/verification-methods'; import VerificationReasons from '../../constants/verification-reasons'; +import AuthenticationMethods from '../../constants/authentication-methods'; import { MOCK_EMAIL, MOCK_KEY_FETCH_TOKEN, MOCK_OAUTH_FLOW_HANDLER_RESPONSE, MOCK_SESSION_TOKEN, + MOCK_STORED_ACCOUNT, MOCK_UID, } from '../mocks'; import { NavigationOptions } from './interfaces'; @@ -18,14 +20,17 @@ import { createMockSigninOAuthIntegration, } from './mocks'; import { + cachedSignIn, handleNavigation, ensureCanLinkAcountOrRedirect, getSyncNavigate, } from './utils'; import * as ReachRouter from '@reach/router'; import * as ReactUtils from 'fxa-react/lib/utils'; +import * as CacheModule from '../../lib/cache'; import firefox from '../../lib/channels/firefox'; import config from '../../lib/config'; +import { AuthUiErrors } from '../../lib/auth-errors/auth-errors'; import { OAuthNativeServices } from '@fxa/accounts/oauth'; jest.mock('@reach/router', () => ({ @@ -583,4 +588,194 @@ describe('Signin utils', () => { }); }); }); + + describe('cachedSignIn', () => { + type SessionStatusDetails = { + accountEmailVerified: boolean; + mustVerify: boolean; + sessionVerified: boolean; + sessionVerificationMethod: string | null; + sessionVerificationMeetsMinimumAAL: boolean; + }; + + const createMockAuthClient = ({ + authenticationMethods = [] as AuthenticationMethods[], + details, + accountProfileRejection, + }: { + authenticationMethods?: AuthenticationMethods[]; + details?: Partial; + accountProfileRejection?: unknown; + } = {}) => { + const accountProfile = accountProfileRejection + ? jest.fn().mockRejectedValue(accountProfileRejection) + : jest.fn().mockResolvedValue({ authenticationMethods }); + const sessionStatus = jest.fn().mockResolvedValue({ + state: 'unverified', + uid: MOCK_UID, + details: { + accountEmailVerified: true, + mustVerify: false, + sessionVerified: false, + sessionVerificationMethod: null, + sessionVerificationMeetsMinimumAAL: true, + ...details, + }, + }); + return { accountProfile, sessionStatus } as any; + }; + + const createMockSession = () => + ({ + sendVerificationCode: jest.fn().mockResolvedValue(undefined), + }) as any; + + let currentAccountSpy: jest.SpyInstance; + let discardSessionTokenSpy: jest.SpyInstance; + + beforeEach(() => { + currentAccountSpy = jest + .spyOn(CacheModule, 'currentAccount') + .mockReturnValue(MOCK_STORED_ACCOUNT); + discardSessionTokenSpy = jest + .spyOn(CacheModule, 'discardSessionToken') + .mockImplementation(() => {}); + }); + + afterEach(() => { + currentAccountSpy.mockRestore(); + discardSessionTokenSpy.mockRestore(); + }); + + it('sends a verification code and returns SIGN_IN/EMAIL_OTP when mustVerify=true and session is unverified', async () => { + const authClient = createMockAuthClient({ + details: { mustVerify: true, sessionVerified: false }, + }); + const session = createMockSession(); + + const result = await cachedSignIn( + MOCK_SESSION_TOKEN, + authClient, + session + ); + + expect(session.sendVerificationCode).toHaveBeenCalledTimes(1); + expect(result.data?.verificationReason).toBe(VerificationReasons.SIGN_IN); + expect(result.data?.verificationMethod).toBe( + VerificationMethods.EMAIL_OTP + ); + expect(result.data?.sessionVerified).toBe(false); + expect(result.data?.emailVerified).toBe(true); + expect(result.data?.totpIsActive).toBe(false); + }); + + it('does NOT send a verification code and leaves verification fields undefined when mustVerify=false (regression test for SUMO bug)', async () => { + const authClient = createMockAuthClient({ + details: { mustVerify: false, sessionVerified: false }, + }); + const session = createMockSession(); + + const result = await cachedSignIn( + MOCK_SESSION_TOKEN, + authClient, + session + ); + + expect(session.sendVerificationCode).not.toHaveBeenCalled(); + expect(result.data?.verificationReason).toBeUndefined(); + expect(result.data?.verificationMethod).toBeUndefined(); + expect(result.data?.sessionVerified).toBe(false); + expect(result.data?.emailVerified).toBe(true); + }); + + it('returns TOTP_2FA without sending an email when mustVerify=true and TOTP is active', async () => { + const authClient = createMockAuthClient({ + authenticationMethods: [AuthenticationMethods.OTP], + details: { mustVerify: true, sessionVerified: false }, + }); + const session = createMockSession(); + + const result = await cachedSignIn( + MOCK_SESSION_TOKEN, + authClient, + session + ); + + expect(session.sendVerificationCode).not.toHaveBeenCalled(); + expect(result.data?.verificationReason).toBe(VerificationReasons.SIGN_IN); + expect(result.data?.verificationMethod).toBe( + VerificationMethods.TOTP_2FA + ); + expect(result.data?.totpIsActive).toBe(true); + }); + + it('sends the verification email on the SIGN_UP branch when the primary email is unverified, independent of mustVerify', async () => { + const authClient = createMockAuthClient({ + details: { + accountEmailVerified: false, + mustVerify: false, + sessionVerified: false, + }, + }); + const session = createMockSession(); + + const result = await cachedSignIn( + MOCK_SESSION_TOKEN, + authClient, + session + ); + + expect(session.sendVerificationCode).toHaveBeenCalledTimes(1); + expect(result.data?.verificationReason).toBe(VerificationReasons.SIGN_UP); + expect(result.data?.verificationMethod).toBe( + VerificationMethods.EMAIL_OTP + ); + }); + + it('never sends a verification code when isOauthPromptNone is true', async () => { + const authClient = createMockAuthClient({ + details: { mustVerify: true, sessionVerified: false }, + }); + const session = createMockSession(); + + await cachedSignIn(MOCK_SESSION_TOKEN, authClient, session, true); + + expect(session.sendVerificationCode).not.toHaveBeenCalled(); + }); + + it('returns no verification fields when the session is already verified', async () => { + const authClient = createMockAuthClient({ + details: { mustVerify: false, sessionVerified: true }, + }); + const session = createMockSession(); + + const result = await cachedSignIn( + MOCK_SESSION_TOKEN, + authClient, + session + ); + + expect(session.sendVerificationCode).not.toHaveBeenCalled(); + expect(result.data?.verificationReason).toBeUndefined(); + expect(result.data?.verificationMethod).toBeUndefined(); + expect(result.data?.sessionVerified).toBe(true); + }); + + it('discards the session token and returns SESSION_EXPIRED when accountProfile reports an invalid token', async () => { + const authClient = createMockAuthClient({ + accountProfileRejection: { errno: AuthUiErrors.INVALID_TOKEN.errno }, + }); + const session = createMockSession(); + + const result = await cachedSignIn( + MOCK_SESSION_TOKEN, + authClient, + session + ); + + expect(discardSessionTokenSpy).toHaveBeenCalledTimes(1); + expect(result.error).toBe(AuthUiErrors.SESSION_EXPIRED); + expect(session.sendVerificationCode).not.toHaveBeenCalled(); + }); + }); }); diff --git a/packages/fxa-settings/src/pages/Signin/utils.ts b/packages/fxa-settings/src/pages/Signin/utils.ts index 1c0ffb47063..edbb870644c 100644 --- a/packages/fxa-settings/src/pages/Signin/utils.ts +++ b/packages/fxa-settings/src/pages/Signin/utils.ts @@ -153,6 +153,7 @@ export const cachedSignIn = async ( const { details } = await authClient.sessionStatus(sessionToken); const sessionVerified = details.sessionVerified; const emailVerified = details.accountEmailVerified; + const mustVerify = details.mustVerify; let verificationMethod; let verificationReason; @@ -161,9 +162,12 @@ export const cachedSignIn = async ( verificationReason = VerificationReasons.SIGN_UP; verificationMethod = VerificationMethods.EMAIL_OTP; !isOauthPromptNone && (await session.sendVerificationCode()); - } else if (!sessionVerified) { - // we are inferring verification method and verification reason here - // ideally we would check the server directly to allow for more verification reasons + } else if (!sessionVerified && mustVerify) { + // Only route the user through verification (and pre-send a code) when + // the auth-server actually requires this session to be verified. + // Otherwise we'd send a `verifyLoginCode` email for OAuth RPs that + // don't need confirmation (e.g. SUMO). Mirrors fxa-content-server's + // legacy guard in views/base.js#checkAuthorization. verificationReason = VerificationReasons.SIGN_IN; if (totpIsActive) { verificationMethod = VerificationMethods.TOTP_2FA;