Ps 1291/apply to from json pattern to state (#3425)

* Clean up dangling behaviorSubject

* Handle null in utils

* fix null check

* Await promises, even in async functions

* Add to/fromJSON methods to State and Accounts

This is needed since all storage in manifest v3 is key-value-pair-based
and session storage of most data is actually serialized into an
encrypted string.

* Simplify AccountKeys json parsing

* Fix account key (de)serialization

* Remove unused DecodedToken state

* Correct filename typo

* Simplify keys `toJSON` tests

* Explain AccountKeys `toJSON` return type

* Remove unnecessary `any`s

* Remove unique ArrayBuffer serialization

* Initialize items in MemoryStorageService

* Revert "Fix account key (de)serialization"

This reverts commit b1dffb5, which was breaking serializations

* Move fromJSON to owning object

* Add DeepJsonify type

* Use Records for storage

* Add new Account Settings to serialized data

* Fix failing serialization tests

* Extract complex type conversion to helper methods

* Remove unnecessary decorator

* Return null from json deserializers

* Remove unnecessary decorators

* Remove obsolete test

* Use type-fest `Jsonify` formatting rules for external library

* Update jsonify comment

Co-authored-by: @eliykat

* Remove erroneous comment

* Fix unintended deep-jsonify changes

* Fix prettierignore

* Fix formatting of deep-jsonify.ts

Co-authored-by: Thomas Rittson <trittson@bitwarden.com>
Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>

.prettierignore

@@ -27,3 +27,6 @@ libs/.github
 
 # Github Workflows
 .github/workflows
+
+# Forked library files
+libs/common/src/types/deep-jsonify.ts

apps/browser/src/decorators/session-sync-observable/session-sync.decorator.spec.ts

@@ -19,6 +19,7 @@ describe("sessionSync decorator", () => {
         ctor: ctor,
         initializer: initializer,
       }),
+      testClass.testProperty.complete(),
     ]);
   });
 });

apps/browser/src/decorators/session-sync-observable/session-syncer.spec.ts

@@ -5,6 +5,7 @@ import { BrowserApi } from "../../browser/browserApi";
 import { StateService } from "../../services/abstractions/state.service";
 
 import { SessionSyncer } from "./session-syncer";
