PM- 2060 Update Two Factor Yubikey dialog (#9010)

* PM-2060 Update Two Factor Yubikey Dialog

* PM-2060 Removed old code

* PM-2060 Added event emitter to capture enabled status

* PM-2060 Addressed review comments

* PM-2060 Change in html file for existing key options

* PM-2060 Addressed the latest comments

* PM-2060 Updated remove method as per comments

* PM-2060 Added throw error to enable and disbale in base component

* tailwind updates to yubikey two factor settings

* fixing imports

* remove disable dialog when keys are null to use the error toast

* PM-2060 Addressed the review comments and fixed conflicts

* Removed super.enable removed extra emitter from component class.

* fixing adding multiple keys in one session of a dialog.

* removed thrown error

---------

Co-authored-by: Ike Kottlowski <ikottlowski@bitwarden.com>
Co-authored-by: Ike <137194738+ike-kottlowski@users.noreply.github.com>

apps/web/src/app/auth/settings/two-factor-base.component.ts

@@ -48,7 +48,6 @@ export abstract class TwoFactorBaseComponent {
       this.onUpdated.emit(true);
     } catch (e) {
       this.logService.error(e);
-      throw e;
     }
   }
 

apps/web/src/app/auth/settings/two-factor-setup.component.ts

@@ -160,10 +160,12 @@ export class TwoFactorSetupComponent implements OnInit, OnDestroy {
         if (!result) {
           return;
         }
-        const yubiComp = await this.openModal(this.yubikeyModalRef, TwoFactorYubiKeyComponent);
-        yubiComp.auth(result);
-        this.twoFactorSetupSubscription = yubiComp.onUpdated
-          .pipe(first(), takeUntil(this.destroy$))
+        const yubiComp: DialogRef<boolean, any> = TwoFactorYubiKeyComponent.open(
+          this.dialogService,
+          { data: result },
+        );
+        yubiComp.componentInstance.onUpdated
+          .pipe(takeUntil(this.destroy$))
           .subscribe((enabled: boolean) => {
             this.updateStatus(enabled, TwoFactorProviderType.Yubikey);
           });
@@ -266,7 +268,7 @@ export class TwoFactorSetupComponent implements OnInit, OnDestroy {
       this.modal.close();
     }
     this.providers.forEach((p) => {
-      if (p.type === type) {
+      if (p.type === type && enabled !== undefined) {
         p.enabled = enabled;
       }
     });

apps/web/src/app/auth/settings/two-factor-yubikey.component.html

@@ -1,118 +1,84 @@
-<div class="modal fade" role="dialog" aria-modal="true" aria-labelledby="2faYubiKeyTitle">
-  <div class="modal-dialog modal-lg" role="document">
-    <div class="modal-content">
-      <div class="modal-header">
-        <h1 class="modal-title" id="2faYubiKeyTitle">
-          {{ "twoStepLogin" | i18n }}
-          <small>YubiKey</small>
-        </h1>
-        <button
-          type="button"
-          class="close"
-          data-dismiss="modal"
-          appA11yTitle="{{ 'close' | i18n }}"
-        >
-          <span aria-hidden="true">&times;</span>
-        </button>
-      </div>
-      <form
-        #form
-        (ngSubmit)="submit()"
-        [appApiAction]="formPromise"
-        ngNativeValidate
-        *ngIf="authed"
-        autocomplete="off"
+<form *ngIf="authed" [formGroup]="formGroup" [bitSubmit]="submit">
+  <bit-dialog dialogSize="large">
+    <span bitDialogTitle>
+      {{ "twoStepLogin" | i18n }}
+      <span bitTypography="body1">YubiKey</span>
+    </span>
+    <ng-container bitDialogContent>
+      <app-callout
+        *ngIf="enabled"
+        type="success"
+        title="{{ 'enabled' | i18n }}"
+        icon="bwi bwi-check-circle"
       >
-        <div class="modal-body">
-          <app-callout
-            type="success"
-            title="{{ 'enabled' | i18n }}"
-            icon="bwi bwi-check-circle"
-            *ngIf="enabled"
-          >
-            {{ "twoStepLoginProviderEnabled" | i18n }}
-          </app-callout>
-          <app-callout type="warning">
-            <p>{{ "twoFactorYubikeyWarning" | i18n }}</p>
-            <ul class="mb-0">
-              <li>{{ "twoFactorYubikeySupportUsb" | i18n }}</li>
-              <li>{{ "twoFactorYubikeySupportMobile" | i18n }}</li>
-            </ul>
-          </app-callout>
-          <img class="float-right mfaType3" alt="Yubico OTP security key logo" />
-          <p>{{ "twoFactorYubikeyAdd" | i18n }}:</p>
-          <ol>
-            <li>{{ "twoFactorYubikeyPlugIn" | i18n }}</li>
-            <li>{{ "twoFactorYubikeySelectKey" | i18n }}</li>
-            <li>{{ "twoFactorYubikeyTouchButton" | i18n }}</li>
-            <li>{{ "twoFactorYubikeySaveForm" | i18n }}</li>
-          </ol>
-          <hr />
-          <div class="row">
-            <div class="form-group col-6" *ngFor="let k of keys; let i = index">
-              <label for="key{{ i + 1 }}">{{ "yubikeyX" | i18n: i + 1 }}</label>
-              <input
-                id="key{{ i + 1 }}"
-                type="password"
-                name="Key{{ i + 1 }}"
-                class="form-control"
-                [(ngModel)]="k.key"
-                *ngIf="!k.existingKey"
-                appInputVerbatim
-                autocomplete="new-password"
-              />
-              <div class="d-flex" *ngIf="k.existingKey">
-                <span class="mr-2">{{ k.existingKey }}</span>
-                <button
-                  type="button"
-                  class="btn btn-link text-danger ml-auto"
-                  (click)="remove(k)"
-                  appA11yTitle="{{ 'remove' | i18n }}"
-                >
-                  <i class="bwi bwi-minus-circle bwi-lg" aria-hidden="true"></i>
-                </button>
-              </div>
+        {{ "twoStepLoginProviderEnabled" | i18n }}
+      </app-callout>
+      <app-callout type="warning">
+        <p bitTypography="body1">{{ "twoFactorYubikeyWarning" | i18n }}</p>
+        <ul class="tw-mb-0" bitTypography="body1">
+          <li>{{ "twoFactorYubikeySupportUsb" | i18n }}</li>
+          <li>{{ "twoFactorYubikeySupportMobile" | i18n }}</li>
+        </ul>
+      </app-callout>
+      <img class="tw-float-right mfaType3" alt="YubiKey OTP security key logo" />
+      <p bitTypography="body1">{{ "twoFactorYubikeyAdd" | i18n }}:</p>
+      <ol bitTypography="body1">
+        <li>{{ "twoFactorYubikeyPlugIn" | i18n }}</li>
+        <li>{{ "twoFactorYubikeySelectKey" | i18n }}</li>
+        <li>{{ "twoFactorYubikeyTouchButton" | i18n }}</li>
+        <li>{{ "twoFactorYubikeySaveForm" | i18n }}</li>
+      </ol>
+      <hr />
+      <div class="tw-grid tw-grid-cols-12 tw-gap-4" formArrayName="formKeys">
+        <div class="tw-col-span-6" *ngFor="let k of keys; let i = index">
+          <div [formGroupName]="i">
+            <bit-label>{{ "yubikeyX" | i18n: i + 1 }}</bit-label>
+            <bit-form-field *ngIf="!keys[i].existingKey">
+              <input bitInput type="password" formControlName="key" appInputVerbatim />
+            </bit-form-field>
+            <div class="tw-flex tw-justify-between tw-mb-6" *ngIf="keys[i].existingKey">
+              <span class="tw-mr-2 tw-self-center">{{ keys[i].existingKey }}</span>
+              <button
+                bitIconButton="bwi-minus-circle"
+                type="button"
+                buttonType="danger"
+                (click)="remove(i)"
+                appA11yTitle="{{ 'remove' | i18n }}"
+              ></button>
             </div>
           </div>
-          <strong class="d-block mb-2">{{ "nfcSupport" | i18n }}</strong>
-          <div class="form-check">
-            <input type="checkbox" class="form-check-input" id="nfc" name="Nfc" [(ngModel)]="nfc" />
-            <label class="form-check-label" for="nfc">{{
-              "twoFactorYubikeySupportsNfc" | i18n
-            }}</label>
-          </div>
-          <small class="form-text text-muted">{{ "twoFactorYubikeySupportsNfcDesc" | i18n }}</small>
         </div>
-        <div class="modal-footer">
-          <button type="submit" class="btn btn-primary btn-submit" [disabled]="form.loading">
-            <i
-              class="bwi bwi-spinner bwi-spin"
-              title="{{ 'loading' | i18n }}"
-              aria-hidden="true"
-            ></i>
-            <span>{{ "save" | i18n }}</span>
-          </button>
-          <button
-            #disableBtn
-            type="button"
-            class="btn btn-outline-secondary btn-submit"
-            [appApiAction]="disablePromise"
-            [disabled]="$any(disableBtn).loading"
-            (click)="disable()"
-            *ngIf="enabled"
-          >
-            <i
-              class="bwi bwi-spinner bwi-spin"
-              title="{{ 'loading' | i18n }}"
-              aria-hidden="true"
-            ></i>
-            <span>{{ "disableAllKeys" | i18n }}</span>
-          </button>
-          <button type="button" class="btn btn-outline-secondary" data-dismiss="modal">
-            {{ "close" | i18n }}
-          </button>
-        </div>
-      </form>
-    </div>
-  </div>
-</div>
+      </div>
+      <p bitTypography="body1" class="tw-font-bold tw-mb-2">{{ "nfcSupport" | i18n }}</p>
+      <bit-form-control [disableMargin]="true">
+        <bit-label>{{ "twoFactorYubikeySupportsNfc" | i18n }}</bit-label>
+        <input bitCheckbox type="checkbox" formControlName="anyKeyHasNfc" />
+        <bit-hint class="tw-text-sm">{{ "twoFactorYubikeySupportsNfcDesc" | i18n }}</bit-hint>
+      </bit-form-control>
+    </ng-container>
+    <ng-container bitDialogFooter>
+      <button bitButton bitFormButton type="submit" buttonType="primary">
+        {{ "save" | i18n }}
+      </button>
+      <button
+        *ngIf="enabled"
+        bitButton
+        bitFormButton
+        type="button"
+        buttonType="secondary"
+        [bitAction]="disable"
+      >
+        {{ "disableAllKeys" | i18n }}
+      </button>
+      <button
+        bitButton
+        bitFormButton
+        type="button"
+        buttonType="secondary"
+        [bitDialogClose]="this.enabled"
+      >
+        {{ "close" | i18n }}
+      </button>
+    </ng-container>
+  </bit-dialog>
+</form>

apps/web/src/app/auth/settings/two-factor-yubikey.component.ts

@@ -1,15 +1,17 @@
-import { Component } from "@angular/core";
+import { DIALOG_DATA, DialogConfig } from "@angular/cdk/dialog";
+import { Component, Inject } from "@angular/core";
+import { FormArray, FormBuilder, FormControl, FormGroup } from "@angular/forms";
 
 import { ApiService } from "@bitwarden/common/abstractions/api.service";
 import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction";
 import { TwoFactorProviderType } from "@bitwarden/common/auth/enums/two-factor-provider-type";
-import { UpdateTwoFactorYubioOtpRequest } from "@bitwarden/common/auth/models/request/update-two-factor-yubio-otp.request";
+import { UpdateTwoFactorYubikeyOtpRequest } from "@bitwarden/common/auth/models/request/update-two-factor-yubikey-otp.request";
 import { TwoFactorYubiKeyResponse } from "@bitwarden/common/auth/models/response/two-factor-yubi-key.response";
 import { AuthResponse } from "@bitwarden/common/auth/types/auth-response";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
 import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
 import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
-import { DialogService } from "@bitwarden/components";
+import { DialogService, ToastService } from "@bitwarden/components";
 
 import { TwoFactorBaseComponent } from "./two-factor-base.component";
 
@@ -25,20 +27,35 @@ interface Key {
 export class TwoFactorYubiKeyComponent extends TwoFactorBaseComponent {
   type = TwoFactorProviderType.Yubikey;
   keys: Key[];
-  nfc = false;
+  anyKeyHasNfc = false;
 
   formPromise: Promise<TwoFactorYubiKeyResponse>;
   disablePromise: Promise<unknown>;
 
   override componentName = "app-two-factor-yubikey";
+  formGroup: FormGroup<{
+    formKeys: FormArray<FormControl<Key>>;
+    anyKeyHasNfc: FormControl<boolean>;
+  }>;
+
+  get keysFormControl() {
+    return this.formGroup.controls.formKeys.controls;
+  }
+
+  get anyKeyHasNfcFormControl() {
+    return this.formGroup.controls.anyKeyHasNfc;
+  }
 
   constructor(
+    @Inject(DIALOG_DATA) protected data: AuthResponse<TwoFactorYubiKeyResponse>,
     apiService: ApiService,
     i18nService: I18nService,
     platformUtilsService: PlatformUtilsService,
     logService: LogService,
     userVerificationService: UserVerificationService,
     dialogService: DialogService,
+    private formBuilder: FormBuilder,
+    private toastService: ToastService,
   ) {
     super(
       apiService,
@@ -50,39 +67,83 @@ export class TwoFactorYubiKeyComponent extends TwoFactorBaseComponent {
     );
   }
 
+  ngOnInit() {
+    this.auth(this.data);
+    this.formGroup = this.formBuilder.group({
+      formKeys: this.formBuilder.array<Key>([]),
+      anyKeyHasNfc: this.formBuilder.control(this.anyKeyHasNfc),
+    });
+    this.refreshFormArrayData();
+  }
+
+  refreshFormArrayData() {
+    const formKeys = <FormArray>this.formGroup.get("formKeys");
+    formKeys.clear();
+    this.keys.forEach((val) => {
+      const fb = this.formBuilder.group({
+        key: val.key,
+        existingKey: val.existingKey,
+      });
+      formKeys.push(fb);
+    });
+  }
+
   auth(authResponse: AuthResponse<TwoFactorYubiKeyResponse>) {
     super.auth(authResponse);
     this.processResponse(authResponse.response);
   }
 
-  async submit() {
-    const request = await this.buildRequestModel(UpdateTwoFactorYubioOtpRequest);
-    request.key1 = this.keys != null && this.keys.length > 0 ? this.keys[0].key : null;
-    request.key2 = this.keys != null && this.keys.length > 1 ? this.keys[1].key : null;
-    request.key3 = this.keys != null && this.keys.length > 2 ? this.keys[2].key : null;
-    request.key4 = this.keys != null && this.keys.length > 3 ? this.keys[3].key : null;
-    request.key5 = this.keys != null && this.keys.length > 4 ? this.keys[4].key : null;
-    request.nfc = this.nfc;
+  submit = async () => {
+    this.formGroup.markAllAsTouched();
+    if (this.formGroup.invalid) {
+      return;
+    }
+    await this.enable();
+  };
 
-    return super.enable(async () => {
-      this.formPromise = this.apiService.putTwoFactorYubiKey(request);
-      const response = await this.formPromise;
-      await this.processResponse(response);
-      this.platformUtilsService.showToast("success", null, this.i18nService.t("yubikeysUpdated"));
+  disable = async () => {
+    await this.disableMethod();
+
+    if (!this.enabled) {
+      for (let i = 0; i < this.keys.length; i++) {
+        this.remove(i);
+      }
+    }
+  };
+
+  protected async enable() {
+    const keys = this.formGroup.controls.formKeys.value;
+    const request = await this.buildRequestModel(UpdateTwoFactorYubikeyOtpRequest);
+    request.key1 = keys != null && keys.length > 0 ? keys[0].key : null;
+    request.key2 = keys != null && keys.length > 1 ? keys[1].key : null;
+    request.key3 = keys != null && keys.length > 2 ? keys[2].key : null;
+    request.key4 = keys != null && keys.length > 3 ? keys[3].key : null;
+    request.key5 = keys != null && keys.length > 4 ? keys[4].key : null;
+    request.nfc = this.formGroup.value.anyKeyHasNfc;
+
+    this.processResponse(await this.apiService.putTwoFactorYubiKey(request));
+    this.refreshFormArrayData();
+    this.toastService.showToast({
+      title: this.i18nService.t("success"),
+      message: this.i18nService.t("yubikeysUpdated"),
+      variant: "success",
     });
+    this.onUpdated.emit(this.enabled);
   }
 
-  disable() {
-    return super.disable(this.disablePromise);
-  }
+  remove(pos: number) {
+    this.keys[pos].key = null;
+    this.keys[pos].existingKey = null;
 
-  remove(key: Key) {
-    key.existingKey = null;
-    key.key = null;
+    this.keysFormControl[pos].setValue({
+      existingKey: null,
+      key: null,
+    });
   }
 
   private processResponse(response: TwoFactorYubiKeyResponse) {
     this.enabled = response.enabled;
+    this.anyKeyHasNfc = response.nfc || !response.enabled;
     this.keys = [
       { key: response.key1, existingKey: this.padRight(response.key1) },
       { key: response.key2, existingKey: this.padRight(response.key2) },
@@ -90,7 +151,6 @@ export class TwoFactorYubiKeyComponent extends TwoFactorBaseComponent {
       { key: response.key4, existingKey: this.padRight(response.key4) },
       { key: response.key5, existingKey: this.padRight(response.key5) },
     ];
-    this.nfc = response.nfc || !response.enabled;
   }
 
   private padRight(str: string, character = "•", size = 44) {
@@ -103,4 +163,11 @@ export class TwoFactorYubiKeyComponent extends TwoFactorBaseComponent {
     }
     return str;
   }
+
+  static open(
+    dialogService: DialogService,
+    config: DialogConfig<AuthResponse<TwoFactorYubiKeyResponse>>,
+  ) {
+    return dialogService.open<boolean>(TwoFactorYubiKeyComponent, config);
+  }
 }

libs/common/src/abstractions/api.service.ts

@@ -53,7 +53,7 @@ import { UpdateTwoFactorDuoRequest } from "../auth/models/request/update-two-fac
 import { UpdateTwoFactorEmailRequest } from "../auth/models/request/update-two-factor-email.request";
 import { UpdateTwoFactorWebAuthnDeleteRequest } from "../auth/models/request/update-two-factor-web-authn-delete.request";
 import { UpdateTwoFactorWebAuthnRequest } from "../auth/models/request/update-two-factor-web-authn.request";
-import { UpdateTwoFactorYubioOtpRequest } from "../auth/models/request/update-two-factor-yubio-otp.request";
+import { UpdateTwoFactorYubikeyOtpRequest } from "../auth/models/request/update-two-factor-yubikey-otp.request";
 import { ApiKeyResponse } from "../auth/models/response/api-key.response";
 import { AuthRequestResponse } from "../auth/models/response/auth-request.response";
 import { DeviceVerificationResponse } from "../auth/models/response/device-verification.response";
@@ -330,7 +330,7 @@ export abstract class ApiService {
     request: UpdateTwoFactorDuoRequest,
   ) => Promise<TwoFactorDuoResponse>;
   putTwoFactorYubiKey: (
-    request: UpdateTwoFactorYubioOtpRequest,
+    request: UpdateTwoFactorYubikeyOtpRequest,
   ) => Promise<TwoFactorYubiKeyResponse>;
   putTwoFactorWebAuthn: (
     request: UpdateTwoFactorWebAuthnRequest,

libs/common/src/auth/models/request/update-two-factor-yubikey-otp.request.ts

@@ -1,6 +1,6 @@
 import { SecretVerificationRequest } from "./secret-verification.request";
 
-export class UpdateTwoFactorYubioOtpRequest extends SecretVerificationRequest {
+export class UpdateTwoFactorYubikeyOtpRequest extends SecretVerificationRequest {
   key1: string;
   key2: string;
   key3: string;

libs/common/src/services/api.service.ts

@@ -62,7 +62,7 @@ import { UpdateTwoFactorDuoRequest } from "../auth/models/request/update-two-fac
 import { UpdateTwoFactorEmailRequest } from "../auth/models/request/update-two-factor-email.request";
 import { UpdateTwoFactorWebAuthnDeleteRequest } from "../auth/models/request/update-two-factor-web-authn-delete.request";
 import { UpdateTwoFactorWebAuthnRequest } from "../auth/models/request/update-two-factor-web-authn.request";
-import { UpdateTwoFactorYubioOtpRequest } from "../auth/models/request/update-two-factor-yubio-otp.request";
+import { UpdateTwoFactorYubikeyOtpRequest } from "../auth/models/request/update-two-factor-yubikey-otp.request";
 import { ApiKeyResponse } from "../auth/models/response/api-key.response";
 import { AuthRequestResponse } from "../auth/models/response/auth-request.response";
 import { DeviceVerificationResponse } from "../auth/models/response/device-verification.response";
@@ -1023,7 +1023,7 @@ export class ApiService implements ApiServiceAbstraction {
   }
 
   async putTwoFactorYubiKey(
-    request: UpdateTwoFactorYubioOtpRequest,
+    request: UpdateTwoFactorYubikeyOtpRequest,
   ): Promise<TwoFactorYubiKeyResponse> {
     const r = await this.send("PUT", "/two-factor/yubikey", request, true, true);
     return new TwoFactorYubiKeyResponse(r);