Add State Provider Framework (#6640)

* Add StateDefinition

Add a class for encapsulation information about state
this will often be for a domain but creations of this will
exist outside of a specific domain, hence just the name State.

* Add KeyDefinition

This adds a type that extends state definition into another sub-key
and forces creators to define the data that will be stored and how
to read the data that they expect to be stored.

* Add key-builders helper functions

Adds to function to help building keys for both keys scoped
to a specific user and for keys scoped to global storage.

Co-authored-by: Matt Gibson <[email protected]>

* Add updates$ stream to existing storageServices

Original commit by Matt: 823d954
Co-authored-by: Matt Gibson <[email protected]>

* Add fromChromeEvent helper

Create a helper that creats an Observable from a chrome event
and removes the listener when the subscription is completed.

* Implement `updates$` property for chrome storage

Use fromChromeEvent to create an observable from chrome
event and map that into our expected shape.

* Add GlobalState Abstractions

* Add UserState Abstractions

* Add Default Implementations of User/Global state

Co-authored-by: Matt Gibson <[email protected]>

* Add Barrel File for state

Co-authored-by: Matt Gibson <[email protected]>

* Fix ChromeStorageServices

* Rework fromChromeEvent

Rework fromChromeEvent so we have to lie to TS less and
remove unneeded generics. I did this by caring less about
the function and more about the parameters only.

Co-authored-by: Matt Gibson <[email protected]>

* Fix UserStateProvider Test

* Add Inner Mock & Assert Calls

* Update Tests to use new keys

Use different key format

* Prefer returns over mutations in update

* Update Tests

* Address PR Feedback

* Be stricter with userId parameter

* Add Better Way To Determine if it was a remove

* Fix Web & Browser Storage Services

* Fix Desktop & CLI Storage Services

* Fix Test Storage Service

* Use createKey Helper

* Prefer implement to extending

* Determine storage location in providers

* Export default providers publicly

* Fix user state tests

* Name tests

* Fix CLI

* Prefer Implement In Chrome Storage

* Remove Secure Storage Option

Also throw an exception for subscribes to the secure storage observable.

* Update apps/browser/src/platform/browser/from-chrome-event.ts

Co-authored-by: Oscar Hinton <[email protected]>

* Enforce state module barrel file

* Fix Linting Error

* Allow state module import from other modules

* Globally Unregister fromChromeEvent Listeners

Changed fromChromeEvent to add its listeners through the BrowserApi, so that
they will be unregistered when safari closes.

* Test default global state

* Use Proper Casing in Parameter

* Address Feedback

* Update libs/common/src/platform/state/key-definition.ts

Co-authored-by: Oscar Hinton <[email protected]>

* Add `buildCacheKey` Method

* Fix lint errors

* Add Comment

Co-authored-by: Oscar Hinton <[email protected]>

* Use Generic in callback parameter

* Refactor Out DerivedStateDefinition

* Persist Listener Return Type

* Add Ticket Link

---------

Co-authored-by: Matt Gibson <[email protected]>
Co-authored-by: Matt Gibson <[email protected]>
Co-authored-by: Oscar Hinton <[email protected]>

.eslintrc.json

@@ -100,6 +100,19 @@
                   "./libs/importer/**/*",
                   "./libs/exporter/**/*"
                 ]
+              },
+              {
+                // avoid import of unexported state objects
+                "target": [
+                  "!(libs)/**/*",
+                  "libs/!(common)/**/*",
+                  "libs/common/!(src)/**/*",
+                  "libs/common/src/!(platform)/**/*",
+                  "libs/common/src/platform/!(state)/**/*"
+                ],
+                "from": ["./libs/common/src/platform/state/**/*"],
+                // allow module index import
+                "except": ["**/state/index.ts"]
               }
             ]
           }
