Fix clear keys state logic (#8143)

* Fix clear keys state logic

Disk state should only be cleared if memory only is not specified.

* Remove unnecessary zip from testing

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

@@ -19,7 +19,13 @@ import { EncString } from "../models/domain/enc-string";
 import { SymmetricCryptoKey } from "../models/domain/symmetric-crypto-key";
 import { CryptoService } from "../services/crypto.service";
 
-import { USER_EVER_HAD_USER_KEY, USER_KEY } from "./key-state/user-key.state";
+import { USER_ENCRYPTED_ORGANIZATION_KEYS } from "./key-state/org-keys.state";
+import { USER_ENCRYPTED_PROVIDER_KEYS } from "./key-state/provider-keys.state";
+import {
+  USER_ENCRYPTED_PRIVATE_KEY,
+  USER_EVER_HAD_USER_KEY,
+  USER_KEY,
+} from "./key-state/user-key.state";
 
 describe("cryptoService", () => {
   let cryptoService: CryptoService;
@@ -315,4 +321,218 @@ describe("cryptoService", () => {
       },
     );
   });
+
+  describe("clearOrgKeys", () => {
+    let forceMemorySpy: jest.Mock;
+    beforeEach(() => {
+      forceMemorySpy = cryptoService["activeUserOrgKeysState"].forceValue = jest.fn();
+    });
+    it("clears in memory org keys when called with memoryOnly", async () => {
+      await cryptoService.clearOrgKeys(true);
+
+      expect(forceMemorySpy).toHaveBeenCalledWith({});
+    });
+
+    it("does not clear memory when called with the non active user and memory only", async () => {
+      await cryptoService.clearOrgKeys(true, "someOtherUser" as UserId);
+
+      expect(forceMemorySpy).not.toHaveBeenCalled();
+    });
+
+    it("does not write to disk state if called with memory only", async () => {
+      await cryptoService.clearOrgKeys(true);
+
+      expect(stateProvider.singleUser.mock.get).not.toHaveBeenCalled();
+    });
+
+    it("clears disk state when called with diskOnly", async () => {
+      await cryptoService.clearOrgKeys(false);
+
+      expect(stateProvider.singleUser.mock.get).toHaveBeenCalledWith(
+        mockUserId,
+        USER_ENCRYPTED_ORGANIZATION_KEYS,
+      );
+      expect(
+        stateProvider.singleUser.getFake(mockUserId, USER_ENCRYPTED_ORGANIZATION_KEYS).nextMock,
+      ).toHaveBeenCalledWith(null);
+    });
+
+    it("clears another user's disk state when called with diskOnly and that user", async () => {
+      await cryptoService.clearOrgKeys(false, "someOtherUser" as UserId);
+
+      expect(stateProvider.singleUser.mock.get).toHaveBeenCalledWith(
+        "someOtherUser" as UserId,
+        USER_ENCRYPTED_ORGANIZATION_KEYS,
+      );
+      expect(
+        stateProvider.singleUser.getFake(
+          "someOtherUser" as UserId,
+          USER_ENCRYPTED_ORGANIZATION_KEYS,
+        ).nextMock,
+      ).toHaveBeenCalledWith(null);
+    });
+
+    it("does not clear active user disk state when called with diskOnly and a different specified user", async () => {
+      await cryptoService.clearOrgKeys(false, "someOtherUser" as UserId);
+
+      expect(stateProvider.singleUser.mock.get).not.toHaveBeenCalledWith(
+        mockUserId,
+        USER_ENCRYPTED_ORGANIZATION_KEYS,
+      );
+    });
+  });
+
+  describe("clearProviderKeys", () => {
+    let forceMemorySpy: jest.Mock;
+    beforeEach(() => {
+      forceMemorySpy = cryptoService["activeUserProviderKeysState"].forceValue = jest.fn();
+    });
+    it("clears in memory org keys when called with memoryOnly", async () => {
+      await cryptoService.clearProviderKeys(true);
+
+      expect(forceMemorySpy).toHaveBeenCalledWith({});
+    });
+
+    it("does not clear memory when called with the non active user and memory only", async () => {
+      await cryptoService.clearProviderKeys(true, "someOtherUser" as UserId);
+
+      expect(forceMemorySpy).not.toHaveBeenCalled();
+    });
+
+    it("does not write to disk state if called with memory only", async () => {
+      await cryptoService.clearProviderKeys(true);
+
+      expect(stateProvider.singleUser.mock.get).not.toHaveBeenCalled();
+    });
+
+    it("clears disk state when called with diskOnly", async () => {
+      await cryptoService.clearProviderKeys(false);
+
+      expect(stateProvider.singleUser.mock.get).toHaveBeenCalledWith(
+        mockUserId,
+        USER_ENCRYPTED_PROVIDER_KEYS,
+      );
+      expect(
+        stateProvider.singleUser.getFake(mockUserId, USER_ENCRYPTED_PROVIDER_KEYS).nextMock,
+      ).toHaveBeenCalledWith(null);
+    });
+
+    it("clears another user's disk state when called with diskOnly and that user", async () => {
+      await cryptoService.clearProviderKeys(false, "someOtherUser" as UserId);
+
+      expect(stateProvider.singleUser.mock.get).toHaveBeenCalledWith(
+        "someOtherUser" as UserId,
+        USER_ENCRYPTED_PROVIDER_KEYS,
+      );
+      expect(
+        stateProvider.singleUser.getFake("someOtherUser" as UserId, USER_ENCRYPTED_PROVIDER_KEYS)
+          .nextMock,
+      ).toHaveBeenCalledWith(null);
+    });
+
+    it("does not clear active user disk state when called with diskOnly and a different specified user", async () => {
+      await cryptoService.clearProviderKeys(false, "someOtherUser" as UserId);
+
+      expect(stateProvider.singleUser.mock.get).not.toHaveBeenCalledWith(
+        mockUserId,
+        USER_ENCRYPTED_PROVIDER_KEYS,
+      );
+    });
+  });
+
+  describe("clearKeyPair", () => {
+    let forceMemoryPrivateKeySpy: jest.Mock;
+    let forceMemoryPublicKeySpy: jest.Mock;
+    beforeEach(() => {
+      forceMemoryPrivateKeySpy = cryptoService["activeUserPrivateKeyState"].forceValue = jest.fn();
+      forceMemoryPublicKeySpy = cryptoService["activeUserPublicKeyState"].forceValue = jest.fn();
+    });
+    it("clears in memory org keys when called with memoryOnly", async () => {
+      await cryptoService.clearKeyPair(true);
+
+      expect(forceMemoryPrivateKeySpy).toHaveBeenCalledWith(null);
+      expect(forceMemoryPublicKeySpy).toHaveBeenCalledWith(null);
+    });
+
+    it("does not clear memory when called with the non active user and memory only", async () => {
+      await cryptoService.clearKeyPair(true, "someOtherUser" as UserId);
+
+      expect(forceMemoryPrivateKeySpy).not.toHaveBeenCalled();
+      expect(forceMemoryPublicKeySpy).not.toHaveBeenCalled();
+    });
+
+    it("does not write to disk state if called with memory only", async () => {
+      await cryptoService.clearKeyPair(true);
+
+      expect(stateProvider.singleUser.mock.get).not.toHaveBeenCalled();
+    });
+
+    it("clears disk state when called with diskOnly", async () => {
+      await cryptoService.clearKeyPair(false);
+
+      expect(stateProvider.singleUser.mock.get).toHaveBeenCalledWith(
+        mockUserId,
+        USER_ENCRYPTED_PRIVATE_KEY,
+      );
+      expect(
+        stateProvider.singleUser.getFake(mockUserId, USER_ENCRYPTED_PRIVATE_KEY).nextMock,
+      ).toHaveBeenCalledWith(null);
+    });
+
+    it("clears another user's disk state when called with diskOnly and that user", async () => {
+      await cryptoService.clearKeyPair(false, "someOtherUser" as UserId);
+
+      expect(stateProvider.singleUser.mock.get).toHaveBeenCalledWith(
+        "someOtherUser" as UserId,
+        USER_ENCRYPTED_PRIVATE_KEY,
+      );
+      expect(
+        stateProvider.singleUser.getFake("someOtherUser" as UserId, USER_ENCRYPTED_PRIVATE_KEY)
+          .nextMock,
+      ).toHaveBeenCalledWith(null);
+    });
+
+    it("does not clear active user disk state when called with diskOnly and a different specified user", async () => {
+      await cryptoService.clearKeyPair(false, "someOtherUser" as UserId);
+
+      expect(stateProvider.singleUser.mock.get).not.toHaveBeenCalledWith(
+        mockUserId,
+        USER_ENCRYPTED_PRIVATE_KEY,
+      );
+    });
+  });
+
+  describe("clearUserKey", () => {
+    it("clears the user key for the active user when no userId is specified", async () => {
+      await cryptoService.clearUserKey(false);
+      expect(stateProvider.mock.setUserState).toHaveBeenCalledWith(USER_KEY, null, undefined);
+    });
+
+    it("clears the user key for the specified user when a userId is specified", async () => {
+      await cryptoService.clearUserKey(false, "someOtherUser" as UserId);
+      expect(stateProvider.mock.setUserState).toHaveBeenCalledWith(USER_KEY, null, "someOtherUser");
+    });
+
+    it("sets the maximum account status of the active user id to locked when user id is not specified", async () => {
+      await cryptoService.clearUserKey(false);
+      expect(accountService.mock.setMaxAccountStatus).toHaveBeenCalledWith(
+        mockUserId,
+        AuthenticationStatus.Locked,
+      );
+    });
+
+    it("sets the maximum account status of the specified user id to locked when user id is specified", async () => {
+      await cryptoService.clearUserKey(false, "someOtherUser" as UserId);
+      expect(accountService.mock.setMaxAccountStatus).toHaveBeenCalledWith(
+        "someOtherUser" as UserId,
+        AuthenticationStatus.Locked,
+      );
+    });
+
+    it("clears all stored user keys when clearAll is true", async () => {
+      const clearAllSpy = (cryptoService["clearAllStoredUserKeys"] = jest.fn());
+      await cryptoService.clearUserKey(true);
+      expect(clearAllSpy).toHaveBeenCalledWith(mockUserId);
+    });
+  });
 });

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

