From f286dbedeab23a97acfb71a6595bcace5c23cede Mon Sep 17 00:00:00 2001 From: dschom Date: Mon, 13 May 2024 16:06:44 -0700 Subject: [PATCH] task(settings): Fallback to key stretch v1 in the event of upgrade failure --- .../tests/misc/keyStretchingV2.spec.ts | 103 +++++++++++++++++- packages/fxa-auth-client/lib/client.ts | 20 +++- packages/fxa-auth-client/lib/error.ts | 9 -- .../src/pages/Signin/container.tsx | 6 +- 4 files changed, 116 insertions(+), 22 deletions(-) delete mode 100644 packages/fxa-auth-client/lib/error.ts diff --git a/packages/functional-tests/tests/misc/keyStretchingV2.spec.ts b/packages/functional-tests/tests/misc/keyStretchingV2.spec.ts index 4e78006b2c..4bef7bb963 100644 --- a/packages/functional-tests/tests/misc/keyStretchingV2.spec.ts +++ b/packages/functional-tests/tests/misc/keyStretchingV2.spec.ts @@ -12,6 +12,8 @@ import { SettingsPage } from '../../pages/settings'; import { ChangePasswordPage } from '../../pages/settings/changePassword'; import { RecoveryKeyPage } from '../../pages/settings/recoveryKey'; import { SignupReactPage } from '../../pages/signupReact'; +import { TotpPage } from '../../pages/settings/totp'; +import { getCode } from 'fxa-settings/src/lib/totp'; // Disable this check for these tests. We are holding assertion in shared functions due // to how test permutations work, and these setup falsely trips this rule. @@ -42,6 +44,7 @@ test.describe('key-stretching-v2', () => { resetPasswordReact: ResetPasswordReactPage; changePassword: ChangePasswordPage; recoveryKey: RecoveryKeyPage; + totp: TotpPage; }; // Helpers @@ -119,7 +122,8 @@ test.describe('key-stretching-v2', () => { >, signOut: boolean, email: string, - password: string + password: string, + totpCredentials?: { secret: string; recoveryCodes: string[] } ) { const { page, target, signupReact, login, settings } = opts; const stretch = version === 2 ? 'stretch=2' : ''; @@ -130,16 +134,29 @@ test.describe('key-stretching-v2', () => { await signupReact.fillOutEmailForm(email); await page.fill('[name="password"]', password); await page.click('[type="submit"]'); - await page.waitForURL(/settings/); + if (totpCredentials) { + await page.waitForURL(/signin_totp_code/); + const code = await getCode(totpCredentials.secret); + await page.fill('[name="code"]', code); + await page.click('[type="submit"]'); + } } else { await page.goto(`${target.contentServerUrl}?${stretch}`); await login.setEmail(email); await login.clickSubmit(); await login.setPassword(password); await login.clickSubmit(); - expect(await login.isUserLoggedIn()).toBe(true); + if (totpCredentials) { + await page.waitForURL(/signin_totp_code/); + const code = await getCode(totpCredentials.secret); + await page.fill('[type="number"]', code); + await page.click('[type="submit"]'); + } } + await page.waitForURL(/settings/); + expect(await login.isUserLoggedIn()).toBe(true); + if (signOut) { await settings.signOut(); } @@ -158,6 +175,20 @@ test.describe('key-stretching-v2', () => { return await recoveryKey.createRecoveryKey(password, hint); } + async function _enabledTotp(opts: Pick) { + const { settings, totp } = opts; + + await expect(settings.settingsHeading).toBeVisible(); + await settings.totp.addButton.click(); + const totpCredentials = await totp.fillOutTotpForms(); + await expect(settings.settingsHeading).toBeVisible(); + await expect(settings.totp.status).toHaveText('Enabled'); + + await settings.signOut(); + + return totpCredentials; + } + async function _changePassword( version: 1 | 2, opts: Pick, @@ -328,6 +359,25 @@ test.describe('key-stretching-v2', () => { ); }); + async function testTotpLogin( + p1: 1 | 2, + p2: 1 | 2, + opts: Pick< + Opts, + 'page' | 'target' | 'signupReact' | 'settings' | 'login' | 'totp' + >, + email: string, + password: string + ) { + await _signUp(p1, opts, false, email, password); + const totpCredentials = await _enabledTotp(opts); + await _login(p2, opts, false, email, password, totpCredentials); + + // Remove 2fa to allow test cleanup + await opts.settings.totp.disableButton.click(); + await opts.settings.clickModalConfirm(); + } + /** * Checks password reset from 'forgot password' link on login */ @@ -878,6 +928,7 @@ test.describe('key-stretching-v2', () => { newPassword ); }); + test(`signs up as v2 changes password from settings as v2 for ${mode}`, async ({ page, target, @@ -902,5 +953,51 @@ test.describe('key-stretching-v2', () => { newPassword ); }); + + test(`signs up as v1, enable totp, login as v2 for ${mode}`, async ({ + page, + target, + pages: { settings, signupReact, login, totp }, + testAccountTracker, + }) => { + const { email, password } = testAccountTracker.generateAccountDetails(); + await testTotpLogin( + 1, + 2, + { + page, + target, + login, + signupReact, + settings, + totp, + }, + email, + password + ); + }); + + test(`signs up as v2, enable totp, login as v1 for ${mode}`, async ({ + page, + target, + pages: { settings, signupReact, login, totp }, + testAccountTracker, + }) => { + const { email, password } = testAccountTracker.generateAccountDetails(); + await testTotpLogin( + 2, + 1, + { + page, + target, + login, + signupReact, + settings, + totp, + }, + email, + password + ); + }); }); }); diff --git a/packages/fxa-auth-client/lib/client.ts b/packages/fxa-auth-client/lib/client.ts index 7b205b62b4..0e88167b30 100644 --- a/packages/fxa-auth-client/lib/client.ts +++ b/packages/fxa-auth-client/lib/client.ts @@ -4,9 +4,9 @@ import * as crypto from './crypto'; import { Credentials } from './crypto'; -import { PasswordV2UpgradeError } from './error'; import * as hawk from './hawk'; import { SaltVersion, createSaltV2 } from './salt'; +import * as Sentry from '@sentry/browser'; enum ERRORS { INVALID_TIMESTAMP = 111, @@ -448,14 +448,24 @@ export default class AuthClient { if (this.keyStretchVersion === 2) { if (credentials.upgradeNeeded) { // This condition indicates an upgrade already was attempted but did not take hold. - // If this state occurs, it means something went wrong, and an error should occur. - // If an error is not raised, we might end up in an infinite recursive loop! + // If this state occurs, it means something went wrong, and we want to fallback to + // v1 credentials and set the client's target key stretch version back to 1. if (options.postPasswordUpgrade === true) { - throw new PasswordV2UpgradeError(); + this.keyStretchVersion = 1; + return await this.signIn(email, password, { + ...options, + postPasswordUpgrade: true, + }); } // Try to upgrade the password, and sign in. - await this.passwordChange(email, password, password, options); + try { + await this.passwordChange(email, password, password, options); + } catch (err) { + Sentry.captureMessage( + 'Failure to complete v2 key stretch upgrade.' + ); + } return await this.signIn(email, password, { ...options, postPasswordUpgrade: true, diff --git a/packages/fxa-auth-client/lib/error.ts b/packages/fxa-auth-client/lib/error.ts deleted file mode 100644 index 5aad262bc9..0000000000 --- a/packages/fxa-auth-client/lib/error.ts +++ /dev/null @@ -1,9 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * 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/. */ - -export class PasswordV2UpgradeError extends Error { - constructor() { - super('V2 Password Upgrade Failed') - } -} diff --git a/packages/fxa-settings/src/pages/Signin/container.tsx b/packages/fxa-settings/src/pages/Signin/container.tsx index a8ef065b93..580b9e2cdf 100644 --- a/packages/fxa-settings/src/pages/Signin/container.tsx +++ b/packages/fxa-settings/src/pages/Signin/container.tsx @@ -228,10 +228,6 @@ const SigninContainer = ({ passwordChangeFinish ); - if (error) { - return { error }; - } - const options = { verificationMethod: VerificationMethods.EMAIL_OTP, keys: wantsKeys, @@ -243,7 +239,7 @@ const SigninContainer = ({ return await trySignIn( email, v1Credentials, - v2Credentials, + error ? undefined : v2Credentials, unverifiedAccount, beginSignin, options