@@ -179,17 +192,23 @@
     },
     {
       "files": ["apps/browser/src/**/*.ts", "libs/**/*.ts"],
-      "excludedFiles": "apps/browser/src/autofill/{content,notification}/**/*.ts",
+      "excludedFiles": [
+        "apps/browser/src/autofill/{content,notification}/**/*.ts",
+        "apps/browser/src/**/background/**/*.ts", // It's okay to have long lived listeners in the background
+        "apps/browser/src/platform/background.ts"
+      ],
       "rules": {
         "no-restricted-syntax": [
           "error",
           {
-            "message": "Using addListener in the browser popup produces a memory leak in Safari, use `BrowserApi.messageListener` instead",
-            "selector": "CallExpression > [object.object.object.name='chrome'][object.object.property.name='runtime'][object.property.name='onMessage'][property.name='addListener']"
+            "message": "Using addListener in the browser popup produces a memory leak in Safari, use `BrowserApi.addListener` instead",
+            // This selector covers events like chrome.storage.onChange & chrome.runtime.onMessage
+            "selector": "CallExpression > [object.object.object.name='chrome'][property.name='addListener']"
           },
           {
-            "message": "Using addListener in the browser popup produces a memory leak in Safari, use `BrowserApi.storageChangeListener` instead",
-            "selector": "CallExpression > [object.object.object.name='chrome'][object.object.property.name='storage'][object.property.name='onChanged'][property.name='addListener']"
+            "message": "Using addListener in the browser popup produces a memory leak in Safari, use `BrowserApi.addListener` instead",
+            // This selector covers events like chrome.storage.local.onChange
+            "selector": "CallExpression > [object.object.object.object.name='chrome'][property.name='addListener']"
           }
         ]
       }

apps/browser/src/platform/browser/browser-api.ts

@@ -193,6 +193,9 @@ export class BrowserApi {
   }
 
   static async onWindowCreated(callback: (win: chrome.windows.Window) => any) {
+    // FIXME: Make sure that is does not cause a memory leak in Safari or use BrowserApi.AddListener
+    // and test that it doesn't break.
+    // eslint-disable-next-line no-restricted-syntax
     return chrome.windows.onCreated.addListener(callback);
   }
 
@@ -220,8 +223,10 @@ export class BrowserApi {
 
   // Keep track of all the events registered in a Safari popup so we can remove
   // them when the popup gets unloaded, otherwise we cause a memory leak
-  private static registeredMessageListeners: any[] = [];
-  private static registeredStorageChangeListeners: any[] = [];
+  private static trackedChromeEventListeners: [
+    event: chrome.events.Event<(...args: unknown[]) => unknown>,
+    callback: (...args: unknown[]) => unknown
+  ][] = [];
 
   static messageListener(
     name: string,
@@ -231,13 +236,7 @@ export class BrowserApi {
       sendResponse: any
     ) => boolean | void
   ) {
-    // eslint-disable-next-line no-restricted-syntax
-    chrome.runtime.onMessage.addListener(callback);
-
-    if (BrowserApi.isSafariApi && !BrowserApi.isBackgroundPage(window)) {
-      BrowserApi.registeredMessageListeners.push(callback);
-      BrowserApi.setupUnloadListeners();
-    }
+    BrowserApi.addListener(chrome.runtime.onMessage, callback);
   }
 
   static messageListener$() {
@@ -246,44 +245,67 @@ export class BrowserApi {
         subscriber.next(message);
       };
 
-      BrowserApi.messageListener("message", handler);
+      BrowserApi.addListener(chrome.runtime.onMessage, handler);
 
-      return () => {
-        chrome.runtime.onMessage.removeListener(handler);
-
-        if (BrowserApi.isSafariApi) {
-          const index = BrowserApi.registeredMessageListeners.indexOf(handler);
-          if (index !== -1) {
-            BrowserApi.registeredMessageListeners.splice(index, 1);
-          }
-        }
-      };
+      return () => BrowserApi.removeListener(chrome.runtime.onMessage, handler);
     });
   }
 
   static storageChangeListener(
     callback: Parameters<typeof chrome.storage.onChanged.addListener>[0]
   ) {
-    // eslint-disable-next-line no-restricted-syntax
-    chrome.storage.onChanged.addListener(callback);
+    BrowserApi.addListener(chrome.storage.onChanged, callback);
+  }
+
+  /**
+   * Adds a callback to the given chrome event in a cross-browser platform manner.
+   *
+   * **Important:** All event listeners in the browser extension popup context must
+   * use this instead of the native APIs to handle unsubscribing from Safari properly.
+   *
+   * @param event - The event in which to add the listener to.
+   * @param callback - The callback you want registered onto the event.
+   */
+  static addListener<T extends (...args: readonly unknown[]) => unknown>(
+    event: chrome.events.Event<T>,
+    callback: T
+  ) {
+    event.addListener(callback);
 
     if (BrowserApi.isSafariApi && !BrowserApi.isBackgroundPage(window)) {
-      BrowserApi.registeredStorageChangeListeners.push(callback);
+      BrowserApi.trackedChromeEventListeners.push([event, callback]);
       BrowserApi.setupUnloadListeners();
     }
   }
 