@@ -467,10 +467,8 @@ export class CryptoService implements CryptoServiceAbstraction {
   async clearOrgKeys(memoryOnly?: boolean, userId?: UserId): Promise<void> {
     const activeUserId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
     const userIdIsActive = userId == null || userId === activeUserId;
-    if (memoryOnly && userIdIsActive) {
-      // org keys are only cached for active users
-      await this.activeUserOrgKeysState.forceValue({});
-    } else {
+
+    if (!memoryOnly) {
       if (userId == null && activeUserId == null) {
         // nothing to do
         return;
@@ -478,13 +476,17 @@ export class CryptoService implements CryptoServiceAbstraction {
       await this.stateProvider
         .getUser(userId ?? activeUserId, USER_ENCRYPTED_ORGANIZATION_KEYS)
         .update(() => null);
+      return;
+    }
+
+    // org keys are only cached for active users
+    if (userIdIsActive) {
+      await this.activeUserOrgKeysState.forceValue({});
     }
   }
 
   async setProviderKeys(providers: ProfileProviderResponse[]): Promise<void> {
-    // 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.activeUserEncryptedProviderKeysState.update((_) => {
+    await this.activeUserEncryptedProviderKeysState.update((_) => {
       const encProviderKeys: { [providerId: ProviderId]: EncryptedString } = {};
 
       providers.forEach((provider) => {
@@ -511,10 +513,8 @@ export class CryptoService implements CryptoServiceAbstraction {
   async clearProviderKeys(memoryOnly?: boolean, userId?: UserId): Promise<void> {
     const activeUserId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
     const userIdIsActive = userId == null || userId === activeUserId;
-    if (memoryOnly && userIdIsActive) {
-      // provider keys are only cached for active users
-      await this.activeUserProviderKeysState.forceValue({});
-    } else {
+
+    if (!memoryOnly) {
       if (userId == null && activeUserId == null) {
         // nothing to do
         return;
@@ -522,6 +522,12 @@ export class CryptoService implements CryptoServiceAbstraction {
       await this.stateProvider
         .getUser(userId ?? activeUserId, USER_ENCRYPTED_PROVIDER_KEYS)
         .update(() => null);
+      return;
+    }
+
+    // provider keys are only cached for active users
+    if (userIdIsActive) {
+      await this.activeUserProviderKeysState.forceValue({});
     }
   }
 
@@ -578,20 +584,22 @@ export class CryptoService implements CryptoServiceAbstraction {
   async clearKeyPair(memoryOnly?: boolean, userId?: UserId): Promise<void[]> {
     const activeUserId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
     const userIdIsActive = userId == null || userId === activeUserId;
-    if (memoryOnly && userIdIsActive) {
-      // key pair is only cached for active users
-      await this.activeUserPrivateKeyState.forceValue(null);
-      await this.activeUserPublicKeyState.forceValue(null);
-      return;
-    } else {
+
+    if (!memoryOnly) {
       if (userId == null && activeUserId == null) {
         // nothing to do
         return;
       }
-      // below updates decrypted private key and public keys if this is the active user as well since those are derived from the encrypted private key
       await this.stateProvider
         .getUser(userId ?? activeUserId, USER_ENCRYPTED_PRIVATE_KEY)
         .update(() => null);
+      return;
+    }
+
+    // decrypted key pair is only cached for active users
+    if (userIdIsActive) {
+      await this.activeUserPrivateKeyState.forceValue(null);
+      await this.activeUserPublicKeyState.forceValue(null);
     }
   }