From 72f10bab656f4442fba211354857b3da0a75f97a Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Tue, 16 May 2023 15:16:24 +0200 Subject: [PATCH] [PM-2014] fix: move error handling to components After discussing it with Jake we decided that following convention was best. --- .../webauthn/webauthn.service.spec.ts | 73 +------------------ .../services/webauthn/webauthn.service.ts | 69 +++--------------- .../create-credential-dialog.component.ts | 50 +++++++++---- .../delete-credential-dialog.component.ts | 35 +++++++-- 4 files changed, 76 insertions(+), 151 deletions(-) diff --git a/apps/web/src/app/auth/core/services/webauthn/webauthn.service.spec.ts b/apps/web/src/app/auth/core/services/webauthn/webauthn.service.spec.ts index 929ac1a0a8..fbffbe28ff 100644 --- a/apps/web/src/app/auth/core/services/webauthn/webauthn.service.spec.ts +++ b/apps/web/src/app/auth/core/services/webauthn/webauthn.service.spec.ts @@ -1,20 +1,12 @@ import { mock, MockProxy } from "jest-mock-extended"; -import { I18nService } from "@bitwarden/common/abstractions/i18n.service"; -import { PlatformUtilsService } from "@bitwarden/common/abstractions/platformUtils.service"; -import { VerificationType } from "@bitwarden/common/auth/enums/verification-type"; -import { Verification } from "@bitwarden/common/types/verification"; - import { CredentialCreateOptionsView } from "../../views/credential-create-options.view"; -import { CredentialCreateOptionsResponse } from "./response/credential-create-options.response"; import { WebauthnApiService } from "./webauthn-api.service"; import { WebauthnService } from "./webauthn.service"; describe("WebauthnService", () => { let apiService!: MockProxy; - let platformUtilsService!: MockProxy; - let i18nService!: MockProxy; let credentials: MockProxy; let webauthnService!: WebauthnService; @@ -23,39 +15,8 @@ describe("WebauthnService", () => { window.PublicKeyCredential = class {} as any; window.AuthenticatorAttestationResponse = class {} as any; apiService = mock(); - platformUtilsService = mock(); - i18nService = mock(); credentials = mock(); - webauthnService = new WebauthnService( - apiService, - platformUtilsService, - i18nService, - credentials - ); - }); - - describe("getNewCredentialOptions", () => { - it("should return undefined and show toast when api service call throws", async () => { - apiService.getCredentialCreateOptions.mockRejectedValue(new Error("Mock error")); - const verification = createVerification(); - - const result = await webauthnService.getCredentialCreateOptions(verification); - - expect(result).toBeUndefined(); - expect(platformUtilsService.showToast).toHaveBeenCalled(); - }); - - it("should return options when api service call is successfull", async () => { - const options = Symbol() as any; - const token = Symbol() as any; - const response = { options, token } as CredentialCreateOptionsResponse; - apiService.getCredentialCreateOptions.mockResolvedValue(response); - const verification = createVerification(); - - const result = await webauthnService.getCredentialCreateOptions(verification); - - expect(result).toEqual({ options, token }); - }); + webauthnService = new WebauthnService(apiService, credentials); }); describe("createCredential", () => { @@ -78,40 +39,8 @@ describe("WebauthnService", () => { expect(result).toBe(credential); }); }); - - describe("saveCredential", () => { - it("should return false and show toast when api service call throws", async () => { - apiService.saveCredential.mockRejectedValue(new Error("Mock error")); - const options = createCredentialCreateOptions(); - const deviceResponse = Symbol() as any; - - const result = await webauthnService.saveCredential(options, deviceResponse, "name"); - - expect(result).toBe(false); - expect(platformUtilsService.showToast).toHaveBeenCalled(); - }); - - it("should return true when api service call is successfull", async () => { - apiService.saveCredential.mockResolvedValue(true); - const options = createCredentialCreateOptions(); - const deviceResponse = createDeviceResponse(); - - const result = await webauthnService.saveCredential(options, deviceResponse, "name"); - - expect(result).toBe(true); - expect(apiService.saveCredential).toHaveBeenCalled(); - expect(platformUtilsService.showToast).toHaveBeenCalled(); - }); - }); }); -function createVerification(): Verification { - return { - type: VerificationType.MasterPassword, - secret: "secret", - }; -} - function createCredentialCreateOptions(): CredentialCreateOptionsView { return new CredentialCreateOptionsView(Symbol() as any, Symbol() as any); } diff --git a/apps/web/src/app/auth/core/services/webauthn/webauthn.service.ts b/apps/web/src/app/auth/core/services/webauthn/webauthn.service.ts index 7c09aa51e0..4c5e1fb2c6 100644 --- a/apps/web/src/app/auth/core/services/webauthn/webauthn.service.ts +++ b/apps/web/src/app/auth/core/services/webauthn/webauthn.service.ts @@ -1,10 +1,7 @@ import { Injectable, Optional } from "@angular/core"; import { BehaviorSubject, filter, from, map, Observable, shareReplay, switchMap, tap } from "rxjs"; -import { I18nService } from "@bitwarden/common/abstractions/i18n.service"; import { LogService } from "@bitwarden/common/abstractions/log.service"; -import { PlatformUtilsService } from "@bitwarden/common/abstractions/platformUtils.service"; -import { ErrorResponse } from "@bitwarden/common/models/response/error.response"; import { Verification } from "@bitwarden/common/types/verification"; import { CoreAuthModule } from "../../core.module"; @@ -31,8 +28,6 @@ export class WebauthnService { constructor( private apiService: WebauthnApiService, - private platformUtilsService: PlatformUtilsService, - private i18nService: I18nService, @Optional() navigatorCredentials?: CredentialsContainer, @Optional() private logService?: LogService ) { @@ -43,22 +38,8 @@ export class WebauthnService { async getCredentialCreateOptions( verification: Verification ): Promise { - try { - const response = await this.apiService.getCredentialCreateOptions(verification); - return new CredentialCreateOptionsView(response.options, response.token); - } catch (error) { - if (error instanceof ErrorResponse && error.statusCode === 400) { - this.platformUtilsService.showToast( - "error", - this.i18nService.t("error"), - this.i18nService.t("invalidMasterPassword") - ); - } else { - this.logService?.error(error); - this.platformUtilsService.showToast("error", null, this.i18nService.t("unexpectedError")); - } - return undefined; - } + const response = await this.apiService.getCredentialCreateOptions(verification); + return new CredentialCreateOptionsView(response.options, response.token); } async createCredential( @@ -85,24 +66,12 @@ export class WebauthnService { deviceResponse: PublicKeyCredential, name: string ) { - try { - const request = new SaveCredentialRequest(); - request.deviceResponse = new WebauthnAttestationResponseRequest(deviceResponse); - request.token = credentialOptions.token; - request.name = name; - await this.apiService.saveCredential(request); - this.platformUtilsService.showToast( - "success", - null, - this.i18nService.t("passkeySaved", name) - ); - this.refresh(); - return true; - } catch (error) { - this.logService?.error(error); - this.platformUtilsService.showToast("error", null, this.i18nService.t("unexpectedError")); - return false; - } + const request = new SaveCredentialRequest(); + request.deviceResponse = new WebauthnAttestationResponseRequest(deviceResponse); + request.token = credentialOptions.token; + request.name = name; + await this.apiService.saveCredential(request); + this.refresh(); } getCredential$(credentialId: string): Observable { @@ -114,25 +83,9 @@ export class WebauthnService { ); } - async deleteCredential(credentialId: string, verification: Verification): Promise { - try { - await this.apiService.deleteCredential(credentialId, verification); - this.platformUtilsService.showToast("success", null, this.i18nService.t("passkeyRemoved")); - this.refresh(); - return true; - } catch (error) { - if (error instanceof ErrorResponse && error.statusCode === 400) { - this.platformUtilsService.showToast( - "error", - this.i18nService.t("error"), - this.i18nService.t("invalidMasterPassword") - ); - } else { - this.logService?.error(error); - this.platformUtilsService.showToast("error", null, this.i18nService.t("unexpectedError")); - } - return false; - } + async deleteCredential(credentialId: string, verification: Verification): Promise { + await this.apiService.deleteCredential(credentialId, verification); + this.refresh(); } private getCredentials$(): Observable { diff --git a/apps/web/src/app/auth/settings/fido2-login-settings/create-credential-dialog/create-credential-dialog.component.ts b/apps/web/src/app/auth/settings/fido2-login-settings/create-credential-dialog/create-credential-dialog.component.ts index fa8424b051..06b2276af8 100644 --- a/apps/web/src/app/auth/settings/fido2-login-settings/create-credential-dialog/create-credential-dialog.component.ts +++ b/apps/web/src/app/auth/settings/fido2-login-settings/create-credential-dialog/create-credential-dialog.component.ts @@ -4,7 +4,11 @@ import { FormBuilder, Validators } from "@angular/forms"; import { map, Observable } from "rxjs"; import { DialogServiceAbstraction } from "@bitwarden/angular/services/dialog"; +import { I18nService } from "@bitwarden/common/abstractions/i18n.service"; +import { LogService } from "@bitwarden/common/abstractions/log.service"; +import { PlatformUtilsService } from "@bitwarden/common/abstractions/platformUtils.service"; import { VerificationType } from "@bitwarden/common/auth/enums/verification-type"; +import { ErrorResponse } from "@bitwarden/common/models/response/error.response"; import { WebauthnService } from "../../../core"; import { CredentialCreateOptionsView } from "../../../core/views/credential-create-options.view"; @@ -46,7 +50,10 @@ export class CreateCredentialDialogComponent implements OnInit { constructor( private formBuilder: FormBuilder, private dialogRef: DialogRef, - private webauthnService: WebauthnService + private webauthnService: WebauthnService, + private platformUtilsService: PlatformUtilsService, + private i18nService: I18nService, + private logService: LogService ) {} ngOnInit(): void { @@ -80,12 +87,22 @@ export class CreateCredentialDialogComponent implements OnInit { return; } - this.credentialOptions = await this.webauthnService.getCredentialCreateOptions({ - type: VerificationType.MasterPassword, - secret: this.formGroup.value.userVerification.masterPassword, - }); - - if (this.credentialOptions === undefined) { + try { + this.credentialOptions = await this.webauthnService.getCredentialCreateOptions({ + type: VerificationType.MasterPassword, + secret: this.formGroup.value.userVerification.masterPassword, + }); + } catch (error) { + if (error instanceof ErrorResponse && error.statusCode === 400) { + this.platformUtilsService.showToast( + "error", + this.i18nService.t("error"), + this.i18nService.t("invalidMasterPassword") + ); + } else { + this.logService?.error(error); + this.platformUtilsService.showToast("error", null, this.i18nService.t("unexpectedError")); + } return; } @@ -114,16 +131,21 @@ export class CreateCredentialDialogComponent implements OnInit { return; } - const result = await this.webauthnService.saveCredential( - this.credentialOptions, - this.deviceResponse, - this.formGroup.value.credentialNaming.name - ); - - if (!result) { + const name = this.formGroup.value.credentialNaming.name; + try { + await this.webauthnService.saveCredential( + this.credentialOptions, + this.deviceResponse, + this.formGroup.value.credentialNaming.name + ); + } catch (error) { + this.logService?.error(error); + this.platformUtilsService.showToast("error", null, this.i18nService.t("unexpectedError")); return; } + this.platformUtilsService.showToast("success", null, this.i18nService.t("passkeySaved", name)); + this.dialogRef.close(CreateCredentialDialogResult.Success); } } diff --git a/apps/web/src/app/auth/settings/fido2-login-settings/delete-credential-dialog/delete-credential-dialog.component.ts b/apps/web/src/app/auth/settings/fido2-login-settings/delete-credential-dialog/delete-credential-dialog.component.ts index 1f99eaed60..1feef15965 100644 --- a/apps/web/src/app/auth/settings/fido2-login-settings/delete-credential-dialog/delete-credential-dialog.component.ts +++ b/apps/web/src/app/auth/settings/fido2-login-settings/delete-credential-dialog/delete-credential-dialog.component.ts @@ -4,7 +4,11 @@ import { FormBuilder, Validators } from "@angular/forms"; import { Subject, takeUntil } from "rxjs"; import { DialogServiceAbstraction } from "@bitwarden/angular/services/dialog"; +import { I18nService } from "@bitwarden/common/abstractions/i18n.service"; +import { LogService } from "@bitwarden/common/abstractions/log.service"; +import { PlatformUtilsService } from "@bitwarden/common/abstractions/platformUtils.service"; import { VerificationType } from "@bitwarden/common/auth/enums/verification-type"; +import { ErrorResponse } from "@bitwarden/common/models/response/error.response"; import { WebauthnService } from "../../../core"; import { WebauthnCredentialView } from "../../../core/views/webauth-credential.view"; @@ -28,7 +32,10 @@ export class DeleteCredentialDialogComponent implements OnInit, OnDestroy { @Inject(DIALOG_DATA) private params: DeleteCredentialDialogParams, private formBuilder: FormBuilder, private dialogRef: DialogRef, - private webauthnService: WebauthnService + private webauthnService: WebauthnService, + private platformUtilsService: PlatformUtilsService, + private i18nService: I18nService, + private logService: LogService ) {} ngOnInit(): void { @@ -44,14 +51,28 @@ export class DeleteCredentialDialogComponent implements OnInit, OnDestroy { } this.dialogRef.disableClose = true; - const success = await this.webauthnService.deleteCredential(this.credential.id, { - type: VerificationType.MasterPassword, - secret: this.formGroup.value.masterPassword, - }); - if (!success) { + try { + await this.webauthnService.deleteCredential(this.credential.id, { + type: VerificationType.MasterPassword, + secret: this.formGroup.value.masterPassword, + }); + this.platformUtilsService.showToast("success", null, this.i18nService.t("passkeyRemoved")); + } catch (error) { + if (error instanceof ErrorResponse && error.statusCode === 400) { + this.platformUtilsService.showToast( + "error", + this.i18nService.t("error"), + this.i18nService.t("invalidMasterPassword") + ); + } else { + this.logService.error(error); + this.platformUtilsService.showToast("error", null, this.i18nService.t("unexpectedError")); + } + return false; + } finally { this.dialogRef.disableClose = false; - return; } + this.dialogRef.close(); };