[PM-5560] Implement Autofill Settings state provider (#7767)

* Begin migration of autofill settings Co-authored-by: Cesar Gonzalez <[email protected]> Co-authored-by: Thomas Avery <[email protected]> Co-authored-by: Jonathan Prusik <[email protected]> Co-authored-by: Colton Hurst <[email protected]> * add browser dependency for AutofillSettingsService Co-authored-by: Matt Gibson <[email protected]> * update autofill settings service * replace usages of stateService get/set autofillOnPageLoad with autofillSettingsService * replace usages of stateService get/set autofillOnPageLoadDefault with autofillSettingsService * replace usages of stateService get/set autoCopyTotp with autofillSettingsService * replace usages of stateService get/set autoFillOnPageLoadCalloutIsDismissed with autofillSettingsService * replace usages of stateService get/set activateAutoFillOnPageLoadFromPolicy with autofillSettingsService * replace usages of get/set autoFillOverlayVisibility with autofillSettingsService * inlineMenuVisibility should use global state * add the AutofillSettingsService to background scripts * fix typing * replace additional usages of get/set autoFillOverlayVisibility and disableAutoTotpCopy with autofillSettingsService equivalents * replace additional usages of get/set autofillOnPageLoadDefault with autofillSettingsService equivalent * replace additional usages of get/set activateAutoFillOnPageLoadFromPolicy with autofillSettingsService equivalent * remove additional deprecated and unused state service calls * improve naming conventions and consistency * fix missing mock for policy service test * replace missing overlay background tests * cleanup * fix double inversion * fix reference to wrong setter * move handleActivateAutofillPolicy out of BrowserPolicyService * create state migration script * resolve linting issues * remove migrated setting properties * add AutofillSettingsSErvice to jslib-services * handle conditional content script loading via autofillOnPageLoad check * add deprecated note to getFromLocalStorage * add jsdoc decorators to new autofill service methods * handle undefined globalState * move autofill settings out of BrowserPolicyService * Move autofill settings code out of policyService * fix tests * fix typo in state definition --------- Co-authored-by: Matt Gibson <[email protected]> Co-authored-by: Cesar Gonzalez <[email protected]> Co-authored-by: Thomas Avery <[email protected]> Co-authored-by: Colton Hurst <[email protected]> Co-authored-by: Thomas Rittson <[email protected]>
bitwarden · Feb 12, 2024 · c65e92f · c65e92f
1 parent bf16682
commit c65e92f
Show file tree

Hide file tree

Showing 34 changed files with 975 additions and 306 deletions.
diff --git a/apps/browser/src/admin-console/services/browser-policy.service.ts b/apps/browser/src/admin-console/services/browser-policy.service.ts
@@ -1,11 +1,8 @@
-import { BehaviorSubject, filter, map, Observable, switchMap, tap } from "rxjs";
+import { BehaviorSubject } from "rxjs";
 import { Jsonify } from "type-fest";
 
-import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
-import { PolicyType } from "@bitwarden/common/admin-console/enums";
 import { Policy } from "@bitwarden/common/admin-console/models/domain/policy";
 import { PolicyService } from "@bitwarden/common/admin-console/services/policy/policy.service";
-import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
 
 import { browserSession, sessionSync } from "../../platform/decorators/session-sync-observable";
 
@@ -16,31 +13,4 @@ export class BrowserPolicyService extends PolicyService {
     initializeAs: "array",
   })
   protected _policies: BehaviorSubject<Policy[]>;
-
-  constructor(stateService: StateService, organizationService: OrganizationService) {
-    super(stateService, organizationService);
-    this._policies.pipe(this.handleActivateAutofillPolicy.bind(this)).subscribe();
-  }
-
-  /**
-   * If the ActivateAutofill policy is enabled, save a flag indicating if we need to
-   * enable Autofill on page load.
-   */
-  private handleActivateAutofillPolicy(policies$: Observable<Policy[]>) {
-    return policies$.pipe(
-      map((policies) => policies.find((p) => p.type == PolicyType.ActivateAutofill && p.enabled)),
-      filter((p) => p != null),
-      switchMap(async (_) => [
-        await this.stateService.getActivateAutoFillOnPageLoadFromPolicy(),
-        await this.stateService.getEnableAutoFillOnPageLoad(),
-      ]),
-      tap(([activated, autofillEnabled]) => {
-        if (activated === undefined) {
-          // 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.stateService.setActivateAutoFillOnPageLoadFromPolicy(!autofillEnabled);
-        }
-      }),
-    );
-  }
 }
diff --git a/apps/browser/src/autofill/background/overlay.background.spec.ts b/apps/browser/src/autofill/background/overlay.background.spec.ts
@@ -2,6 +2,7 @@ import { mock, mockReset } from "jest-mock-extended";
 
 import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
 import { AuthService } from "@bitwarden/common/auth/services/auth.service";
