Favor simple boolean response from triggerXNotification type functions.

2026-02-08 12:40:26 +00:00 · 2025-05-21 11:24:46 -04:00
parent 4d24d4429e
commit 8c91f2a646
5 changed files with 70 additions and 64 deletions
--- a/apps/browser/src/autofill/background/abstractions/notification.background.ts
+++ b/apps/browser/src/autofill/background/abstractions/notification.background.ts
@@ -119,17 +119,19 @@ type NotificationBackgroundExtensionMessageHandlers = {
    sender,
  }: BackgroundOnMessageHandlerParams) => Promise<CollectionView[]>;
  bgCloseNotificationBar: ({ message, sender }: BackgroundOnMessageHandlerParams) => Promise<void>;
-  bgTriggerAtRiskPasswordNotification: ({
+  bgAdjustNotificationBar: ({ message, sender }: BackgroundOnMessageHandlerParams) => Promise<void>;
+  bgTriggerAddLoginNotification: ({
    message,
    sender,
-  }: BackgroundOnMessageHandlerParams) => Promise<void>;
-  bgOpenAtRisksPasswords: ({ message, sender }: BackgroundOnMessageHandlerParams) => Promise<void>;
-  bgAdjustNotificationBar: ({ message, sender }: BackgroundOnMessageHandlerParams) => Promise<void>;
-  bgAddLogin: ({ message, sender }: BackgroundOnMessageHandlerParams) => Promise<true | never>;
+  }: BackgroundOnMessageHandlerParams) => Promise<boolean>;
  bgTriggerChangedPasswordNotification: ({
    message,
    sender,
-  }: BackgroundOnMessageHandlerParams) => Promise<true | never>;
+  }: BackgroundOnMessageHandlerParams) => Promise<boolean>;
+  bgTriggerAtRiskPasswordNotification: ({
+    message,
+    sender,
+  }: BackgroundOnMessageHandlerParams) => Promise<boolean>;
  bgRemoveTabFromNotificationQueue: ({ sender }: BackgroundSenderParam) => void;
  bgSaveCipher: ({ message, sender }: BackgroundOnMessageHandlerParams) => void;
  bgOpenAddEditVaultItemPopout: ({
--- a/apps/browser/src/autofill/background/notification.background.spec.ts
+++ b/apps/browser/src/autofill/background/notification.background.spec.ts
@@ -274,7 +274,7 @@ describe("NotificationBackground", () => {
      });
    });