+  /**
+   * Removes a callback from the given chrome event in a cross-browser platform manner.
+   * @param event - The event in which to remove the listener from.
+   * @param callback - The callback you want removed from the event.
+   */
+  static removeListener<T extends (...args: readonly unknown[]) => unknown>(
+    event: chrome.events.Event<T>,
+    callback: T
+  ) {
+    event.removeListener(callback);
+
+    if (BrowserApi.isSafariApi && !BrowserApi.isBackgroundPage(window)) {
+      const index = BrowserApi.trackedChromeEventListeners.findIndex(([_event, eventListener]) => {
+        return eventListener == callback;
+      });
+      if (index !== -1) {
+        BrowserApi.trackedChromeEventListeners.splice(index, 1);
+      }
+    }
+  }
+
   // Setup the event to destroy all the listeners when the popup gets unloaded in Safari, otherwise we get a memory leak
   private static setupUnloadListeners() {
     // The MDN recommend using 'visibilitychange' but that event is fired any time the popup window is obscured as well
     // 'pagehide' works just like 'unload' but is compatible with the back/forward cache, so we prefer using that one
     window.onpagehide = () => {
-      for (const callback of BrowserApi.registeredMessageListeners) {
-        chrome.runtime.onMessage.removeListener(callback);
-      }
-
-      for (const callback of BrowserApi.registeredStorageChangeListeners) {
-        chrome.storage.onChanged.removeListener(callback);
+      for (const [event, callback] of BrowserApi.trackedChromeEventListeners) {
+        event.removeListener(callback);
       }
     };
   }

apps/browser/src/platform/browser/from-chrome-event.spec.ts

@@ -0,0 +1,103 @@
+import { fromChromeEvent } from "./from-chrome-event";
+
+describe("fromChromeEvent", () => {
+  class FakeEvent implements chrome.events.Event<(arg1: string, arg2: number) => void> {
+    listenerWasAdded: boolean;
+    listenerWasRemoved: boolean;
+    activeListeners: ((arg1: string, arg2: number) => void)[] = [];
+
+    addListener(callback: (arg1: string, arg2: number) => void): void {
+      this.listenerWasAdded = true;
+      this.activeListeners.push(callback);
+    }
+    getRules(callback: (rules: chrome.events.Rule[]) => void): void;
+    getRules(ruleIdentifiers: string[], callback: (rules: chrome.events.Rule[]) => void): void;
+    getRules(ruleIdentifiers: unknown, callback?: unknown): void {
+      throw new Error("Method not implemented.");
+    }
+    hasListener(callback: (arg1: string, arg2: number) => void): boolean {
+      throw new Error("Method not implemented.");
+    }
+    removeRules(ruleIdentifiers?: string[], callback?: () => void): void;
+    removeRules(callback?: () => void): void;
+    removeRules(ruleIdentifiers?: unknown, callback?: unknown): void {
+      throw new Error("Method not implemented.");
+    }
+    addRules(rules: chrome.events.Rule[], callback?: (rules: chrome.events.Rule[]) => void): void {
+      throw new Error("Method not implemented.");
+    }
+    removeListener(callback: (arg1: string, arg2: number) => void): void {
+      const index = this.activeListeners.findIndex((c) => c == callback);
+      if (index === -1) {
+        throw new Error("No registered callback.");
+      }
+
+      this.listenerWasRemoved = true;
+      this.activeListeners.splice(index, 1);
+    }
+    hasListeners(): boolean {
+      throw new Error("Method not implemented.");
+    }
+
+    fireEvent(arg1: string, arg2: number) {
+      this.activeListeners.forEach((listener) => {
+        listener(arg1, arg2);
+      });
+    }
+  }
+
+  let event: FakeEvent;
+
+  beforeEach(() => {
+    event = new FakeEvent();
+  });
+
+  it("should never call addListener when never subscribed to", () => {
+    fromChromeEvent(event);
+    expect(event.listenerWasAdded).toBeFalsy();
+  });
+
+  it("should add a listener when subscribed to.", () => {
+    const eventObservable = fromChromeEvent(event);
+    eventObservable.subscribe();
+    expect(event.listenerWasAdded).toBeTruthy();
+    expect(event.activeListeners).toHaveLength(1);
+  });
+
+  it("should call remove listener when the created subscription is unsubscribed", () => {
+    const eventObservable = fromChromeEvent(event);
+    const subscription = eventObservable.subscribe();
+    subscription.unsubscribe();
+    expect(event.listenerWasAdded).toBeTruthy();
+    expect(event.listenerWasRemoved).toBeTruthy();
+    expect(event.activeListeners).toHaveLength(0);
+  });
+
+  it("should fire each callback given to subscribe", () => {
+    const eventObservable = fromChromeEvent(event);
+
+    let subscription1Called = false;
+    let subscription2Called = false;
+
+    const subscription1 = eventObservable.subscribe(([arg1, arg2]) => {
+      expect(arg1).toBe("Hi!");
+      expect(arg2).toBe(2);
+      subscription1Called = true;
+    });
+
+    const subscription2 = eventObservable.subscribe(([arg1, arg2]) => {
+      expect(arg1).toBe("Hi!");
+      expect(arg2).toBe(2);
+      subscription2Called = true;
+    });
+
+    event.fireEvent("Hi!", 2);
+
+    subscription1.unsubscribe();
+    subscription2.unsubscribe();
+
+    expect(event.activeListeners).toHaveLength(0);
+    expect(subscription1Called).toBeTruthy();
+    expect(subscription2Called).toBeTruthy();
+  });
+});

apps/browser/src/platform/browser/from-chrome-event.ts

@@ -0,0 +1,39 @@
+import { Observable } from "rxjs";
+
+import { BrowserApi } from "./browser-api";
+
+/**
+ * Converts a Chrome event to an Observable stream.
+ *
+ * @typeParam T - The type of the event arguments.
+ * @param event - The Chrome event to convert.
+ * @returns An Observable stream of the event arguments.
+ *
+ * @remarks
+ * This function creates an Observable stream that listens to a Chrome event and emits its arguments
+ * whenever the event is triggered. If the event throws an error, the Observable will emit an error
+ * notification with the error message.
+ *
+ * @example
+ * ```typescript
+ * const onMessage = fromChromeEvent(chrome.runtime.onMessage);
+ * onMessage.subscribe((message) => console.log('Received message:', message));
+ * ```
+ */
+export function fromChromeEvent<T extends unknown[]>(
+  event: chrome.events.Event<(...args: T) => void>
+): Observable<T> {
+  return new Observable<T>((subscriber) => {
+    const handler = (...args: T) => {
+      if (chrome.runtime.lastError) {
+        subscriber.error(chrome.runtime.lastError);
+        return;
+      }
+
+      subscriber.next(args);
+    };
+
+    BrowserApi.addListener(event, handler);
+    return () => BrowserApi.removeListener(event, handler);
+  });
+}

apps/browser/src/platform/services/abstractions/abstract-chrome-storage-api.service.ts

@@ -1,7 +1,39 @@
-import { AbstractStorageService } from "@bitwarden/common/platform/abstractions/storage.service";
+import { Observable, mergeMap } from "rxjs";
+
+import {
+  AbstractStorageService,
+  StorageUpdate,
+  StorageUpdateType,
+} from "@bitwarden/common/platform/abstractions/storage.service";
+
+import { fromChromeEvent } from "../../browser/from-chrome-event";
 
 export default abstract class AbstractChromeStorageService implements AbstractStorageService {
-  protected abstract chromeStorageApi: chrome.storage.StorageArea;
+  constructor(protected chromeStorageApi: chrome.storage.StorageArea) {}
+
+  get updates$(): Observable<StorageUpdate> {
+    return fromChromeEvent(this.chromeStorageApi.onChanged).pipe(
+      mergeMap(([changes]) => {
+        return Object.entries(changes).map(([key, change]) => {
+          // The `newValue` property isn't on the StorageChange object
+          // when the change was from a remove. Similarly a check of the `oldValue`
+          // could be used to tell if the operation was the first creation of this key
+          // but we currently do not differentiate that.
+          // Ref: https://developer.chrome.com/docs/extensions/reference/storage/#type-StorageChange
+          // Ref: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/StorageChange
+          const updateType: StorageUpdateType = "newValue" in change ? "save" : "remove";
+
+          return {
+            key: key,
+            // For removes this property will not exist but then it will just be
+            // undefined which is fine.
+            value: change.newValue,
+            updateType: updateType,
+          };
+        });
+      })
+    );
+  }
 
   async get<T>(key: string): Promise<T> {
     return new Promise((resolve) => {
@@ -22,11 +54,7 @@ export default abstract class AbstractChromeStorageService implements AbstractSt
   async save(key: string, obj: any): Promise<void> {
     if (obj == null) {
       // Fix safari not liking null in set
-      return new Promise<void>((resolve) => {
-        this.chromeStorageApi.remove(key, () => {
-          resolve();
-        });
-      });
+      return this.remove(key);
     }
 
     if (obj instanceof Set) {

apps/browser/src/platform/services/browser-local-storage.service.ts

@@ -1,5 +1,7 @@
 import AbstractChromeStorageService from "./abstractions/abstract-chrome-storage-api.service";
 
 export default class BrowserLocalStorageService extends AbstractChromeStorageService {
-  protected chromeStorageApi = chrome.storage.local;
+  constructor() {
+    super(chrome.storage.local);
+  }
 }

apps/browser/src/platform/services/browser-memory-storage.service.ts

@@ -1,5 +1,7 @@
 import AbstractChromeStorageService from "./abstractions/abstract-chrome-storage-api.service";
 
 export default class BrowserMemoryStorageService extends AbstractChromeStorageService {
-  protected chromeStorageApi = chrome.storage.session;
+  constructor() {
+    super(chrome.storage.session);
+  }
 }