From ea0f5fa7714a16cd1c83698f2f40ead8e9a27eb5 Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Wed, 28 Feb 2024 14:14:07 -0500 Subject: [PATCH] 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 --- .../platform/services/crypto.service.spec.ts | 222 +++++++++++++++++- .../src/platform/services/crypto.service.ts | 44 ++-- 2 files changed, 247 insertions(+), 19 deletions(-) diff --git a/libs/common/src/platform/services/crypto.service.spec.ts b/libs/common/src/platform/services/crypto.service.spec.ts index a8fa48f36f..3f3d3f6636 100644 --- a/libs/common/src/platform/services/crypto.service.spec.ts +++ b/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); + }); + }); }); diff --git a/libs/common/src/platform/services/crypto.service.ts b/libs/common/src/platform/services/crypto.service.ts index 82dd7ce5c2..9bfa6d661b 100644 --- a/libs/common/src/platform/services/crypto.service.ts +++ b/libs/common/src/platform/services/crypto.service.ts @@ -467,10 +467,8 @@ export class CryptoService implements CryptoServiceAbstraction { async clearOrgKeys(memoryOnly?: boolean, userId?: UserId): Promise { 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 { - // 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 { 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 { 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); } }