From 1f96f171b7fc5d0d73c0647c27c3e9cc66d26690 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Mon, 20 Oct 2025 08:46:56 -0700 Subject: [PATCH] extract listener setup --- apps/browser/src/popup/app.component.ts | 39 +------------- apps/desktop/src/app/app.component.ts | 13 ++--- .../src/vault/app/vault/vault-v2.component.ts | 9 ---- ...h-request-answering.service.abstraction.ts | 16 ++++-- .../default-auth-request-answering.service.ts | 51 ++++++++++++++++++- .../noop-auth-request-answering.service.ts | 2 + .../default-server-notifications.service.ts | 11 ++-- 7 files changed, 72 insertions(+), 69 deletions(-) diff --git a/apps/browser/src/popup/app.component.ts b/apps/browser/src/popup/app.component.ts index 3cd0b072841..a2924afe27e 100644 --- a/apps/browser/src/popup/app.component.ts +++ b/apps/browser/src/popup/app.component.ts @@ -14,16 +14,11 @@ import { NavigationEnd, Router, RouterOutlet } from "@angular/router"; import { catchError, concatMap, - distinctUntilChanged, filter, firstValueFrom, map, of, - pairwise, - startWith, Subject, - switchMap, - take, takeUntil, tap, } from "rxjs"; @@ -132,22 +127,7 @@ export class AppComponent implements OnInit, OnDestroy { this.activeUserId = account?.id; }); - // Trigger processing auth requests when the active user is in an unlocked state. Runs once when - // the popup is open. - this.accountService.activeAccount$ - .pipe( - map((a) => a?.id), // Extract active userId - distinctUntilChanged(), // Only when userId actually changes - filter((userId) => userId != null), // Require a valid userId - switchMap((userId) => this.authService.authStatusFor$(userId).pipe(take(1))), // Get current auth status once for new user - filter((status) => status === AuthenticationStatus.Unlocked), // Only when the new user is Unlocked - tap(() => { - // Trigger processing when switching users while popup is open - void this.authRequestAnsweringService.processPendingAuthRequests(); - }), - takeUntil(this.destroy$), - ) - .subscribe(); + this.authRequestAnsweringService.setupUnlockListenersForProcessingAuthRequests(this.destroy$); this.authService.activeAccountStatus$ .pipe( @@ -159,23 +139,6 @@ export class AppComponent implements OnInit, OnDestroy { ) .subscribe(); - // When the popup is already open and the active account transitions to Unlocked, - // process any pending auth requests for the active user. The above subscription does not handle - // this case. - this.authService.activeAccountStatus$ - .pipe( - startWith(null as unknown as AuthenticationStatus), // Seed previous value to handle initial emission - pairwise(), // Compare previous and current statuses - filter( - ([prev, curr]) => - prev !== AuthenticationStatus.Unlocked && curr === AuthenticationStatus.Unlocked, // Fire on transitions into Unlocked (incl. initial) - ), - takeUntil(this.destroy$), - ) - .subscribe(() => { - void this.authRequestAnsweringService.processPendingAuthRequests(); - }); - this.ngZone.runOutsideAngular(() => { window.onmousedown = () => this.recordActivity(); window.ontouchstart = () => this.recordActivity(); diff --git a/apps/desktop/src/app/app.component.ts b/apps/desktop/src/app/app.component.ts index a17484a4195..60aa3bf7f6e 100644 --- a/apps/desktop/src/app/app.component.ts +++ b/apps/desktop/src/app/app.component.ts @@ -40,6 +40,7 @@ import { EventUploadService } from "@bitwarden/common/abstractions/event/event-u import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { InternalPolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { AuthRequestAnsweringService } from "@bitwarden/common/auth/abstractions/auth-request-answering/auth-request-answering.service.abstraction"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { TokenService } from "@bitwarden/common/auth/abstractions/token.service"; import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction"; @@ -186,6 +187,7 @@ export class AppComponent implements OnInit, OnDestroy { private desktopAutotypeDefaultSettingPolicy: DesktopAutotypeDefaultSettingPolicy, private pendingAuthRequestsState: PendingAuthRequestsStateService, private authRequestService: AuthRequestServiceAbstraction, + private authRequestAnsweringService: AuthRequestAnsweringService, ) { this.deviceTrustToastService.setupListeners$.pipe(takeUntilDestroyed()).subscribe(); @@ -198,6 +200,8 @@ export class AppComponent implements OnInit, OnDestroy { this.activeUserId = account?.id; }); + this.authRequestAnsweringService.setupUnlockListenersForProcessingAuthRequests(this.destroy$); + this.ngZone.runOutsideAngular(() => { setTimeout(async () => { await this.updateAppMenu(); @@ -530,15 +534,6 @@ export class AppComponent implements OnInit, OnDestroy { } finally { this.processingPendingAuth = false; } - - // if (message.notificationId != null) { - // this.dialogService.closeAll(); - // const dialogRef = LoginApprovalDialogComponent.open(this.dialogService, { - // notificationId: message.notificationId, - // }); - // await firstValueFrom(dialogRef.closed); - // } - break; case "redrawMenu": await this.updateAppMenu(); diff --git a/apps/desktop/src/vault/app/vault/vault-v2.component.ts b/apps/desktop/src/vault/app/vault/vault-v2.component.ts index 13830edc4bf..e6fff50beb7 100644 --- a/apps/desktop/src/vault/app/vault/vault-v2.component.ts +++ b/apps/desktop/src/vault/app/vault/vault-v2.component.ts @@ -320,15 +320,6 @@ export class VaultV2Component this.searchBarService.setEnabled(true); this.searchBarService.setPlaceholderText(this.i18nService.t("searchVault")); - // const authRequests = await firstValueFrom( - // this.authRequestService.getLatestPendingAuthRequest$()!, - // ); - // if (authRequests != null) { - // this.messagingService.send("openLoginApproval", { - // notificationId: authRequests.id, - // }); - // } - this.activeUserId = await firstValueFrom( this.accountService.activeAccount$.pipe(getUserId), ).catch((): any => null); diff --git a/libs/common/src/auth/abstractions/auth-request-answering/auth-request-answering.service.abstraction.ts b/libs/common/src/auth/abstractions/auth-request-answering/auth-request-answering.service.abstraction.ts index ec4bfdd698c..04db1efa27b 100644 --- a/libs/common/src/auth/abstractions/auth-request-answering/auth-request-answering.service.abstraction.ts +++ b/libs/common/src/auth/abstractions/auth-request-answering/auth-request-answering.service.abstraction.ts @@ -1,3 +1,5 @@ +import { Observable } from "rxjs"; + import { SystemNotificationEvent } from "@bitwarden/common/platform/system-notifications/system-notifications.service"; import { UserId } from "@bitwarden/user-core"; @@ -17,10 +19,7 @@ export abstract class AuthRequestAnsweringService { /** * Confirms whether or not the user meets the conditions required to show an approval - * dialog immediately. Those conditions are: - * - User must be Unlocked - * - User must be the active user - * - User must not be required to set/change their password + * dialog immediately. * * @param userId the UserId that the auth request is for. * @returns boolean stating whether or not the user meets conditions necessary to show @@ -41,4 +40,13 @@ export abstract class AuthRequestAnsweringService { * approval dialog. */ abstract processPendingAuthRequests(): Promise; + + /** + * Sets up listeners for scenarios where the user unlocks and we want to process + * any pending auth requests in state. + * - Implemented in Extension and Desktop + * + * @param destroy$ The destroy$ observable from the caller + */ + abstract setupUnlockListenersForProcessingAuthRequests(destroy$: Observable): void; } diff --git a/libs/common/src/auth/services/auth-request-answering/default-auth-request-answering.service.ts b/libs/common/src/auth/services/auth-request-answering/default-auth-request-answering.service.ts index 99c6eb3e6f7..aadbcd179ca 100644 --- a/libs/common/src/auth/services/auth-request-answering/default-auth-request-answering.service.ts +++ b/libs/common/src/auth/services/auth-request-answering/default-auth-request-answering.service.ts @@ -1,4 +1,16 @@ -import { firstValueFrom } from "rxjs"; +import { + distinctUntilChanged, + filter, + firstValueFrom, + map, + Observable, + pairwise, + startWith, + switchMap, + take, + takeUntil, + tap, +} from "rxjs"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; @@ -39,6 +51,7 @@ export class DefaultAuthRequestAnsweringService implements AuthRequestAnsweringS this.masterPasswordService.forceSetPasswordReason$(userId), ); + // Use must be unlocked, active, and must not be required to set/change their master password const meetsConditions = authStatus === AuthenticationStatus.Unlocked && activeUserId === userId && @@ -47,7 +60,7 @@ export class DefaultAuthRequestAnsweringService implements AuthRequestAnsweringS return meetsConditions; } - async handleAuthRequestNotificationClicked(event: SystemNotificationEvent) { + async handleAuthRequestNotificationClicked(event: SystemNotificationEvent): Promise { throw new Error("handleAuthRequestNotificationClicked() not implemented for this client"); } @@ -73,4 +86,38 @@ export class DefaultAuthRequestAnsweringService implements AuthRequestAnsweringS } } } + + setupUnlockListenersForProcessingAuthRequests(destroy$: Observable): void { + // Trigger processing auth requests when the active user is in an unlocked state. + this.accountService.activeAccount$ + .pipe( + map((a) => a?.id), // Extract active userId + distinctUntilChanged(), // Only when userId actually changes + filter((userId) => userId != null), // Require a valid userId + switchMap((userId) => this.authService.authStatusFor$(userId).pipe(take(1))), // Get current auth status once for new user + filter((status) => status === AuthenticationStatus.Unlocked), // Only when the new user is Unlocked + tap(() => { + // Trigger processing when switching users while app is visible. + void this.processPendingAuthRequests(); + }), + takeUntil(destroy$), + ) + .subscribe(); + + // When the app is already visible and the active account transitions to Unlocked, process any + // pending auth requests for the active user. The above subscription does not handle this case. + this.authService.activeAccountStatus$ + .pipe( + startWith(null as unknown as AuthenticationStatus), // Seed previous value to handle initial emission + pairwise(), // Compare previous and current statuses + filter( + ([prev, curr]) => + prev !== AuthenticationStatus.Unlocked && curr === AuthenticationStatus.Unlocked, // Fire on transitions into Unlocked (incl. initial) + ), + takeUntil(destroy$), + ) + .subscribe(() => { + void this.processPendingAuthRequests(); + }); + } } diff --git a/libs/common/src/auth/services/auth-request-answering/noop-auth-request-answering.service.ts b/libs/common/src/auth/services/auth-request-answering/noop-auth-request-answering.service.ts index e6f062f85a0..191ef93329e 100644 --- a/libs/common/src/auth/services/auth-request-answering/noop-auth-request-answering.service.ts +++ b/libs/common/src/auth/services/auth-request-answering/noop-auth-request-answering.service.ts @@ -15,4 +15,6 @@ export class NoopAuthRequestAnsweringService implements AuthRequestAnsweringServ async handleAuthRequestNotificationClicked(event: SystemNotificationEvent) {} async processPendingAuthRequests(): Promise {} + + setupUnlockListenersForProcessingAuthRequests(): void {} } diff --git a/libs/common/src/platform/server-notifications/internal/default-server-notifications.service.ts b/libs/common/src/platform/server-notifications/internal/default-server-notifications.service.ts index 9f9b72622f2..6f29ab3b31d 100644 --- a/libs/common/src/platform/server-notifications/internal/default-server-notifications.service.ts +++ b/libs/common/src/platform/server-notifications/internal/default-server-notifications.service.ts @@ -284,14 +284,11 @@ export class DefaultServerNotificationsService implements ServerNotificationsSer ); /** - * This call is necessary for Desktop, which for the time being uses a noop for the - * authRequestAnsweringService.receivedPendingAuthRequest() call just above. Desktop - * will eventually use the new AuthRequestAnsweringService, at which point we can remove - * this second call. + * This call is necessary for Web, which uses a noop for the AuthRequstAnsweringService. * - * The Extension AppComponent has logic (see processingPendingAuth) that only allows one - * pending auth request to process at a time, so this second call will not cause any - * duplicate processing conflicts on Extension. + * The Extension and Desktop AppComponent have logic that allows only one pending + * auth request to process at a time (see processingPendingAuth), so this second call + * will not cause any duplicate processing conflicts on Extension/Desktop. */ this.messagingService.send("openLoginApproval", { notificationId: notification.payload.id,