Merge pull request #18247 from mozilla/fxa-10373

Wire functionality to remove a recovery phone

Author: vbudhram

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

@@ -44,6 +44,16 @@ export class RecoveryNumberNotSupportedError extends RecoveryPhoneError {
   }
 }
 
+export class RecoveryNumberRemoveMissingBackupCodes extends RecoveryPhoneError {
+  constructor(uid: string, cause?: Error) {
+    super(
+      'Unable to remove recovery phone, missing backup authentication codes.',
+      { uid },
+      cause
+    );
+  }
+}
+
 export class SmsSendRateLimitExceededError extends RecoveryPhoneError {
   constructor(
     uid: string,

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

@@ -12,6 +12,7 @@ import {
   RecoveryNumberNotExistsError,
   RecoveryNumberNotSupportedError,
   RecoveryPhoneNotEnabled,
+  RecoveryNumberRemoveMissingBackupCodes,
 } from './recovery-phone.errors';
 
 describe('RecoveryPhoneService', () => {
@@ -46,6 +47,7 @@ describe('RecoveryPhoneService', () => {
 
   beforeEach(async () => {
     mockSmsManager.sendSMS.mockReturnValue({ status: 'success' });
+    mockRecoveryPhoneManager.hasRecoveryCodes.mockResolvedValue(true);
 
     const module: TestingModule = await Test.createTestingModule({
       providers: [
@@ -280,6 +282,13 @@ describe('RecoveryPhoneService', () => {
       mockRecoveryPhoneManager.removePhoneNumber.mockRejectedValueOnce(error);
       expect(service.removePhoneNumber(uid)).rejects.toThrow(error);
     });
+
+    it('should throw if missing backup authentication codes', () => {
+      mockRecoveryPhoneManager.hasRecoveryCodes.mockResolvedValue(false);
+      const error = new RecoveryNumberRemoveMissingBackupCodes(uid);
+      mockRecoveryPhoneManager.removePhoneNumber.mockRejectedValueOnce(error);
+      expect(service.removePhoneNumber(uid)).rejects.toThrow(error);
+    });
   });
 
   describe('sendCode', () => {

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

@@ -11,6 +11,7 @@ import {
   RecoveryNumberNotExistsError,
   RecoveryNumberNotSupportedError,
   RecoveryPhoneNotEnabled,
+  RecoveryNumberRemoveMissingBackupCodes,
 } from './recovery-phone.errors';
 import { MessageInstance } from 'twilio/lib/rest/api/v2010/account/message';
 
@@ -161,16 +162,25 @@ export class RecoveryPhoneService {
 
   /**
    * Remove phone number from an account. Each user can only have one associated
-   * phone number.
+   * phone number. A user must have backup codes before removing a phone number.
    *
    * @param uid An account id
    * @returns True if successful
    */
   public async removePhoneNumber(uid: string) {
+
     if (!this.config.enabled) {
       throw new RecoveryPhoneNotEnabled();
     }
 
+    const hasRecoveryCodes = await this.recoveryPhoneManager.hasRecoveryCodes(
+      uid
+    );
+
+    if (!hasRecoveryCodes) {
+      throw new RecoveryNumberRemoveMissingBackupCodes(uid);
+    }
+
     return await this.recoveryPhoneManager.removePhoneNumber(uid);
   }
 

packages/fxa-auth-server/lib/error.js

@@ -594,6 +594,16 @@ AppError.recoveryPhoneNumberDoesNotExist = () => {
   });
 };
 
+AppError.recoveryPhoneRemoveMissingRecoveryCodes = () => {
+  return new AppError({
+    code: 400,
+    error: 'Bad Request',
+    errno: ERRNO.RECOVERY_PHONE_REMOVE_MISSING_RECOVERY_CODES,
+    message:
+      'Unable to remove recovery phone, missing backup authentication codes.',
+  });
+};
+
 AppError.smsSendRateLimitExceeded = () => {
   return new AppError({
     code: 429,

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

@@ -10,6 +10,7 @@ import {
   RecoveryNumberAlreadyExistsError,
   RecoveryNumberNotExistsError,
   SmsSendRateLimitExceededError,
+  RecoveryNumberRemoveMissingBackupCodes,
 } from '@fxa/accounts/recovery-phone';
 import {
   AccountManager,
@@ -211,15 +212,24 @@ class RecoveryPhoneHandler {
     try {
       success = await this.recoveryPhoneService.removePhoneNumber(uid);
     } catch (error) {
+
       if (error instanceof RecoveryPhoneNotEnabled) {
         throw AppError.featureNotEnabled();
       }
 
+      if (error instanceof RecoveryNumberNotExistsError) {
+        throw AppError.recoveryPhoneNumberDoesNotExist();
+      }
+
+      if (error instanceof RecoveryNumberRemoveMissingBackupCodes) {
+        throw AppError.recoveryPhoneRemoveMissingRecoveryCodes();
+      }
+
       throw AppError.backendServiceFailure(
         'RecoveryPhoneService',
         'destroy',
         { uid },
-        error
+        error,
       );
     }
 

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

@@ -79,6 +79,7 @@ describe(`#integration - recovery phone`, function () {
     await db.deleteFrom('accounts').execute();
     await db.deleteFrom('recoveryPhones').execute();
     await db.deleteFrom('sessionTokens').execute();
+    await db.deleteFrom('recoveryCodes').execute();
   }
 
   beforeEach(async function () {
@@ -92,6 +93,15 @@ describe(`#integration - recovery phone`, function () {
         version: 'V2',
       }
     );
+
+    // Add totp to account
+    client.totpAuthenticator = new otplib.authenticator.Authenticator();
+    const totpResult = await client.createTotpToken();
+    client.totpAuthenticator.options = {
+      secret: totpResult.secret,
+      crypto: crypto,
+    };
+    await client.verifyTotpCode(client.totpAuthenticator.generate());
   });
 
   afterEach(async function () {
@@ -123,15 +133,6 @@ describe(`#integration - recovery phone`, function () {
       this.skip('Invalid twilio accountSid or authToken. Check env / config!');
     }
 
-    // Add totp to account
-    client.totpAuthenticator = new otplib.authenticator.Authenticator();
-    const totpResult = await client.createTotpToken();
-    client.totpAuthenticator.options = {
-      secret: totpResult.secret,
-      crypto: crypto,
-    };
-    await client.verifyTotpCode(client.totpAuthenticator.generate());
-
     // Add recovery phone
     await client.recoveryPhoneNumberCreate(phoneNumber);
     await client.recoveryPhoneConfirmSetup(

packages/fxa-settings/src/components/Banner/index.stories.tsx

@@ -133,6 +133,34 @@ export const Variation3cHeadingDescriptionDismiss = () => (
   </AppLayout>
 );
 
+export const Variation4aIconStart = () => (
+  <AppLayout>
+    <Banner
+      type="error"
+      content={{
+        localizedHeading: sampleHeading,
+        localizedDescription: sampleDescription,
+      }}
+      iconAlign={'start'}
+      dismissButton={{ action: () => alert('Dismiss clicked') }}
+    />
+  </AppLayout>
+);
+
+export const Variation4bIconCenter = () => (
+  <AppLayout>
+    <Banner
+      type="error"
+      content={{
+        localizedHeading: sampleHeading,
+        localizedDescription: sampleDescription,
+      }}
+      iconAlign={'center'}
+      dismissButton={{ action: () => alert('Dismiss clicked') }}
+    />
+  </AppLayout>
+);
+
 export const TypeError = () => (
   <AppLayout>
     <Banner

packages/fxa-settings/src/components/Banner/index.tsx

@@ -27,7 +27,9 @@ export const Banner = ({
   link,
   isFancy,
   bannerId,
+  iconAlign = 'center',
 }: BannerProps) => {
+  const iconClassName = `shrink-0 self-${iconAlign}`;
   return (
     <div
       id={bannerId || ''}
@@ -48,22 +50,22 @@ export const Banner = ({
       tabIndex={-1}
     >
       {/* Icon fills use 'currentColor' (from text color) for better accessibility in HCM mode */}
-      {type === 'error' && <ErrorIcon className="shrink-0" ariaHidden />}
+      {type === 'error' && <ErrorIcon className={iconClassName} aria-hidden />}
       {type === 'info' && !isFancy && (
-        <InfoIcon className="shrink-0" ariaHidden />
+        <InfoIcon className={iconClassName} aria-hidden />
       )}
       {type === 'info' && isFancy && (
-        <InformationOutlineBlueIcon className="shrink-0" ariaHidden />
+        <InformationOutlineBlueIcon className={iconClassName} aria-hidden />
       )}
       {type === 'success' && (
         <CheckmarkCircleOutlineCurrentIcon
-          className="shrink-0"
+          className={iconClassName}
           mode="success"
           ariaHidden
         />
       )}
       {type === 'warning' && (
-        <WarningIcon className="shrink-0" mode="attention" ariaHidden />
+        <WarningIcon className={iconClassName} mode="attention" ariaHidden />
       )}
 
       <div className="flex flex-col grow">

packages/fxa-settings/src/components/Banner/interfaces.ts

@@ -25,6 +25,7 @@ export type BannerProps = {
   link?: BannerLinkProps;
   isFancy?: boolean;
   bannerId?: string;
+  iconAlign?: 'start' | 'center';
 };
 
 export type Animation = {

packages/fxa-settings/src/components/Settings/FlowContainer/index.tsx

@@ -14,6 +14,7 @@ type FlowContainerProps = {
   onBackButtonClick?: (
     event: React.MouseEvent<HTMLButtonElement, MouseEvent>
   ) => void;
+  hideBackButton?: boolean;
   localizedBackButtonTitle?: string;
   children?: React.ReactNode;
 };
@@ -22,6 +23,7 @@ export const FlowContainer = ({
   title,
   subtitle,
   onBackButtonClick = () => window.history.back(),
+  hideBackButton = false,
   localizedBackButtonTitle,
   children,
 }: FlowContainerProps & RouteComponentProps) => {
@@ -38,12 +40,14 @@ export const FlowContainer = ({
       <Head title={title} />
 
       <div className="relative flex items-center">
-        <ButtonBack
-          onClick={onBackButtonClick}
-          dataTestId="flow-container-back-btn"
-          localizedTitle={backButtonTitle}
-          localizedAriaLabel={backButtonTitle}
-        />
+        {!hideBackButton && (
+          <ButtonBack
+            onClick={onBackButtonClick}
+            dataTestId="flow-container-back-btn"
+            localizedTitle={backButtonTitle}
+            localizedAriaLabel={backButtonTitle}
+          />
+        )}
         <h1 className="font-header text-md text-grey-400">{title}</h1>
       </div>
       {subtitle && (