Merge pull request #18337 from mozilla/FXA-11025

dschom · web-flow · commit cda50844255e · 2025-02-10T16:19:20.000-08:00
task(auth): Have totp/destroy remove recovery phone
diff --git a/libs/accounts/recovery-phone/project.json b/libs/accounts/recovery-phone/project.json
@@ -1,9 +1,9 @@
 {
-  "name": "recovery-phone",
+  "name": "accounts-recovery-phone",
   "$schema": "../../../node_modules/nx/schemas/project-schema.json",
   "sourceRoot": "libs/accounts/recovery-phone/src",
   "projectType": "library",
-  "tags": [],
+  "tags": ["scope:shared:lib"],
   "targets": {
     "build": {
       "executor": "@nx/esbuild:esbuild",
diff --git a/libs/accounts/recovery-phone/src/lib/recovery-phone.manager.in.spec.ts b/libs/accounts/recovery-phone/src/lib/recovery-phone.manager.in.spec.ts
@@ -6,13 +6,15 @@ import {
   AccountDatabase,
   AccountDbProvider,
   testAccountDatabaseSetup,
+  RecoveryPhoneFactory,
 } from '@fxa/shared/db/mysql/account';
 import { Test } from '@nestjs/testing';
 import { RecoveryPhoneFactory } from '@fxa/shared/db/mysql/account';
 
 describe('RecoveryPhoneManager', () => {
   let recoveryPhoneManager: RecoveryPhoneManager;
   let db: AccountDatabase;
+  const dateMock = jest.spyOn(global.Date, 'now');
 
   // Taken from: https://www.twilio.com/docs/lookup/v2-api#code-lookup-with-data-packages
   const mockLookUpData: PhoneNumberLookupData = {
@@ -44,6 +46,7 @@ describe('RecoveryPhoneManager', () => {
   };
 
   beforeAll(async () => {
+    dateMock.mockImplementation(() => 1739227529776);
     db = await testAccountDatabaseSetup([
       'accounts',
       'recoveryPhones',
@@ -68,6 +71,7 @@ describe('RecoveryPhoneManager', () => {
 
   afterAll(async () => {
     await db.destroy();
+    dateMock.mockReset();
   });
 
   it('should get a recovery phone', async () => {
@@ -202,6 +206,7 @@ describe('RecoveryPhoneManager', () => {
     );
 
     const expectedData = JSON.stringify({
+      createdAt: 1739227529776,
       phoneNumber,
       isSetup,
       lookupData: JSON.stringify(mockLookUpData),
diff --git a/libs/accounts/recovery-phone/src/lib/recovery-phone.repository.ts b/libs/accounts/recovery-phone/src/lib/recovery-phone.repository.ts
@@ -29,7 +29,9 @@ export async function removePhoneNumber(
   const result = await db
     .deleteFrom('recoveryPhones')
     .where('uid', '=', uid)
-    .executeTakeFirstOrThrow();
+    // TBD: Didn't seem like handled an error coming off this call, so
+    // removed the orThrow...
+    .executeTakeFirst();
 
   return result.numDeletedRows === BigInt(1);
 }
diff --git a/libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts b/libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts
@@ -198,6 +198,12 @@ export class RecoveryPhoneService {
       uid
     );
 
+    // TBD: Random, obs. why do we do this? It seems like a potential edge case, what if the user
+    // consumes all the recovery codes? This could then return false...
+    //
+    // Just curious, about the rationale surrounding why this would block a user from removing their
+    // phone.
+    //
     if (!hasRecoveryCodes) {
       throw new RecoveryNumberRemoveMissingBackupCodes(uid);
     }
diff --git a/libs/accounts/two-factor/project.json b/libs/accounts/two-factor/project.json
@@ -3,7 +3,7 @@
   "$schema": "../../../node_modules/nx/schemas/project-schema.json",
   "sourceRoot": "libs/accounts/two-factor/src",
   "projectType": "library",
-  "tags": [],
+  "tags": ["scope:shared:lib"],
   "targets": {
     "build": {
       "executor": "@nx/esbuild:esbuild",
diff --git a/libs/accounts/two-factor/src/lib/backup-code.manager.in.spec.ts b/libs/accounts/two-factor/src/lib/backup-code.manager.in.spec.ts
@@ -92,4 +92,16 @@ describe('BackupCodeManager', () => {
       backupCodeManager.getCountForUserId(getMockUid())
     ).rejects.toThrow('Database error');
   });
+
+  it('should delete codes', async () => {
+    const mockUid1 = getMockUid();
+    const mockUid2 = getMockUid();
+    await createRecoveryCode(db, mockUid1);
+
+    const result1 = await backupCodeManager.deleteRecoveryCodes(mockUid1);
+    const result2 = await backupCodeManager.deleteRecoveryCodes(mockUid2);
+
+    expect(result1).toBeTruthy();
+    expect(result2).toBeFalsy();
+  });
 });
diff --git a/libs/accounts/two-factor/src/lib/backup-code.manager.ts b/libs/accounts/two-factor/src/lib/backup-code.manager.ts
@@ -7,7 +7,10 @@ import {
   AccountDbProvider,
 } from '@fxa/shared/db/mysql/account';
 import { Inject, Injectable } from '@nestjs/common';
-import { getRecoveryCodes } from './backup-code.repository';
+import {
+  deleteRecoveryCodes,
+  getRecoveryCodes,
+} from './backup-code.repository';
 
 @Injectable()
 export class BackupCodeManager {
@@ -34,4 +37,15 @@ export class BackupCodeManager {
       count: recoveryCodes.length,
     };
   }
+
+  /**
+   * Removes recover codes for a given uid.
+   *
+   * @param uid - The uid in hexadecimal string format.
+   * @returns True if one or more codes were removed
+   */
+  async deleteRecoveryCodes(uid: string) {
+    const success = await deleteRecoveryCodes(this.db, Buffer.from(uid, 'hex'));
+    return success;
+  }
 }
diff --git a/libs/accounts/two-factor/src/lib/backup-code.repository.ts b/libs/accounts/two-factor/src/lib/backup-code.repository.ts
@@ -10,3 +10,12 @@ export async function getRecoveryCodes(db: AccountDatabase, uid: Buffer) {
     .selectAll()
     .execute();
 }
+
+export async function deleteRecoveryCodes(db: AccountDatabase, uid: Buffer) {
+  const result = await db
+    .deleteFrom('recoveryCodes')
+    .where('uid', '=', uid)
+    .executeTakeFirst();
+
+  return result.numDeletedRows === BigInt(1);
+}
diff --git a/packages/fxa-auth-server/bin/key_server.js b/packages/fxa-auth-server/bin/key_server.js
@@ -130,7 +130,7 @@ async function run(config) {
   Container.set(AccountManager, accountManager);
 
   const backupCodeManager = new BackupCodeManager(accountDatabase);
-  Container.set('BackupCodeManager', backupCodeManager);
+  Container.set(BackupCodeManager, backupCodeManager);
 
   // Set currencyHelper before stripe and paypal helpers, so they can use it.
   try {
diff --git a/packages/fxa-auth-server/lib/routes/recovery-codes.js b/packages/fxa-auth-server/lib/routes/recovery-codes.js
@@ -10,13 +10,14 @@ const validators = require('./validators');
 const { Container } = require('typedi');
 const RECOVERY_CODES_DOCS =
   require('../../docs/swagger/recovery-codes-api').default;
+const { BackupCodeManager } = require('@fxa/accounts/two-factor');
 
 const RECOVERY_CODE_SANE_MAX_LENGTH = 20;
 
 module.exports = (log, db, config, customs, mailer, glean) => {
   const codeConfig = config.recoveryCodes;
   const RECOVERY_CODE_COUNT = (codeConfig && codeConfig.count) || 8;
-  const backupCodeManager = Container.get('BackupCodeManager');
+  const backupCodeManager = Container.get(BackupCodeManager);
 
   // Validate backup authentication codes
   const recoveryCodesSchema = validators.recoveryCodes(
diff --git a/packages/fxa-auth-server/lib/routes/totp.js b/packages/fxa-auth-server/lib/routes/totp.js
@@ -17,7 +17,10 @@ const { Container } = require('typedi');
 const { AccountEventsManager } = require('../account-events');
 const {
   RecoveryPhoneService,
-} = require('../../../../libs/accounts/recovery-phone/src');
+  RecoveryNumberNotExistsError,
+  RecoveryNumberRemoveMissingBackupCodes,
+} = require('@fxa/accounts/recovery-phone');
+const { BackupCodeManager } = require('@fxa/accounts/two-factor');
 
 const RECOVERY_CODE_SANE_MAX_LENGTH = 20;
 
@@ -54,6 +57,7 @@ module.exports = (
 
   const accountEventsManager = Container.get(AccountEventsManager);
   const recoveryPhoneService = Container.get(RecoveryPhoneService);
+  const backupCodeManager = Container.get(BackupCodeManager);
 
   // This helps us distinguish between testing environments and
   // totp codes per environment.
@@ -155,16 +159,6 @@ module.exports = (
         auth: {
           strategy: 'sessionToken',
         },
-        validate: {
-          payload: isA.object({
-            code: isA
-              .string()
-              .max(32)
-              .regex(validators.DIGITS)
-              .optional()
-              .description(DESCRIPTION.codeTotp),
-          }),
-        },
         response: {},
       },
       handler: async function (request) {
@@ -186,36 +180,6 @@ module.exports = (
           throw errors.unverifiedSession();
         }
 
-        // Will also use the recovery phone to validate the code.
-        const code = request.payload.code;
-        if (code) {
-          // Client has posted a code, but we need make it was a code that was actually
-          // sent to the client
-          const success = await recoveryPhoneService.confirmCode(uid, code);
-          if (!success) {
-            throw errors.invalidOrExpiredOtpCode();
-          }
-
-          // Next make sure the code hasn't expired
-          const token = await db.totpToken(sessionToken.uid);
-          const isValidCode = otpUtils.verifyOtpCode(
-            code,
-            token.sharedSecret,
-            {
-              encoding: 'hex',
-              step: config.step,
-              window: config.window,
-            },
-            'totp.destroy'
-          );
-          if (!isValidCode) {
-            throw errors.invalidOrExpiredOtpCode();
-          }
-
-          // Code seems legit. Record metric and proceed with totp deletion
-          glean.twoStepAuthRemove.success();
-        }
-
         await db.deleteTotpToken(uid);
 
         // Downgrade the session to email-based verification when TOTP is
@@ -268,6 +232,45 @@ module.exports = (
           tokenId: sessionToken && sessionToken.id,
         });
 
+        // Clean up the recovery phone if it was registered.
+        // Don't fail if this doesn't work, but monitor success rate with stats.
+        try {
+          const success = await recoveryPhoneService.removePhoneNumber(uid);
+          if (success) {
+            statsd.increment('totp.destroy.remove_phone_number.success');
+            await glean.twoStepAuthPhoneRemove.success(request);
+          } else {
+            statsd.increment('totp.destroy.remove_phone_number.fail');
+          }
+        } catch (error) {
+          if (
+            error instanceof RecoveryNumberNotExistsError ||
+            error instanceof RecoveryNumberRemoveMissingBackupCodes
+          ) {
+            statsd.increment('totp.destroy.remove_phone_number.fail');
+          } else {
+            statsd.increment('totp.destroy.remove_phone_number.error');
+            log.error('totp.destroy.remove_phone_number.error', error);
+          }
+        }
+
+        // Clean up any associated backup codes.
+        // Again, don't fail if errors out, but monitor with stats.
+        try {
+          const success = await backupCodeManager.deleteRecoveryCodes(uid);
+          if (success) {
+            statsd.increment('totp.destroy.delete_recovery_codes.success');
+          } else {
+            statsd.increment('totp.destroy.delete_recovery_codes.fail');
+          }
+        } catch (error) {
+          statsd.increment('totp.destroy.delete_recovery_codes.error');
+          log.error('totp.destroy.delete_recovery_codes.error', error);
+        }
+
+        // Record that the 2fa was successfully removed
+        glean.twoStepAuthRemove.success();
+
         return {};
       },
     },
diff --git a/packages/fxa-auth-server/test/local/routes/recovery-codes.js b/packages/fxa-auth-server/test/local/routes/recovery-codes.js
@@ -10,6 +10,7 @@ const getRoute = require('../../routes_helpers').getRoute;
 const mocks = require('../../mocks');
 const error = require('../../../lib/error');
 const { Container } = require('typedi');
+const { BackupCodeManager } = require('@fxa/accounts/two-factor');
 
 let log, db, customs, routes, route, request, requestOptions, mailer, glean;
 const TEST_EMAIL = 'test@email.com';
@@ -69,7 +70,7 @@ describe('backup authentication codes', () => {
         },
       },
     };
-    Container.set('BackupCodeManager', mockBackupCodeManager);
+    Container.set(BackupCodeManager, mockBackupCodeManager);
   });
 
   afterEach(() => {
diff --git a/packages/fxa-auth-server/test/local/routes/totp.js b/packages/fxa-auth-server/test/local/routes/totp.js
diff --git a/tsconfig.base.json b/tsconfig.base.json
