From bd6b3266d43327183cb33c4df18005fc0436e9a1 Mon Sep 17 00:00:00 2001 From: Jake Fink Date: Thu, 28 Mar 2024 09:34:21 -0400 Subject: [PATCH] move auth request notification to service (#8451) - cleanup hanging promises --- .../popup/login-via-auth-request.component.ts | 7 +- .../login/login-via-auth-request.component.ts | 11 +-- .../login/login-via-auth-request.component.ts | 70 +------------------ .../login-via-auth-request.component.ts | 21 +++--- .../src/services/jslib-services.module.ts | 2 +- .../auth-request.service.abstraction.ts | 12 ++++ .../abstractions/login-strategy.service.ts | 9 --- .../auth-request/auth-request.service.ts | 12 ++++ .../login-strategy.service.ts | 14 ---- .../abstractions/anonymous-hub.service.ts | 4 +- .../auth/services/anonymous-hub.service.ts | 28 ++++---- 11 files changed, 57 insertions(+), 133 deletions(-) diff --git a/apps/browser/src/auth/popup/login-via-auth-request.component.ts b/apps/browser/src/auth/popup/login-via-auth-request.component.ts index a22636389a..4ef1c78cb4 100644 --- a/apps/browser/src/auth/popup/login-via-auth-request.component.ts +++ b/apps/browser/src/auth/popup/login-via-auth-request.component.ts @@ -1,5 +1,5 @@ import { Location } from "@angular/common"; -import { Component, OnDestroy, OnInit } from "@angular/core"; +import { Component } from "@angular/core"; import { Router } from "@angular/router"; import { LoginViaAuthRequestComponent as BaseLoginWithDeviceComponent } from "@bitwarden/angular/auth/components/login-via-auth-request.component"; @@ -28,10 +28,7 @@ import { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.serv selector: "app-login-via-auth-request", templateUrl: "login-via-auth-request.component.html", }) -export class LoginViaAuthRequestComponent - extends BaseLoginWithDeviceComponent - implements OnInit, OnDestroy -{ +export class LoginViaAuthRequestComponent extends BaseLoginWithDeviceComponent { constructor( router: Router, cryptoService: CryptoService, diff --git a/apps/desktop/src/auth/login/login-via-auth-request.component.ts b/apps/desktop/src/auth/login/login-via-auth-request.component.ts index b4242c36fb..9a6fa8e388 100644 --- a/apps/desktop/src/auth/login/login-via-auth-request.component.ts +++ b/apps/desktop/src/auth/login/login-via-auth-request.component.ts @@ -1,5 +1,5 @@ import { Location } from "@angular/common"; -import { Component, OnDestroy, OnInit, ViewChild, ViewContainerRef } from "@angular/core"; +import { Component, ViewChild, ViewContainerRef } from "@angular/core"; import { Router } from "@angular/router"; import { LoginViaAuthRequestComponent as BaseLoginWithDeviceComponent } from "@bitwarden/angular/auth/components/login-via-auth-request.component"; @@ -31,10 +31,7 @@ import { EnvironmentComponent } from "../environment.component"; selector: "app-login-via-auth-request", templateUrl: "login-via-auth-request.component.html", }) -export class LoginViaAuthRequestComponent - extends BaseLoginWithDeviceComponent - implements OnInit, OnDestroy -{ +export class LoginViaAuthRequestComponent extends BaseLoginWithDeviceComponent { @ViewChild("environment", { read: ViewContainerRef, static: true }) environmentModal: ViewContainerRef; showingModal = false; @@ -109,10 +106,6 @@ export class LoginViaAuthRequestComponent }); } - ngOnDestroy(): void { - super.ngOnDestroy(); - } - back() { this.location.back(); } diff --git a/apps/web/src/app/auth/login/login-via-auth-request.component.ts b/apps/web/src/app/auth/login/login-via-auth-request.component.ts index a3bf1160a3..5bca718304 100644 --- a/apps/web/src/app/auth/login/login-via-auth-request.component.ts +++ b/apps/web/src/app/auth/login/login-via-auth-request.component.ts @@ -1,75 +1,9 @@ -import { Component, OnDestroy, OnInit } from "@angular/core"; -import { Router } from "@angular/router"; +import { Component } from "@angular/core"; import { LoginViaAuthRequestComponent as BaseLoginWithDeviceComponent } from "@bitwarden/angular/auth/components/login-via-auth-request.component"; -import { - AuthRequestServiceAbstraction, - LoginStrategyServiceAbstraction, -} from "@bitwarden/auth/common"; -import { ApiService } from "@bitwarden/common/abstractions/api.service"; -import { AnonymousHubService } from "@bitwarden/common/auth/abstractions/anonymous-hub.service"; -import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; -import { DeviceTrustCryptoServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust-crypto.service.abstraction"; -import { LoginService } from "@bitwarden/common/auth/abstractions/login.service"; -import { AppIdService } from "@bitwarden/common/platform/abstractions/app-id.service"; -import { CryptoFunctionService } from "@bitwarden/common/platform/abstractions/crypto-function.service"; -import { CryptoService } from "@bitwarden/common/platform/abstractions/crypto.service"; -import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; -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 { ValidationService } from "@bitwarden/common/platform/abstractions/validation.service"; -import { PasswordGenerationServiceAbstraction } from "@bitwarden/common/tools/generator/password"; - -import { StateService } from "../../core"; @Component({ selector: "app-login-via-auth-request", templateUrl: "login-via-auth-request.component.html", }) -export class LoginViaAuthRequestComponent - extends BaseLoginWithDeviceComponent - implements OnInit, OnDestroy -{ - constructor( - router: Router, - cryptoService: CryptoService, - cryptoFunctionService: CryptoFunctionService, - appIdService: AppIdService, - passwordGenerationService: PasswordGenerationServiceAbstraction, - apiService: ApiService, - authService: AuthService, - logService: LogService, - environmentService: EnvironmentService, - i18nService: I18nService, - platformUtilsService: PlatformUtilsService, - anonymousHubService: AnonymousHubService, - validationService: ValidationService, - stateService: StateService, - loginService: LoginService, - deviceTrustCryptoService: DeviceTrustCryptoServiceAbstraction, - authRequestService: AuthRequestServiceAbstraction, - loginStrategyService: LoginStrategyServiceAbstraction, - ) { - super( - router, - cryptoService, - cryptoFunctionService, - appIdService, - passwordGenerationService, - apiService, - authService, - logService, - environmentService, - i18nService, - platformUtilsService, - anonymousHubService, - validationService, - stateService, - loginService, - deviceTrustCryptoService, - authRequestService, - loginStrategyService, - ); - } -} +export class LoginViaAuthRequestComponent extends BaseLoginWithDeviceComponent {} diff --git a/libs/angular/src/auth/components/login-via-auth-request.component.ts b/libs/angular/src/auth/components/login-via-auth-request.component.ts index 45d7f563f7..b1d0b81922 100644 --- a/libs/angular/src/auth/components/login-via-auth-request.component.ts +++ b/libs/angular/src/auth/components/login-via-auth-request.component.ts @@ -68,7 +68,6 @@ export class LoginViaAuthRequestComponent private authRequestKeyPair: { publicKey: Uint8Array; privateKey: Uint8Array }; - // TODO: in future, go to child components and remove child constructors and let deps fall through to the super class constructor( protected router: Router, private cryptoService: CryptoService, @@ -98,14 +97,16 @@ export class LoginViaAuthRequestComponent this.email = this.loginService.getEmail(); } - //gets signalR push notification - this.loginStrategyService.authRequestPushNotification$ + // Gets signalR push notification + // Only fires on approval to prevent enumeration + this.authRequestService.authRequestPushNotification$ .pipe(takeUntil(this.destroy$)) .subscribe((id) => { - // Only fires on approval currently - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.verifyAndHandleApprovedAuthReq(id); + this.verifyAndHandleApprovedAuthReq(id).catch((e: Error) => { + this.platformUtilsService.showToast("error", this.i18nService.t("error"), e.message); + this.logService.error("Failed to use approved auth request: " + e.message); + }); }); } @@ -164,10 +165,10 @@ export class LoginViaAuthRequestComponent } } - ngOnDestroy(): void { + async ngOnDestroy() { + await this.anonymousHubService.stopHubConnection(); this.destroy$.next(); this.destroy$.complete(); - this.anonymousHubService.stopHubConnection(); } private async handleExistingAdminAuthRequest(adminAuthReqStorable: AdminAuthRequestStorable) { @@ -213,7 +214,7 @@ export class LoginViaAuthRequestComponent // Request still pending response from admin // So, create hub connection so that any approvals will be received via push notification - this.anonymousHubService.createHubConnection(adminAuthReqStorable.id); + await this.anonymousHubService.createHubConnection(adminAuthReqStorable.id); } private async handleExistingAdminAuthReqDeletedOrDenied() { @@ -273,7 +274,7 @@ export class LoginViaAuthRequestComponent } if (reqResponse.id) { - this.anonymousHubService.createHubConnection(reqResponse.id); + await this.anonymousHubService.createHubConnection(reqResponse.id); } } catch (e) { this.logService.error(e); diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 521387181b..b2f5ee1f89 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -866,7 +866,7 @@ const safeProviders: SafeProvider[] = [ safeProvider({ provide: AnonymousHubServiceAbstraction, useClass: AnonymousHubService, - deps: [EnvironmentService, LoginStrategyServiceAbstraction, LogService], + deps: [EnvironmentService, AuthRequestServiceAbstraction], }), safeProvider({ provide: ValidationServiceAbstraction, diff --git a/libs/auth/src/common/abstractions/auth-request.service.abstraction.ts b/libs/auth/src/common/abstractions/auth-request.service.abstraction.ts index b91444d3e6..7af92fc8f8 100644 --- a/libs/auth/src/common/abstractions/auth-request.service.abstraction.ts +++ b/libs/auth/src/common/abstractions/auth-request.service.abstraction.ts @@ -1,7 +1,12 @@ +import { Observable } from "rxjs"; + import { AuthRequestResponse } from "@bitwarden/common/auth/models/response/auth-request.response"; +import { AuthRequestPushNotification } from "@bitwarden/common/models/response/notification.response"; import { UserKey, MasterKey } from "@bitwarden/common/types/key"; export abstract class AuthRequestServiceAbstraction { + /** Emits an auth request id when an auth request has been approved. */ + authRequestPushNotification$: Observable; /** * Approve or deny an auth request. * @param approve True to approve, false to deny. @@ -54,4 +59,11 @@ export abstract class AuthRequestServiceAbstraction { pubKeyEncryptedMasterKeyHash: string, privateKey: ArrayBuffer, ) => Promise<{ masterKey: MasterKey; masterKeyHash: string }>; + + /** + * Handles incoming auth request push notifications. + * @param notification push notification. + * @remark We should only be receiving approved push notifications to prevent enumeration. + */ + abstract sendAuthRequestPushNotification: (notification: AuthRequestPushNotification) => void; } diff --git a/libs/auth/src/common/abstractions/login-strategy.service.ts b/libs/auth/src/common/abstractions/login-strategy.service.ts index e3ed63c737..eae6dc2a27 100644 --- a/libs/auth/src/common/abstractions/login-strategy.service.ts +++ b/libs/auth/src/common/abstractions/login-strategy.service.ts @@ -4,7 +4,6 @@ import { AuthenticationType } from "@bitwarden/common/auth/enums/authentication- import { AuthResult } from "@bitwarden/common/auth/models/domain/auth-result"; import { TokenTwoFactorRequest } from "@bitwarden/common/auth/models/request/identity-token/token-two-factor.request"; import { AuthRequestResponse } from "@bitwarden/common/auth/models/response/auth-request.response"; -import { AuthRequestPushNotification } from "@bitwarden/common/models/response/notification.response"; import { MasterKey } from "@bitwarden/common/types/key"; import { @@ -21,10 +20,6 @@ export abstract class LoginStrategyServiceAbstraction { * Emits null if the session has timed out. */ currentAuthType$: Observable; - /** - * Emits when an auth request has been approved. - */ - authRequestPushNotification$: Observable; /** * If the login strategy uses the email address of the user, this * will return it. Otherwise, it will return null. @@ -77,10 +72,6 @@ export abstract class LoginStrategyServiceAbstraction { * Creates a master key from the provided master password and email. */ makePreloginKey: (masterPassword: string, email: string) => Promise; - /** - * Sends a notification to {@link LoginStrategyServiceAbstraction.authRequestPushNotification} - */ - sendAuthRequestPushNotification: (notification: AuthRequestPushNotification) => Promise; /** * Sends a response to an auth request. */ diff --git a/libs/auth/src/common/services/auth-request/auth-request.service.ts b/libs/auth/src/common/services/auth-request/auth-request.service.ts index ab33780fe6..ff33eadfba 100644 --- a/libs/auth/src/common/services/auth-request/auth-request.service.ts +++ b/libs/auth/src/common/services/auth-request/auth-request.service.ts @@ -1,6 +1,9 @@ +import { Observable, Subject } from "rxjs"; + import { ApiService } from "@bitwarden/common/abstractions/api.service"; import { PasswordlessAuthRequest } from "@bitwarden/common/auth/models/request/passwordless-auth.request"; import { AuthRequestResponse } from "@bitwarden/common/auth/models/response/auth-request.response"; +import { AuthRequestPushNotification } from "@bitwarden/common/models/response/notification.response"; import { AppIdService } from "@bitwarden/common/platform/abstractions/app-id.service"; import { CryptoService } from "@bitwarden/common/platform/abstractions/crypto.service"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; @@ -11,6 +14,9 @@ import { MasterKey, UserKey } from "@bitwarden/common/types/key"; import { AuthRequestServiceAbstraction } from "../../abstractions/auth-request.service.abstraction"; export class AuthRequestService implements AuthRequestServiceAbstraction { + private authRequestPushNotificationSubject = new Subject(); + authRequestPushNotification$: Observable; + constructor( private appIdService: AppIdService, private cryptoService: CryptoService, @@ -126,4 +132,10 @@ export class AuthRequestService implements AuthRequestServiceAbstraction { masterKeyHash, }; } + + sendAuthRequestPushNotification(notification: AuthRequestPushNotification): void { + if (notification.id != null) { + this.authRequestPushNotificationSubject.next(notification.id); + } + } } diff --git a/libs/auth/src/common/services/login-strategies/login-strategy.service.ts b/libs/auth/src/common/services/login-strategies/login-strategy.service.ts index 428258308a..b55f38af7f 100644 --- a/libs/auth/src/common/services/login-strategies/login-strategy.service.ts +++ b/libs/auth/src/common/services/login-strategies/login-strategy.service.ts @@ -1,7 +1,6 @@ import { combineLatestWith, distinctUntilChanged, - filter, firstValueFrom, map, Observable, @@ -23,7 +22,6 @@ import { AuthRequestResponse } from "@bitwarden/common/auth/models/response/auth import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service"; import { PreloginRequest } from "@bitwarden/common/models/request/prelogin.request"; import { ErrorResponse } from "@bitwarden/common/models/response/error.response"; -import { AuthRequestPushNotification } from "@bitwarden/common/models/response/notification.response"; import { AppIdService } from "@bitwarden/common/platform/abstractions/app-id.service"; import { CryptoService } from "@bitwarden/common/platform/abstractions/crypto.service"; import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service"; @@ -81,8 +79,6 @@ export class LoginStrategyService implements LoginStrategyServiceAbstraction { >; currentAuthType$: Observable; - // TODO: move to auth request service - authRequestPushNotification$: Observable; constructor( protected cryptoService: CryptoService, @@ -114,9 +110,6 @@ export class LoginStrategyService implements LoginStrategyServiceAbstraction { ); this.currentAuthType$ = this.currentAuthnTypeState.state$; - this.authRequestPushNotification$ = this.authRequestPushNotificationState.state$.pipe( - filter((id) => id != null), - ); this.loginStrategy$ = this.currentAuthnTypeState.state$.pipe( distinctUntilChanged(), combineLatestWith(this.loginStrategyCacheState.state$), @@ -256,13 +249,6 @@ export class LoginStrategyService implements LoginStrategyServiceAbstraction { return await this.cryptoService.makeMasterKey(masterPassword, email, kdf, kdfConfig); } - // TODO move to auth request service - async sendAuthRequestPushNotification(notification: AuthRequestPushNotification): Promise { - if (notification.id != null) { - await this.authRequestPushNotificationState.update((_) => notification.id); - } - } - // TODO: move to auth request service async passwordlessLogin( id: string, diff --git a/libs/common/src/auth/abstractions/anonymous-hub.service.ts b/libs/common/src/auth/abstractions/anonymous-hub.service.ts index 43bdabd512..e108dccbb6 100644 --- a/libs/common/src/auth/abstractions/anonymous-hub.service.ts +++ b/libs/common/src/auth/abstractions/anonymous-hub.service.ts @@ -1,4 +1,4 @@ export abstract class AnonymousHubService { - createHubConnection: (token: string) => void; - stopHubConnection: () => void; + createHubConnection: (token: string) => Promise; + stopHubConnection: () => Promise; } diff --git a/libs/common/src/auth/services/anonymous-hub.service.ts b/libs/common/src/auth/services/anonymous-hub.service.ts index fe8ae64183..747fbc3917 100644 --- a/libs/common/src/auth/services/anonymous-hub.service.ts +++ b/libs/common/src/auth/services/anonymous-hub.service.ts @@ -7,13 +7,13 @@ import { import { MessagePackHubProtocol } from "@microsoft/signalr-protocol-msgpack"; import { firstValueFrom } from "rxjs"; -import { LoginStrategyServiceAbstraction } from "../../../../auth/src/common/abstractions/login-strategy.service"; +import { AuthRequestServiceAbstraction } from "../../../../auth/src/common/abstractions"; +import { NotificationType } from "../../enums"; import { AuthRequestPushNotification, NotificationResponse, } from "../../models/response/notification.response"; import { EnvironmentService } from "../../platform/abstractions/environment.service"; -import { LogService } from "../../platform/abstractions/log.service"; import { AnonymousHubService as AnonymousHubServiceAbstraction } from "../abstractions/anonymous-hub.service"; export class AnonymousHubService implements AnonymousHubServiceAbstraction { @@ -22,8 +22,7 @@ export class AnonymousHubService implements AnonymousHubServiceAbstraction { constructor( private environmentService: EnvironmentService, - private loginStrategyService: LoginStrategyServiceAbstraction, - private logService: LogService, + private authRequestService: AuthRequestServiceAbstraction, ) {} async createHubConnection(token: string) { @@ -37,26 +36,25 @@ export class AnonymousHubService implements AnonymousHubServiceAbstraction { .withHubProtocol(new MessagePackHubProtocol() as IHubProtocol) .build(); - this.anonHubConnection.start().catch((error) => this.logService.error(error)); + await this.anonHubConnection.start(); this.anonHubConnection.on("AuthRequestResponseRecieved", (data: any) => { - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises this.ProcessNotification(new NotificationResponse(data)); }); } - stopHubConnection() { + async stopHubConnection() { if (this.anonHubConnection) { - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.anonHubConnection.stop(); + await this.anonHubConnection.stop(); } } - private async ProcessNotification(notification: NotificationResponse) { - await this.loginStrategyService.sendAuthRequestPushNotification( - notification.payload as AuthRequestPushNotification, - ); + private ProcessNotification(notification: NotificationResponse) { + switch (notification.type) { + case NotificationType.AuthRequestResponse: + this.authRequestService.sendAuthRequestPushNotification( + notification.payload as AuthRequestPushNotification, + ); + } } }