-    describe("bgAddLogin message handler", () => {
+    describe("bgTriggerAddLoginNotification message handler", () => {
      let tab: chrome.tabs.Tab;
      let sender: chrome.runtime.MessageSender;
      let getEnableAddedLoginPromptSpy: jest.SpyInstance;
@@ -304,7 +304,7 @@ describe("NotificationBackground", () => {

      it("skips attempting to add the login if the user is logged out", async () => {
        const message: NotificationBackgroundExtensionMessage = {
-          command: "bgAddLogin",
+          command: "bgTriggerAddLoginNotification",
          login: { username: "test", password: "password", url: "https://example.com" },
        };
        activeAccountStatusMock$.next(AuthenticationStatus.LoggedOut);
@@ -318,7 +318,7 @@ describe("NotificationBackground", () => {

      it("skips attempting to add the login if the login data does not contain a valid url", async () => {
        const message: NotificationBackgroundExtensionMessage = {
-          command: "bgAddLogin",
+          command: "bgTriggerAddLoginNotification",
          login: { username: "test", password: "password", url: "" },
        };
        activeAccountStatusMock$.next(AuthenticationStatus.Locked);
@@ -332,7 +332,7 @@ describe("NotificationBackground", () => {

      it("skips attempting to add the login if the user with a locked vault has disabled the login notification", async () => {
        const message: NotificationBackgroundExtensionMessage = {
-          command: "bgAddLogin",
+          command: "bgTriggerAddLoginNotification",
          login: { username: "test", password: "password", url: "https://example.com" },
        };
        activeAccountStatusMock$.next(AuthenticationStatus.Locked);
@@ -349,7 +349,7 @@ describe("NotificationBackground", () => {

      it("skips attempting to add the login if the user with an unlocked vault has disabled the login notification", async () => {
        const message: NotificationBackgroundExtensionMessage = {
-          command: "bgAddLogin",
+          command: "bgTriggerAddLoginNotification",
          login: { username: "test", password: "password", url: "https://example.com" },
        };
        activeAccountStatusMock$.next(AuthenticationStatus.Unlocked);
@@ -367,7 +367,7 @@ describe("NotificationBackground", () => {

      it("skips attempting to change the password for an existing login if the user has disabled changing the password notification", async () => {
        const message: NotificationBackgroundExtensionMessage = {
-          command: "bgAddLogin",
+          command: "bgTriggerAddLoginNotification",
          login: { username: "test", password: "password", url: "https://example.com" },
        };
        activeAccountStatusMock$.next(AuthenticationStatus.Unlocked);
@@ -389,7 +389,7 @@ describe("NotificationBackground", () => {

      it("skips attempting to change the password for an existing login if the password has not changed", async () => {
        const message: NotificationBackgroundExtensionMessage = {
-          command: "bgAddLogin",
+          command: "bgTriggerAddLoginNotification",
          login: { username: "test", password: "password", url: "https://example.com" },
        };
        activeAccountStatusMock$.next(AuthenticationStatus.Unlocked);
@@ -409,7 +409,10 @@ describe("NotificationBackground", () => {

      it("adds the login to the queue if the user has a locked account", async () => {
        const login = { username: "test", password: "password", url: "https://example.com" };
-        const message: NotificationBackgroundExtensionMessage = { command: "bgAddLogin", login };
+        const message: NotificationBackgroundExtensionMessage = {
+          command: "bgTriggerAddLoginNotification",
+          login,
+        };
        activeAccountStatusMock$.next(AuthenticationStatus.Locked);
        getEnableAddedLoginPromptSpy.mockReturnValueOnce(true);

@@ -425,7 +428,10 @@ describe("NotificationBackground", () => {
          password: "password",
          url: "https://example.com",
        } as any;
-        const message: NotificationBackgroundExtensionMessage = { command: "bgAddLogin", login };
+        const message: NotificationBackgroundExtensionMessage = {
+          command: "bgTriggerAddLoginNotification",
+          login,
+        };
        activeAccountStatusMock$.next(AuthenticationStatus.Unlocked);
        getEnableAddedLoginPromptSpy.mockReturnValueOnce(true);
        getAllDecryptedForUrlSpy.mockResolvedValueOnce([
@@ -440,7 +446,10 @@ describe("NotificationBackground", () => {

      it("adds a change password message to the queue if the user has changed an existing cipher's password", async () => {
        const login = { username: "tEsT", password: "password", url: "https://example.com" };
-        const message: NotificationBackgroundExtensionMessage = { command: "bgAddLogin", login };
+        const message: NotificationBackgroundExtensionMessage = {
+          command: "bgTriggerAddLoginNotification",
+          login,
+        };
        activeAccountStatusMock$.next(AuthenticationStatus.Unlocked);
        getEnableAddedLoginPromptSpy.mockResolvedValueOnce(true);
        getEnableChangedPasswordPromptSpy.mockResolvedValueOnce(true);
@@ -463,7 +472,7 @@ describe("NotificationBackground", () => {
      });
    });

-    describe("bgChangedPassword message handler", () => {
+    describe("bgTriggerChangedPasswordNotification message handler", () => {
      let tab: chrome.tabs.Tab;
      let sender: chrome.runtime.MessageSender;
      let pushChangePasswordToQueueSpy: jest.SpyInstance;
@@ -481,7 +490,7 @@ describe("NotificationBackground", () => {

      it("skips attempting to add the change password message to the queue if the passed url is not valid", async () => {
        const message: NotificationBackgroundExtensionMessage = {
-          command: "bgChangedPassword",
+          command: "bgTriggerChangedPasswordNotification",
          data: { newPassword: "newPassword", currentPassword: "currentPassword", url: "" },
        };

@@ -493,7 +502,7 @@ describe("NotificationBackground", () => {

      it("adds a change password message to the queue if the user does not have an unlocked account", async () => {
        const message: NotificationBackgroundExtensionMessage = {
-          command: "bgChangedPassword",
+          command: "bgTriggerChangedPasswordNotification",
          data: {
            newPassword: "newPassword",
            currentPassword: "currentPassword",
@@ -516,7 +525,7 @@ describe("NotificationBackground", () => {

      it("skips adding a change password message to the queue if the multiple ciphers exist for the passed URL and the current password is not found within the list of ciphers", async () => {
        const message: NotificationBackgroundExtensionMessage = {
-          command: "bgChangedPassword",
+          command: "bgTriggerChangedPasswordNotification",
          data: {
            newPassword: "newPassword",
            currentPassword: "currentPassword",
@@ -537,7 +546,7 @@ describe("NotificationBackground", () => {

      it("skips adding a change password message if more than one existing cipher is found with a matching password ", async () => {
        const message: NotificationBackgroundExtensionMessage = {
-          command: "bgChangedPassword",
+          command: "bgTriggerChangedPasswordNotification",
          data: {
            newPassword: "newPassword",
            currentPassword: "currentPassword",
@@ -559,7 +568,7 @@ describe("NotificationBackground", () => {

      it("adds a change password message to the queue if a single cipher matches the passed current password", async () => {
        const message: NotificationBackgroundExtensionMessage = {
-          command: "bgChangedPassword",
+          command: "bgTriggerChangedPasswordNotification",
          data: {
            newPassword: "newPassword",
            currentPassword: "currentPassword",
@@ -587,7 +596,7 @@ describe("NotificationBackground", () => {

      it("skips adding a change password message if no current password is passed in the message and more than one cipher is found for a url", async () => {
        const message: NotificationBackgroundExtensionMessage = {
-          command: "bgChangedPassword",
+          command: "bgTriggerChangedPasswordNotification",
          data: {
            newPassword: "newPassword",
            url: "https://example.com",
@@ -608,7 +617,7 @@ describe("NotificationBackground", () => {

      it("adds a change password message to the queue if no current password is passed with the message, but a single cipher is matched for the uri", async () => {
        const message: NotificationBackgroundExtensionMessage = {
-          command: "bgChangedPassword",
+          command: "bgTriggerChangedPasswordNotification",
          data: {
            newPassword: "newPassword",
            url: "https://example.com",
--- a/apps/browser/src/autofill/background/notification.background.ts
+++ b/apps/browser/src/autofill/background/notification.background.ts
@@ -85,17 +85,16 @@ export default class NotificationBackground {
    ExtensionCommand.AutofillIdentity,
  ]);
  private readonly extensionMessageHandlers: NotificationBackgroundExtensionMessageHandlers = {
-    bgAddLogin: ({ message, sender }) => this.triggerAddLoginNotification(message, sender),
    bgAdjustNotificationBar: ({ message, sender }) =>
      this.handleAdjustNotificationBarMessage(message, sender),
+    bgTriggerAddLoginNotification: ({ message, sender }) =>
+      this.triggerAddLoginNotification(message, sender),
    bgTriggerChangedPasswordNotification: ({ message, sender }) =>
      this.triggerChangedPasswordNotification(message, sender),
    bgTriggerAtRiskPasswordNotification: ({ message, sender }) =>
      this.triggerAtRiskPasswordNotification(message, sender),
    bgCloseNotificationBar: ({ message, sender }) =>
      this.handleCloseNotificationBarMessage(message, sender),
-    bgOpenAtRisksPasswords: ({ message, sender }) =>
-      this.handleOpenAtRisksPasswordsMessage(message, sender),
    bgGetActiveUserServerConfig: () => this.getActiveUserServerConfig(),
    bgGetDecryptedCiphers: () => this.getNotificationCipherData(),
    bgGetEnableChangedPasswordPrompt: () => this.getEnableChangedPasswordPrompt(),
@@ -397,7 +396,7 @@ export default class NotificationBackground {
  async triggerAtRiskPasswordNotification(
    message: NotificationBackgroundExtensionMessage,
    sender: chrome.runtime.MessageSender,
-  ) {
+  ): Promise<boolean> {
    const { activeUserId, securityTask, cipher } = message.data;
    const domain = Utils.getDomain(sender.tab.url);
    const passwordChangeUri =
@@ -426,6 +425,7 @@ export default class NotificationBackground {
    };
    this.notificationQueue.push(queueMessage);
    await this.checkNotificationQueue(sender.tab);
+    return true;
  }

  /**
@@ -439,17 +439,17 @@ export default class NotificationBackground {
  async triggerAddLoginNotification(
    message: NotificationBackgroundExtensionMessage,
    sender: chrome.runtime.MessageSender,
-  ): Promise<true | never> {
+  ): Promise<boolean> {
    const authStatus = await this.getAuthStatus();
    if (authStatus === AuthenticationStatus.LoggedOut) {
-      throw Error("Auth status is logged out.");
+      return false;
    }

    const loginInfo = message.login;
    const normalizedUsername = loginInfo.username ? loginInfo.username.toLowerCase() : "";
    const loginDomain = Utils.getDomain(loginInfo.url);
    if (loginDomain == null) {
-      throw Error("Login domain is not present.");
+      return false;
    }

    const addLoginIsEnabled = await this.getEnableAddedLoginPrompt();
@@ -459,14 +459,14 @@ export default class NotificationBackground {
        await this.pushAddLoginToQueue(loginDomain, loginInfo, sender.tab, true);
      }

-      return true;
+      return false;
    }

    const activeUserId = await firstValueFrom(
      this.accountService.activeAccount$.pipe(getOptionalUserId),
    );
    if (activeUserId == null) {
-      throw Error("No active user.");
+      return false;
    }

    const ciphers = await this.cipherService.getAllDecryptedForUrl(loginInfo.url, activeUserId);
@@ -493,7 +493,7 @@ export default class NotificationBackground {
      );
      return true;
    }
-    throw Error("No change password notification pushed to queue.");
+    return false;
  }

  private async pushAddLoginToQueue(
@@ -530,11 +530,11 @@ export default class NotificationBackground {
  async triggerChangedPasswordNotification(
    message: NotificationBackgroundExtensionMessage,
    sender: chrome.runtime.MessageSender,
-  ): Promise<true | never> {
+  ) {
    const changeData = message.data as ChangePasswordMessageData;
    const loginDomain = Utils.getDomain(changeData.url);
    if (loginDomain == null) {
-      throw new Error("No login domain present.");
+      return false;
    }

    if ((await this.getAuthStatus()) < AuthenticationStatus.Unlocked) {
@@ -553,7 +553,7 @@ export default class NotificationBackground {
      this.accountService.activeAccount$.pipe(getOptionalUserId),
    );
    if (activeUserId == null) {
-      throw new Error("No active user.");
+      return false;
    }

    const ciphers = await this.cipherService.getAllDecryptedForUrl(changeData.url, activeUserId);
@@ -571,7 +571,7 @@ export default class NotificationBackground {
      await this.pushChangePasswordToQueue(id, loginDomain, changeData.newPassword, sender.tab);
      return true;
    }
-    throw Error("No change password notification queued.");
+    return false;
  }

  /**
@@ -644,7 +644,6 @@ export default class NotificationBackground {
    };
    this.notificationQueue.push(message);
    await this.checkNotificationQueue(tab);
-    return true;
  }

  private async pushUnlockVaultToQueue(loginDomain: string, tab: chrome.tabs.Tab) {
--- a/apps/browser/src/autofill/background/overlay-notifications.background.spec.ts
+++ b/apps/browser/src/autofill/background/overlay-notifications.background.spec.ts
@@ -335,7 +335,6 @@ describe("OverlayNotificationsBackground", () => {
    const pageDetails = mock<AutofillPageDetails>({ fields: [mock<AutofillField>()] });
    let notificationChangedPasswordSpy: jest.SpyInstance;
    let notificationAddLoginSpy: jest.SpyInstance;
-    let notificationAtRiskPasswordSpy: jest.SpyInstance;

    beforeEach(async () => {
      sender = mock<chrome.runtime.MessageSender>({
@@ -347,10 +346,6 @@ describe("OverlayNotificationsBackground", () => {
        "triggerChangedPasswordNotification",
      );
      notificationAddLoginSpy = jest.spyOn(notificationBackground, "triggerAddLoginNotification");
-      notificationAtRiskPasswordSpy = jest.spyOn(
-        notificationBackground,
-        "triggerAtRiskPasswordNotification",
-      );

      sendMockExtensionMessage(
        { command: "collectPageDetailsResponse", details: pageDetails },
@@ -411,7 +406,6 @@ describe("OverlayNotificationsBackground", () => {

        expect(notificationChangedPasswordSpy).not.toHaveBeenCalled();
        expect(notificationAddLoginSpy).not.toHaveBeenCalled();
-        expect(notificationAtRiskPasswordSpy).not.toHaveBeenCalled();
      });

      it("ignores requests for tabs that do not contain stored login data", async () => {
@@ -436,7 +430,6 @@ describe("OverlayNotificationsBackground", () => {

        expect(notificationChangedPasswordSpy).not.toHaveBeenCalled();
        expect(notificationAddLoginSpy).not.toHaveBeenCalled();
-        expect(notificationAtRiskPasswordSpy).not.toHaveBeenCalled();
      });

      it("clears the notification fallback timeout if the request is completed with an invalid status code", async () => {
--- a/apps/browser/src/autofill/background/overlay-notifications.background.ts
+++ b/apps/browser/src/autofill/background/overlay-notifications.background.ts
@@ -269,7 +269,7 @@ export class OverlayNotificationsBackground implements OverlayNotificationsBackg
    return (
      !modifyLoginData ||
      !this.shouldAttemptAddLoginNotification(modifyLoginData) ||
-      !this.shouldAttemptChangePasswordNotification(modifyLoginData)
+      !this.shouldAttemptChangedPasswordNotification(modifyLoginData)
    );
  };

@@ -401,7 +401,7 @@ export class OverlayNotificationsBackground implements OverlayNotificationsBackg
  };

  /**
-   * Initializes the add login, change, or at risk password notification based on the modified login form data
+   * Initializes the add login or change password notification based on the modified login form data
   * and the tab details. This will trigger the notification to be displayed to the user.
   *
   * @param requestId - The details of the web response
@@ -413,15 +413,14 @@ export class OverlayNotificationsBackground implements OverlayNotificationsBackg
    modifyLoginData: ModifyLoginCipherFormData,
    tab: chrome.tabs.Tab,
  ) => {
-    let error: Error;
-
-    if (this.shouldAttemptChangePasswordNotification(modifyLoginData)) {
+    let error: string;
+    if (this.shouldAttemptChangedPasswordNotification(modifyLoginData)) {
      // These notifications are temporarily setup as "messages" to the notification background.
      // This will be structured differently in a future refactor.
      try {
-        await this.notificationBackground.triggerChangedPasswordNotification(
+        const result = await this.notificationBackground.triggerChangedPasswordNotification(
          {
-            command: "bgTriggerChangedPasswordNotification",
+            command: "bgChangedPassword",
            data: {
              url: modifyLoginData.uri,
              currentPassword: modifyLoginData.password,
@@ -430,18 +429,21 @@ export class OverlayNotificationsBackground implements OverlayNotificationsBackg
          },
          { tab },
        );
-        this.clearCompletedWebRequest(requestId, tab);
-        return;
+        if (!result) {
+          throw Error("Didn't trigger changedPassword ");
+        }
      } catch (e) {
        error = e;
      }
+      this.clearCompletedWebRequest(requestId, tab);
+      return;
    }

    if (this.shouldAttemptAddLoginNotification(modifyLoginData)) {
      try {
-        await this.notificationBackground.triggerAddLoginNotification(
+        const result = await this.notificationBackground.triggerAddLoginNotification(
          {
-            command: "bgAddLogin",
+            command: "bgTriggerAddLoginNotification",
            login: {
              url: modifyLoginData.uri,
              username: modifyLoginData.username,
@@ -450,11 +452,14 @@ export class OverlayNotificationsBackground implements OverlayNotificationsBackg
          },
          { tab },
        );
-        this.clearCompletedWebRequest(requestId, tab);
-        return;
+        if (!result) {
+          throw Error("Didn't trigger addLogin notification");
+        }
      } catch (e) {
        error = e;
      }
+      this.clearCompletedWebRequest(requestId, tab);
+      return error;
    }

    const shouldGetTasks: boolean = await this.notificationBackground.getNotificationFlag();
@@ -482,25 +487,23 @@ export class OverlayNotificationsBackground implements OverlayNotificationsBackg
          { tab },
        );
        this.clearCompletedWebRequest(requestId, tab);
-        return;
      }
    }
-    return error;
  };

  /**
-   * Determines if the change password notification should be attempted.
+   * Determines if the change password notification should be triggered.
   *
   * @param modifyLoginData - The modified login form data
   */
-  private shouldAttemptChangePasswordNotification = (
+  private shouldAttemptChangedPasswordNotification = (
    modifyLoginData: ModifyLoginCipherFormData,
  ) => {
    return modifyLoginData?.newPassword && !modifyLoginData.username;
  };

  /**
-   * Determines if the add login notification should be attempted.
+   * Determines if the add login notification should be triggered.
   *
   * @param modifyLoginData - The modified login form data
   */