fix(extension-device-approval): [PM-14943] Answering Service Full Implementation - Restructuring some code, adding comments.

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<void>;
+    }> = [];
+
+    const register = (
+      startsWith: string,
+      handler: (event: SystemNotificationEvent) => Promise<void>,
+    ) => {
+      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<void>[] = handlers.map(({ startsWith, handler }) =>
+      this.systemNotificationService.notificationClicked$.pipe(
+        filter((event: SystemNotificationEvent): boolean => event.id.startsWith(startsWith + "_")),
+        switchMap((event: SystemNotificationEvent): Observable<void> => from(Promise.resolve(handler(event)))),
+      ),
+    );
+
+    if (streams.length > 0) {
+      merge(...streams).subscribe();
+    }
   }
 
   /**

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))
+      )
     }
   }
 }

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<void>;
+  /**
+   * 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<void>;
 
+  /**
+   * 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<void>;
 
-  abstract processPendingAuthRequests(): void;
+  /**
+   * Process notifications that have been received but didn't meet the conditions to display the
+   * approval dialog.
+   */
+  abstract processPendingAuthRequests(): Promise<void>;
 }

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<void> {
-    // 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<void> {
-    if (event.buttonIdentifier === ButtonLocation.NotificationButton) {
-      await this.systemNotificationsService.clear({
-        id: `${event.id}`,
-      });
-      await this.actionService.openPopup();
-    }
-  }
-
   async receivedPendingAuthRequest(userId: UserId, authRequestId: string): Promise<void> {
     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<void> {
+    if (event.buttonIdentifier === ButtonLocation.NotificationButton) {
+      await this.systemNotificationsService.clear({
+        id: `${event.id}`,
+      });
+      await this.actionService.openPopup();
+    }
+  }
+
+  async processPendingAuthRequests(): Promise<void> {
+    // 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");
+        }
+      }
+    }
+  }
 }

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<void> {
+
+  receivedPendingAuthRequest(userId: UserId, notificationId: string): Promise<void> {
     throw new Error("Received pending auth request not supported.");
   }
 
-  async receivedPendingAuthRequest(userId: UserId, notificationId: string): Promise<void> {
+  handleAuthRequestNotificationClicked(event: SystemNotificationEvent): Promise<void> {
     throw new Error("Received pending auth request not supported.");
   }
 
-  processPendingAuthRequests(): void {
+  processPendingAuthRequests(): Promise<void> {
     throw new Error("Popup opened not supported.");
   }
 }