+import { SyncedItemMetadata } from "./sync-item-metadata";
 
 describe("session syncer", () => {
   const propertyKey = "behaviorSubject";
@@ -140,12 +141,14 @@ describe("session syncer", () => {
     });
 
     it("should update from message on emit from another instance", async () => {
+      const builder = jest.fn();
+      jest.spyOn(SyncedItemMetadata, "builder").mockReturnValue(builder);
       stateService.getFromSessionMemory.mockResolvedValue("test");
 
       await sut.updateFromMessage({ command: `${sessionKey}_update`, id: "different_id" });
 
       expect(stateService.getFromSessionMemory).toHaveBeenCalledTimes(1);
-      expect(stateService.getFromSessionMemory).toHaveBeenCalledWith(sessionKey);
+      expect(stateService.getFromSessionMemory).toHaveBeenCalledWith(sessionKey, builder);
 
       expect(nextSpy).toHaveBeenCalledTimes(1);
       expect(nextSpy).toHaveBeenCalledWith("test");

apps/browser/src/decorators/session-sync-observable/session-syncer.ts

@@ -66,8 +66,8 @@ export class SessionSyncer {
     if (message.command != this.updateMessageCommand || message.id === this.id) {
       return;
     }
-    const keyValuePair = await this.stateService.getFromSessionMemory(this.metaData.sessionKey);
-    const value = SyncedItemMetadata.buildFromKeyValuePair(keyValuePair, this.metaData);
+    const builder = SyncedItemMetadata.builder(this.metaData);
+    const value = await this.stateService.getFromSessionMemory(this.metaData.sessionKey, builder);
     this.ignoreNextUpdate = true;
     this.behaviorSubject.next(value);
   }

apps/browser/src/decorators/session-sync-observable/sync-item-metadata.ts

@@ -5,19 +5,15 @@ export class SyncedItemMetadata {
   initializer?: (keyValuePair: any) => any;
   initializeAsArray?: boolean;
 
-  static buildFromKeyValuePair(keyValuePair: any, metadata: SyncedItemMetadata): any {
-    const builder = SyncedItemMetadata.getBuilder(metadata);
-
+  static builder(metadata: SyncedItemMetadata): (o: any) => any {
+    const itemBuilder =
+      metadata.initializer != null
+        ? metadata.initializer
+        : (o: any) => Object.assign(new metadata.ctor(), o);
     if (metadata.initializeAsArray) {
-      return keyValuePair.map((o: any) => builder(o));
+      return (keyValuePair: any) => keyValuePair.map((o: any) => itemBuilder(o));
     } else {
-      return builder(keyValuePair);
+      return (keyValuePair: any) => itemBuilder(keyValuePair);
     }
   }
-
-  private static getBuilder(metadata: SyncedItemMetadata): (o: any) => any {
-    return metadata.initializer != null
-      ? metadata.initializer
-      : (o: any) => Object.assign(new metadata.ctor(), o);
-  }
 }

apps/browser/src/decorators/session-sync-observable/synced-item-metadata.spec.ts

@@ -1,59 +1,39 @@
 import { SyncedItemMetadata } from "./sync-item-metadata";
 
-describe("build from key value pair", () => {
+describe("builder", () => {
   const propertyKey = "propertyKey";
   const key = "key";
   const initializer = (s: any) => "used initializer";
   class TestClass {}
   const ctor = TestClass;
 
-  it("should call initializer if provided", () => {
-    const actual = SyncedItemMetadata.buildFromKeyValuePair(
-      {},
-      {
-        propertyKey,
-        sessionKey: "key",
-        initializer: initializer,
-      }
-    );
-
-    expect(actual).toEqual("used initializer");
+  it("should use initializer if provided", () => {
+    const metadata = { propertyKey, sessionKey: key, initializer };
+    const builder = SyncedItemMetadata.builder(metadata);
+    expect(builder({})).toBe("used initializer");
   });
 
-  it("should call ctor if provided", () => {
-    const expected = { provided: "value" };
-    const actual = SyncedItemMetadata.buildFromKeyValuePair(expected, {
-      propertyKey,
-      sessionKey: key,
-      ctor: ctor,
-    });
-
-    expect(actual).toBeInstanceOf(ctor);
-    expect(actual).toEqual(expect.objectContaining(expected));
+  it("should use ctor if initializer is not provided", () => {
+    const metadata = { propertyKey, sessionKey: key, ctor };
+    const builder = SyncedItemMetadata.builder(metadata);
+    expect(builder({})).toBeInstanceOf(TestClass);
   });
 
-  it("should prefer using initializer if both are provided", () => {
-    const actual = SyncedItemMetadata.buildFromKeyValuePair(
-      {},
-      {
-        propertyKey,
-        sessionKey: key,
-        initializer: initializer,
-        ctor: ctor,
-      }
-    );
-
-    expect(actual).toEqual("used initializer");
+  it("should prefer initializer over ctor", () => {
+    const metadata = { propertyKey, sessionKey: key, ctor, initializer };
+    const builder = SyncedItemMetadata.builder(metadata);
+    expect(builder({})).toBe("used initializer");
   });
 
   it("should honor initialize as array", () => {
-    const actual = SyncedItemMetadata.buildFromKeyValuePair([1, 2], {
+    const metadata = {
       propertyKey,
       sessionKey: key,
       initializer: initializer,
       initializeAsArray: true,
-    });
-
-    expect(actual).toEqual(["used initializer", "used initializer"]);
+    };
+    const builder = SyncedItemMetadata.builder(metadata);
+    expect(builder([{}])).toBeInstanceOf(Array);
+    expect(builder([{}])[0]).toBe("used initializer");
   });
 });

apps/browser/src/services/abstractions/state.service.ts

@@ -1,3 +1,5 @@
+import { Jsonify } from "type-fest";
+
 import { StateService as BaseStateServiceAbstraction } from "@bitwarden/common/abstractions/state.service";
 import { StorageOptions } from "@bitwarden/common/models/domain/storageOptions";
 
@@ -7,7 +9,7 @@ import { BrowserGroupingsComponentState } from "src/models/browserGroupingsCompo
 import { BrowserSendComponentState } from "src/models/browserSendComponentState";
 
 export abstract class StateService extends BaseStateServiceAbstraction<Account> {
-  abstract getFromSessionMemory<T>(key: string): Promise<T>;
+  abstract getFromSessionMemory<T>(key: string, deserializer?: (obj: Jsonify<T>) => T): Promise<T>;
   abstract setInSessionMemory(key: string, value: any): Promise<void>;
   getBrowserGroupingComponentState: (
     options?: StorageOptions

apps/browser/src/services/localBackedSessionStorage.service.spec.ts

@@ -96,6 +96,13 @@ describe("Browser Session Storage Service", () => {
           expect(cache.has("test")).toBe(true);
           expect(cache.get("test")).toEqual(session.test);
         });
+
+        it("should use a deserializer if provided", async () => {
+          const deserializer = jest.fn().mockReturnValue(testObj);
+          const result = await sut.get("test", { deserializer: deserializer });
+          expect(deserializer).toHaveBeenCalledWith(session.test);
+          expect(result).toEqual(testObj);
+        });
       });
     });
   });

apps/browser/src/services/localBackedSessionStorage.service.ts

@@ -1,6 +1,12 @@
+import { Jsonify } from "type-fest";
+
 import { AbstractEncryptService } from "@bitwarden/common/abstractions/abstractEncrypt.service";
-import { AbstractCachedStorageService } from "@bitwarden/common/abstractions/storage.service";
+import {
+  AbstractCachedStorageService,
+  MemoryStorageServiceInterface,
+} from "@bitwarden/common/abstractions/storage.service";
 import { EncString } from "@bitwarden/common/models/domain/encString";
+import { MemoryStorageOptions } from "@bitwarden/common/models/domain/storageOptions";
 import { SymmetricCryptoKey } from "@bitwarden/common/models/domain/symmetricCryptoKey";
 
 import { devFlag } from "../decorators/dev-flag.decorator";
@@ -15,7 +21,10 @@ const keys = {
   sessionKey: "session",
 };
 
-export class LocalBackedSessionStorageService extends AbstractCachedStorageService {
+export class LocalBackedSessionStorageService
+  extends AbstractCachedStorageService
+  implements MemoryStorageServiceInterface
+{
   private cache = new Map<string, unknown>();
   private localStorage = new BrowserLocalStorageService();
   private sessionStorage = new BrowserMemoryStorageService();
@@ -27,21 +36,26 @@ export class LocalBackedSessionStorageService extends AbstractCachedStorageServi
     super();
   }
 
-  async get<T>(key: string): Promise<T> {
+  async get<T>(key: string, options?: MemoryStorageOptions<T>): Promise<T> {
     if (this.cache.has(key)) {
       return this.cache.get(key) as T;
     }
 
-    return await this.getBypassCache(key);
+    return await this.getBypassCache(key, options);
   }
 
-  async getBypassCache<T>(key: string): Promise<T> {
+  async getBypassCache<T>(key: string, options?: MemoryStorageOptions<T>): Promise<T> {
     const session = await this.getLocalSession(await this.getSessionEncKey());
     if (session == null || !Object.keys(session).includes(key)) {
       return null;
     }
 
-    this.cache.set(key, session[key]);
+    let value = session[key];
+    if (options?.deserializer != null) {
+      value = options.deserializer(value as Jsonify<T>);
+    }
+
+    this.cache.set(key, value);
     return this.cache.get(key) as T;
   }
 

apps/browser/src/services/state.service.spec.ts

@@ -1,8 +1,8 @@
-import { Substitute, SubstituteOf } from "@fluffy-spoon/substitute";
+import { Arg, Substitute, SubstituteOf } from "@fluffy-spoon/substitute";
 
 import { LogService } from "@bitwarden/common/abstractions/log.service";
 import {
-  AbstractCachedStorageService,
+  MemoryStorageServiceInterface,
   AbstractStorageService,
 } from "@bitwarden/common/abstractions/storage.service";
 import { SendType } from "@bitwarden/common/enums/sendType";
@@ -49,7 +49,7 @@ describe("Browser State Service", () => {
   });
 
   describe("direct memory storage access", () => {
-    let memoryStorageService: AbstractCachedStorageService;
+    let memoryStorageService: LocalBackedSessionStorageService;
 
     beforeEach(() => {
       // We need `AbstractCachedStorageService` in the prototype chain to correctly test cache bypass.
@@ -79,12 +79,12 @@ describe("Browser State Service", () => {
   });
 
   describe("state methods", () => {
-    let memoryStorageService: SubstituteOf<AbstractStorageService>;
+    let memoryStorageService: SubstituteOf<AbstractStorageService & MemoryStorageServiceInterface>;
 
     beforeEach(() => {
       memoryStorageService = Substitute.for();
       const stateGetter = (key: string) => Promise.resolve(JSON.parse(JSON.stringify(state)));
-      memoryStorageService.get("state").mimicks(stateGetter);
+      memoryStorageService.get("state", Arg.any()).mimicks(stateGetter);
 
       sut = new StateService(
         diskStorageService,

apps/browser/src/services/state.service.ts

@@ -1,3 +1,5 @@
+import { Jsonify } from "type-fest";
+
 import { AbstractCachedStorageService } from "@bitwarden/common/abstractions/storage.service";
 import { GlobalState } from "@bitwarden/common/models/domain/globalState";
 import { StorageOptions } from "@bitwarden/common/models/domain/storageOptions";
@@ -17,9 +19,9 @@ export class StateService
   extends BaseStateService<GlobalState, Account>
   implements StateServiceAbstraction
 {
-  async getFromSessionMemory<T>(key: string): Promise<T> {
+  async getFromSessionMemory<T>(key: string, deserializer?: (obj: Jsonify<T>) => T): Promise<T> {
     return this.memoryStorageService instanceof AbstractCachedStorageService
-      ? await this.memoryStorageService.getBypassCache<T>(key)
+      ? await this.memoryStorageService.getBypassCache<T>(key, { deserializer: deserializer })
       : await this.memoryStorageService.get<T>(key);
   }
 

libs/common/spec/misc/utils.spec.ts

@@ -70,4 +70,10 @@ describe("Utils Service", () => {
       expect(Utils.newGuid()).toMatch(validGuid);
     });
   });
+
+  describe("fromByteStringToArray", () => {
+    it("should handle null", () => {
+      expect(Utils.fromByteStringToArray(null)).toEqual(null);
+    });
+  });
 });

libs/common/spec/services/stateMigration.service.spec.ts

@@ -116,8 +116,8 @@ describe("State Migration Service", () => {
                 key: "orgThreeEncKey",
               },
             },
-          },
-        },
+          } as any,
+        } as any,
       });
 
       const migratedAccount = await (stateMigrationService as any).migrateAccountFrom4To5(