Merge pull request #18264 from mozilla/add-recovery-enabled-config-check-to-all-recovery-phone-endpoints

task(auth): Check recovery-phone enabled config for all endpoints

Author: dschom

libs/accounts/recovery-phone/src/lib/recovery-phone.errors.ts

@@ -58,3 +58,9 @@ export class SmsSendRateLimitExceededError extends RecoveryPhoneError {
     );
   }
 }
+
+export class RecoveryPhoneNotEnabled extends RecoveryPhoneError {
+  constructor(cause?: Error) {
+    super('Recovery phone not enabled.', {}, cause);
+  }
+}

libs/accounts/recovery-phone/src/lib/recovery-phone.service.spec.ts

@@ -11,6 +11,7 @@ import { RecoveryPhoneManager } from './recovery-phone.manager';
 import {
   RecoveryNumberNotExistsError,
   RecoveryNumberNotSupportedError,
+  RecoveryPhoneNotEnabled,
 } from './recovery-phone.errors';
 
 describe('RecoveryPhoneService', () => {
@@ -335,12 +336,6 @@ describe('RecoveryPhoneService', () => {
       mockRecoveryPhoneConfig.allowedRegions = ['US'];
     });
 
-    it('should return false if config is not enabled', async () => {
-      mockRecoveryPhoneConfig.enabled = false;
-      const result = await service.available(uid, region);
-      expect(result).toBe(false);
-    });
-
     it('should return false if region is not in allowedRegions', async () => {
       mockRecoveryPhoneConfig.allowedRegions = ['USA'];
       const result = await service.available(uid, region);
@@ -368,6 +363,41 @@ describe('RecoveryPhoneService', () => {
     });
   });
 
+  describe('can disabled service', () => {
+    beforeAll(() => {
+      mockRecoveryPhoneConfig.enabled = false;
+    });
+    afterAll(() => {
+      mockRecoveryPhoneConfig.enabled = true;
+    });
+    it('can disable', () => {
+      expect(service.available(uid, 'US')).rejects.toEqual(
+        new RecoveryPhoneNotEnabled()
+      );
+      expect(service.confirmSetupCode(uid, '000000')).rejects.toEqual(
+        new RecoveryPhoneNotEnabled()
+      );
+      expect(service.confirmSigninCode(uid, '000000')).rejects.toEqual(
+        new RecoveryPhoneNotEnabled()
+      );
+      expect(service.hasConfirmed(uid)).rejects.toEqual(
+        new RecoveryPhoneNotEnabled()
+      );
+      expect(() => service.maskPhoneNumber('+15550005555')).toThrow(
+        new RecoveryPhoneNotEnabled()
+      );
+      expect(service.removePhoneNumber(uid)).rejects.toEqual(
+        new RecoveryPhoneNotEnabled()
+      );
+      expect(service.sendCode(uid)).rejects.toEqual(
+        new RecoveryPhoneNotEnabled()
+      );
+      expect(service.setupPhoneNumber(uid, '+15550005555')).rejects.toEqual(
+        new RecoveryPhoneNotEnabled()
+      );
+    });
+  });
+
   describe('mask phone number', () => {
     it('can mask number', () => {
       const phoneNumber = '+123456789';

libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts

@@ -10,6 +10,7 @@ import { RecoveryPhoneManager } from './recovery-phone.manager';
 import {
   RecoveryNumberNotExistsError,
   RecoveryNumberNotSupportedError,
+  RecoveryPhoneNotEnabled,
 } from './recovery-phone.errors';
 import { MessageInstance } from 'twilio/lib/rest/api/v2010/account/message';
 
@@ -31,7 +32,7 @@ export class RecoveryPhoneService {
    */
   public async available(uid: string, region: string): Promise<boolean> {
     if (!this.config.enabled) {
-      return false;
+      throw new RecoveryPhoneNotEnabled();
     }
 
     if (!this.config.allowedRegions?.includes(region)) {
@@ -55,6 +56,10 @@ export class RecoveryPhoneService {
    * @returns True if code was sent and stored
    */
   public async setupPhoneNumber(uid: string, phoneNumber: string) {
+    if (!this.config.enabled) {
+      throw new RecoveryPhoneNotEnabled();
+    }
+
     if (this.config.sms && this.config.sms.validNumberPrefixes) {
       const allowed = this.config.sms.validNumberPrefixes.some((check) => {
         return phoneNumber.startsWith(check);
@@ -92,6 +97,10 @@ export class RecoveryPhoneService {
    * @returns True if successful
    */
   public async confirmSetupCode(uid: string, code: string) {
+    if (!this.config.enabled) {
+      throw new RecoveryPhoneNotEnabled();
+    }
+
     const data = await this.recoveryPhoneManager.getUnconfirmed(uid, code);
 
     // If there is no data, it means there's no record of this code being sent to the uid provided
@@ -121,7 +130,17 @@ export class RecoveryPhoneService {
     return true;
   }
 
+  /**
+   * Confirms a signin code.
+   * @param uid An account id
+   * @param code A otp code
+   * @returns True if successful
+   */
   public async confirmSigninCode(uid: string, code: string) {
+    if (!this.config.enabled) {
+      throw new RecoveryPhoneNotEnabled();
+    }
+
     const data = await this.recoveryPhoneManager.getUnconfirmed(uid, code);
 
     // If there is no data, it means there's no record of this code being sent to the uid provided
@@ -145,8 +164,13 @@ export class RecoveryPhoneService {
    * phone number.
    *
    * @param uid An account id
+   * @returns True if successful
    */
   public async removePhoneNumber(uid: string) {
+    if (!this.config.enabled) {
+      throw new RecoveryPhoneNotEnabled();
+    }
+
     return await this.recoveryPhoneManager.removePhoneNumber(uid);
   }
 
@@ -166,6 +190,10 @@ export class RecoveryPhoneService {
     exists: boolean;
     phoneNumber?: string;
   }> {
+    if (!this.config.enabled) {
+      throw new RecoveryPhoneNotEnabled();
+    }
+
     try {
       const { phoneNumber } =
         await this.recoveryPhoneManager.getConfirmedPhoneNumber(uid);
@@ -198,6 +226,10 @@ export class RecoveryPhoneService {
    * number. e.g. +15005551234 would be masked as +*******1234 if lastN was 4.
    */
   public maskPhoneNumber(phoneNumber: string, lastN?: number) {
+    if (!this.config.enabled) {
+      throw new RecoveryPhoneNotEnabled();
+    }
+
     // The + notation can be confusing in a masked number. Don't count it
     // as a digit.
     let prefix = '';
@@ -228,6 +260,10 @@ export class RecoveryPhoneService {
    * @returns True if message didn't fail to send.
    */
   public async sendCode(uid: string) {
+    if (!this.config.enabled) {
+      throw new RecoveryPhoneNotEnabled();
+    }
+
     const { phoneNumber } =
       await this.recoveryPhoneManager.getConfirmedPhoneNumber(uid);
     const code = await this.otpCode.generateCode();

packages/fxa-auth-server/lib/routes/recovery-phone.ts

@@ -4,6 +4,7 @@
 
 import {
   RecoveryPhoneService,
+  RecoveryPhoneNotEnabled,
   RecoveryNumberNotSupportedError,
   RecoveryNumberInvalidFormatError,
   RecoveryNumberAlreadyExistsError,
@@ -66,6 +67,10 @@ class RecoveryPhoneHandler {
         throw AppError.smsSendRateLimitExceeded();
       }
 
+      if (error instanceof RecoveryPhoneNotEnabled) {
+        throw AppError.featureNotEnabled();
+      }
+
       throw AppError.backendServiceFailure(
         'RecoveryPhoneService',
         'sendCode',
@@ -108,6 +113,10 @@ class RecoveryPhoneHandler {
       await this.glean.twoStepAuthPhoneCode.sendError(request);
       return { status: RecoveryPhoneStatus.FAILURE };
     } catch (error) {
+      if (error instanceof RecoveryPhoneNotEnabled) {
+        throw AppError.featureNotEnabled();
+      }
+
       await this.glean.twoStepAuthPhoneCode.sendError(request);
 
       if (
@@ -171,6 +180,10 @@ class RecoveryPhoneHandler {
         }
       }
     } catch (error) {
+      if (error instanceof RecoveryPhoneNotEnabled) {
+        throw AppError.featureNotEnabled();
+      }
+
       if (error instanceof RecoveryNumberAlreadyExistsError) {
         throw AppError.recoveryPhoneNumberAlreadyExists();
       }
@@ -198,6 +211,10 @@ class RecoveryPhoneHandler {
     try {
       success = await this.recoveryPhoneService.removePhoneNumber(uid);
     } catch (error) {
+      if (error instanceof RecoveryPhoneNotEnabled) {
+        throw AppError.featureNotEnabled();
+      }
+
       throw AppError.backendServiceFailure(
         'RecoveryPhoneService',
         'destroy',
@@ -237,14 +254,33 @@ class RecoveryPhoneHandler {
       };
     }
 
-    const available = await this.recoveryPhoneService.available(
-      uid,
-      location.countryCode
-    );
+    try {
+      const available = await this.recoveryPhoneService.available(
+        uid,
+        location.countryCode
+      );
 
-    return {
-      available,
-    };
+      return {
+        available,
+      };
+    } catch (error) {
+      if (error instanceof RecoveryPhoneNotEnabled) {
+        // In this case we won't throw an AppError. Unlike other endpoints,
+        // this drives whether or not the feature shows up in the UI, so
+        // if the recovery phone services isn't enabled, we can simply
+        // return available false.
+        return {
+          available: false,
+        };
+      }
+
+      throw AppError.backendServiceFailure(
+        'RecoveryPhoneService',
+        'destroy',
+        { uid },
+        error
+      );
+    }
   }
 
   async exists(request: AuthRequest) {
@@ -266,6 +302,10 @@ class RecoveryPhoneHandler {
     try {
       return await this.recoveryPhoneService.hasConfirmed(uid, phoneNumberMask);
     } catch (error) {
+      if (error instanceof RecoveryPhoneNotEnabled) {
+        throw AppError.featureNotEnabled();
+      }
+
       throw AppError.backendServiceFailure(
         'RecoveryPhoneService',
         'destroy',

packages/fxa-auth-server/test/remote/recovery_phone_tests.js

@@ -67,6 +67,7 @@ describe(`#integration - recovery phone`, function () {
   const password = 'password';
 
   before(async function () {
+    config.recoveryPhone.enabled = true;
     config.securityHistory.ipProfiling.allowedRecency = 0;
     config.signinConfirmation.skipForNewAccounts.enabled = false;
     server = await TestServer.start(config);