feat(password): Don't revoke recovery key when doing a password change

Author: vbudhram

packages/db-migrations/databases/fxa/patches/patch-159-160.sql

@@ -0,0 +1,68 @@
+SET NAMES utf8mb4 COLLATE utf8mb4_bin;
+
+CALL assertPatchLevel('159');
+
+CREATE PROCEDURE `resetAccount_19` (
+  IN `uidArg` BINARY(16),
+  IN `verifyHashArg` BINARY(32),
+  IN `verifyHashVersion2Arg` BINARY(32),
+  IN `authSaltArg` BINARY(32),
+  IN `clientSaltArg` VARCHAR(128),
+  IN `wrapWrapKbArg` BINARY(32),
+  IN `wrapWrapKbVersion2Arg` BINARY(32),
+  IN `verifierSetAtArg` BIGINT UNSIGNED,
+  IN `verifierVersionArg` TINYINT UNSIGNED,
+  IN `keysHaveChangedArg` BOOLEAN,
+  IN `isPasswordUpgrade` BOOLEAN
+)
+BEGIN
+  DECLARE EXIT HANDLER FOR SQLEXCEPTION
+  BEGIN
+    ROLLBACK;
+    RESIGNAL;
+  END;
+
+  START TRANSACTION;
+
+  -- When we upgrade accounts to key stretching v2, we do
+  -- an 'automated reset' for the user. When we do this, we
+  -- preserve the underlying private key, so there's actually
+  -- no reason to drop associated data.
+  IF isPasswordUpgrade = 0 THEN
+    DELETE FROM sessionTokens WHERE uid = uidArg;
+    DELETE FROM keyFetchTokens WHERE uid = uidArg;
+    DELETE FROM accountResetTokens WHERE uid = uidArg;
+    DELETE FROM passwordChangeTokens WHERE uid = uidArg;
+    DELETE FROM passwordForgotTokens WHERE uid = uidArg;
+    DELETE devices, deviceCommands FROM devices LEFT JOIN deviceCommands
+        ON (deviceCommands.uid = devices.uid AND deviceCommands.deviceId = devices.id)
+        WHERE devices.uid = uidArg;  DELETE FROM unverifiedTokens WHERE uid = uidArg;
+  END IF;
+
+  -- We should only revoke the recovery key if the users private key has changed.
+  IF keysHaveChangedArg = 1 THEN
+    DELETE FROM recoveryKeys WHERE uid = uidArg;
+  END IF;
+
+  UPDATE accounts
+  SET
+    verifyHash = verifyHashArg,
+    verifyHashVersion2 = verifyHashVersion2Arg,
+    wrapWrapKb = wrapWrapKbArg,
+    wrapWrapKbVersion2 = wrapWrapKbVersion2Arg,
+    authSalt = authSaltArg,
+    clientSalt = clientSaltArg,
+    verifierVersion = verifierVersionArg,
+    profileChangedAt = verifierSetAtArg,
+    -- The `keysChangedAt` column was added in a migration, so its default value
+    -- is NULL meaning "we don't know".  Now that we do know whether or not the keys
+    -- are being changed, ensure it gets set to some concrete non-NULL value.
+    keysChangedAt = IF(keysHaveChangedArg, verifierSetAtArg,  COALESCE(keysChangedAt, verifierSetAt, createdAt)),
+    verifierSetAt = verifierSetAtArg,
+    lockedAt = NULL
+  WHERE uid = uidArg;
+
+  COMMIT;
+END;
+
+UPDATE dbMetadata SET value = '160' WHERE name = 'schema-patch-level';

packages/db-migrations/databases/fxa/patches/patch-160-159.sql

