From 56bffb04bb3171fefc2801c84c6651d126955df9 Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Thu, 22 Feb 2024 15:07:26 -0500 Subject: [PATCH] Ps/pm 5533/migrate decrypted user key (#7970) * Move user key memory state to state providers Note: state service observable change is because these updates are no longer internal to the class, but reporter directly to account service through crypto service on update of a user key * remove decrypted user key state Note, we're going to move the encrypted cryptoSymmetric key (and associated master key encrypted user keys) as part of the master key service creation. Crypto service will no longer be responsible for the encrypted forms of user key. * Deprecate notices belong on abstraction * Allow for single-direction status updates This is necessary since we don't want to have to guarantee that the update to logged out occurs after the update to locked. * Remove deprecated subject It turns out the set for cryptoMasterKey was also unused :hooray: --- apps/browser/jest.config.js | 9 +- .../services/browser-crypto.service.ts | 9 +- .../services/browser-state.service.spec.ts | 12 +- .../src/popup/services/services.module.ts | 34 ++++- libs/common/spec/fake-account-service.ts | 4 + libs/common/spec/fake-state-provider.ts | 3 + .../src/auth/abstractions/account.service.ts | 11 ++ .../src/auth/services/account.service.spec.ts | 22 +++ .../src/auth/services/account.service.ts | 18 +++ ...ice-trust-crypto.service.implementation.ts | 4 +- .../device-trust-crypto.service.spec.ts | 7 +- .../platform/abstractions/crypto.service.ts | 3 + .../platform/abstractions/state.service.ts | 17 +-- .../src/platform/models/domain/account.ts | 4 +- .../platform/services/crypto.service.spec.ts | 81 +++++++++-- .../src/platform/services/crypto.service.ts | 29 ++-- .../services/key-state/user-key.state.ts | 8 +- .../src/platform/services/state.service.ts | 131 ++---------------- .../src/platform/state/state-definitions.ts | 1 + 19 files changed, 224 insertions(+), 183 deletions(-) diff --git a/apps/browser/jest.config.js b/apps/browser/jest.config.js index cde02cd995..73f5ada287 100644 --- a/apps/browser/jest.config.js +++ b/apps/browser/jest.config.js @@ -9,7 +9,10 @@ module.exports = { ...sharedConfig, preset: "jest-preset-angular", setupFilesAfterEnv: ["/test.setup.ts"], - moduleNameMapper: pathsToModuleNameMapper(compilerOptions?.paths || {}, { - prefix: "/", - }), + moduleNameMapper: pathsToModuleNameMapper( + { "@bitwarden/common/spec": ["../../libs/common/spec"], ...(compilerOptions?.paths ?? {}) }, + { + prefix: "/", + }, + ), }; diff --git a/apps/browser/src/platform/services/browser-crypto.service.ts b/apps/browser/src/platform/services/browser-crypto.service.ts index 36ee4c6717..50cf5a7d75 100644 --- a/apps/browser/src/platform/services/browser-crypto.service.ts +++ b/apps/browser/src/platform/services/browser-crypto.service.ts @@ -1,7 +1,8 @@ +import { firstValueFrom } from "rxjs"; + import { KeySuffixOptions } from "@bitwarden/common/platform/enums"; -import { Utils } from "@bitwarden/common/platform/misc/utils"; -import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; import { CryptoService } from "@bitwarden/common/platform/services/crypto.service"; +import { USER_KEY } from "@bitwarden/common/platform/services/key-state/user-key.state"; import { UserId } from "@bitwarden/common/types/guid"; import { UserKey } from "@bitwarden/common/types/key"; @@ -29,9 +30,9 @@ export class BrowserCryptoService extends CryptoService { return null; } - const userKey = await this.stateService.getUserKey({ userId: userId }); + const userKey = await firstValueFrom(this.stateProvider.getUserState$(USER_KEY, userId)); if (userKey) { - return new SymmetricCryptoKey(Utils.fromB64ToArray(userKey.keyB64)) as UserKey; + return userKey; } } diff --git a/apps/browser/src/platform/services/browser-state.service.spec.ts b/apps/browser/src/platform/services/browser-state.service.spec.ts index 0cb7e4fd44..b5d1b9c38a 100644 --- a/apps/browser/src/platform/services/browser-state.service.spec.ts +++ b/apps/browser/src/platform/services/browser-state.service.spec.ts @@ -1,6 +1,5 @@ import { mock, MockProxy } from "jest-mock-extended"; -import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { @@ -11,8 +10,10 @@ import { StateFactory } from "@bitwarden/common/platform/factories/state-factory import { GlobalState } from "@bitwarden/common/platform/models/domain/global-state"; import { State } from "@bitwarden/common/platform/models/domain/state"; import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner"; +import { mockAccountServiceWith } from "@bitwarden/common/spec"; import { SendType } from "@bitwarden/common/tools/send/enums/send-type"; import { SendView } from "@bitwarden/common/tools/send/models/view/send.view"; +import { UserId } from "@bitwarden/common/types/guid"; import { Account } from "../../models/account"; import { BrowserComponentState } from "../../models/browserComponentState"; @@ -30,12 +31,12 @@ describe("Browser State Service", () => { let logService: MockProxy; let stateFactory: MockProxy>; let useAccountCache: boolean; - let accountService: MockProxy; let environmentService: MockProxy; let migrationRunner: MockProxy; let state: State; - const userId = "userId"; + const userId = "userId" as UserId; + const accountService = mockAccountServiceWith(userId); let sut: BrowserStateService; @@ -44,7 +45,6 @@ describe("Browser State Service", () => { diskStorageService = mock(); logService = mock(); stateFactory = mock(); - accountService = mock(); environmentService = mock(); migrationRunner = mock(); // turn off account cache for tests @@ -57,6 +57,10 @@ describe("Browser State Service", () => { state.activeUserId = userId; }); + afterEach(() => { + jest.resetAllMocks(); + }); + describe("state methods", () => { let memoryStorageService: MockProxy; diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index 175f082163..c76e34e44d 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -73,6 +73,7 @@ import { ConfigService } from "@bitwarden/common/platform/services/config/config import { ConsoleLogService } from "@bitwarden/common/platform/services/console-log.service"; import { ContainerService } from "@bitwarden/common/platform/services/container.service"; import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner"; +import { WebCryptoFunctionService } from "@bitwarden/common/platform/services/web-crypto-function.service"; import { DerivedStateProvider, StateProvider } from "@bitwarden/common/platform/state"; import { SearchService } from "@bitwarden/common/services/search.service"; import { PasswordGenerationServiceAbstraction } from "@bitwarden/common/tools/generator/password"; @@ -109,6 +110,7 @@ import { BrowserApi } from "../../platform/browser/browser-api"; import BrowserPopupUtils from "../../platform/popup/browser-popup-utils"; import { BrowserStateService as StateServiceAbstraction } from "../../platform/services/abstractions/browser-state.service"; import { BrowserConfigService } from "../../platform/services/browser-config.service"; +import { BrowserCryptoService } from "../../platform/services/browser-crypto.service"; import { BrowserEnvironmentService } from "../../platform/services/browser-environment.service"; import { BrowserFileDownloadService } from "../../platform/services/browser-file-download.service"; import { BrowserI18nService } from "../../platform/services/browser-i18n.service"; @@ -210,7 +212,7 @@ function getBgService(service: keyof MainBackground) { { provide: CipherService, useFactory: getBgService("cipherService"), deps: [] }, { provide: CryptoFunctionService, - useFactory: getBgService("cryptoFunctionService"), + useFactory: () => new WebCryptoFunctionService(window), deps: [], }, { @@ -258,12 +260,36 @@ function getBgService(service: keyof MainBackground) { }, { provide: CryptoService, - useFactory: (encryptService: EncryptService) => { - const cryptoService = getBgService("cryptoService")(); + useFactory: ( + cryptoFunctionService: CryptoFunctionService, + encryptService: EncryptService, + platformUtilsService: PlatformUtilsService, + logService: LogServiceAbstraction, + stateService: StateServiceAbstraction, + accountService: AccountServiceAbstraction, + stateProvider: StateProvider, + ) => { + const cryptoService = new BrowserCryptoService( + cryptoFunctionService, + encryptService, + platformUtilsService, + logService, + stateService, + accountService, + stateProvider, + ); new ContainerService(cryptoService, encryptService).attachToGlobal(self); return cryptoService; }, - deps: [EncryptService], + deps: [ + CryptoFunctionService, + EncryptService, + PlatformUtilsService, + LogServiceAbstraction, + StateServiceAbstraction, + AccountServiceAbstraction, + StateProvider, + ], }, { provide: AuthRequestCryptoServiceAbstraction, diff --git a/libs/common/spec/fake-account-service.ts b/libs/common/spec/fake-account-service.ts index ed6ba6e031..1364127f65 100644 --- a/libs/common/spec/fake-account-service.ts +++ b/libs/common/spec/fake-account-service.ts @@ -65,6 +65,10 @@ export class FakeAccountService implements AccountService { await this.mock.setAccountStatus(userId, status); } + async setMaxAccountStatus(userId: UserId, maxStatus: AuthenticationStatus): Promise { + await this.mock.setMaxAccountStatus(userId, maxStatus); + } + async switchAccount(userId: UserId): Promise { await this.mock.switchAccount(userId); } diff --git a/libs/common/spec/fake-state-provider.ts b/libs/common/spec/fake-state-provider.ts index dabfbb34e0..c87561c964 100644 --- a/libs/common/spec/fake-state-provider.ts +++ b/libs/common/spec/fake-state-provider.ts @@ -140,7 +140,9 @@ export class FakeActiveUserStateProvider implements ActiveUserStateProvider { } export class FakeStateProvider implements StateProvider { + mock = mock(); getUserState$(keyDefinition: KeyDefinition, userId?: UserId): Observable { + this.mock.getUserState$(keyDefinition, userId); if (userId) { return this.getUser(userId, keyDefinition).state$; } @@ -152,6 +154,7 @@ export class FakeStateProvider implements StateProvider { value: T, userId?: UserId, ): Promise<[UserId, T]> { + await this.mock.setUserState(keyDefinition, value, userId); if (userId) { return [userId, await this.getUser(userId, keyDefinition).update(() => value)]; } else { diff --git a/libs/common/src/auth/abstractions/account.service.ts b/libs/common/src/auth/abstractions/account.service.ts index ca9e82335f..4e2a462755 100644 --- a/libs/common/src/auth/abstractions/account.service.ts +++ b/libs/common/src/auth/abstractions/account.service.ts @@ -47,6 +47,17 @@ export abstract class AccountService { * @param status */ abstract setAccountStatus(userId: UserId, status: AuthenticationStatus): Promise; + /** + * Updates the `accounts$` observable with the new account status if the current status is higher than the `maxStatus`. + * + * This method only downgrades status to the maximum value sent in, it will not increase authentication status. + * + * @example An account is transitioning from unlocked to logged out. If callbacks that set the status to locked occur + * after it is updated to logged out, the account will be in the incorrect state. + * @param userId The user id of the account to be updated. + * @param maxStatus The new status of the account. + */ + abstract setMaxAccountStatus(userId: UserId, maxStatus: AuthenticationStatus): Promise; /** * Updates the `activeAccount$` observable with the new active account. * @param userId diff --git a/libs/common/src/auth/services/account.service.spec.ts b/libs/common/src/auth/services/account.service.spec.ts index 4df1a39d6d..e4195365f4 100644 --- a/libs/common/src/auth/services/account.service.spec.ts +++ b/libs/common/src/auth/services/account.service.spec.ts @@ -207,4 +207,26 @@ describe("accountService", () => { expect(sut.switchAccount("unknown" as UserId)).rejects.toThrowError("Account does not exist"); }); }); + + describe("setMaxAccountStatus", () => { + it("should update the account", async () => { + accountsState.stateSubject.next({ [userId]: userInfo(AuthenticationStatus.Unlocked) }); + await sut.setMaxAccountStatus(userId, AuthenticationStatus.Locked); + const currentState = await firstValueFrom(accountsState.state$); + + expect(currentState).toEqual({ + [userId]: userInfo(AuthenticationStatus.Locked), + }); + }); + + it("should not update if the new max status is higher than the current", async () => { + accountsState.stateSubject.next({ [userId]: userInfo(AuthenticationStatus.LoggedOut) }); + await sut.setMaxAccountStatus(userId, AuthenticationStatus.Locked); + const currentState = await firstValueFrom(accountsState.state$); + + expect(currentState).toEqual({ + [userId]: userInfo(AuthenticationStatus.LoggedOut), + }); + }); + }); }); diff --git a/libs/common/src/auth/services/account.service.ts b/libs/common/src/auth/services/account.service.ts index 34c8279a46..8ef235d815 100644 --- a/libs/common/src/auth/services/account.service.ts +++ b/libs/common/src/auth/services/account.service.ts @@ -84,6 +84,24 @@ export class AccountServiceImplementation implements InternalAccountService { } } + async setMaxAccountStatus(userId: UserId, maxStatus: AuthenticationStatus): Promise { + await this.accountsState.update( + (accounts) => { + accounts[userId].status = maxStatus; + return accounts; + }, + { + shouldUpdate: (accounts) => { + if (accounts?.[userId] == null) { + throw new Error("Account does not exist"); + } + + return accounts[userId].status > maxStatus; + }, + }, + ); + } + async switchAccount(userId: UserId): Promise { await this.activeAccountIdState.update( (_, accounts) => { diff --git a/libs/common/src/auth/services/device-trust-crypto.service.implementation.ts b/libs/common/src/auth/services/device-trust-crypto.service.implementation.ts index d03e84c704..a5abac5e82 100644 --- a/libs/common/src/auth/services/device-trust-crypto.service.implementation.ts +++ b/libs/common/src/auth/services/device-trust-crypto.service.implementation.ts @@ -1,3 +1,5 @@ +import { firstValueFrom } from "rxjs"; + import { AppIdService } from "../../platform/abstractions/app-id.service"; import { CryptoFunctionService } from "../../platform/abstractions/crypto-function.service"; import { CryptoService } from "../../platform/abstractions/crypto.service"; @@ -108,7 +110,7 @@ export class DeviceTrustCryptoService implements DeviceTrustCryptoServiceAbstrac } // At this point of rotating their keys, they should still have their old user key in state - const oldUserKey = await this.stateService.getUserKey(); + const oldUserKey = await firstValueFrom(this.cryptoService.activeUserKey$); const deviceIdentifier = await this.appIdService.getAppId(); const secretVerificationRequest = new SecretVerificationRequest(); diff --git a/libs/common/src/auth/services/device-trust-crypto.service.spec.ts b/libs/common/src/auth/services/device-trust-crypto.service.spec.ts index 1093b36184..0fbb954fb3 100644 --- a/libs/common/src/auth/services/device-trust-crypto.service.spec.ts +++ b/libs/common/src/auth/services/device-trust-crypto.service.spec.ts @@ -1,4 +1,5 @@ import { matches, mock } from "jest-mock-extended"; +import { of } from "rxjs"; import { DeviceType } from "../../enums"; import { AppIdService } from "../../platform/abstractions/app-id.service"; @@ -19,6 +20,7 @@ import { UpdateDevicesTrustRequest } from "../models/request/update-devices-trus import { ProtectedDeviceResponse } from "../models/response/protected-device.response"; import { DeviceTrustCryptoService } from "./device-trust-crypto.service.implementation"; + describe("deviceTrustCryptoService", () => { let deviceTrustCryptoService: DeviceTrustCryptoService; @@ -495,6 +497,7 @@ describe("deviceTrustCryptoService", () => { const fakeNewUserKeyData = new Uint8Array(64); fakeNewUserKeyData.fill(FakeNewUserKeyMarker, 0, 1); fakeNewUserKey = new SymmetricCryptoKey(fakeNewUserKeyData) as UserKey; + cryptoService.activeUserKey$ = of(fakeNewUserKey); }); it("does an early exit when the current device is not a trusted device", async () => { @@ -521,9 +524,7 @@ describe("deviceTrustCryptoService", () => { fakeOldUserKeyData.fill(FakeOldUserKeyMarker, 0, 1); // Mock the retrieval of a user key that differs from the new one passed into the method - stateService.getUserKey.mockResolvedValue( - new SymmetricCryptoKey(fakeOldUserKeyData) as UserKey, - ); + cryptoService.activeUserKey$ = of(new SymmetricCryptoKey(fakeOldUserKeyData) as UserKey); appIdService.getAppId.mockResolvedValue("test_device_identifier"); diff --git a/libs/common/src/platform/abstractions/crypto.service.ts b/libs/common/src/platform/abstractions/crypto.service.ts index 6e7a2ba771..21ddccb9d6 100644 --- a/libs/common/src/platform/abstractions/crypto.service.ts +++ b/libs/common/src/platform/abstractions/crypto.service.ts @@ -12,10 +12,13 @@ import { EncString } from "../models/domain/enc-string"; import { SymmetricCryptoKey } from "../models/domain/symmetric-crypto-key"; export abstract class CryptoService { + activeUserKey$: Observable; /** * Sets the provided user key and stores * any other necessary versions (such as auto, biometrics, * or pin) + * + * @throws when key is null. Use {@link clearUserKey} instead * @param key The user key to set * @param userId The desired user */ diff --git a/libs/common/src/platform/abstractions/state.service.ts b/libs/common/src/platform/abstractions/state.service.ts index d905c6ce97..8de781674f 100644 --- a/libs/common/src/platform/abstractions/state.service.ts +++ b/libs/common/src/platform/abstractions/state.service.ts @@ -16,7 +16,7 @@ import { UsernameGeneratorOptions } from "../../tools/generator/username"; import { SendData } from "../../tools/send/models/data/send.data"; import { SendView } from "../../tools/send/models/view/send.view"; import { UserId } from "../../types/guid"; -import { DeviceKey, MasterKey, UserKey } from "../../types/key"; +import { DeviceKey, MasterKey } from "../../types/key"; import { UriMatchType } from "../../vault/enums"; import { CipherData } from "../../vault/models/data/cipher.data"; import { LocalData } from "../../vault/models/data/local.data"; @@ -50,6 +50,9 @@ export type InitOptions = { export abstract class StateService { accounts$: Observable<{ [userId: string]: T }>; activeAccount$: Observable; + /** + * @deprecated use accountService.activeAccount$ instead + */ activeAccountUnlocked$: Observable; addAccount: (account: T) => Promise; @@ -82,14 +85,6 @@ export abstract class StateService { setClearClipboard: (value: number, options?: StorageOptions) => Promise; getConvertAccountToKeyConnector: (options?: StorageOptions) => Promise; setConvertAccountToKeyConnector: (value: boolean, options?: StorageOptions) => Promise; - /** - * gets the user key - */ - getUserKey: (options?: StorageOptions) => Promise; - /** - * Sets the user key - */ - setUserKey: (value: UserKey, options?: StorageOptions) => Promise; /** * Gets the user's master key */ @@ -150,10 +145,6 @@ export abstract class StateService { * @deprecated For migration purposes only, use getUserKeyMasterKey instead */ getEncryptedCryptoSymmetricKey: (options?: StorageOptions) => Promise; - /** - * @deprecated For migration purposes only, use setUserKeyMasterKey instead - */ - setEncryptedCryptoSymmetricKey: (value: string, options?: StorageOptions) => Promise; /** * @deprecated For legacy purposes only, use getMasterKey instead */ diff --git a/libs/common/src/platform/models/domain/account.ts b/libs/common/src/platform/models/domain/account.ts index 53e5fd4465..40fc2e623b 100644 --- a/libs/common/src/platform/models/domain/account.ts +++ b/libs/common/src/platform/models/domain/account.ts @@ -19,7 +19,7 @@ import { UsernameGeneratorOptions } from "../../../tools/generator/username/user import { SendData } from "../../../tools/send/models/data/send.data"; import { SendView } from "../../../tools/send/models/view/send.view"; import { DeepJsonify } from "../../../types/deep-jsonify"; -import { MasterKey, UserKey } from "../../../types/key"; +import { MasterKey } from "../../../types/key"; import { UriMatchType } from "../../../vault/enums"; import { CipherData } from "../../../vault/models/data/cipher.data"; import { CipherView } from "../../../vault/models/view/cipher.view"; @@ -113,7 +113,6 @@ export class AccountData { } export class AccountKeys { - userKey?: UserKey; masterKey?: MasterKey; masterKeyEncryptedUserKey?: string; deviceKey?: ReturnType; @@ -146,7 +145,6 @@ export class AccountKeys { return null; } return Object.assign(new AccountKeys(), obj, { - userKey: SymmetricCryptoKey.fromJSON(obj?.userKey), masterKey: SymmetricCryptoKey.fromJSON(obj?.masterKey), deviceKey: obj?.deviceKey, cryptoMasterKey: SymmetricCryptoKey.fromJSON(obj?.cryptoMasterKey), diff --git a/libs/common/src/platform/services/crypto.service.spec.ts b/libs/common/src/platform/services/crypto.service.spec.ts index 924a8ba269..fd4e5b92c7 100644 --- a/libs/common/src/platform/services/crypto.service.spec.ts +++ b/libs/common/src/platform/services/crypto.service.spec.ts @@ -4,6 +4,7 @@ import { firstValueFrom } from "rxjs"; import { FakeAccountService, mockAccountServiceWith } from "../../../spec/fake-account-service"; import { FakeActiveUserState, FakeSingleUserState } from "../../../spec/fake-state"; import { FakeStateProvider } from "../../../spec/fake-state-provider"; +import { AuthenticationStatus } from "../../auth/enums/authentication-status"; import { CsprngArray } from "../../types/csprng"; import { UserId } from "../../types/guid"; import { UserKey, MasterKey, PinKey } from "../../types/key"; @@ -17,7 +18,7 @@ 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 } from "./key-state/user-key.state"; +import { USER_EVER_HAD_USER_KEY, USER_KEY } from "./key-state/user-key.state"; describe("cryptoService", () => { let cryptoService: CryptoService; @@ -57,42 +58,50 @@ describe("cryptoService", () => { describe("getUserKey", () => { let mockUserKey: UserKey; - let stateSvcGetUserKey: jest.SpyInstance; beforeEach(() => { const mockRandomBytes = new Uint8Array(64) as CsprngArray; mockUserKey = new SymmetricCryptoKey(mockRandomBytes) as UserKey; + }); - stateSvcGetUserKey = jest.spyOn(stateService, "getUserKey"); + it("retrieves the key state of the requested user", async () => { + await cryptoService.getUserKey(mockUserId); + + expect(stateProvider.mock.getUserState$).toHaveBeenCalledWith(USER_KEY, mockUserId); }); it("returns the User Key if available", async () => { - stateSvcGetUserKey.mockResolvedValue(mockUserKey); + stateProvider.singleUser.getFake(mockUserId, USER_KEY).nextState(mockUserKey); const userKey = await cryptoService.getUserKey(mockUserId); - expect(stateSvcGetUserKey).toHaveBeenCalledWith({ userId: mockUserId }); expect(userKey).toEqual(mockUserKey); }); - it("sets the Auto key if the User Key if not set", async () => { + it("sets from the Auto key if the User Key if not set", async () => { const autoKeyB64 = "IT5cA1i5Hncd953pb00E58D2FqJX+fWTj4AvoI67qkGHSQPgulAqKv+LaKRAo9Bg0xzP9Nw00wk4TqjMmGSM+g=="; stateService.getUserKeyAutoUnlock.mockResolvedValue(autoKeyB64); + const setKeySpy = jest.spyOn(cryptoService, "setUserKey"); const userKey = await cryptoService.getUserKey(mockUserId); - expect(stateService.setUserKey).toHaveBeenCalledWith(expect.any(SymmetricCryptoKey), { - userId: mockUserId, - }); + expect(setKeySpy).toHaveBeenCalledWith(expect.any(SymmetricCryptoKey), mockUserId); + expect(setKeySpy).toHaveBeenCalledTimes(1); + expect(userKey.keyB64).toEqual(autoKeyB64); }); + + it("returns nullish if there is no auto key and the user key is not set", async () => { + const userKey = await cryptoService.getUserKey(mockUserId); + + expect(userKey).toBeFalsy(); + }); }); describe("getUserKeyWithLegacySupport", () => { let mockUserKey: UserKey; let mockMasterKey: MasterKey; - let stateSvcGetUserKey: jest.SpyInstance; let stateSvcGetMasterKey: jest.SpyInstance; beforeEach(() => { @@ -100,23 +109,22 @@ describe("cryptoService", () => { mockUserKey = new SymmetricCryptoKey(mockRandomBytes) as UserKey; mockMasterKey = new SymmetricCryptoKey(new Uint8Array(64) as CsprngArray) as MasterKey; - stateSvcGetUserKey = jest.spyOn(stateService, "getUserKey"); stateSvcGetMasterKey = jest.spyOn(stateService, "getMasterKey"); }); it("returns the User Key if available", async () => { - stateSvcGetUserKey.mockResolvedValue(mockUserKey); + stateProvider.singleUser.getFake(mockUserId, USER_KEY).nextState(mockUserKey); + const getKeySpy = jest.spyOn(cryptoService, "getUserKey"); const userKey = await cryptoService.getUserKeyWithLegacySupport(mockUserId); - expect(stateSvcGetUserKey).toHaveBeenCalledWith({ userId: mockUserId }); + expect(getKeySpy).toHaveBeenCalledWith(mockUserId); expect(stateSvcGetMasterKey).not.toHaveBeenCalled(); expect(userKey).toEqual(mockUserKey); }); it("returns the user's master key when User Key is not available", async () => { - stateSvcGetUserKey.mockResolvedValue(null); stateSvcGetMasterKey.mockResolvedValue(mockMasterKey); const userKey = await cryptoService.getUserKeyWithLegacySupport(mockUserId); @@ -201,6 +209,19 @@ describe("cryptoService", () => { }); }); + it("throws if key is null", async () => { + await expect(cryptoService.setUserKey(null, mockUserId)).rejects.toThrow("No key provided."); + }); + + it("should update the user's lock state", async () => { + await cryptoService.setUserKey(mockUserKey, mockUserId); + + expect(accountService.mock.setAccountStatus).toHaveBeenCalledWith( + mockUserId, + AuthenticationStatus.Unlocked, + ); + }); + describe("Pin Key refresh", () => { let cryptoSvcMakePinKey: jest.SpyInstance; const protectedPin = @@ -259,4 +280,36 @@ describe("cryptoService", () => { }); }); }); + + describe("clearUserKey", () => { + it.each([mockUserId, null])("should clear the User Key for id %2", async (userId) => { + await cryptoService.clearUserKey(false, userId); + + expect(stateProvider.mock.setUserState).toHaveBeenCalledWith(USER_KEY, null, userId); + }); + + it("should update status to locked", async () => { + await cryptoService.clearUserKey(false, mockUserId); + + expect(accountService.mock.setMaxAccountStatus).toHaveBeenCalledWith( + mockUserId, + AuthenticationStatus.Locked, + ); + }); + + it.each([true, false])( + "should clear stored user keys if clearAll is true (%s)", + async (clear) => { + const clearSpy = (cryptoService["clearAllStoredUserKeys"] = jest.fn()); + await cryptoService.clearUserKey(clear, mockUserId); + + if (clear) { + expect(clearSpy).toHaveBeenCalledWith(mockUserId); + expect(clearSpy).toHaveBeenCalledTimes(1); + } else { + expect(clearSpy).not.toHaveBeenCalled(); + } + }, + ); + }); }); diff --git a/libs/common/src/platform/services/crypto.service.ts b/libs/common/src/platform/services/crypto.service.ts index 2991b6e57a..83faf4f7d3 100644 --- a/libs/common/src/platform/services/crypto.service.ts +++ b/libs/common/src/platform/services/crypto.service.ts @@ -6,6 +6,7 @@ import { ProfileOrganizationResponse } from "../../admin-console/models/response import { ProfileProviderOrganizationResponse } from "../../admin-console/models/response/profile-provider-organization.response"; import { ProfileProviderResponse } from "../../admin-console/models/response/profile-provider.response"; import { AccountService } from "../../auth/abstractions/account.service"; +import { AuthenticationStatus } from "../../auth/enums/authentication-status"; import { KdfConfig } from "../../auth/models/domain/kdf-config"; import { Utils } from "../../platform/misc/utils"; import { OrganizationId, ProviderId, UserId } from "../../types/guid"; @@ -52,9 +53,11 @@ import { USER_EVER_HAD_USER_KEY, USER_PRIVATE_KEY, USER_PUBLIC_KEY, + USER_KEY, } from "./key-state/user-key.state"; export class CryptoService implements CryptoServiceAbstraction { + private readonly activeUserKeyState: ActiveUserState; private readonly activeUserEverHadUserKey: ActiveUserState; private readonly activeUserEncryptedOrgKeysState: ActiveUserState< Record @@ -68,6 +71,8 @@ export class CryptoService implements CryptoServiceAbstraction { private readonly activeUserPrivateKeyState: DerivedState; private readonly activeUserPublicKeyState: DerivedState; + readonly activeUserKey$: Observable; + readonly activeUserOrgKeys$: Observable>; readonly activeUserProviderKeys$: Observable>; readonly activeUserPrivateKey$: Observable; @@ -84,6 +89,8 @@ export class CryptoService implements CryptoServiceAbstraction { protected stateProvider: StateProvider, ) { // User Key + this.activeUserKeyState = stateProvider.getActive(USER_KEY); + this.activeUserKey$ = this.activeUserKeyState.state$; this.activeUserEverHadUserKey = stateProvider.getActive(USER_EVER_HAD_USER_KEY); this.everHadUserKey$ = this.activeUserEverHadUserKey.state$.pipe(map((x) => x ?? false)); @@ -131,13 +138,15 @@ export class CryptoService implements CryptoServiceAbstraction { } async setUserKey(key: UserKey, userId?: UserId): Promise { - // TODO: make this non-nullable in signature - userId ??= (await firstValueFrom(this.accountService.activeAccount$))?.id; - if (key != null) { - // Key should never be null anyway - await this.stateProvider.getUser(userId, USER_EVER_HAD_USER_KEY).update(() => true); + if (key == null) { + throw new Error("No key provided. Use ClearUserKey to clear the key"); } - await this.stateService.setUserKey(key, { userId: userId }); + // Set userId to ensure we have one for the account status update + [userId, key] = await this.stateProvider.setUserState(USER_KEY, key, userId); + await this.stateProvider.setUserState(USER_EVER_HAD_USER_KEY, true, userId); + + await this.accountService.setAccountStatus(userId, AuthenticationStatus.Unlocked); + await this.storeAdditionalKeys(key, userId); } @@ -147,7 +156,7 @@ export class CryptoService implements CryptoServiceAbstraction { } async getUserKey(userId?: UserId): Promise { - let userKey = await this.stateService.getUserKey({ userId: userId }); + let userKey = await firstValueFrom(this.stateProvider.getUserState$(USER_KEY, userId)); if (userKey) { return userKey; } @@ -197,7 +206,7 @@ export class CryptoService implements CryptoServiceAbstraction { } async hasUserKeyInMemory(userId?: UserId): Promise { - return (await this.stateService.getUserKey({ userId: userId })) != null; + return (await firstValueFrom(this.stateProvider.getUserState$(USER_KEY, userId))) != null; } async hasUserKeyStored(keySuffix: KeySuffixOptions, userId?: UserId): Promise { @@ -215,7 +224,9 @@ export class CryptoService implements CryptoServiceAbstraction { } async clearUserKey(clearStoredKeys = true, userId?: UserId): Promise { - await this.stateService.setUserKey(null, { userId: userId }); + // Set userId to ensure we have one for the account status update + [userId] = await this.stateProvider.setUserState(USER_KEY, null, userId); + await this.accountService.setMaxAccountStatus(userId, AuthenticationStatus.Locked); if (clearStoredKeys) { await this.clearAllStoredUserKeys(userId); } diff --git a/libs/common/src/platform/services/key-state/user-key.state.ts b/libs/common/src/platform/services/key-state/user-key.state.ts index a6d1619e94..d0f54c9add 100644 --- a/libs/common/src/platform/services/key-state/user-key.state.ts +++ b/libs/common/src/platform/services/key-state/user-key.state.ts @@ -1,8 +1,9 @@ -import { UserPrivateKey, UserPublicKey } from "../../../types/key"; +import { UserPrivateKey, UserPublicKey, UserKey } from "../../../types/key"; import { CryptoFunctionService } from "../../abstractions/crypto-function.service"; import { EncryptService } from "../../abstractions/encrypt.service"; import { EncString, EncryptedString } from "../../models/domain/enc-string"; -import { KeyDefinition, CRYPTO_DISK, DeriveDefinition } from "../../state"; +import { SymmetricCryptoKey } from "../../models/domain/symmetric-crypto-key"; +import { KeyDefinition, CRYPTO_DISK, DeriveDefinition, CRYPTO_MEMORY } from "../../state"; import { CryptoService } from "../crypto.service"; export const USER_EVER_HAD_USER_KEY = new KeyDefinition(CRYPTO_DISK, "everHadUserKey", { @@ -57,3 +58,6 @@ export const USER_PUBLIC_KEY = DeriveDefinition.from< return (await cryptoFunctionService.rsaExtractPublicKey(privateKey)) as UserPublicKey; }, }); +export const USER_KEY = new KeyDefinition(CRYPTO_MEMORY, "userKey", { + deserializer: (obj) => SymmetricCryptoKey.fromJSON(obj) as UserKey, +}); diff --git a/libs/common/src/platform/services/state.service.ts b/libs/common/src/platform/services/state.service.ts index 6a9abf2be0..96005ef394 100644 --- a/libs/common/src/platform/services/state.service.ts +++ b/libs/common/src/platform/services/state.service.ts @@ -1,4 +1,4 @@ -import { BehaviorSubject, concatMap } from "rxjs"; +import { BehaviorSubject, Observable, map } from "rxjs"; import { Jsonify, JsonValue } from "type-fest"; import { OrganizationData } from "../../admin-console/models/data/organization.data"; @@ -20,7 +20,7 @@ import { UsernameGeneratorOptions } from "../../tools/generator/username"; import { SendData } from "../../tools/send/models/data/send.data"; import { SendView } from "../../tools/send/models/view/send.view"; import { UserId } from "../../types/guid"; -import { DeviceKey, MasterKey, UserKey } from "../../types/key"; +import { DeviceKey, MasterKey } from "../../types/key"; import { UriMatchType } from "../../vault/enums"; import { CipherData } from "../../vault/models/data/cipher.data"; import { LocalData } from "../../vault/models/data/local.data"; @@ -87,8 +87,7 @@ export class StateService< protected activeAccountSubject = new BehaviorSubject(null); activeAccount$ = this.activeAccountSubject.asObservable(); - protected activeAccountUnlockedSubject = new BehaviorSubject(false); - activeAccountUnlocked$ = this.activeAccountUnlockedSubject.asObservable(); + activeAccountUnlocked$: Observable; private hasBeenInited = false; protected isRecoveredSession = false; @@ -109,22 +108,11 @@ export class StateService< private migrationRunner: MigrationRunner, protected useAccountCache: boolean = true, ) { - // If the account gets changed, verify the new account is unlocked - this.activeAccountSubject - .pipe( - concatMap(async (userId) => { - if (userId == null && this.activeAccountUnlockedSubject.getValue() == false) { - return; - } else if (userId == null) { - this.activeAccountUnlockedSubject.next(false); - } - // FIXME: This should be refactored into AuthService or a similar service, - // as checking for the existence of the crypto key is a low level - // implementation detail. - this.activeAccountUnlockedSubject.next((await this.getUserKey()) != null); - }), - ) - .subscribe(); + this.activeAccountUnlocked$ = this.accountService.activeAccount$.pipe( + map((a) => { + return a?.status === AuthenticationStatus.Unlocked; + }), + ); } async init(initOptions: InitOptions = {}): Promise { @@ -522,68 +510,6 @@ export class StateService< return account?.keys?.cryptoMasterKey; } - /** - * @deprecated Do not save the Master Key. Use the User Symmetric Key instead - */ - async setCryptoMasterKey(value: SymmetricCryptoKey, options?: StorageOptions): Promise { - const account = await this.getAccount( - this.reconcileOptions(options, await this.defaultInMemoryOptions()), - ); - account.keys.cryptoMasterKey = value; - await this.saveAccount( - account, - this.reconcileOptions(options, await this.defaultInMemoryOptions()), - ); - - const nextStatus = value != null ? AuthenticationStatus.Unlocked : AuthenticationStatus.Locked; - await this.accountService.setAccountStatus(options.userId as UserId, nextStatus); - - if (options.userId == this.activeAccountSubject.getValue()) { - const nextValue = value != null; - - // Avoid emitting if we are already unlocked - if (this.activeAccountUnlockedSubject.getValue() != nextValue) { - this.activeAccountUnlockedSubject.next(nextValue); - } - } - } - - /** - * user key used to encrypt/decrypt data - */ - async getUserKey(options?: StorageOptions): Promise { - const account = await this.getAccount( - this.reconcileOptions(options, await this.defaultInMemoryOptions()), - ); - return account?.keys?.userKey as UserKey; - } - - /** - * user key used to encrypt/decrypt data - */ - async setUserKey(value: UserKey, options?: StorageOptions): Promise { - const account = await this.getAccount( - this.reconcileOptions(options, await this.defaultInMemoryOptions()), - ); - account.keys.userKey = value; - await this.saveAccount( - account, - this.reconcileOptions(options, await this.defaultInMemoryOptions()), - ); - - const nextStatus = value != null ? AuthenticationStatus.Unlocked : AuthenticationStatus.Locked; - await this.accountService.setAccountStatus(options.userId as UserId, nextStatus); - - if (options?.userId == this.activeAccountSubject.getValue()) { - const nextValue = value != null; - - // Avoid emitting if we are already unlocked - if (this.activeAccountUnlockedSubject.getValue() != nextValue) { - this.activeAccountUnlockedSubject.next(nextValue); - } - } - } - /** * User's master key derived from MP, saved only if we decrypted with MP */ @@ -885,33 +811,6 @@ export class StateService< ); } - /** - * @deprecated Use UserKey instead - */ - async getDecryptedCryptoSymmetricKey(options?: StorageOptions): Promise { - const account = await this.getAccount( - this.reconcileOptions(options, await this.defaultInMemoryOptions()), - ); - return account?.keys?.cryptoSymmetricKey?.decrypted; - } - - /** - * @deprecated Use UserKey instead - */ - async setDecryptedCryptoSymmetricKey( - value: SymmetricCryptoKey, - options?: StorageOptions, - ): Promise { - const account = await this.getAccount( - this.reconcileOptions(options, await this.defaultInMemoryOptions()), - ); - account.keys.cryptoSymmetricKey.decrypted = value; - await this.saveAccount( - account, - this.reconcileOptions(options, await this.defaultInMemoryOptions()), - ); - } - @withPrototypeForArrayMembers(GeneratedPasswordHistory) async getDecryptedPasswordGenerationHistory( options?: StorageOptions, @@ -1565,20 +1464,6 @@ export class StateService< )?.keys.cryptoSymmetricKey.encrypted; } - /** - * @deprecated Use UserKey instead - */ - async setEncryptedCryptoSymmetricKey(value: string, options?: StorageOptions): Promise { - const account = await this.getAccount( - this.reconcileOptions(options, await this.defaultOnDiskOptions()), - ); - account.keys.cryptoSymmetricKey.encrypted = value; - await this.saveAccount( - account, - this.reconcileOptions(options, await this.defaultOnDiskOptions()), - ); - } - @withPrototypeForArrayMembers(GeneratedPasswordHistory) async getEncryptedPasswordGenerationHistory( options?: StorageOptions, diff --git a/libs/common/src/platform/state/state-definitions.ts b/libs/common/src/platform/state/state-definitions.ts index 876399924c..ddfa19bce8 100644 --- a/libs/common/src/platform/state/state-definitions.ts +++ b/libs/common/src/platform/state/state-definitions.ts @@ -22,6 +22,7 @@ export const ACCOUNT_MEMORY = new StateDefinition("account", "memory"); export const BILLING_BANNERS_DISK = new StateDefinition("billingBanners", "disk"); export const CRYPTO_DISK = new StateDefinition("crypto", "disk"); +export const CRYPTO_MEMORY = new StateDefinition("crypto", "memory"); export const SSO_DISK = new StateDefinition("ssoLogin", "disk");