From 56a122980351e2baf49b55002fd020b98c163a32 Mon Sep 17 00:00:00 2001
From: Andreas Coroiu <acoroiu@bitwarden.com>
Date: Wed, 11 Oct 2023 16:09:01 +0200
Subject: [PATCH] [PM-4016] Address feedback on [PM-2014] (#6532)

* [PM-4016] feat: use dialog `loading` attribute

* [PM-4016] chore: move constant to service

* [PM-4016] chore: simplify paddings

* [PM-4016] chore: rename to `AuthSettingsModule`

* [PM-4016] fix: move request creation to service

* [PM-4016] feat: simplify module structure

Remove core.module and use `@Injectable({ providedIn: "root" })` instead.
---
 apps/web/src/app/auth/auth.module.ts             |  7 +++----
 apps/web/src/app/auth/core/core.module.ts        | 15 ---------------
 apps/web/src/app/auth/core/index.ts              |  1 -
 .../webauthn-login/webauthn-login-api.service.ts | 16 +++++-----------
 .../webauthn-login.service.spec.ts               |  6 +++++-
 .../webauthn-login/webauthn-login.service.ts     | 12 +++++++++---
 .../web/src/app/auth/settings/settings.module.ts |  4 ++--
 .../create-credential-dialog.component.html      |  2 +-
 .../create-credential-dialog.component.ts        |  1 +
 .../delete-credential-dialog.component.html      |  2 +-
 .../delete-credential-dialog.component.ts        |  1 +
 .../webauthn-login-settings.component.html       |  4 ++--
 .../webauthn-login-settings.component.ts         |  2 +-
 13 files changed, 31 insertions(+), 42 deletions(-)
 delete mode 100644 apps/web/src/app/auth/core/core.module.ts

diff --git a/apps/web/src/app/auth/auth.module.ts b/apps/web/src/app/auth/auth.module.ts
index 49be17aa264..056b9f161f9 100644
--- a/apps/web/src/app/auth/auth.module.ts
+++ b/apps/web/src/app/auth/auth.module.ts
@@ -1,12 +1,11 @@
 import { NgModule } from "@angular/core";
 
-import { CoreAuthModule } from "./core";
-import { SettingsModule } from "./settings/settings.module";
+import { AuthSettingsModule } from "./settings/settings.module";
 
 @NgModule({
-  imports: [CoreAuthModule, SettingsModule],
+  imports: [AuthSettingsModule],
   declarations: [],
   providers: [],
-  exports: [SettingsModule],
+  exports: [AuthSettingsModule],
 })
 export class AuthModule {}
diff --git a/apps/web/src/app/auth/core/core.module.ts b/apps/web/src/app/auth/core/core.module.ts
deleted file mode 100644
index e196b1c3d76..00000000000
--- a/apps/web/src/app/auth/core/core.module.ts
+++ /dev/null
@@ -1,15 +0,0 @@
-import { NgModule, Optional, SkipSelf } from "@angular/core";
-
-import { WebauthnLoginApiService } from "./services/webauthn-login/webauthn-login-api.service";
-import { WebauthnLoginService } from "./services/webauthn-login/webauthn-login.service";
-
-@NgModule({
-  providers: [WebauthnLoginService, WebauthnLoginApiService],
-})
-export class CoreAuthModule {
-  constructor(@Optional() @SkipSelf() parentModule?: CoreAuthModule) {
-    if (parentModule) {
-      throw new Error("CoreAuthModule is already loaded. Import it in AuthModule only");
-    }
-  }
-}
diff --git a/apps/web/src/app/auth/core/index.ts b/apps/web/src/app/auth/core/index.ts
index 3d2d739adf9..b2221a94a89 100644
--- a/apps/web/src/app/auth/core/index.ts
+++ b/apps/web/src/app/auth/core/index.ts
@@ -1,2 +1 @@
 export * from "./services";
-export * from "./core.module";
diff --git a/apps/web/src/app/auth/core/services/webauthn-login/webauthn-login-api.service.ts b/apps/web/src/app/auth/core/services/webauthn-login/webauthn-login-api.service.ts
index 33e1aea369b..6dc61563491 100644
--- a/apps/web/src/app/auth/core/services/webauthn-login/webauthn-login-api.service.ts
+++ b/apps/web/src/app/auth/core/services/webauthn-login/webauthn-login-api.service.ts
@@ -1,25 +1,20 @@
 import { Injectable } from "@angular/core";
 
 import { ApiService } from "@bitwarden/common/abstractions/api.service";
-import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction";
+import { SecretVerificationRequest } from "@bitwarden/common/auth/models/request/secret-verification.request";
 import { ListResponse } from "@bitwarden/common/models/response/list.response";
-import { Verification } from "@bitwarden/common/types/verification";
 
 import { SaveCredentialRequest } from "./request/save-credential.request";
 import { WebauthnLoginCredentialCreateOptionsResponse } from "./response/webauthn-login-credential-create-options.response";
 import { WebauthnLoginCredentialResponse } from "./response/webauthn-login-credential.response";
 
-@Injectable()
+@Injectable({ providedIn: "root" })
 export class WebauthnLoginApiService {
-  constructor(
-    private apiService: ApiService,
-    private userVerificationService: UserVerificationService
-  ) {}
+  constructor(private apiService: ApiService) {}
 
   async getCredentialCreateOptions(
-    verification: Verification
+    request: SecretVerificationRequest
   ): Promise<WebauthnLoginCredentialCreateOptionsResponse> {
-    const request = await this.userVerificationService.buildRequest(verification);
     const response = await this.apiService.send("POST", "/webauthn/options", request, true, true);
     return new WebauthnLoginCredentialCreateOptionsResponse(response);
   }
@@ -33,8 +28,7 @@ export class WebauthnLoginApiService {
     return this.apiService.send("GET", "/webauthn", null, true, true);
   }
 
-  async deleteCredential(credentialId: string, verification: Verification): Promise<void> {
-    const request = await this.userVerificationService.buildRequest(verification);
+  async deleteCredential(credentialId: string, request: SecretVerificationRequest): Promise<void> {
     await this.apiService.send("POST", `/webauthn/${credentialId}/delete`, request, true, true);
   }
 }
diff --git a/apps/web/src/app/auth/core/services/webauthn-login/webauthn-login.service.spec.ts b/apps/web/src/app/auth/core/services/webauthn-login/webauthn-login.service.spec.ts
index 070513f19e8..1e4f1fa7717 100644
--- a/apps/web/src/app/auth/core/services/webauthn-login/webauthn-login.service.spec.ts
+++ b/apps/web/src/app/auth/core/services/webauthn-login/webauthn-login.service.spec.ts
@@ -1,5 +1,7 @@
 import { mock, MockProxy } from "jest-mock-extended";
 
+import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction";
+
 import { CredentialCreateOptionsView } from "../../views/credential-create-options.view";
 
 import { WebauthnLoginApiService } from "./webauthn-login-api.service";
@@ -7,6 +9,7 @@ import { WebauthnLoginService } from "./webauthn-login.service";
 
 describe("WebauthnService", () => {
   let apiService!: MockProxy<WebauthnLoginApiService>;
+  let userVerificationService!: MockProxy<UserVerificationService>;
   let credentials: MockProxy<CredentialsContainer>;
   let webauthnService!: WebauthnLoginService;
 
@@ -15,8 +18,9 @@ describe("WebauthnService", () => {
     window.PublicKeyCredential = class {} as any;
     window.AuthenticatorAttestationResponse = class {} as any;
     apiService = mock<WebauthnLoginApiService>();
+    userVerificationService = mock<UserVerificationService>();
     credentials = mock<CredentialsContainer>();
-    webauthnService = new WebauthnLoginService(apiService, credentials);
+    webauthnService = new WebauthnLoginService(apiService, userVerificationService, credentials);
   });
 
   describe("createCredential", () => {
diff --git a/apps/web/src/app/auth/core/services/webauthn-login/webauthn-login.service.ts b/apps/web/src/app/auth/core/services/webauthn-login/webauthn-login.service.ts
index 760214961a7..c5979f08c61 100644
--- a/apps/web/src/app/auth/core/services/webauthn-login/webauthn-login.service.ts
+++ b/apps/web/src/app/auth/core/services/webauthn-login/webauthn-login.service.ts
@@ -1,6 +1,7 @@
 import { Injectable, Optional } from "@angular/core";
 import { BehaviorSubject, filter, from, map, Observable, shareReplay, switchMap, tap } from "rxjs";
 
+import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction";
 import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
 import { Verification } from "@bitwarden/common/types/verification";
 
@@ -11,8 +12,10 @@ import { SaveCredentialRequest } from "./request/save-credential.request";
 import { WebauthnLoginAttestationResponseRequest } from "./request/webauthn-login-attestation-response.request";
 import { WebauthnLoginApiService } from "./webauthn-login-api.service";
 
-@Injectable()
+@Injectable({ providedIn: "root" })
 export class WebauthnLoginService {
+  static readonly MaxCredentialCount = 5;
+
   private navigatorCredentials: CredentialsContainer;
   private _refresh$ = new BehaviorSubject<void>(undefined);
   private _loading$ = new BehaviorSubject<boolean>(true);
@@ -27,6 +30,7 @@ export class WebauthnLoginService {
 
   constructor(
     private apiService: WebauthnLoginApiService,
+    private userVerificationService: UserVerificationService,
     @Optional() navigatorCredentials?: CredentialsContainer,
     @Optional() private logService?: LogService
   ) {
@@ -37,7 +41,8 @@ export class WebauthnLoginService {
   async getCredentialCreateOptions(
     verification: Verification
   ): Promise<CredentialCreateOptionsView> {
-    const response = await this.apiService.getCredentialCreateOptions(verification);
+    const request = await this.userVerificationService.buildRequest(verification);
+    const response = await this.apiService.getCredentialCreateOptions(request);
     return new CredentialCreateOptionsView(response.options, response.token);
   }
 
@@ -95,7 +100,8 @@ export class WebauthnLoginService {
   }
 
   async deleteCredential(credentialId: string, verification: Verification): Promise<void> {
-    await this.apiService.deleteCredential(credentialId, verification);
+    const request = await this.userVerificationService.buildRequest(verification);
+    await this.apiService.deleteCredential(credentialId, request);
     this.refresh();
   }
 
diff --git a/apps/web/src/app/auth/settings/settings.module.ts b/apps/web/src/app/auth/settings/settings.module.ts
index 282524d07e4..12ae6bcbf5e 100644
--- a/apps/web/src/app/auth/settings/settings.module.ts
+++ b/apps/web/src/app/auth/settings/settings.module.ts
@@ -11,6 +11,6 @@ import { WebauthnLoginSettingsModule } from "./webauthn-login-settings";
   imports: [SharedModule, WebauthnLoginSettingsModule, PasswordCalloutComponent],
   declarations: [ChangePasswordComponent],
   providers: [],
-  exports: [WebauthnLoginSettingsModule, ChangePasswordComponent],
+  exports: [ChangePasswordComponent],
 })
-export class SettingsModule {}
+export class AuthSettingsModule {}
diff --git a/apps/web/src/app/auth/settings/webauthn-login-settings/create-credential-dialog/create-credential-dialog.component.html b/apps/web/src/app/auth/settings/webauthn-login-settings/create-credential-dialog/create-credential-dialog.component.html
index 57a2c545ca1..aadcf5e5960 100644
--- a/apps/web/src/app/auth/settings/webauthn-login-settings/create-credential-dialog/create-credential-dialog.component.html
+++ b/apps/web/src/app/auth/settings/webauthn-login-settings/create-credential-dialog/create-credential-dialog.component.html
@@ -1,5 +1,5 @@
 <form [formGroup]="formGroup" [bitSubmit]="submit">
-  <bit-dialog dialogSize="large">
+  <bit-dialog dialogSize="large" [loading]="loading$ | async">
     <span bitDialogTitle
       >{{ "loginWithPasskey" | i18n }}
       <span class="tw-text-sm tw-normal-case tw-text-muted">{{ "newPasskey" | i18n }}</span>
diff --git a/apps/web/src/app/auth/settings/webauthn-login-settings/create-credential-dialog/create-credential-dialog.component.ts b/apps/web/src/app/auth/settings/webauthn-login-settings/create-credential-dialog/create-credential-dialog.component.ts
index 5c93d6f25e2..12af83cac5c 100644
--- a/apps/web/src/app/auth/settings/webauthn-login-settings/create-credential-dialog/create-credential-dialog.component.ts
+++ b/apps/web/src/app/auth/settings/webauthn-login-settings/create-credential-dialog/create-credential-dialog.component.ts
@@ -46,6 +46,7 @@ export class CreateCredentialDialogComponent implements OnInit {
   protected credentialOptions?: CredentialCreateOptionsView;
   protected deviceResponse?: PublicKeyCredential;
   protected hasPasskeys$?: Observable<boolean>;
+  protected loading$ = this.webauthnService.loading$;
 
   constructor(
     private formBuilder: FormBuilder,
diff --git a/apps/web/src/app/auth/settings/webauthn-login-settings/delete-credential-dialog/delete-credential-dialog.component.html b/apps/web/src/app/auth/settings/webauthn-login-settings/delete-credential-dialog/delete-credential-dialog.component.html
index 4cfdbbcf7fe..5e87f6d4adf 100644
--- a/apps/web/src/app/auth/settings/webauthn-login-settings/delete-credential-dialog/delete-credential-dialog.component.html
+++ b/apps/web/src/app/auth/settings/webauthn-login-settings/delete-credential-dialog/delete-credential-dialog.component.html
@@ -1,5 +1,5 @@
 <form [formGroup]="formGroup" [bitSubmit]="submit">
-  <bit-dialog dialogSize="large">
+  <bit-dialog dialogSize="large" [loading]="loading$ | async">
     <span bitDialogTitle
       >{{ "removePasskey" | i18n }}
       <span *ngIf="credential" class="tw-text-sm tw-normal-case tw-text-muted">{{
diff --git a/apps/web/src/app/auth/settings/webauthn-login-settings/delete-credential-dialog/delete-credential-dialog.component.ts b/apps/web/src/app/auth/settings/webauthn-login-settings/delete-credential-dialog/delete-credential-dialog.component.ts
index 7cb03238392..9ee1337ffb2 100644
--- a/apps/web/src/app/auth/settings/webauthn-login-settings/delete-credential-dialog/delete-credential-dialog.component.ts
+++ b/apps/web/src/app/auth/settings/webauthn-login-settings/delete-credential-dialog/delete-credential-dialog.component.ts
@@ -27,6 +27,7 @@ export class DeleteCredentialDialogComponent implements OnInit, OnDestroy {
     masterPassword: ["", [Validators.required]],
   });
   protected credential?: WebauthnCredentialView;
+  protected loading$ = this.webauthnService.loading$;
 
   constructor(
     @Inject(DIALOG_DATA) private params: DeleteCredentialDialogParams,
diff --git a/apps/web/src/app/auth/settings/webauthn-login-settings/webauthn-login-settings.component.html b/apps/web/src/app/auth/settings/webauthn-login-settings/webauthn-login-settings.component.html
index 23abe02665c..5896d461bfb 100644
--- a/apps/web/src/app/auth/settings/webauthn-login-settings/webauthn-login-settings.component.html
+++ b/apps/web/src/app/auth/settings/webauthn-login-settings/webauthn-login-settings.component.html
@@ -22,7 +22,7 @@
 <table *ngIf="hasCredentials" class="tw-mb-5">
   <tr *ngFor="let credential of credentials">
     <td class="tw-p-2 tw-pl-0 tw-font-semibold">{{ credential.name }}</td>
-    <td class="tw-p-2 tw-pr-0">
+    <td class="tw-p-2 tw-pr-10">
       <ng-container *ngIf="credential.prfSupport">
         <i class="bwi bwi-lock-encrypted"></i>
         {{ "supportsEncryption" | i18n }}
@@ -31,7 +31,7 @@
         {{ "encryptionNotSupported" | i18n }}
       </span>
     </td>
-    <td class="tw-py-2 tw-pl-10 tw-pr-0">
+    <td class="tw-py-2">
       <button
         type="button"
         bitLink
diff --git a/apps/web/src/app/auth/settings/webauthn-login-settings/webauthn-login-settings.component.ts b/apps/web/src/app/auth/settings/webauthn-login-settings/webauthn-login-settings.component.ts
index 98aa517b98e..1aa4356c667 100644
--- a/apps/web/src/app/auth/settings/webauthn-login-settings/webauthn-login-settings.component.ts
+++ b/apps/web/src/app/auth/settings/webauthn-login-settings/webauthn-login-settings.component.ts
@@ -19,7 +19,7 @@ import { openDeleteCredentialDialogComponent } from "./delete-credential-dialog/
 export class WebauthnLoginSettingsComponent implements OnInit, OnDestroy {
   private destroy$ = new Subject<void>();
 
-  protected readonly MaxCredentialCount = 5;
+  protected readonly MaxCredentialCount = WebauthnLoginService.MaxCredentialCount;
 
   protected credentials?: WebauthnCredentialView[];
   protected loading = true;