@@ -0,0 +1,3 @@
+-- DROP PROCEDURE `CREATE PROCEDURE `resetAccount_19`;
+
+-- UPDATE dbMetadata SET value = '159' WHERE name = 'schema-patch-level';

packages/db-migrations/databases/fxa/target-patch.json

@@ -1,3 +1,3 @@
 {
-  "level": 159
+  "level": 160
 }

packages/functional-tests/pages/resetPassword.ts

@@ -218,4 +218,8 @@ export class ResetPasswordPage extends BaseLayout {
     ]);
     return download;
   }
+
+  async continueWithoutDownloadingRecoveryKey() {
+    return this.page.getByText('Continue without downloading').click();
+  }
 }

packages/functional-tests/tests/misc/recoveryKeyPromoInline.spec.ts

@@ -43,7 +43,13 @@ test.describe('recovery key promo', () => {
 
     test('not shown if user already has a recovery key', async ({
       target,
-      pages: { page, signin, connectAnotherDevice, settings, recoveryKey },
+      syncBrowserPages: {
+        page,
+        signin,
+        connectAnotherDevice,
+        settings,
+        recoveryKey,
+      },
       testAccountTracker,
     }) => {
       const credentials = await testAccountTracker.signUp();

packages/functional-tests/tests/misc/recoveryKeyPromoSettingsBanner.spec.ts

@@ -8,7 +8,13 @@ test.describe('recovery key promo', () => {
   test.describe('settings banner', () => {
     test('can setup recovery key from settings promo banner', async ({
       target,
-      pages: { page, inlineRecoveryKey, signin, settings, recoveryKey },
+      syncBrowserPages: {
+        page,
+        inlineRecoveryKey,
+        signin,
+        settings,
+        recoveryKey,
+      },
       testAccountTracker,
     }) => {
       const credentials = await testAccountTracker.signUp();
@@ -32,7 +38,13 @@ test.describe('recovery key promo', () => {
 
     test('can dismiss', async ({
       target,
-      pages: { page, inlineRecoveryKey, signin, settings, recoveryKey },
+      syncBrowserPages: {
+        page,
+        inlineRecoveryKey,
+        signin,
+        settings,
+        recoveryKey,
+      },
       testAccountTracker,
     }) => {
       const credentials = await testAccountTracker.signUp();

packages/functional-tests/tests/resetPassword/resetPasswordRecoveryKey.spec.ts

@@ -31,6 +31,8 @@ test.describe('severity-1 #smoke', () => {
         settings
       );
 
+      await settings.signOut();
+
       // Stash original encryption keys to be verified later
       const accountData = await target.authClient.sessionReauth(
         credentials.sessionToken,
@@ -127,6 +129,8 @@ test.describe('severity-1 #smoke', () => {
 
       await enableRecoveryKey(credentials.password, recoveryKey, settings);
 
+      await settings.signOut();
+
       await resetPassword.goto();
 
       await resetPassword.fillOutEmailForm(credentials.email);
@@ -149,6 +153,60 @@ test.describe('severity-1 #smoke', () => {
       // update password for cleanup function
       credentials.password = newPassword;
     });
+
+    test('can reset password with recovery key after changing password in settings', async ({
+      target,
+      pages: { settings, recoveryKey, resetPassword, signin, changePassword },
+      testAccountTracker,
+    }) => {
+      const credentials = await testAccountTracker.signUp();
+      const newPassword = testAccountTracker.generatePassword();
+
+      await signinAccount(
+        credentials.email,
+        credentials.password,
+        settings,
+        signin
+      );
+
+      const key = await enableRecoveryKey(
+        credentials.password,
+        recoveryKey,
+        settings
+      );
+
+      // Change password
+      await settings.password.changeButton.click();
+      await changePassword.fillOutChangePassword(
+        credentials.password,
+        newPassword
+      );
+
+      await expect(settings.settingsHeading).toBeVisible();
+      await expect(settings.alertBar).toHaveText('Password updated');
+
+      await resetPassword.goto();
+
+      await resetPassword.fillOutEmailForm(credentials.email);
+      const code = await target.emailClient.getResetPasswordCode(
+        credentials.email
+      );
+
+      await resetPassword.fillOutResetPasswordCodeForm(code);
+
+      await resetPassword.fillOutRecoveryKeyForm(key);
+      await resetPassword.fillOutNewPasswordForm(newPassword);
+
+      await resetPassword.continueWithoutDownloadingRecoveryKey();
+
+      await resetPassword.recoveryKeyHintTextbox.fill('area 51');
+      await resetPassword.recoveryKeyFinishButton.click();
+
+      await expect(settings.recoveryKey.status).toHaveText('Enabled');
+
+      // update password for cleanup function
+      credentials.password = newPassword;
+    });
   });
 
   async function signinAccount(
@@ -177,8 +235,6 @@ test.describe('severity-1 #smoke', () => {
     await expect(settings.settingsHeading).toBeVisible();
     await expect(settings.recoveryKey.status).toHaveText('Enabled');
 
-    await settings.signOut();
-
     return key;
   }
 });

packages/fxa-shared/db/models/auth/base-auth.ts

@@ -52,7 +52,7 @@ export enum Proc {
   Prune = 'prune_10',
   RecoveryCodes = 'recoveryCodes_1',
   RecoveryKey = 'getRecoveryKey_4',
-  ResetAccount = 'resetAccount_18',
+  ResetAccount = 'resetAccount_19',
   ResetAccountTokens = 'resetAccountTokens_1',
   SessionWithDevice = 'sessionWithDevice_19',
   Sessions = 'sessions_13',