From e04daf3a9ce1fb2d935b6a247894f85f4cac58f6 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Thu, 28 Aug 2025 14:54:49 -0400 Subject: [PATCH] fix(extension-device-approval): [PM-14943] Answering Service Full Implementation - Restructuring some code, adding comments. --- .../browser/src/background/main.background.ts | 46 ++++++++--- .../device-management.component.ts | 29 +++++-- .../auth-request-answering/README.md | 0 ...h-request-answering.service.abstraction.ts | 24 +++++- .../auth-request-answering.service.ts | 82 +++++++++---------- ...upported-auth-request-answering.service.ts | 7 +- 6 files changed, 122 insertions(+), 66 deletions(-) create mode 100644 libs/common/src/auth/abstractions/auth-request-answering/README.md diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index cf021fa9c69..44e995e57b4 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -2,7 +2,7 @@ // @ts-strict-ignore import "core-js/proposals/explicit-resource-management"; -import { filter, firstValueFrom, map, merge, Subject, switchMap, timeout } from "rxjs"; +import { filter, firstValueFrom, from, map, merge, Observable, Subject, switchMap, timeout } from "rxjs"; import { CollectionService, DefaultCollectionService } from "@bitwarden/admin-console/common"; import { @@ -155,6 +155,7 @@ import { SyncService } from "@bitwarden/common/platform/sync"; // eslint-disable-next-line no-restricted-imports -- Needed for service creation import { DefaultSyncService } from "@bitwarden/common/platform/sync/internal"; import { SystemNotificationsService } from "@bitwarden/common/platform/system-notifications/"; +import { SystemNotificationEvent } from "@bitwarden/common/platform/system-notifications/system-notifications.service"; import { UnsupportedSystemNotificationsService } from "@bitwarden/common/platform/system-notifications/unsupported-system-notifications.service"; import { DefaultThemeStateService } from "@bitwarden/common/platform/theming/theme-state.service"; import { ApiService } from "@bitwarden/common/services/api.service"; @@ -1411,7 +1412,7 @@ export default class MainBackground { // Only the "true" background should run migrations await this.migrationRunner.run(); - // This is here instead of in in the InitService b/c we don't plan for + // This is here instead of in the InitService b/c we don't plan for // side effects to run in the Browser InitService. const accounts = await firstValueFrom(this.accountService.accounts$); @@ -1790,18 +1791,39 @@ export default class MainBackground { /** * This function is for creating any subscriptions for the background service worker. We do this * here because it's important to run this during the evaluation period of the browser extension - * service worker. + * service worker. If it's not done this way we risk the service worker being closed before it's + * registered these system notification click events. */ initNotificationSubscriptions() { - this.systemNotificationService.notificationClicked$ - .pipe( - filter((n) => n.id.startsWith(AuthServerNotificationTags.AuthRequest + "_")), - map((n) => ({ event: n, authRequestId: n.id.split("_")[1] })), - switchMap(({ event }) => - this.authRequestAnsweringService.handleAuthRequestNotificationClicked(event), - ), - ) - .subscribe(); + const handlers: Array<{ + startsWith: string; + handler: (event: SystemNotificationEvent) => Promise; + }> = []; + + const register = ( + startsWith: string, + handler: (event: SystemNotificationEvent) => Promise, + ) => { + handlers.push({ startsWith, handler }); + }; + + // ======= Register All System Notification Handlers Here ======= + register( + AuthServerNotificationTags.AuthRequest, + (event) => this.authRequestAnsweringService.handleAuthRequestNotificationClicked(event), + ); + // ======= End Register All System Notification Handlers ======= + + const streams: Observable[] = handlers.map(({ startsWith, handler }) => + this.systemNotificationService.notificationClicked$.pipe( + filter((event: SystemNotificationEvent): boolean => event.id.startsWith(startsWith + "_")), + switchMap((event: SystemNotificationEvent): Observable => from(Promise.resolve(handler(event)))), + ), + ); + + if (streams.length > 0) { + merge(...streams).subscribe(); + } } /** diff --git a/libs/angular/src/auth/device-management/device-management.component.ts b/libs/angular/src/auth/device-management/device-management.component.ts index 3ab9b2146c5..ce27a03431c 100644 --- a/libs/angular/src/auth/device-management/device-management.component.ts +++ b/libs/angular/src/auth/device-management/device-management.component.ts @@ -6,12 +6,17 @@ import { firstValueFrom } from "rxjs"; // This import has been flagged as unallowed for this class. It may be involved in a circular dependency loop. // eslint-disable-next-line no-restricted-imports import { AuthRequestApiServiceAbstraction } from "@bitwarden/auth/common"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { DevicesServiceAbstraction } from "@bitwarden/common/auth/abstractions/devices/devices.service.abstraction"; import { DevicePendingAuthRequest, DeviceResponse, } from "@bitwarden/common/auth/abstractions/devices/responses/device.response"; import { DeviceView } from "@bitwarden/common/auth/abstractions/devices/views/device.view"; +import { getUserId } from "@bitwarden/common/auth/services/account.service"; +import { + PendingAuthRequestsStateService +} from "@bitwarden/common/auth/services/auth-request-answering/pending-auth-requests.state"; import { DeviceType, DeviceTypeMetadata } from "@bitwarden/common/enums"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { ValidationService } from "@bitwarden/common/platform/abstractions/validation.service"; @@ -66,14 +71,16 @@ export class DeviceManagementComponent implements OnInit { protected showHeaderInfo = false; constructor( - private authRequestApiService: AuthRequestApiServiceAbstraction, - private destroyRef: DestroyRef, - private deviceManagementComponentService: DeviceManagementComponentServiceAbstraction, - private devicesService: DevicesServiceAbstraction, - private dialogService: DialogService, - private i18nService: I18nService, - private messageListener: MessageListener, - private validationService: ValidationService, + private readonly accountService: AccountService, + private readonly authRequestApiService: AuthRequestApiServiceAbstraction, + private readonly destroyRef: DestroyRef, + private readonly deviceManagementComponentService: DeviceManagementComponentServiceAbstraction, + private readonly devicesService: DevicesServiceAbstraction, + private readonly dialogService: DialogService, + private readonly i18nService: I18nService, + private readonly messageListener: MessageListener, + private readonly pendingAuthRequestStateService: PendingAuthRequestsStateService, + private readonly validationService: ValidationService, ) { this.showHeaderInfo = this.deviceManagementComponentService.showHeaderInformation(); } @@ -248,6 +255,12 @@ export class DeviceManagementComponent implements OnInit { // Auth request was approved or denied, so clear the // pending auth request and re-sort the device array this.devices = clearAuthRequestAndResortDevices(this.devices, pendingAuthRequest); + + // If a user ignores or doesn't see the auth request dialog, but comes to account settings + // to approve a device login attempt, clear out the state for that user. + await this.pendingAuthRequestStateService.clearByUserId( + await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)) + ) } } } diff --git a/libs/common/src/auth/abstractions/auth-request-answering/README.md b/libs/common/src/auth/abstractions/auth-request-answering/README.md new file mode 100644 index 00000000000..e69de29bb2d 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 217b2f547d8..f45cb34496e 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 @@ -2,9 +2,29 @@ import { SystemNotificationEvent } from "@bitwarden/common/platform/system-notif import { UserId } from "@bitwarden/user-core"; export abstract class AuthRequestAnsweringServiceAbstraction { - abstract receivedPendingAuthRequest(userId: UserId, notificationId: string): Promise; + /** + * Tries to either display the dialog for the user or will preserve its data and show it at a + * later time. Even in the event the dialog is shown immediately, this will write to global state + * so that even if someone closes a window or a popup and comes back, it could be processed later. + * Only way to clear out the global state is to respond to the auth request. + * + * Currently, this is only implemented for browser extension. + * + * @param userId The UserId that the auth request is for. + * @param authRequestId The id of the auth request that is to be processed. + */ + abstract receivedPendingAuthRequest(userId: UserId, authRequestId: string): Promise; + /** + * When a system notification is clicked, this function is used to process that event. + * + * @param event The event passed in. Check initNotificationSubscriptions in main.background.ts. + */ abstract handleAuthRequestNotificationClicked(event: SystemNotificationEvent): Promise; - abstract processPendingAuthRequests(): void; + /** + * Process notifications that have been received but didn't meet the conditions to display the + * approval dialog. + */ + abstract processPendingAuthRequests(): Promise; } diff --git a/libs/common/src/auth/services/auth-request-answering/auth-request-answering.service.ts b/libs/common/src/auth/services/auth-request-answering/auth-request-answering.service.ts index bf47086c982..d8b2b4eb5f5 100644 --- a/libs/common/src/auth/services/auth-request-answering/auth-request-answering.service.ts +++ b/libs/common/src/auth/services/auth-request-answering/auth-request-answering.service.ts @@ -35,46 +35,6 @@ export class AuthRequestAnsweringService implements AuthRequestAnsweringServiceA private readonly systemNotificationsService: SystemNotificationsService, ) {} - async processPendingAuthRequests(): Promise { - // Prune any stale pending requests (older than 15 minutes) - // This comes from GlobalSettings.cs - // public TimeSpan UserRequestExpiration { get; set; } = TimeSpan.FromMinutes(15); - const fifteenMinutesMs = 15 * 60 * 1000; - - await this.pendingAuthRequestsState.pruneOlderThan(fifteenMinutesMs); - - const pending = (await firstValueFrom(this.pendingAuthRequestsState.getAll$())) ?? []; - - if (pending.length > 0) { - const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)); - const pendingForActive = pending.some((e) => e.userId === activeUserId); - - if (pendingForActive) { - const isUnlocked = - (await firstValueFrom(this.authService.authStatusFor$(activeUserId))) === - AuthenticationStatus.Unlocked; - - const forceSetPasswordReason = await firstValueFrom( - this.masterPasswordService.forceSetPasswordReason$(activeUserId), - ); - - if (isUnlocked && forceSetPasswordReason === ForceSetPasswordReason.None) { - console.debug("[AuthRequestAnsweringService] popupOpened - Opening popup."); - this.messagingService.send("openLoginApproval"); - } - } - } - } - - async handleAuthRequestNotificationClicked(event: SystemNotificationEvent): Promise { - if (event.buttonIdentifier === ButtonLocation.NotificationButton) { - await this.systemNotificationsService.clear({ - id: `${event.id}`, - }); - await this.actionService.openPopup(); - } - } - async receivedPendingAuthRequest(userId: UserId, authRequestId: string): Promise { console.debug( "[AuthRequestAnsweringService] receivedPendingAuthRequest", @@ -112,7 +72,7 @@ export class AuthRequestAnsweringService implements AuthRequestAnsweringServiceA const emailForUser = accounts[userId].email; await this.systemNotificationsService.create({ - id: `${AuthServerNotificationTags.AuthRequest}_${authRequestId}`, + id: `${AuthServerNotificationTags.AuthRequest}_${authRequestId}`, // the underscore is an important delimiter. title: this.i18nService.t("accountAccessRequested"), body: this.i18nService.t("confirmAccessAttempt", emailForUser), buttons: [], @@ -124,4 +84,44 @@ export class AuthRequestAnsweringService implements AuthRequestAnsweringServiceA console.debug("[AuthRequestAnsweringService] receivedPendingAuthRequest - Opening popup."); this.messagingService.send("openLoginApproval"); } + + async handleAuthRequestNotificationClicked(event: SystemNotificationEvent): Promise { + if (event.buttonIdentifier === ButtonLocation.NotificationButton) { + await this.systemNotificationsService.clear({ + id: `${event.id}`, + }); + await this.actionService.openPopup(); + } + } + + async processPendingAuthRequests(): Promise { + // Prune any stale pending requests (older than 15 minutes) + // This comes from GlobalSettings.cs + // public TimeSpan UserRequestExpiration { get; set; } = TimeSpan.FromMinutes(15); + const fifteenMinutesMs = 15 * 60 * 1000; + + await this.pendingAuthRequestsState.pruneOlderThan(fifteenMinutesMs); + + const pending = (await firstValueFrom(this.pendingAuthRequestsState.getAll$())) ?? []; + + if (pending.length > 0) { + const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)); + const pendingForActive = pending.some((e) => e.userId === activeUserId); + + if (pendingForActive) { + const isUnlocked = + (await firstValueFrom(this.authService.authStatusFor$(activeUserId))) === + AuthenticationStatus.Unlocked; + + const forceSetPasswordReason = await firstValueFrom( + this.masterPasswordService.forceSetPasswordReason$(activeUserId), + ); + + if (isUnlocked && forceSetPasswordReason === ForceSetPasswordReason.None) { + console.debug("[AuthRequestAnsweringService] popupOpened - Opening popup."); + this.messagingService.send("openLoginApproval"); + } + } + } + } } diff --git a/libs/common/src/auth/services/auth-request-answering/unsupported-auth-request-answering.service.ts b/libs/common/src/auth/services/auth-request-answering/unsupported-auth-request-answering.service.ts index b987a68ad02..92c8c80255f 100644 --- a/libs/common/src/auth/services/auth-request-answering/unsupported-auth-request-answering.service.ts +++ b/libs/common/src/auth/services/auth-request-answering/unsupported-auth-request-answering.service.ts @@ -7,15 +7,16 @@ export class UnsupportedAuthRequestAnsweringService implements AuthRequestAnsweringServiceAbstraction { constructor() {} - async handleAuthRequestNotificationClicked(event: SystemNotificationEvent): Promise { + + receivedPendingAuthRequest(userId: UserId, notificationId: string): Promise { throw new Error("Received pending auth request not supported."); } - async receivedPendingAuthRequest(userId: UserId, notificationId: string): Promise { + handleAuthRequestNotificationClicked(event: SystemNotificationEvent): Promise { throw new Error("Received pending auth request not supported."); } - processPendingAuthRequests(): void { + processPendingAuthRequests(): Promise { throw new Error("Popup opened not supported."); } }