+import { AutofillSettingsService } from "@bitwarden/common/autofill/services/autofill-settings.service";
 import { ThemeType } from "@bitwarden/common/platform/enums";
 import { EnvironmentService } from "@bitwarden/common/platform/services/environment.service";
 import { I18nService } from "@bitwarden/common/platform/services/i18n.service";
@@ -47,21 +48,18 @@ describe("OverlayBackground", () => {
   });
   const settingsService = mock<SettingsService>();
   const stateService = mock<BrowserStateService>();
+  const autofillSettingsService = mock<AutofillSettingsService>();
   const i18nService = mock<I18nService>();
   const platformUtilsService = mock<BrowserPlatformUtilsService>();
-  const initOverlayElementPorts = (options = { initList: true, initButton: true }) => {
+  const initOverlayElementPorts = async (options = { initList: true, initButton: true }) => {
     const { initList, initButton } = options;
     if (initButton) {
-      // 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
-      overlayBackground["handlePortOnConnect"](createPortSpyMock(AutofillOverlayPort.Button));
+      await overlayBackground["handlePortOnConnect"](createPortSpyMock(AutofillOverlayPort.Button));
       buttonPortSpy = overlayBackground["overlayButtonPort"];
     }
 
     if (initList) {
-      // 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
-      overlayBackground["handlePortOnConnect"](createPortSpyMock(AutofillOverlayPort.List));
+      await overlayBackground["handlePortOnConnect"](createPortSpyMock(AutofillOverlayPort.List));
       listPortSpy = overlayBackground["overlayListPort"];
     }
 
@@ -76,12 +74,16 @@ describe("OverlayBackground", () => {
       environmentService,
       settingsService,
       stateService,
+      autofillSettingsService,
       i18nService,
       platformUtilsService,
     );
-    // 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
-    overlayBackground.init();
+
+    jest
+      .spyOn(overlayBackground as any, "getOverlayVisibility")
+      .mockResolvedValue(AutofillOverlayVisibility.OnFieldFocus);
+
+    void overlayBackground.init();
   });
 
   afterEach(() => {
@@ -570,8 +572,8 @@ describe("OverlayBackground", () => {
       });
 
       describe("autofillOverlayElementClosed message handler", () => {
-        beforeEach(() => {
-          initOverlayElementPorts();
+        beforeEach(async () => {
+          await initOverlayElementPorts();
         });
 
         it("disconnects the button element port", () => {
@@ -635,15 +637,15 @@ describe("OverlayBackground", () => {
       describe("getAutofillOverlayVisibility message handler", () => {
         beforeEach(() => {
           jest
-            .spyOn(overlayBackground["settingsService"], "getAutoFillOverlayVisibility")
+            .spyOn(overlayBackground as any, "getOverlayVisibility")
             .mockResolvedValue(AutofillOverlayVisibility.OnFieldFocus);
         });
 
         it("will set the overlayVisibility property", async () => {
           sendExtensionRuntimeMessage({ command: "getAutofillOverlayVisibility" });
           await flushPromises();
 
-          expect(overlayBackground["overlayVisibility"]).toBe(
+          expect(await overlayBackground["getOverlayVisibility"]()).toBe(
             AutofillOverlayVisibility.OnFieldFocus,
           );
         });
@@ -663,8 +665,8 @@ describe("OverlayBackground", () => {
       });
 
       describe("checkAutofillOverlayFocused message handler", () => {
-        beforeEach(() => {
-          initOverlayElementPorts();
+        beforeEach(async () => {
+          await initOverlayElementPorts();
         });
 
         it("will check if the overlay list is focused if the list port is open", () => {
@@ -693,8 +695,8 @@ describe("OverlayBackground", () => {
       });
 
       describe("focusAutofillOverlayList message handler", () => {
-        it("will send a `focusOverlayList` message to the overlay list port", () => {
-          initOverlayElementPorts({ initList: true, initButton: false });
+        it("will send a `focusOverlayList` message to the overlay list port", async () => {
+          await initOverlayElementPorts({ initList: true, initButton: false });
 
           sendExtensionRuntimeMessage({ command: "focusAutofillOverlayList" });
 
@@ -703,14 +705,15 @@ describe("OverlayBackground", () => {
       });
 
       describe("updateAutofillOverlayPosition message handler", () => {
-        beforeEach(() => {
-          // 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
-          overlayBackground["handlePortOnConnect"](createPortSpyMock(AutofillOverlayPort.List));
+        beforeEach(async () => {
+          await overlayBackground["handlePortOnConnect"](
+            createPortSpyMock(AutofillOverlayPort.List),
+          );
           listPortSpy = overlayBackground["overlayListPort"];
-          // 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
-          overlayBackground["handlePortOnConnect"](createPortSpyMock(AutofillOverlayPort.Button));
+
+          await overlayBackground["handlePortOnConnect"](
+            createPortSpyMock(AutofillOverlayPort.Button),
+          );
           buttonPortSpy = overlayBackground["overlayButtonPort"];
         });
 
@@ -813,8 +816,8 @@ describe("OverlayBackground", () => {
       });
 
       describe("updateOverlayHidden", () => {
-        beforeEach(() => {
-          initOverlayElementPorts();
+        beforeEach(async () => {
+          await initOverlayElementPorts();
         });
 
         it("returns early if the display value is not provided", () => {
@@ -984,19 +987,17 @@ describe("OverlayBackground", () => {
       jest.spyOn(overlayBackground as any, "getOverlayCipherData").mockImplementation();
     });
 
-    it("skips setting up the overlay port if the port connection is not for an overlay element", () => {
+    it("skips setting up the overlay port if the port connection is not for an overlay element", async () => {
       const port = createPortSpyMock("not-an-overlay-element");
 
-      // 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
-      overlayBackground["handlePortOnConnect"](port);
+      await overlayBackground["handlePortOnConnect"](port);
 
       expect(port.onMessage.addListener).not.toHaveBeenCalled();
       expect(port.postMessage).not.toHaveBeenCalled();
     });
 
     it("sets up the overlay list port if the port connection is for the overlay list", async () => {
-      initOverlayElementPorts({ initList: true, initButton: false });
+      await initOverlayElementPorts({ initList: true, initButton: false });
       await flushPromises();
 
       expect(overlayBackground["overlayButtonPort"]).toBeUndefined();
@@ -1012,7 +1013,7 @@ describe("OverlayBackground", () => {
     });
 
     it("sets up the overlay button port if the port connection is for the overlay button", async () => {
-      initOverlayElementPorts({ initList: false, initButton: true });
+      await initOverlayElementPorts({ initList: false, initButton: true });
       await flushPromises();
 
       expect(overlayBackground["overlayListPort"]).toBeUndefined();
@@ -1029,7 +1030,7 @@ describe("OverlayBackground", () => {
     it("gets the system theme", async () => {
       jest.spyOn(overlayBackground["stateService"], "getTheme").mockResolvedValue(ThemeType.System);
 
-      initOverlayElementPorts({ initList: true, initButton: false });
+      await initOverlayElementPorts({ initList: true, initButton: false });
       await flushPromises();
 
       expect(listPortSpy.postMessage).toHaveBeenCalledWith(
@@ -1039,8 +1040,8 @@ describe("OverlayBackground", () => {
   });
 
   describe("handleOverlayElementPortMessage", () => {
-    beforeEach(() => {
-      initOverlayElementPorts();
+    beforeEach(async () => {
+      await initOverlayElementPorts();
       overlayBackground["userAuthStatus"] = AuthenticationStatus.Unlocked;
     });
 

diff --git a/apps/browser/src/autofill/background/overlay.background.ts b/apps/browser/src/autofill/background/overlay.background.ts
@@ -1,6 +1,9 @@
+import { firstValueFrom } from "rxjs";
+
 import { SettingsService } from "@bitwarden/common/abstractions/settings.service";
 import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
 import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
+import { AutofillSettingsServiceAbstraction } from "@bitwarden/common/autofill/services/autofill-settings.service";
 import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
 import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
@@ -21,7 +24,11 @@ import {
 } from "../../vault/popup/utils/vault-popout-window";
 import { SHOW_AUTOFILL_BUTTON } from "../constants";
 import { AutofillService, PageDetail } from "../services/abstractions/autofill.service";
-import { AutofillOverlayElement, AutofillOverlayPort } from "../utils/autofill-overlay.enum";
+import {
+  InlineMenuVisibilitySetting,
+  AutofillOverlayElement,
+  AutofillOverlayPort,
+} from "../utils/autofill-overlay.enum";
 
 import { LockedVaultPendingNotificationsData } from "./abstractions/notification.background";
 import {
@@ -41,7 +48,6 @@ class OverlayBackground implements OverlayBackgroundInterface {
   private readonly openUnlockPopout = openUnlockPopout;
   private readonly openViewVaultItemPopout = openViewVaultItemPopout;
   private readonly openAddEditVaultItemPopout = openAddEditVaultItemPopout;
-  private overlayVisibility: number;
   private overlayLoginCiphers: Map<string, CipherView> = new Map();
   private pageDetailsForTab: Record<number, PageDetail[]> = {};
   private userAuthStatus: AuthenticationStatus = AuthenticationStatus.LoggedOut;
@@ -90,6 +96,7 @@ class OverlayBackground implements OverlayBackgroundInterface {
     private environmentService: EnvironmentService,
     private settingsService: SettingsService,
     private stateService: StateService,
+    private autofillSettingsService: AutofillSettingsServiceAbstraction,
     private i18nService: I18nService,
     private platformUtilsService: PlatformUtilsService,
   ) {
@@ -455,10 +462,8 @@ class OverlayBackground implements OverlayBackgroundInterface {
   /**
    * Gets the overlay's visibility setting from the settings service.
    */
-  private async getOverlayVisibility(): Promise<number> {
-    this.overlayVisibility = await this.settingsService.getAutoFillOverlayVisibility();
-
-    return this.overlayVisibility;
+  private async getOverlayVisibility(): Promise<InlineMenuVisibilitySetting> {
+    return await firstValueFrom(this.autofillSettingsService.inlineMenuVisibility$);
   }
 
   /**

diff --git a/apps/browser/src/autofill/background/service_factories/autofill-service.factory.ts b/apps/browser/src/autofill/background/service_factories/autofill-service.factory.ts
@@ -34,11 +34,17 @@ import {
 import { AutofillService as AbstractAutoFillService } from "../../services/abstractions/autofill.service";
 import AutofillService from "../../services/autofill.service";
 
+import {
+  AutofillSettingsServiceInitOptions,
+  autofillSettingsServiceFactory,
+} from "./autofill-settings-service.factory";
+
 type AutoFillServiceOptions = FactoryOptions;
 
 export type AutoFillServiceInitOptions = AutoFillServiceOptions &
   CipherServiceInitOptions &
   StateServiceInitOptions &
+  AutofillSettingsServiceInitOptions &
   TotpServiceInitOptions &
   EventCollectionServiceInitOptions &
   LogServiceInitOptions &
@@ -57,6 +63,7 @@ export function autofillServiceFactory(
       new AutofillService(
         await cipherServiceFactory(cache, opts),
         await stateServiceFactory(cache, opts),
+        await autofillSettingsServiceFactory(cache, opts),
         await totpServiceFactory(cache, opts),
         await eventCollectionServiceFactory(cache, opts),
         await logServiceFactory(cache, opts),

diff --git a/apps/browser/src/autofill/background/service_factories/autofill-settings-service.factory.ts b/apps/browser/src/autofill/background/service_factories/autofill-settings-service.factory.ts
@@ -0,0 +1,35 @@
+import { AutofillSettingsService } from "@bitwarden/common/autofill/services/autofill-settings.service";
+
+import {
+  policyServiceFactory,
+  PolicyServiceInitOptions,
+} from "../../../admin-console/background/service-factories/policy-service.factory";
+import {
+  CachedServices,
+  factory,
+  FactoryOptions,
+} from "../../../platform/background/service-factories/factory-options";
+import {
+  stateProviderFactory,
+  StateProviderInitOptions,
+} from "../../../platform/background/service-factories/state-provider.factory";
+
+export type AutofillSettingsServiceInitOptions = FactoryOptions &
+  StateProviderInitOptions &
+  PolicyServiceInitOptions;
+
+export function autofillSettingsServiceFactory(
+  cache: { autofillSettingsService?: AutofillSettingsService } & CachedServices,
+  opts: AutofillSettingsServiceInitOptions,
+): Promise<AutofillSettingsService> {
+  return factory(
+    cache,
+    "autofillSettingsService",
+    opts,
+    async () =>
+      new AutofillSettingsService(
+        await stateProviderFactory(cache, opts),
+        await policyServiceFactory(cache, opts),
+      ),
+  );
+}
diff --git a/apps/browser/src/autofill/content/autofiller.ts b/apps/browser/src/autofill/content/autofiller.ts
@@ -1,4 +1,4 @@
-import { getFromLocalStorage, setupExtensionDisconnectAction } from "../utils";
+import { setupExtensionDisconnectAction } from "../utils";
 
 if (document.readyState === "loading") {
   document.addEventListener("DOMContentLoaded", loadAutofiller);
@@ -22,19 +22,11 @@ function loadAutofiller() {
   };
 
   setupExtensionEventListeners();
-  // 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
   triggerUserFillOnLoad();
 
-  async function triggerUserFillOnLoad() {
-    const activeUserIdKey = "activeUserId";
-    const userKeyStorage = await getFromLocalStorage(activeUserIdKey);
-    const activeUserId = userKeyStorage[activeUserIdKey];
-    const activeUserStorage = await getFromLocalStorage(activeUserId);
-    if (activeUserStorage?.[activeUserId]?.settings?.enableAutoFillOnPageLoad === true) {
-      clearDoFillInterval();
-      doFillInterval = setInterval(() => doFillIfNeeded(), 500);
-    }
+  function triggerUserFillOnLoad() {
+    clearDoFillInterval();
+    doFillInterval = setInterval(() => doFillIfNeeded(), 500);
   }
 
   function doFillIfNeeded(force = false) {