Fix SingleUserStateProvider (#7593)

* Fix SingleUserStateProvider

- Fix cache key to be unique per instance per user

* Add Specific State Provider Tests

* Add Missing await

libs/common/src/platform/services/crypto.service.ts

@@ -62,7 +62,7 @@ export class CryptoService implements CryptoServiceAbstraction {
     userId ??= (await firstValueFrom(this.accountService.activeAccount$))?.id;
     if (key != null) {
       // Key should never be null anyway
-      this.stateProvider.getUser(userId, USER_EVER_HAD_USER_KEY).update(() => true);
+      await this.stateProvider.getUser(userId, USER_EVER_HAD_USER_KEY).update(() => true);
     }
     await this.stateService.setUserKey(key, { userId: userId });
     await this.storeAdditionalKeys(key, userId);

libs/common/src/platform/state/implementations/specific-state.provider.spec.ts

@@ -0,0 +1,124 @@
+import { mockAccountServiceWith } from "../../../../spec/fake-account-service";
+import { FakeStorageService } from "../../../../spec/fake-storage.service";
+import { UserId } from "../../../types/guid";
+import { KeyDefinition } from "../key-definition";
+import { StateDefinition } from "../state-definition";
+
+import { DefaultActiveUserState } from "./default-active-user-state";
+import { DefaultActiveUserStateProvider } from "./default-active-user-state.provider";
+import { DefaultGlobalState } from "./default-global-state";
+import { DefaultGlobalStateProvider } from "./default-global-state.provider";
+import { DefaultSingleUserState } from "./default-single-user-state";
+import { DefaultSingleUserStateProvider } from "./default-single-user-state.provider";
+
+describe("Specific State Providers", () => {
+  let singleSut: DefaultSingleUserStateProvider;
+  let activeSut: DefaultActiveUserStateProvider;
+  let globalSut: DefaultGlobalStateProvider;
+
+  const fakeUser1 = "00000000-0000-1000-a000-000000000001" as UserId;
+
+  beforeEach(() => {
+    singleSut = new DefaultSingleUserStateProvider(
+      new FakeStorageService() as any,
+      new FakeStorageService(),
+    );
+    activeSut = new DefaultActiveUserStateProvider(
+      mockAccountServiceWith(null),
+      new FakeStorageService() as any,
+      new FakeStorageService(),
+    );
+    globalSut = new DefaultGlobalStateProvider(
+      new FakeStorageService() as any,
+      new FakeStorageService(),
+    );
+  });
+
+  const fakeDiskStateDefinition = new StateDefinition("fake", "disk");
+  const fakeAlternateDiskStateDefinition = new StateDefinition("fakeAlternate", "disk");
+  const fakeMemoryStateDefinition = new StateDefinition("fake", "memory");
+
+  const fakeDiskKeyDefinition = new KeyDefinition<boolean>(fakeDiskStateDefinition, "fake", {
+    deserializer: (b) => b,
+  });
+  const fakeAlternateKeyDefinition = new KeyDefinition<boolean>(
+    fakeAlternateDiskStateDefinition,
+    "fake",
+    {
+      deserializer: (b) => b,
+    },
+  );
+  const fakeMemoryKeyDefinition = new KeyDefinition<boolean>(fakeMemoryStateDefinition, "fake", {
+    deserializer: (b) => b,
+  });
+  const fakeDiskKeyDefinitionAlternate = new KeyDefinition<boolean>(
+    fakeDiskStateDefinition,
+    "fakeAlternate",
+    {
+      deserializer: (b) => b,
+    },
+  );
+
+  describe.each([
+    {
+      // Use a static user id so that it has the same signature as the rest and then write special tests
+      // handling differing user id
+      getMethod: (keyDefinition: KeyDefinition<boolean>) => singleSut.get(fakeUser1, keyDefinition),
+      expectedInstance: DefaultSingleUserState,
+    },
+    {
+      getMethod: (keyDefinition: KeyDefinition<boolean>) => activeSut.get(keyDefinition),
+      expectedInstance: DefaultActiveUserState,
+    },
+    {
+      getMethod: (keyDefinition: KeyDefinition<boolean>) => globalSut.get(keyDefinition),
+      expectedInstance: DefaultGlobalState,
+    },
+  ])("common behavior %s", ({ getMethod, expectedInstance }) => {
+    it("returns expected instance", () => {
+      const state = getMethod(fakeDiskKeyDefinition);
+
+      expect(state).toBeTruthy();
+      expect(state).toBeInstanceOf(expectedInstance);
+    });
+
+    it("returns cached instance on repeated request", () => {
+      const stateFirst = getMethod(fakeDiskKeyDefinition);
+      const stateCached = getMethod(fakeDiskKeyDefinition);
+      expect(stateFirst).toStrictEqual(stateCached);
+    });
+
+    it("returns different instances when the storage location differs", () => {
+      const stateDisk = getMethod(fakeDiskKeyDefinition);
+      const stateMemory = getMethod(fakeMemoryKeyDefinition);
+      expect(stateDisk).not.toStrictEqual(stateMemory);
+    });
+
+    it("returns different instances when the state name differs", () => {
+      const state = getMethod(fakeDiskKeyDefinition);
+      const stateAlt = getMethod(fakeAlternateKeyDefinition);
+      expect(state).not.toStrictEqual(stateAlt);
+    });
+
+    it("returns different instances when the key differs", () => {
+      const state = getMethod(fakeDiskKeyDefinition);
+      const stateAlt = getMethod(fakeDiskKeyDefinitionAlternate);
+      expect(state).not.toStrictEqual(stateAlt);
+    });
+  });
+
+  describe("DefaultSingleUserStateProvider only behavior", () => {
+    const fakeUser2 = "00000000-0000-1000-a000-000000000002" as UserId;
+
+    it("returns different instances when the user id differs", () => {
+      const user1State = singleSut.get(fakeUser1, fakeDiskKeyDefinition);
+      const user2State = singleSut.get(fakeUser2, fakeDiskKeyDefinition);
+      expect(user1State).not.toStrictEqual(user2State);
+    });
+
+    it("returns an instance with the userId property corresponding to the user id passed in", () => {
+      const userState = singleSut.get(fakeUser1, fakeDiskKeyDefinition);
+      expect(userState.userId).toBe(fakeUser1);
+    });
+  });
+});

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

@@ -1,5 +1,7 @@
 import { Opaque } from "type-fest";
 
+import { UserId } from "../../types/guid";
+
 import { KeyDefinition } from "./key-definition";
 import { StateDefinition } from "./state-definition";
 
@@ -109,4 +111,24 @@ 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

@@ -148,11 +148,13 @@ export class KeyDefinition<T> {
    */
   buildCacheKey(scope: "user" | "global", userId?: "active" | UserId): string {
     if (scope === "user" && userId == null) {
-      throw new Error("You must provide a userId when building a user scoped cache key.");
+      throw new Error(
+        "You must provide a userId or 'active' when building a user scoped cache key.",
+      );
     }
     return userId === null
-      ? `${this.stateDefinition.storageLocation}_${scope}_${userId}_${this.stateDefinition.name}_${this.key}`
-      : `${this.stateDefinition.storageLocation}_${scope}_${this.stateDefinition.name}_${this.key}`;
+      ? `${this.stateDefinition.storageLocation}_${scope}_${this.stateDefinition.name}_${this.key}`
+      : `${this.stateDefinition.storageLocation}_${scope}_${userId}_${this.stateDefinition.name}_${this.key}`;
   }
 
   private get errorKeyName() {