Merge pull request #17766 from mozilla/fix-dropped-rows-on-pw-upgrade

bug(auth): Prevent password upgrade from dropping assocaited rows

packages/db-migrations/databases/fxa/patches/patch-156-157.sql

@@ -0,0 +1,64 @@
+SET NAMES utf8mb4 COLLATE utf8mb4_bin;
+
+CALL assertPatchLevel('156');
+
+CREATE PROCEDURE `resetAccount_18` (
+  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 FROM recoveryKeys 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;
+
+  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 = '157' WHERE name = 'schema-patch-level';

packages/db-migrations/databases/fxa/patches/patch-157-156.sql

@@ -0,0 +1,2 @@
+-- DROP PROCEDURE `CREATE PROCEDURE `resetAccount_18`;
+-- UPDATE dbMetadata SET value = '156' WHERE name = 'schema-patch-level';

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

@@ -1,3 +1,3 @@
 {
-  "level": 156
+  "level": 157
 }

packages/functional-tests/tests/key-stretching-v2/recoveryKey.spec.ts

@@ -129,4 +129,41 @@ test.describe('severity-2 #smoke', () => {
       expect(keys2.kB).toEqual(keys.kB);
     });
   }
+
+  test(`recovery key is preserved upon upgrade`, async ({
+    page,
+    target,
+    pages: { signin, signup, settings, recoveryKey, confirmSignupCode },
+    testAccountTracker,
+  }) => {
+    const { email, password } = testAccountTracker.generateAccountDetails();
+    await page.goto(
+      `${target.contentServerUrl}/?forceExperiment=generalizedReactApp&forceExperimentGroup=react&stretch=1`
+    );
+    await signup.fillOutEmailForm(email);
+    await signup.fillOutSignupForm(password, AGE_21);
+    await expect(page).toHaveURL(/confirm_signup_code/);
+    const code = await target.emailClient.getVerifyShortCode(email);
+    await confirmSignupCode.fillOutCodeForm(code);
+
+    await expect(page).toHaveURL(/settings/);
+    await expect(settings.recoveryKey.status).toHaveText('Not Set');
+
+    await settings.recoveryKey.createButton.click();
+    await recoveryKey.createRecoveryKey(password, HINT);
+    await expect(settings.recoveryKey.status).toHaveText('Enabled');
+    await settings.signOut();
+
+    await page.goto(
+      `${target.contentServerUrl}/?forceExperiment=generalizedReactApp&forceExperimentGroup=react&stretch=2`
+    );
+    await signin.fillOutEmailFirstForm(email);
+    await signin.fillOutPasswordForm(password);
+
+    await expect(page).toHaveURL(/settings/);
+
+    await expect(page).toHaveURL(/settings/);
+    await expect(settings.settingsHeading).toBeVisible();
+    await expect(settings.recoveryKey.status).toHaveText('Enabled');
+  });
 });

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

@@ -343,6 +343,7 @@ module.exports = function (
                 wrapWrapKb: wrapWrapKb,
                 wrapWrapKbVersion2: wrapWrapKbVersion2,
                 keysHaveChanged: false,
+                isPasswordUpgrade: true,
               },
               true
             );

packages/fxa-settings/src/models/experiments/key-stretch-experiment.ts

@@ -34,6 +34,12 @@ export class KeyStretchExperiment extends ModelDataProvider {
       return true;
     }
 
+    // If stretch=1 in the URL, then force V1 key stretching for this page render,
+    // This is used for dev/test purposes.
+    if (this.stretch === '1') {
+      return false;
+    }
+
     // If force experiment params are in URL, then force V2 key stretching, and
     // automatically enroll in experiment, so that content server will pick it up.
     if (

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

@@ -197,6 +197,7 @@ export class Account extends BaseAuthModel {
     verifierSetAt,
     verifierVersion,
     keysHaveChanged,
+    isPasswordUpgrade,
   }: Pick<
     Account,
     | 'uid'
@@ -210,6 +211,7 @@ export class Account extends BaseAuthModel {
     | 'clientSalt'
   > & {
     keysHaveChanged?: boolean;
+    isPasswordUpgrade?: boolean;
   }) {
     return Account.callProcedure(
       Proc.ResetAccount,
@@ -222,7 +224,8 @@ export class Account extends BaseAuthModel {
       wrapWrapKbVersion2 ? uuidTransformer.to(wrapWrapKbVersion2) : null,
       verifierSetAt || Date.now(),
       verifierVersion,
-      !!keysHaveChanged
+      !!keysHaveChanged,
+      !!isPasswordUpgrade
     );
   }
 

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_17',
+  ResetAccount = 'resetAccount_18',
   ResetAccountTokens = 'resetAccountTokens_1',
   SessionWithDevice = 'sessionWithDevice_19',
   Sessions = 'sessions_13',