[PM-5829] Add disk-local option for web (#7669)

* Add `disk-local` option for web

* Fix `web` DI

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

Co-authored-by: Matt Gibson <mgibson@bitwarden.com>

* Rely On Default Implementation for Most of Cache Key

---------

Co-authored-by: Matt Gibson <mgibson@bitwarden.com>

libs/common/src/platform/state/implementations/default-active-user-state.provider.ts

@@ -5,7 +5,7 @@ import {
   ObservableStorageService,
 } from "../../abstractions/storage.service";
 import { KeyDefinition } from "../key-definition";
-import { StorageLocation } from "../state-definition";
+import { StateDefinition } from "../state-definition";
 import { ActiveUserState } from "../user-state";
 import { ActiveUserStateProvider } from "../user-state.provider";
 
@@ -15,13 +15,13 @@ export class DefaultActiveUserStateProvider implements ActiveUserStateProvider {
   private cache: Record<string, ActiveUserState<unknown>> = {};
 
   constructor(
-    protected accountService: AccountService,
-    protected memoryStorage: AbstractMemoryStorageService & ObservableStorageService,
-    protected diskStorage: AbstractStorageService & ObservableStorageService,
+    protected readonly accountService: AccountService,
+    protected readonly memoryStorage: AbstractMemoryStorageService & ObservableStorageService,
+    protected readonly diskStorage: AbstractStorageService & ObservableStorageService,
   ) {}
 
   get<T>(keyDefinition: KeyDefinition<T>): ActiveUserState<T> {
-    const cacheKey = keyDefinition.buildCacheKey("user", "active");
+    const cacheKey = this.buildCacheKey(keyDefinition);
     const existingUserState = this.cache[cacheKey];
     if (existingUserState != null) {
       // I have to cast out of the unknown generic but this should be safe if rules
@@ -34,15 +34,26 @@ export class DefaultActiveUserStateProvider implements ActiveUserStateProvider {
     return newUserState;
   }
 
+  private buildCacheKey(keyDefinition: KeyDefinition<unknown>) {
+    return `${this.getLocationString(keyDefinition)}_${keyDefinition.fullName}`;
+  }
+
   protected buildActiveUserState<T>(keyDefinition: KeyDefinition<T>): ActiveUserState<T> {
     return new DefaultActiveUserState<T>(
       keyDefinition,
       this.accountService,
-      this.getLocation(keyDefinition.stateDefinition.storageLocation),
+      this.getLocation(keyDefinition.stateDefinition),
     );
   }
 
-  private getLocation(location: StorageLocation) {
+  protected getLocationString(keyDefinition: KeyDefinition<unknown>): string {
+    return keyDefinition.stateDefinition.defaultStorageLocation;
+  }
+
+  protected getLocation(stateDefinition: StateDefinition) {
+    // The default implementations don't support the client overrides
+    // it is up to the client to extend this class and add that support
+    const location = stateDefinition.defaultStorageLocation;
     switch (location) {
       case "disk":
         return this.diskStorage;

libs/common/src/platform/state/implementations/default-global-state.provider.ts

@@ -6,7 +6,7 @@ import {
 import { GlobalState } from "../global-state";
 import { GlobalStateProvider } from "../global-state.provider";
 import { KeyDefinition } from "../key-definition";
-import { StorageLocation } from "../state-definition";
+import { StateDefinition } from "../state-definition";
 
 import { DefaultGlobalState } from "./default-global-state";
 
@@ -14,12 +14,12 @@ export class DefaultGlobalStateProvider implements GlobalStateProvider {
   private globalStateCache: Record<string, GlobalState<unknown>> = {};
 
   constructor(
-    private memoryStorage: AbstractMemoryStorageService & ObservableStorageService,
-    private diskStorage: AbstractStorageService & ObservableStorageService,
+    protected readonly memoryStorage: AbstractMemoryStorageService & ObservableStorageService,
+    protected readonly diskStorage: AbstractStorageService & ObservableStorageService,
   ) {}
 
   get<T>(keyDefinition: KeyDefinition<T>): GlobalState<T> {
-    const cacheKey = keyDefinition.buildCacheKey("global");
+    const cacheKey = this.buildCacheKey(keyDefinition);
     const existingGlobalState = this.globalStateCache[cacheKey];
     if (existingGlobalState != null) {
       // The cast into the actual generic is safe because of rules around key definitions
@@ -29,14 +29,23 @@ export class DefaultGlobalStateProvider implements GlobalStateProvider {
 
     const newGlobalState = new DefaultGlobalState<T>(
       keyDefinition,
-      this.getLocation(keyDefinition.stateDefinition.storageLocation),
+      this.getLocation(keyDefinition.stateDefinition),
     );
 
     this.globalStateCache[cacheKey] = newGlobalState;
     return newGlobalState;
   }
 
-  private getLocation(location: StorageLocation) {
+  private buildCacheKey(keyDefinition: KeyDefinition<unknown>) {
+    return `${this.getLocationString(keyDefinition)}_${keyDefinition.fullName}`;
+  }
+
+  protected getLocationString(keyDefinition: KeyDefinition<unknown>): string {
+    return keyDefinition.stateDefinition.defaultStorageLocation;
+  }
+
+  protected getLocation(stateDefinition: StateDefinition) {
+    const location = stateDefinition.defaultStorageLocation;
     switch (location) {
       case "disk":
         return this.diskStorage;

libs/common/src/platform/state/implementations/default-single-user-state.provider.ts

@@ -5,7 +5,7 @@ import {
   ObservableStorageService,
 } from "../../abstractions/storage.service";
 import { KeyDefinition } from "../key-definition";
-import { StorageLocation } from "../state-definition";
+import { StateDefinition } from "../state-definition";
 import { SingleUserState } from "../user-state";
 import { SingleUserStateProvider } from "../user-state.provider";
 
@@ -15,12 +15,12 @@ export class DefaultSingleUserStateProvider implements SingleUserStateProvider {
   private cache: Record<string, SingleUserState<unknown>> = {};
 
   constructor(
-    protected memoryStorage: AbstractMemoryStorageService & ObservableStorageService,
-    protected diskStorage: AbstractStorageService & ObservableStorageService,
+    protected readonly memoryStorage: AbstractMemoryStorageService & ObservableStorageService,
+    protected readonly diskStorage: AbstractStorageService & ObservableStorageService,
   ) {}
 
   get<T>(userId: UserId, keyDefinition: KeyDefinition<T>): SingleUserState<T> {
-    const cacheKey = keyDefinition.buildCacheKey("user", userId);
+    const cacheKey = this.buildCacheKey(userId, keyDefinition);
     const existingUserState = this.cache[cacheKey];
     if (existingUserState != null) {
       // I have to cast out of the unknown generic but this should be safe if rules
@@ -33,6 +33,10 @@ export class DefaultSingleUserStateProvider implements SingleUserStateProvider {
     return newUserState;
   }
 
+  private buildCacheKey(userId: UserId, keyDefinition: KeyDefinition<unknown>) {
+    return `${this.getLocationString(keyDefinition)}_${keyDefinition.fullName}_${userId}`;
+  }
+
   protected buildSingleUserState<T>(
     userId: UserId,
     keyDefinition: KeyDefinition<T>,
@@ -40,12 +44,18 @@ export class DefaultSingleUserStateProvider implements SingleUserStateProvider {
     return new DefaultSingleUserState<T>(
       userId,
       keyDefinition,
-      this.getLocation(keyDefinition.stateDefinition.storageLocation),
+      this.getLocation(keyDefinition.stateDefinition),
     );
   }
 
-  private getLocation(location: StorageLocation) {
-    switch (location) {
+  protected getLocationString(keyDefinition: KeyDefinition<unknown>): string {
+    return keyDefinition.stateDefinition.defaultStorageLocation;
+  }
+
+  protected getLocation(stateDefinition: StateDefinition) {
+    // The default implementations don't support the client overrides
+    // it is up to the client to extend this class and add that support
+    switch (stateDefinition.defaultStorageLocation) {
       case "disk":
         return this.diskStorage;
       case "memory":

libs/common/src/platform/state/key-definition.spec.ts

@@ -1,7 +1,5 @@
 import { Opaque } from "type-fest";
 
-import { UserId } from "../../types/guid";
-
 import { KeyDefinition } from "./key-definition";
 import { StateDefinition } from "./state-definition";
 
@@ -111,24 +109,4 @@ describe("KeyDefinition", () => {
       expect(deserializedValue[1]).toBeFalsy();
     });
   });
-
-  describe("buildCacheKey", () => {
-    const keyDefinition = new KeyDefinition(fakeStateDefinition, "fake", {
-      deserializer: (s) => s,
-    });
-
-    it("builds unique cache key for each user", () => {
-      const cacheKeys: string[] = [];
-
-      // single user keys
-      cacheKeys.push(keyDefinition.buildCacheKey("user", "1" as UserId));
-      cacheKeys.push(keyDefinition.buildCacheKey("user", "2" as UserId));
-
-      expect(new Set(cacheKeys).size).toBe(cacheKeys.length);
-    });
-
-    it("throws with bad usage", () => {
-      expect(() => keyDefinition.buildCacheKey("user", null)).toThrow();
-    });
-  });
 });

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

@@ -142,19 +142,8 @@ export class KeyDefinition<T> {
     });
   }
 
-  /**
-   * Create a string that should be unique across the entire application.
-   * @returns A string that can be used to cache instances created via this key.
-   */
-  buildCacheKey(scope: "user" | "global", userId?: "active" | UserId): string {
-    if (scope === "user" && userId == null) {
-      throw new Error(
-        "You must provide a userId or 'active' when building a user scoped cache key.",
-      );
-    }
-    return userId === null
-      ? `${this.stateDefinition.storageLocation}_${scope}_${this.stateDefinition.name}_${this.key}`
-      : `${this.stateDefinition.storageLocation}_${scope}_${userId}_${this.stateDefinition.name}_${this.key}`;
+  get fullName() {
+    return `${this.stateDefinition.name}_${this.key}`;
   }
 
   private get errorKeyName() {

libs/common/src/platform/state/state-definition.ts

@@ -1,16 +1,57 @@
+/**
+ * Default storage location options.
+ *
+ * `disk` generally means state that is accessible between restarts of the application,
+ * with the exception of the web client. In web this means `sessionStorage`. The data is
+ * through refreshes of the page but not available once that tab is closed or from any
+ * other tabs.
+ *
+ * `memory` means that the information stored there goes away during application
+ * restarts.
+ */
 export type StorageLocation = "disk" | "memory";
 
+/**
+ * *Note*: The property names of this object should match exactly with the string values of the {@link ClientType} enum
+ */
+export type ClientLocations = {
+  /**
+   * Overriding storage location for the web client.
+   *
+   * Includes an extra storage location to store data in `localStorage`
+   * that is available from different tabs and after a tab has closed.
+   */
+  web: StorageLocation | "disk-local";
+  /**
+   * Overriding storage location for browser clients.
+   */
+  //browser: StorageLocation;
+  /**
+   * Overriding storage location for desktop clients.
+   */
+  //desktop: StorageLocation;
+  /**
+   * Overriding storage location for CLI clients.
+   */
+  //cli: StorageLocation;
+};
+
 /**
  * Defines the base location and instruction of where this state is expected to be located.
  */
 export class StateDefinition {
+  readonly storageLocationOverrides: Partial<ClientLocations>;
+
   /**
    * Creates a new instance of {@link StateDefinition}, the creation of which is owned by the platform team.
    * @param name The name of the state, this needs to be unique from all other {@link StateDefinition}'s.
-   * @param storageLocation The location of where this state should be stored.
+   * @param defaultStorageLocation The location of where this state should be stored.
    */
   constructor(
     readonly name: string,
-    readonly storageLocation: StorageLocation,
-  ) {}
+    readonly defaultStorageLocation: StorageLocation,
+    storageLocationOverrides?: Partial<ClientLocations>,
+  ) {
+    this.storageLocationOverrides = storageLocationOverrides ?? {};
+  }
 }

libs/common/src/platform/state/state-definitions.spec.ts

@@ -1,53 +1,60 @@
-import { StateDefinition } from "./state-definition";
+import { ClientLocations, StateDefinition } from "./state-definition";
 import * as stateDefinitionsRecord from "./state-definitions";
 
-describe("state definitions", () => {
-  const trackedNames: [string, string][] = [];
+describe.each(["web", "cli", "desktop", "browser"])(
+  "state definitions follow rules for client %s",
+  (clientType: keyof ClientLocations) => {
+    const trackedNames: [string, string][] = [];
 
-  test.each(Object.entries(stateDefinitionsRecord))(
-    "that export %s follows all rules",
-    (exportName, stateDefinition) => {
-      // All exports from state-definitions are expected to be StateDefinition's
-      if (!(stateDefinition instanceof StateDefinition)) {
-        throw new Error(`export ${exportName} is expected to be a StateDefinition`);
-      }
+    test.each(Object.entries(stateDefinitionsRecord))(
+      "that export %s follows all rules",
+      (exportName, stateDefinition) => {
+        // All exports from state-definitions are expected to be StateDefinition's
+        if (!(stateDefinition instanceof StateDefinition)) {
+          throw new Error(`export ${exportName} is expected to be a StateDefinition`);
+        }
 
-      const fullName = `${stateDefinition.name}_${stateDefinition.storageLocation}`;
+        const storageLocation =
+          stateDefinition.storageLocationOverrides[clientType] ??
+          stateDefinition.defaultStorageLocation;
 
-      const exactConflictingExport = trackedNames.find(
-        ([_, trackedName]) => trackedName === fullName,
-      );
-      if (exactConflictingExport !== undefined) {
-        const [conflictingExportName] = exactConflictingExport;
-        throw new Error(
-          `The export '${exportName}' has a conflicting state name and storage location with export ` +
-            `'${conflictingExportName}' please ensure that you choose a unique name and location.`,
+        const fullName = `${stateDefinition.name}_${storageLocation}`;
+
+        const exactConflictingExport = trackedNames.find(
+          ([_, trackedName]) => trackedName === fullName,
         );
-      }
+        if (exactConflictingExport !== undefined) {
+          const [conflictingExportName] = exactConflictingExport;
+          throw new Error(
+            `The export '${exportName}' has a conflicting state name and storage location with export ` +
+              `'${conflictingExportName}' please ensure that you choose a unique name and location for all clients.`,
+          );
+        }
 
-      const roughConflictingExport = trackedNames.find(
-        ([_, trackedName]) => trackedName.toLowerCase() === fullName.toLowerCase(),
-      );
-      if (roughConflictingExport !== undefined) {
-        const [conflictingExportName] = roughConflictingExport;
-        throw new Error(
-          `The export '${exportName}' differs its state name and storage location ` +
-            `only by casing with export '${conflictingExportName}' please ensure it differs by more than casing.`,
+        const roughConflictingExport = trackedNames.find(
+          ([_, trackedName]) => trackedName.toLowerCase() === fullName.toLowerCase(),
         );
-      }
+        if (roughConflictingExport !== undefined) {
+          const [conflictingExportName] = roughConflictingExport;
+          throw new Error(
+            `The export '${exportName}' differs its state name and storage location ` +
+              `only by casing with export '${conflictingExportName}' please ensure it differs by more than casing.`,
+          );
+        }
 
-      const name = stateDefinition.name;
+        const name = stateDefinition.name;
 
-      expect(name).not.toBeUndefined(); // undefined in an invalid name
-      expect(name).not.toBeNull(); // null is in invalid name
-      expect(name.length).toBeGreaterThan(3); // A 3 characters or less name is not descriptive enough
-      expect(name[0]).toEqual(name[0].toLowerCase()); // First character should be lower case since camelCase is required
-      expect(name).not.toContain(" "); // There should be no spaces in a state name
-      expect(name).not.toContain("_"); // We should not be doing snake_case for state name
+        expect(name).not.toBeUndefined(); // undefined in an invalid name
+        expect(name).not.toBeNull(); // null is in invalid name
+        expect(name.length).toBeGreaterThan(3); // A 3 characters or less name is not descriptive enough
+        expect(name[0]).toEqual(name[0].toLowerCase()); // First character should be lower case since camelCase is required
+        expect(name).not.toContain(" "); // There should be no spaces in a state name
+        expect(name).not.toContain("_"); // We should not be doing snake_case for state name
 
-      // NOTE: We could expect some details about the export name as well
+        // NOTE: We could expect some details about the export name as well
 
-      trackedNames.push([exportName, fullName]);
-    },
-  );
-});
+        trackedNames.push([exportName, fullName]);
+      },
+    );
+  },
+);