From c202c93378f1d48e15767d74e902e4888210cf26 Mon Sep 17 00:00:00 2001 From: Jared Snider <116684653+JaredSnider-Bitwarden@users.noreply.github.com> Date: Mon, 1 Apr 2024 16:02:58 -0400 Subject: [PATCH] Auth/PM-5268 - DeviceTrustCryptoService state provider migration (#7882) * PM-5268 - Add DEVICE_TRUST_DISK to state definitions * PM-5268 - DeviceTrustCryptoService - Get most of state provider refactor done - WIP - commented out stuff for now. * PM-5268 - DeviceTrustCryptoServiceStateProviderMigrator - WIP - got first draft of migrator in place and working on tests. Rollback tests are failing for some reason TBD. * PM-5268 - more WIP on device trust crypto service migrator tests * PM-5268 - DeviceTrustCryptoServiceStateProviderMigrator - Refactor based on call with platform * PM-5268 - DeviceTrustCryptoServiceStateProviderMigrator - tests passing * PM-5268 - Update DeviceTrustCryptoService to convert over to state providers + update all service instantiations / dependencies to ensure state provider is passed in or injected. * PM-5268 - Register new migration * PM-5268 - Temporarily remove device trust crypto service from migrator to ease merge conflicts as there are 6 more migrators before I can apply mine in main. * PM-5268 - Update migration numbers of DeviceTrustCryptoServiceStateProviderMigrator based on latest migrations from main. * PM-5268 - (1) Export new KeyDefinitions from DeviceTrustCryptoService for use in test suite (2) Update DeviceTrustCryptoService test file to use state provider. * PM-5268 - Fix DeviceTrustCryptoServiceStateProviderMigrator tests to use proper versions * PM-5268 - Actually fix all instances of DeviceTrustCryptoServiceStateProviderMigrator test failures * PM-5268 - Clean up state service, account, and login strategy of all migrated references * PM-5268 - Account - finish cleaning up device key * PM-5268 - StateService - clean up last reference to device key * PM-5268 - Remove even more device key refs. *facepalm* * PM-5268 - Finish resolving merge conflicts by incrementing migration version from 22 to 23 * PM-5268 - bump migration versions * PM-5268 - DeviceTrustCryptoService - Implement secure storage functionality for getDeviceKey and setDeviceKey (to achieve feature parity with the ElectronStateService implementation prior to the state provider migration). Tests to follow shortly. * PM-5268 - DeviceTrustCryptoService tests - getDeviceKey now tested with all new secure storage scenarios. SetDeviceKey tests to follow. * PM-5268 - DeviceTrustCryptoService tests - test all setDeviceKey scenarios with state provider & secure storage * PM-5268 - Update DeviceTrustCryptoService deps to actually use secure storage svc on platforms that support it. * PM-5268 - Bump migration version due to merge conflicts. * PM-5268 - Bump migration version * PM-5268 - tweak jsdocs to be single line per PR feedback * PM-5268 - DeviceTrustCryptoSvc - improve debuggability. * PM-5268 - Remove state service as a dependency on the device trust crypto service (woo!) * PM-5268 - Update migration test json to correctly reflect reality. * PM-5268 - DeviceTrustCryptoSvc - getDeviceKey - add throw error for active user id missing. * PM-5268 - Fix tests * PM-5268 - WIP start on adding user id to every method on device trust crypto service. * PM-5268 - Update lock comp dependencies across clients * PM-5268 - Update login via auth request deps across clients to add acct service. * PM-5268 - UserKeyRotationSvc - add acct service to get active acct id for call to rotateDevicesTrust and then update tests. * PM-5268 - WIP on trying to fix device trust crypto svc tests. * PM-5268 - More WIP device trust crypto svc tests passing * PM-5268 - Device Trust crypto service - get all tests passing * PM-5268 - DeviceTrustCryptoService.getDeviceKey - fix secure storage b64 to symmetric crypto key conversion * PM-5268 - Add more tests and update test names * PM-5268 - rename state to indicate it was disk local * PM-5268 - DeviceTrustCryptoService - save symmetric key in JSON format * PM-5268 - Fix lock comp tests by adding acct service dep * PM-5268 - Update set device key tests to pass * PM-5268 - Bump migration versions again * PM-5268 - Fix user key rotation svc tests * PM-5268 - Update web jest config to allow use of common spec in user-key-rotation-svc tests * PM-5268 - Bump migration version * PM-5268 - Per PR feedback, save off user id * PM-5268 - bump migration version * PM-5268 - Per PR feedback, remove unnecessary await. * PM-5268 - Bump migration verson --- .../device-trust-crypto-service.factory.ts | 16 +- apps/browser/src/auth/popup/lock.component.ts | 3 + .../popup/login-via-auth-request.component.ts | 3 + .../browser/src/background/main.background.ts | 3 +- apps/cli/src/bw.ts | 3 +- apps/desktop/src/auth/lock.component.spec.ts | 11 + apps/desktop/src/auth/lock.component.ts | 3 + .../login/login-via-auth-request.component.ts | 3 + .../services/electron-state.service.ts | 35 -- apps/web/jest.config.js | 10 +- .../user-key-rotation.service.spec.ts | 7 + .../key-rotation/user-key-rotation.service.ts | 9 +- apps/web/src/app/auth/lock.component.ts | 3 + ...base-login-decryption-options.component.ts | 16 +- .../src/auth/components/lock.component.ts | 5 +- .../login-via-auth-request.component.ts | 7 +- .../src/services/jslib-services.module.ts | 3 +- .../auth-request-login.strategy.ts | 5 +- .../login-strategies/login.strategy.spec.ts | 25 +- .../common/login-strategies/login.strategy.ts | 12 - .../login-strategies/sso-login.strategy.ts | 9 +- ...device-trust-crypto.service.abstraction.ts | 22 +- ...ice-trust-crypto.service.implementation.ts | 138 +++++-- .../device-trust-crypto.service.spec.ts | 337 ++++++++++++------ .../platform/abstractions/state.service.ts | 6 +- .../models/domain/account-keys.spec.ts | 38 -- .../src/platform/models/domain/account.ts | 3 - .../src/platform/services/state.service.ts | 61 +--- .../src/platform/state/state-definitions.ts | 3 + libs/common/src/state-migrations/migrate.ts | 7 +- ...rust-crypto-svc-to-state-providers.spec.ts | 171 +++++++++ ...ice-trust-crypto-svc-to-state-providers.ts | 95 +++++ 32 files changed, 738 insertions(+), 334 deletions(-) create mode 100644 libs/common/src/state-migrations/migrations/53-migrate-device-trust-crypto-svc-to-state-providers.spec.ts create mode 100644 libs/common/src/state-migrations/migrations/53-migrate-device-trust-crypto-svc-to-state-providers.ts diff --git a/apps/browser/src/auth/background/service-factories/device-trust-crypto-service.factory.ts b/apps/browser/src/auth/background/service-factories/device-trust-crypto-service.factory.ts index 5916f38441..cac6f9bbe8 100644 --- a/apps/browser/src/auth/background/service-factories/device-trust-crypto-service.factory.ts +++ b/apps/browser/src/auth/background/service-factories/device-trust-crypto-service.factory.ts @@ -39,9 +39,13 @@ import { platformUtilsServiceFactory, } from "../../../platform/background/service-factories/platform-utils-service.factory"; import { - StateServiceInitOptions, - stateServiceFactory, -} from "../../../platform/background/service-factories/state-service.factory"; + StateProviderInitOptions, + stateProviderFactory, +} from "../../../platform/background/service-factories/state-provider.factory"; +import { + SecureStorageServiceInitOptions, + secureStorageServiceFactory, +} from "../../../platform/background/service-factories/storage-service.factory"; import { UserDecryptionOptionsServiceInitOptions, @@ -55,11 +59,12 @@ export type DeviceTrustCryptoServiceInitOptions = DeviceTrustCryptoServiceFactor CryptoFunctionServiceInitOptions & CryptoServiceInitOptions & EncryptServiceInitOptions & - StateServiceInitOptions & AppIdServiceInitOptions & DevicesApiServiceInitOptions & I18nServiceInitOptions & PlatformUtilsServiceInitOptions & + StateProviderInitOptions & + SecureStorageServiceInitOptions & UserDecryptionOptionsServiceInitOptions; export function deviceTrustCryptoServiceFactory( @@ -76,11 +81,12 @@ export function deviceTrustCryptoServiceFactory( await cryptoFunctionServiceFactory(cache, opts), await cryptoServiceFactory(cache, opts), await encryptServiceFactory(cache, opts), - await stateServiceFactory(cache, opts), await appIdServiceFactory(cache, opts), await devicesApiServiceFactory(cache, opts), await i18nServiceFactory(cache, opts), await platformUtilsServiceFactory(cache, opts), + await stateProviderFactory(cache, opts), + await secureStorageServiceFactory(cache, opts), await userDecryptionOptionsServiceFactory(cache, opts), ), ); diff --git a/apps/browser/src/auth/popup/lock.component.ts b/apps/browser/src/auth/popup/lock.component.ts index f2c56a23ae..f232eca45a 100644 --- a/apps/browser/src/auth/popup/lock.component.ts +++ b/apps/browser/src/auth/popup/lock.component.ts @@ -9,6 +9,7 @@ import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vaul import { VaultTimeoutService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout.service"; import { PolicyApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/policy/policy-api.service.abstraction"; import { InternalPolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { DeviceTrustCryptoServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust-crypto.service.abstraction"; import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction"; @@ -62,6 +63,7 @@ export class LockComponent extends BaseLockComponent { pinCryptoService: PinCryptoServiceAbstraction, private routerService: BrowserRouterService, biometricStateService: BiometricStateService, + accountService: AccountService, ) { super( router, @@ -84,6 +86,7 @@ export class LockComponent extends BaseLockComponent { userVerificationService, pinCryptoService, biometricStateService, + accountService, ); this.successRoute = "/tabs/current"; this.isInitialLockScreen = (window as any).previousPopupUrl == null; diff --git a/apps/browser/src/auth/popup/login-via-auth-request.component.ts b/apps/browser/src/auth/popup/login-via-auth-request.component.ts index 8d438d5b78..52f311ce7b 100644 --- a/apps/browser/src/auth/popup/login-via-auth-request.component.ts +++ b/apps/browser/src/auth/popup/login-via-auth-request.component.ts @@ -9,6 +9,7 @@ import { LoginEmailServiceAbstraction, } from "@bitwarden/auth/common"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AnonymousHubService } from "@bitwarden/common/auth/abstractions/anonymous-hub.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { DeviceTrustCryptoServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust-crypto.service.abstraction"; @@ -49,6 +50,7 @@ export class LoginViaAuthRequestComponent extends BaseLoginWithDeviceComponent { deviceTrustCryptoService: DeviceTrustCryptoServiceAbstraction, authRequestService: AuthRequestServiceAbstraction, loginStrategyService: LoginStrategyServiceAbstraction, + accountService: AccountService, private location: Location, ) { super( @@ -70,6 +72,7 @@ export class LoginViaAuthRequestComponent extends BaseLoginWithDeviceComponent { deviceTrustCryptoService, authRequestService, loginStrategyService, + accountService, ); super.onSuccessfulLogin = async () => { await syncService.fullSync(true); diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 49b4b96249..25befdcf80 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -556,11 +556,12 @@ export default class MainBackground { this.cryptoFunctionService, this.cryptoService, this.encryptService, - this.stateService, this.appIdService, this.devicesApiService, this.i18nService, this.platformUtilsService, + this.stateProvider, + this.secureStorageService, this.userDecryptionOptionsService, ); diff --git a/apps/cli/src/bw.ts b/apps/cli/src/bw.ts index bba381b84a..804b05e8e3 100644 --- a/apps/cli/src/bw.ts +++ b/apps/cli/src/bw.ts @@ -455,11 +455,12 @@ export class Main { this.cryptoFunctionService, this.cryptoService, this.encryptService, - this.stateService, this.appIdService, this.devicesApiService, this.i18nService, this.platformUtilsService, + this.stateProvider, + this.secureStorageService, this.userDecryptionOptionsService, ); diff --git a/apps/desktop/src/auth/lock.component.spec.ts b/apps/desktop/src/auth/lock.component.spec.ts index 6ecf93deb8..0339889bf7 100644 --- a/apps/desktop/src/auth/lock.component.spec.ts +++ b/apps/desktop/src/auth/lock.component.spec.ts @@ -12,6 +12,7 @@ import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vaul import { VaultTimeoutService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout.service"; import { PolicyApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/policy/policy-api.service.abstraction"; import { InternalPolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { DeviceTrustCryptoServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust-crypto.service.abstraction"; import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction"; import { BroadcasterService } from "@bitwarden/common/platform/abstractions/broadcaster.service"; @@ -23,7 +24,10 @@ import { MessagingService } from "@bitwarden/common/platform/abstractions/messag import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { BiometricStateService } from "@bitwarden/common/platform/biometrics/biometric-state.service"; +import { Utils } from "@bitwarden/common/platform/misc/utils"; +import { FakeAccountService, mockAccountServiceWith } from "@bitwarden/common/spec"; import { PasswordStrengthServiceAbstraction } from "@bitwarden/common/tools/password-strength"; +import { UserId } from "@bitwarden/common/types/guid"; import { DialogService } from "@bitwarden/components"; import { LockComponent } from "./lock.component"; @@ -49,6 +53,9 @@ describe("LockComponent", () => { let platformUtilsServiceMock: MockProxy; let activatedRouteMock: MockProxy; + const mockUserId = Utils.newGuid() as UserId; + const accountService: FakeAccountService = mockAccountServiceWith(mockUserId); + beforeEach(async () => { stateServiceMock = mock(); stateServiceMock.activeAccount$ = of(null); @@ -147,6 +154,10 @@ describe("LockComponent", () => { provide: BiometricStateService, useValue: biometricStateService, }, + { + provide: AccountService, + useValue: accountService, + }, ], schemas: [NO_ERRORS_SCHEMA], }).compileComponents(); diff --git a/apps/desktop/src/auth/lock.component.ts b/apps/desktop/src/auth/lock.component.ts index 7403f7481d..8b1448c06f 100644 --- a/apps/desktop/src/auth/lock.component.ts +++ b/apps/desktop/src/auth/lock.component.ts @@ -9,6 +9,7 @@ import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vaul import { VaultTimeoutService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout.service"; import { PolicyApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/policy/policy-api.service.abstraction"; import { InternalPolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { DeviceTrustCryptoServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust-crypto.service.abstraction"; import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction"; import { DeviceType } from "@bitwarden/common/enums"; @@ -59,6 +60,7 @@ export class LockComponent extends BaseLockComponent { userVerificationService: UserVerificationService, pinCryptoService: PinCryptoServiceAbstraction, biometricStateService: BiometricStateService, + accountService: AccountService, ) { super( router, @@ -81,6 +83,7 @@ export class LockComponent extends BaseLockComponent { userVerificationService, pinCryptoService, biometricStateService, + accountService, ); } diff --git a/apps/desktop/src/auth/login/login-via-auth-request.component.ts b/apps/desktop/src/auth/login/login-via-auth-request.component.ts index 28163d09d0..0a339030ba 100644 --- a/apps/desktop/src/auth/login/login-via-auth-request.component.ts +++ b/apps/desktop/src/auth/login/login-via-auth-request.component.ts @@ -10,6 +10,7 @@ import { LoginEmailServiceAbstraction, } from "@bitwarden/auth/common"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AnonymousHubService } from "@bitwarden/common/auth/abstractions/anonymous-hub.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { DeviceTrustCryptoServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust-crypto.service.abstraction"; @@ -57,6 +58,7 @@ export class LoginViaAuthRequestComponent extends BaseLoginWithDeviceComponent { deviceTrustCryptoService: DeviceTrustCryptoServiceAbstraction, authRequestService: AuthRequestServiceAbstraction, loginStrategyService: LoginStrategyServiceAbstraction, + accountService: AccountService, private location: Location, ) { super( @@ -78,6 +80,7 @@ export class LoginViaAuthRequestComponent extends BaseLoginWithDeviceComponent { deviceTrustCryptoService, authRequestService, loginStrategyService, + accountService, ); super.onSuccessfulLogin = () => { diff --git a/apps/desktop/src/platform/services/electron-state.service.ts b/apps/desktop/src/platform/services/electron-state.service.ts index f4399221d2..33c97f48af 100644 --- a/apps/desktop/src/platform/services/electron-state.service.ts +++ b/apps/desktop/src/platform/services/electron-state.service.ts @@ -1,47 +1,12 @@ -import { Utils } from "@bitwarden/common/platform/misc/utils"; import { GlobalState } from "@bitwarden/common/platform/models/domain/global-state"; -import { StorageOptions } from "@bitwarden/common/platform/models/domain/storage-options"; -import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; import { StateService as BaseStateService } from "@bitwarden/common/platform/services/state.service"; -import { DeviceKey } from "@bitwarden/common/types/key"; import { Account } from "../../models/account"; export class ElectronStateService extends BaseStateService { - private partialKeys = { - deviceKey: "_deviceKey", - }; - async addAccount(account: Account) { // Apply desktop overides to default account values account = new Account(account); await super.addAccount(account); } - - override async getDeviceKey(options?: StorageOptions): Promise { - options = this.reconcileOptions(options, await this.defaultSecureStorageOptions()); - if (options?.userId == null) { - return; - } - - const b64DeviceKey = await this.secureStorageService.get( - `${options.userId}${this.partialKeys.deviceKey}`, - options, - ); - - if (b64DeviceKey == null) { - return null; - } - - return new SymmetricCryptoKey(Utils.fromB64ToArray(b64DeviceKey)) as DeviceKey; - } - - override async setDeviceKey(value: DeviceKey, options?: StorageOptions): Promise { - options = this.reconcileOptions(options, await this.defaultSecureStorageOptions()); - if (options?.userId == null) { - return; - } - - await this.saveSecureStorageKey(this.partialKeys.deviceKey, value.keyB64, options); - } } diff --git a/apps/web/jest.config.js b/apps/web/jest.config.js index cde02cd995..f121823ade 100644 --- a/apps/web/jest.config.js +++ b/apps/web/jest.config.js @@ -9,7 +9,11 @@ module.exports = { ...sharedConfig, preset: "jest-preset-angular", setupFilesAfterEnv: ["/test.setup.ts"], - moduleNameMapper: pathsToModuleNameMapper(compilerOptions?.paths || {}, { - prefix: "/", - }), + moduleNameMapper: pathsToModuleNameMapper( + // lets us use @bitwarden/common/spec in web tests + { "@bitwarden/common/spec": ["../../libs/common/spec"], ...(compilerOptions?.paths ?? {}) }, + { + prefix: "/", + }, + ), }; diff --git a/apps/web/src/app/auth/key-rotation/user-key-rotation.service.spec.ts b/apps/web/src/app/auth/key-rotation/user-key-rotation.service.spec.ts index 7eabbbb5c1..09c7bf9ace 100644 --- a/apps/web/src/app/auth/key-rotation/user-key-rotation.service.spec.ts +++ b/apps/web/src/app/auth/key-rotation/user-key-rotation.service.spec.ts @@ -6,10 +6,13 @@ import { ConfigService } from "@bitwarden/common/platform/abstractions/config/co import { CryptoService } from "@bitwarden/common/platform/abstractions/crypto.service"; import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service"; import { EncryptionType } from "@bitwarden/common/platform/enums"; +import { Utils } from "@bitwarden/common/platform/misc/utils"; import { EncString } from "@bitwarden/common/platform/models/domain/enc-string"; import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; +import { FakeAccountService, mockAccountServiceWith } from "@bitwarden/common/spec"; import { Send } from "@bitwarden/common/tools/send/models/domain/send"; import { SendService } from "@bitwarden/common/tools/send/services/send.service.abstraction"; +import { UserId } from "@bitwarden/common/types/guid"; import { UserKey } from "@bitwarden/common/types/key"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction"; @@ -41,6 +44,9 @@ describe("KeyRotationService", () => { let mockStateService: MockProxy; let mockConfigService: MockProxy; + const mockUserId = Utils.newGuid() as UserId; + const mockAccountService: FakeAccountService = mockAccountServiceWith(mockUserId); + beforeAll(() => { mockApiService = mock(); mockCipherService = mock(); @@ -65,6 +71,7 @@ describe("KeyRotationService", () => { mockCryptoService, mockEncryptService, mockStateService, + mockAccountService, mockConfigService, ); }); diff --git a/apps/web/src/app/auth/key-rotation/user-key-rotation.service.ts b/apps/web/src/app/auth/key-rotation/user-key-rotation.service.ts index b53c71cb2e..03bc604b4d 100644 --- a/apps/web/src/app/auth/key-rotation/user-key-rotation.service.ts +++ b/apps/web/src/app/auth/key-rotation/user-key-rotation.service.ts @@ -1,6 +1,7 @@ import { Injectable } from "@angular/core"; import { firstValueFrom } from "rxjs"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { DeviceTrustCryptoServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust-crypto.service.abstraction"; import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; @@ -34,6 +35,7 @@ export class UserKeyRotationService { private cryptoService: CryptoService, private encryptService: EncryptService, private stateService: StateService, + private accountService: AccountService, private configService: ConfigService, ) {} @@ -90,7 +92,12 @@ export class UserKeyRotationService { await this.rotateUserKeyAndEncryptedDataLegacy(request); } - await this.deviceTrustCryptoService.rotateDevicesTrust(newUserKey, masterPasswordHash); + const activeAccount = await firstValueFrom(this.accountService.activeAccount$); + await this.deviceTrustCryptoService.rotateDevicesTrust( + activeAccount.id, + newUserKey, + masterPasswordHash, + ); } private async encryptPrivateKey(newUserKey: UserKey): Promise { diff --git a/apps/web/src/app/auth/lock.component.ts b/apps/web/src/app/auth/lock.component.ts index c4f8d276bb..a1d4724396 100644 --- a/apps/web/src/app/auth/lock.component.ts +++ b/apps/web/src/app/auth/lock.component.ts @@ -8,6 +8,7 @@ import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vaul import { VaultTimeoutService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout.service"; import { PolicyApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/policy/policy-api.service.abstraction"; import { InternalPolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { DeviceTrustCryptoServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust-crypto.service.abstraction"; import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction"; import { CryptoService } from "@bitwarden/common/platform/abstractions/crypto.service"; @@ -47,6 +48,7 @@ export class LockComponent extends BaseLockComponent { userVerificationService: UserVerificationService, pinCryptoService: PinCryptoServiceAbstraction, biometricStateService: BiometricStateService, + accountService: AccountService, ) { super( router, @@ -69,6 +71,7 @@ export class LockComponent extends BaseLockComponent { userVerificationService, pinCryptoService, biometricStateService, + accountService, ); } diff --git a/libs/angular/src/auth/components/base-login-decryption-options.component.ts b/libs/angular/src/auth/components/base-login-decryption-options.component.ts index 6bb545c4b5..8345bb9939 100644 --- a/libs/angular/src/auth/components/base-login-decryption-options.component.ts +++ b/libs/angular/src/auth/components/base-login-decryption-options.component.ts @@ -22,6 +22,7 @@ import { import { ApiService } from "@bitwarden/common/abstractions/api.service"; import { OrganizationApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/organization/organization-api.service.abstraction"; import { OrganizationUserService } from "@bitwarden/common/admin-console/abstractions/organization-user/organization-user.service"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { DeviceTrustCryptoServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust-crypto.service.abstraction"; import { DevicesServiceAbstraction } from "@bitwarden/common/auth/abstractions/devices/devices.service.abstraction"; import { PasswordResetEnrollmentServiceAbstraction } from "@bitwarden/common/auth/abstractions/password-reset-enrollment.service.abstraction"; @@ -34,6 +35,7 @@ import { MessagingService } from "@bitwarden/common/platform/abstractions/messag import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { ValidationService } from "@bitwarden/common/platform/abstractions/validation.service"; +import { UserId } from "@bitwarden/common/types/guid"; enum State { NewUser, @@ -65,6 +67,8 @@ export class BaseLoginDecryptionOptionsComponent implements OnInit, OnDestroy { protected data?: Data; protected loading = true; + activeAccountId: UserId; + // Remember device means for the user to trust the device rememberDeviceForm = this.formBuilder.group({ rememberDevice: [true], @@ -94,10 +98,12 @@ export class BaseLoginDecryptionOptionsComponent implements OnInit, OnDestroy { protected userDecryptionOptionsService: UserDecryptionOptionsServiceAbstraction, protected passwordResetEnrollmentService: PasswordResetEnrollmentServiceAbstraction, protected ssoLoginService: SsoLoginServiceAbstraction, + protected accountService: AccountService, ) {} async ngOnInit() { this.loading = true; + this.activeAccountId = (await firstValueFrom(this.accountService.activeAccount$))?.id; this.setupRememberDeviceValueChanges(); @@ -150,7 +156,9 @@ export class BaseLoginDecryptionOptionsComponent implements OnInit, OnDestroy { } private async setRememberDeviceDefaultValue() { - const rememberDeviceFromState = await this.deviceTrustCryptoService.getShouldTrustDevice(); + const rememberDeviceFromState = await this.deviceTrustCryptoService.getShouldTrustDevice( + this.activeAccountId, + ); const rememberDevice = rememberDeviceFromState ?? true; @@ -161,7 +169,9 @@ export class BaseLoginDecryptionOptionsComponent implements OnInit, OnDestroy { this.rememberDevice.valueChanges .pipe( switchMap((value) => - defer(() => this.deviceTrustCryptoService.setShouldTrustDevice(value)), + defer(() => + this.deviceTrustCryptoService.setShouldTrustDevice(this.activeAccountId, value), + ), ), takeUntil(this.destroy$), ) @@ -278,7 +288,7 @@ export class BaseLoginDecryptionOptionsComponent implements OnInit, OnDestroy { await this.passwordResetEnrollmentService.enroll(this.data.organizationId); if (this.rememberDeviceForm.value.rememberDevice) { - await this.deviceTrustCryptoService.trustDevice(); + await this.deviceTrustCryptoService.trustDevice(this.activeAccountId); } } catch (error) { this.validationService.showError(error); diff --git a/libs/angular/src/auth/components/lock.component.ts b/libs/angular/src/auth/components/lock.component.ts index c21ba1a75a..aa3b801ded 100644 --- a/libs/angular/src/auth/components/lock.component.ts +++ b/libs/angular/src/auth/components/lock.component.ts @@ -10,6 +10,7 @@ import { VaultTimeoutService } from "@bitwarden/common/abstractions/vault-timeou import { PolicyApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/policy/policy-api.service.abstraction"; import { InternalPolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { MasterPasswordPolicyOptions } from "@bitwarden/common/admin-console/models/domain/master-password-policy-options"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { DeviceTrustCryptoServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust-crypto.service.abstraction"; import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction"; import { ForceSetPasswordReason } from "@bitwarden/common/auth/models/domain/force-set-password-reason"; @@ -75,6 +76,7 @@ export class LockComponent implements OnInit, OnDestroy { protected userVerificationService: UserVerificationService, protected pinCryptoService: PinCryptoServiceAbstraction, protected biometricStateService: BiometricStateService, + protected accountService: AccountService, ) {} async ngOnInit() { @@ -269,7 +271,8 @@ export class LockComponent implements OnInit, OnDestroy { // Now that we have a decrypted user key in memory, we can check if we // need to establish trust on the current device - await this.deviceTrustCryptoService.trustDeviceIfRequired(); + const activeAccount = await firstValueFrom(this.accountService.activeAccount$); + await this.deviceTrustCryptoService.trustDeviceIfRequired(activeAccount.id); await this.doContinue(evaluatePasswordAfterUnlock); } diff --git a/libs/angular/src/auth/components/login-via-auth-request.component.ts b/libs/angular/src/auth/components/login-via-auth-request.component.ts index 66b7c1918c..6ba94d3001 100644 --- a/libs/angular/src/auth/components/login-via-auth-request.component.ts +++ b/libs/angular/src/auth/components/login-via-auth-request.component.ts @@ -1,6 +1,6 @@ import { Directive, OnDestroy, OnInit } from "@angular/core"; import { IsActiveMatchOptions, Router } from "@angular/router"; -import { Subject, takeUntil } from "rxjs"; +import { Subject, firstValueFrom, takeUntil } from "rxjs"; import { AuthRequestLoginCredentials, @@ -9,6 +9,7 @@ import { LoginEmailServiceAbstraction, } from "@bitwarden/auth/common"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AnonymousHubService } from "@bitwarden/common/auth/abstractions/anonymous-hub.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { DeviceTrustCryptoServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust-crypto.service.abstraction"; @@ -87,6 +88,7 @@ export class LoginViaAuthRequestComponent private deviceTrustCryptoService: DeviceTrustCryptoServiceAbstraction, private authRequestService: AuthRequestServiceAbstraction, private loginStrategyService: LoginStrategyServiceAbstraction, + private accountService: AccountService, ) { super(environmentService, i18nService, platformUtilsService); @@ -388,7 +390,8 @@ export class LoginViaAuthRequestComponent // Now that we have a decrypted user key in memory, we can check if we // need to establish trust on the current device - await this.deviceTrustCryptoService.trustDeviceIfRequired(); + const activeAccount = await firstValueFrom(this.accountService.activeAccount$); + await this.deviceTrustCryptoService.trustDeviceIfRequired(activeAccount.id); // TODO: don't forget to use auto enrollment service everywhere we trust device diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index c9a39eed0a..9a8a2bc6a2 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -912,11 +912,12 @@ const safeProviders: SafeProvider[] = [ CryptoFunctionServiceAbstraction, CryptoServiceAbstraction, EncryptService, - StateServiceAbstraction, AppIdServiceAbstraction, DevicesApiServiceAbstraction, I18nServiceAbstraction, PlatformUtilsServiceAbstraction, + StateProvider, + SECURE_STORAGE, UserDecryptionOptionsServiceAbstraction, ], }), diff --git a/libs/auth/src/common/login-strategies/auth-request-login.strategy.ts b/libs/auth/src/common/login-strategies/auth-request-login.strategy.ts index c42f43e764..31a0cebbfe 100644 --- a/libs/auth/src/common/login-strategies/auth-request-login.strategy.ts +++ b/libs/auth/src/common/login-strategies/auth-request-login.strategy.ts @@ -16,6 +16,7 @@ import { LogService } from "@bitwarden/common/platform/abstractions/log.service" import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; +import { UserId } from "@bitwarden/common/types/guid"; import { InternalUserDecryptionOptionsServiceAbstraction } from "../abstractions/user-decryption-options.service.abstraction"; import { AuthRequestLoginCredentials } from "../models/domain/login-credentials"; @@ -128,8 +129,10 @@ export class AuthRequestLoginStrategy extends LoginStrategy { await this.cryptoService.setUserKey(authRequestCredentials.decryptedUserKey); } else { await this.trySetUserKeyWithMasterKey(); + + const userId = (await this.stateService.getUserId()) as UserId; // Establish trust if required after setting user key - await this.deviceTrustCryptoService.trustDeviceIfRequired(); + await this.deviceTrustCryptoService.trustDeviceIfRequired(userId); } } diff --git a/libs/auth/src/common/login-strategies/login.strategy.spec.ts b/libs/auth/src/common/login-strategies/login.strategy.spec.ts index 42541808c8..0ac22047c5 100644 --- a/libs/auth/src/common/login-strategies/login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/login.strategy.spec.ts @@ -36,7 +36,7 @@ import { PasswordStrengthService, } from "@bitwarden/common/tools/password-strength"; import { CsprngArray } from "@bitwarden/common/types/csprng"; -import { UserKey, MasterKey, DeviceKey } from "@bitwarden/common/types/key"; +import { UserKey, MasterKey } from "@bitwarden/common/types/key"; import { LoginStrategyServiceAbstraction } from "../abstractions"; import { InternalUserDecryptionOptionsServiceAbstraction } from "../abstractions/user-decryption-options.service.abstraction"; @@ -215,29 +215,6 @@ describe("LoginStrategy", () => { expect(messagingService.send).toHaveBeenCalledWith("loggedIn"); }); - it("persists a device key for trusted device encryption when it exists on login", async () => { - // Arrange - const idTokenResponse = identityTokenResponseFactory(); - apiService.postIdentityToken.mockResolvedValue(idTokenResponse); - - const deviceKey = new SymmetricCryptoKey( - new Uint8Array(userKeyBytesLength).buffer as CsprngArray, - ) as DeviceKey; - - stateService.getDeviceKey.mockResolvedValue(deviceKey); - - const accountKeys = new AccountKeys(); - accountKeys.deviceKey = deviceKey; - - // Act - await passwordLoginStrategy.logIn(credentials); - - // Assert - expect(stateService.addAccount).toHaveBeenCalledWith( - expect.objectContaining({ keys: accountKeys }), - ); - }); - it("builds AuthResult", async () => { const tokenResponse = identityTokenResponseFactory(); tokenResponse.forcePasswordReset = true; diff --git a/libs/auth/src/common/login-strategies/login.strategy.ts b/libs/auth/src/common/login-strategies/login.strategy.ts index 8e927c2cc4..4fe99b276c 100644 --- a/libs/auth/src/common/login-strategies/login.strategy.ts +++ b/libs/auth/src/common/login-strategies/login.strategy.ts @@ -26,7 +26,6 @@ import { MessagingService } from "@bitwarden/common/platform/abstractions/messag import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { - AccountKeys, Account, AccountProfile, AccountTokens, @@ -160,18 +159,8 @@ export abstract class LoginStrategy { protected async saveAccountInformation(tokenResponse: IdentityTokenResponse): Promise { const accountInformation = await this.tokenService.decodeAccessToken(tokenResponse.accessToken); - // Must persist existing device key if it exists for trusted device decryption to work - // However, we must provide a user id so that the device key can be retrieved - // as the state service won't have an active account at this point in time - // even though the data exists in local storage. const userId = accountInformation.sub; - const deviceKey = await this.stateService.getDeviceKey({ userId }); - const accountKeys = new AccountKeys(); - if (deviceKey) { - accountKeys.deviceKey = deviceKey; - } - // If you don't persist existing admin auth requests on login, they will get deleted. const adminAuthRequest = await this.stateService.getAdminAuthRequest({ userId }); @@ -204,7 +193,6 @@ export abstract class LoginStrategy { tokens: { ...new AccountTokens(), }, - keys: accountKeys, adminAuthRequest: adminAuthRequest?.toJSON(), }), ); diff --git a/libs/auth/src/common/login-strategies/sso-login.strategy.ts b/libs/auth/src/common/login-strategies/sso-login.strategy.ts index 04f158d30a..7745104bd1 100644 --- a/libs/auth/src/common/login-strategies/sso-login.strategy.ts +++ b/libs/auth/src/common/login-strategies/sso-login.strategy.ts @@ -20,6 +20,7 @@ import { LogService } from "@bitwarden/common/platform/abstractions/log.service" import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; +import { UserId } from "@bitwarden/common/types/guid"; import { InternalUserDecryptionOptionsServiceAbstraction, @@ -284,7 +285,8 @@ export class SsoLoginStrategy extends LoginStrategy { if (await this.cryptoService.hasUserKey()) { // Now that we have a decrypted user key in memory, we can check if we // need to establish trust on the current device - await this.deviceTrustCryptoService.trustDeviceIfRequired(); + const userId = (await this.stateService.getUserId()) as UserId; + await this.deviceTrustCryptoService.trustDeviceIfRequired(userId); // if we successfully decrypted the user key, we can delete the admin auth request out of state // TODO: eventually we post and clean up DB as well once consumed on client @@ -298,7 +300,9 @@ export class SsoLoginStrategy extends LoginStrategy { private async trySetUserKeyWithDeviceKey(tokenResponse: IdentityTokenResponse): Promise { const trustedDeviceOption = tokenResponse.userDecryptionOptions?.trustedDeviceOption; - const deviceKey = await this.deviceTrustCryptoService.getDeviceKey(); + const userId = (await this.stateService.getUserId()) as UserId; + + const deviceKey = await this.deviceTrustCryptoService.getDeviceKey(userId); const encDevicePrivateKey = trustedDeviceOption?.encryptedPrivateKey; const encUserKey = trustedDeviceOption?.encryptedUserKey; @@ -307,6 +311,7 @@ export class SsoLoginStrategy extends LoginStrategy { } const userKey = await this.deviceTrustCryptoService.decryptUserKeyWithDeviceKey( + userId, encDevicePrivateKey, encUserKey, deviceKey, diff --git a/libs/common/src/auth/abstractions/device-trust-crypto.service.abstraction.ts b/libs/common/src/auth/abstractions/device-trust-crypto.service.abstraction.ts index 415355cfc7..53fe214035 100644 --- a/libs/common/src/auth/abstractions/device-trust-crypto.service.abstraction.ts +++ b/libs/common/src/auth/abstractions/device-trust-crypto.service.abstraction.ts @@ -1,6 +1,7 @@ import { Observable } from "rxjs"; import { EncString } from "../../platform/models/domain/enc-string"; +import { UserId } from "../../types/guid"; import { DeviceKey, UserKey } from "../../types/key"; import { DeviceResponse } from "../abstractions/devices/responses/device.response"; @@ -10,17 +11,24 @@ export abstract class DeviceTrustCryptoServiceAbstraction { * @description Retrieves the users choice to trust the device which can only happen after decryption * Note: this value should only be used once and then reset */ - getShouldTrustDevice: () => Promise; - setShouldTrustDevice: (value: boolean) => Promise; + getShouldTrustDevice: (userId: UserId) => Promise; + setShouldTrustDevice: (userId: UserId, value: boolean) => Promise; - trustDeviceIfRequired: () => Promise; + trustDeviceIfRequired: (userId: UserId) => Promise; - trustDevice: () => Promise; - getDeviceKey: () => Promise; + trustDevice: (userId: UserId) => Promise; + + /** Retrieves the device key if it exists from state or secure storage if supported for the active user. */ + getDeviceKey: (userId: UserId) => Promise; decryptUserKeyWithDeviceKey: ( + userId: UserId, encryptedDevicePrivateKey: EncString, encryptedUserKey: EncString, - deviceKey?: DeviceKey, + deviceKey: DeviceKey, ) => Promise; - rotateDevicesTrust: (newUserKey: UserKey, masterPasswordHash: string) => Promise; + rotateDevicesTrust: ( + userId: UserId, + newUserKey: UserKey, + masterPasswordHash: string, + ) => Promise; } 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 71f83f07c3..e65c5cd499 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 @@ -9,9 +9,13 @@ import { EncryptService } from "../../platform/abstractions/encrypt.service"; import { I18nService } from "../../platform/abstractions/i18n.service"; import { KeyGenerationService } from "../../platform/abstractions/key-generation.service"; import { PlatformUtilsService } from "../../platform/abstractions/platform-utils.service"; -import { StateService } from "../../platform/abstractions/state.service"; +import { AbstractStorageService } from "../../platform/abstractions/storage.service"; +import { StorageLocation } from "../../platform/enums"; import { EncString } from "../../platform/models/domain/enc-string"; +import { StorageOptions } from "../../platform/models/domain/storage-options"; import { SymmetricCryptoKey } from "../../platform/models/domain/symmetric-crypto-key"; +import { DEVICE_TRUST_DISK_LOCAL, KeyDefinition, StateProvider } from "../../platform/state"; +import { UserId } from "../../types/guid"; import { UserKey, DeviceKey } from "../../types/key"; import { DeviceTrustCryptoServiceAbstraction } from "../abstractions/device-trust-crypto.service.abstraction"; import { DeviceResponse } from "../abstractions/devices/responses/device.response"; @@ -22,7 +26,25 @@ import { UpdateDevicesTrustRequest, } from "../models/request/update-devices-trust.request"; +/** Uses disk storage so that the device key can persist after log out and tab removal. */ +export const DEVICE_KEY = new KeyDefinition(DEVICE_TRUST_DISK_LOCAL, "deviceKey", { + deserializer: (deviceKey) => SymmetricCryptoKey.fromJSON(deviceKey) as DeviceKey, +}); + +/** Uses disk storage so that the shouldTrustDevice bool can persist across login. */ +export const SHOULD_TRUST_DEVICE = new KeyDefinition( + DEVICE_TRUST_DISK_LOCAL, + "shouldTrustDevice", + { + deserializer: (shouldTrustDevice) => shouldTrustDevice, + }, +); + export class DeviceTrustCryptoService implements DeviceTrustCryptoServiceAbstraction { + private readonly platformSupportsSecureStorage = + this.platformUtilsService.supportsSecureStorage(); + private readonly deviceKeySecureStorageKey: string = "_deviceKey"; + supportsDeviceTrust$: Observable; constructor( @@ -30,11 +52,12 @@ export class DeviceTrustCryptoService implements DeviceTrustCryptoServiceAbstrac private cryptoFunctionService: CryptoFunctionService, private cryptoService: CryptoService, private encryptService: EncryptService, - private stateService: StateService, private appIdService: AppIdService, private devicesApiService: DevicesApiServiceAbstraction, private i18nService: I18nService, private platformUtilsService: PlatformUtilsService, + private stateProvider: StateProvider, + private secureStorageService: AbstractStorageService, private userDecryptionOptionsService: UserDecryptionOptionsServiceAbstraction, ) { this.supportsDeviceTrust$ = this.userDecryptionOptionsService.userDecryptionOptions$.pipe( @@ -46,24 +69,44 @@ export class DeviceTrustCryptoService implements DeviceTrustCryptoServiceAbstrac * @description Retrieves the users choice to trust the device which can only happen after decryption * Note: this value should only be used once and then reset */ - async getShouldTrustDevice(): Promise { - return await this.stateService.getShouldTrustDevice(); + async getShouldTrustDevice(userId: UserId): Promise { + if (!userId) { + throw new Error("UserId is required. Cannot get should trust device."); + } + + const shouldTrustDevice = await firstValueFrom( + this.stateProvider.getUserState$(SHOULD_TRUST_DEVICE, userId), + ); + + return shouldTrustDevice; } - async setShouldTrustDevice(value: boolean): Promise { - await this.stateService.setShouldTrustDevice(value); + async setShouldTrustDevice(userId: UserId, value: boolean): Promise { + if (!userId) { + throw new Error("UserId is required. Cannot set should trust device."); + } + + await this.stateProvider.setUserState(SHOULD_TRUST_DEVICE, value, userId); } - async trustDeviceIfRequired(): Promise { - const shouldTrustDevice = await this.getShouldTrustDevice(); + async trustDeviceIfRequired(userId: UserId): Promise { + if (!userId) { + throw new Error("UserId is required. Cannot trust device if required."); + } + + const shouldTrustDevice = await this.getShouldTrustDevice(userId); if (shouldTrustDevice) { - await this.trustDevice(); + await this.trustDevice(userId); // reset the trust choice - await this.setShouldTrustDevice(false); + await this.setShouldTrustDevice(userId, false); } } - async trustDevice(): Promise { + async trustDevice(userId: UserId): Promise { + if (!userId) { + throw new Error("UserId is required. Cannot trust device."); + } + // Attempt to get user key const userKey: UserKey = await this.cryptoService.getUserKey(); @@ -104,15 +147,23 @@ export class DeviceTrustCryptoService implements DeviceTrustCryptoServiceAbstrac ); // store device key in local/secure storage if enc keys posted to server successfully - await this.setDeviceKey(deviceKey); + await this.setDeviceKey(userId, deviceKey); this.platformUtilsService.showToast("success", null, this.i18nService.t("deviceTrusted")); return deviceResponse; } - async rotateDevicesTrust(newUserKey: UserKey, masterPasswordHash: string): Promise { - const currentDeviceKey = await this.getDeviceKey(); + async rotateDevicesTrust( + userId: UserId, + newUserKey: UserKey, + masterPasswordHash: string, + ): Promise { + if (!userId) { + throw new Error("UserId is required. Cannot rotate device's trust."); + } + + const currentDeviceKey = await this.getDeviceKey(userId); if (currentDeviceKey == null) { // If the current device doesn't have a device key available to it, then we can't // rotate any trust at all, so early return. @@ -165,26 +216,59 @@ export class DeviceTrustCryptoService implements DeviceTrustCryptoServiceAbstrac await this.devicesApiService.updateTrust(trustRequest, deviceIdentifier); } - async getDeviceKey(): Promise { - return await this.stateService.getDeviceKey(); + async getDeviceKey(userId: UserId): Promise { + if (!userId) { + throw new Error("UserId is required. Cannot get device key."); + } + + if (this.platformSupportsSecureStorage) { + const deviceKeyB64 = await this.secureStorageService.get< + ReturnType + >(`${userId}${this.deviceKeySecureStorageKey}`, this.getSecureStorageOptions(userId)); + + const deviceKey = SymmetricCryptoKey.fromJSON(deviceKeyB64) as DeviceKey; + + return deviceKey; + } + + const deviceKey = await firstValueFrom(this.stateProvider.getUserState$(DEVICE_KEY, userId)); + + return deviceKey; } - private async setDeviceKey(deviceKey: DeviceKey | null): Promise { - await this.stateService.setDeviceKey(deviceKey); + private async setDeviceKey(userId: UserId, deviceKey: DeviceKey | null): Promise { + if (!userId) { + throw new Error("UserId is required. Cannot set device key."); + } + + if (this.platformSupportsSecureStorage) { + await this.secureStorageService.save( + `${userId}${this.deviceKeySecureStorageKey}`, + deviceKey, + this.getSecureStorageOptions(userId), + ); + return; + } + + await this.stateProvider.setUserState(DEVICE_KEY, deviceKey?.toJSON(), userId); } private async makeDeviceKey(): Promise { // Create 512-bit device key - return (await this.keyGenerationService.createKey(512)) as DeviceKey; + const deviceKey = (await this.keyGenerationService.createKey(512)) as DeviceKey; + + return deviceKey; } async decryptUserKeyWithDeviceKey( + userId: UserId, encryptedDevicePrivateKey: EncString, encryptedUserKey: EncString, - deviceKey?: DeviceKey, + deviceKey: DeviceKey, ): Promise { - // If device key provided use it, otherwise try to retrieve from storage - deviceKey ||= await this.getDeviceKey(); + if (!userId) { + throw new Error("UserId is required. Cannot decrypt user key with device key."); + } if (!deviceKey) { // User doesn't have a device key anymore so device is untrusted @@ -207,9 +291,17 @@ export class DeviceTrustCryptoService implements DeviceTrustCryptoServiceAbstrac return new SymmetricCryptoKey(userKey) as UserKey; } catch (e) { // If either decryption effort fails, we want to remove the device key - await this.setDeviceKey(null); + await this.setDeviceKey(userId, null); return null; } } + + private getSecureStorageOptions(userId: UserId): StorageOptions { + return { + storageLocation: StorageLocation.Disk, + useSecureStorage: true, + userId: userId, + }; + } } 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 1d33223ddd..af147b3481 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 @@ -4,6 +4,9 @@ import { BehaviorSubject, of } from "rxjs"; import { UserDecryptionOptionsServiceAbstraction } from "@bitwarden/auth/common"; import { UserDecryptionOptions } from "../../../../auth/src/common/models/domain/user-decryption-options"; +import { FakeAccountService, mockAccountServiceWith } from "../../../spec/fake-account-service"; +import { FakeActiveUserState } from "../../../spec/fake-state"; +import { FakeStateProvider } from "../../../spec/fake-state-provider"; import { DeviceType } from "../../enums"; import { AppIdService } from "../../platform/abstractions/app-id.service"; import { CryptoFunctionService } from "../../platform/abstractions/crypto-function.service"; @@ -12,18 +15,26 @@ import { EncryptService } from "../../platform/abstractions/encrypt.service"; import { I18nService } from "../../platform/abstractions/i18n.service"; import { KeyGenerationService } from "../../platform/abstractions/key-generation.service"; import { PlatformUtilsService } from "../../platform/abstractions/platform-utils.service"; -import { StateService } from "../../platform/abstractions/state.service"; +import { AbstractStorageService } from "../../platform/abstractions/storage.service"; +import { StorageLocation } from "../../platform/enums"; import { EncryptionType } from "../../platform/enums/encryption-type.enum"; +import { Utils } from "../../platform/misc/utils"; import { EncString } from "../../platform/models/domain/enc-string"; +import { StorageOptions } from "../../platform/models/domain/storage-options"; import { SymmetricCryptoKey } from "../../platform/models/domain/symmetric-crypto-key"; import { CsprngArray } from "../../types/csprng"; +import { UserId } from "../../types/guid"; import { DeviceKey, UserKey } from "../../types/key"; import { DeviceResponse } from "../abstractions/devices/responses/device.response"; import { DevicesApiServiceAbstraction } from "../abstractions/devices-api.service.abstraction"; import { UpdateDevicesTrustRequest } from "../models/request/update-devices-trust.request"; import { ProtectedDeviceResponse } from "../models/response/protected-device.response"; -import { DeviceTrustCryptoService } from "./device-trust-crypto.service.implementation"; +import { + SHOULD_TRUST_DEVICE, + DEVICE_KEY, + DeviceTrustCryptoService, +} from "./device-trust-crypto.service.implementation"; describe("deviceTrustCryptoService", () => { let deviceTrustCryptoService: DeviceTrustCryptoService; @@ -32,33 +43,34 @@ describe("deviceTrustCryptoService", () => { const cryptoFunctionService = mock(); const cryptoService = mock(); const encryptService = mock(); - const stateService = mock(); const appIdService = mock(); const devicesApiService = mock(); const i18nService = mock(); const platformUtilsService = mock(); - const userDecryptionOptionsService = mock(); + const secureStorageService = mock(); + const userDecryptionOptionsService = mock(); const decryptionOptions = new BehaviorSubject(null); + let stateProvider: FakeStateProvider; + + const mockUserId = Utils.newGuid() as UserId; + let accountService: FakeAccountService; + + const deviceKeyPartialSecureStorageKey = "_deviceKey"; + const deviceKeySecureStorageKey = `${mockUserId}${deviceKeyPartialSecureStorageKey}`; + + const secureStorageOptions: StorageOptions = { + storageLocation: StorageLocation.Disk, + useSecureStorage: true, + userId: mockUserId, + }; + beforeEach(() => { jest.clearAllMocks(); - - decryptionOptions.next({} as any); - userDecryptionOptionsService.userDecryptionOptions$ = decryptionOptions; - - deviceTrustCryptoService = new DeviceTrustCryptoService( - keyGenerationService, - cryptoFunctionService, - cryptoService, - encryptService, - stateService, - appIdService, - devicesApiService, - i18nService, - platformUtilsService, - userDecryptionOptionsService, - ); + const supportsSecureStorage = false; // default to false; tests will override as needed + // By default all the tests will have a mocked active user in state provider. + deviceTrustCryptoService = createDeviceTrustCryptoService(mockUserId, supportsSecureStorage); }); it("instantiates", () => { @@ -67,27 +79,26 @@ describe("deviceTrustCryptoService", () => { describe("User Trust Device Choice For Decryption", () => { describe("getShouldTrustDevice", () => { - it("gets the user trust device choice for decryption from the state service", async () => { - const stateSvcGetShouldTrustDeviceSpy = jest.spyOn(stateService, "getShouldTrustDevice"); + it("gets the user trust device choice for decryption", async () => { + const newValue = true; - const expectedValue = true; - stateSvcGetShouldTrustDeviceSpy.mockResolvedValue(expectedValue); - const result = await deviceTrustCryptoService.getShouldTrustDevice(); + await stateProvider.setUserState(SHOULD_TRUST_DEVICE, newValue, mockUserId); - expect(stateSvcGetShouldTrustDeviceSpy).toHaveBeenCalledTimes(1); - expect(result).toEqual(expectedValue); + const result = await deviceTrustCryptoService.getShouldTrustDevice(mockUserId); + + expect(result).toEqual(newValue); }); }); describe("setShouldTrustDevice", () => { - it("sets the user trust device choice for decryption in the state service", async () => { - const stateSvcSetShouldTrustDeviceSpy = jest.spyOn(stateService, "setShouldTrustDevice"); + it("sets the user trust device choice for decryption ", async () => { + await stateProvider.setUserState(SHOULD_TRUST_DEVICE, false, mockUserId); const newValue = true; - await deviceTrustCryptoService.setShouldTrustDevice(newValue); + await deviceTrustCryptoService.setShouldTrustDevice(mockUserId, newValue); - expect(stateSvcSetShouldTrustDeviceSpy).toHaveBeenCalledTimes(1); - expect(stateSvcSetShouldTrustDeviceSpy).toHaveBeenCalledWith(newValue); + const result = await deviceTrustCryptoService.getShouldTrustDevice(mockUserId); + expect(result).toEqual(newValue); }); }); }); @@ -98,11 +109,11 @@ describe("deviceTrustCryptoService", () => { jest.spyOn(deviceTrustCryptoService, "trustDevice").mockResolvedValue({} as DeviceResponse); jest.spyOn(deviceTrustCryptoService, "setShouldTrustDevice").mockResolvedValue(); - await deviceTrustCryptoService.trustDeviceIfRequired(); + await deviceTrustCryptoService.trustDeviceIfRequired(mockUserId); expect(deviceTrustCryptoService.getShouldTrustDevice).toHaveBeenCalledTimes(1); expect(deviceTrustCryptoService.trustDevice).toHaveBeenCalledTimes(1); - expect(deviceTrustCryptoService.setShouldTrustDevice).toHaveBeenCalledWith(false); + expect(deviceTrustCryptoService.setShouldTrustDevice).toHaveBeenCalledWith(mockUserId, false); }); it("should not trust device nor reset when getShouldTrustDevice returns false", async () => { @@ -112,7 +123,7 @@ describe("deviceTrustCryptoService", () => { const trustDeviceSpy = jest.spyOn(deviceTrustCryptoService, "trustDevice"); const setShouldTrustDeviceSpy = jest.spyOn(deviceTrustCryptoService, "setShouldTrustDevice"); - await deviceTrustCryptoService.trustDeviceIfRequired(); + await deviceTrustCryptoService.trustDeviceIfRequired(mockUserId); expect(getShouldTrustDeviceSpy).toHaveBeenCalledTimes(1); expect(trustDeviceSpy).not.toHaveBeenCalled(); @@ -126,53 +137,140 @@ describe("deviceTrustCryptoService", () => { describe("getDeviceKey", () => { let existingDeviceKey: DeviceKey; - let stateSvcGetDeviceKeySpy: jest.SpyInstance; + let existingDeviceKeyB64: { keyB64: string }; beforeEach(() => { existingDeviceKey = new SymmetricCryptoKey( new Uint8Array(deviceKeyBytesLength) as CsprngArray, ) as DeviceKey; - stateSvcGetDeviceKeySpy = jest.spyOn(stateService, "getDeviceKey"); + existingDeviceKeyB64 = existingDeviceKey.toJSON(); }); - it("returns null when there is not an existing device key", async () => { - stateSvcGetDeviceKeySpy.mockResolvedValue(null); + describe("Secure Storage not supported", () => { + it("returns null when there is not an existing device key", async () => { + await stateProvider.setUserState(DEVICE_KEY, null, mockUserId); - const deviceKey = await deviceTrustCryptoService.getDeviceKey(); + const deviceKey = await deviceTrustCryptoService.getDeviceKey(mockUserId); - expect(stateSvcGetDeviceKeySpy).toHaveBeenCalledTimes(1); + expect(deviceKey).toBeNull(); + expect(secureStorageService.get).not.toHaveBeenCalled(); + }); - expect(deviceKey).toBeNull(); + it("returns the device key when there is an existing device key", async () => { + await stateProvider.setUserState(DEVICE_KEY, existingDeviceKey, mockUserId); + + const deviceKey = await deviceTrustCryptoService.getDeviceKey(mockUserId); + + expect(deviceKey).not.toBeNull(); + expect(deviceKey).toBeInstanceOf(SymmetricCryptoKey); + expect(deviceKey).toEqual(existingDeviceKey); + expect(secureStorageService.get).not.toHaveBeenCalled(); + }); }); - it("returns the device key when there is an existing device key", async () => { - stateSvcGetDeviceKeySpy.mockResolvedValue(existingDeviceKey); + describe("Secure Storage supported", () => { + beforeEach(() => { + const supportsSecureStorage = true; + deviceTrustCryptoService = createDeviceTrustCryptoService( + mockUserId, + supportsSecureStorage, + ); + }); - const deviceKey = await deviceTrustCryptoService.getDeviceKey(); + it("returns null when there is not an existing device key for the passed in user id", async () => { + secureStorageService.get.mockResolvedValue(null); - expect(stateSvcGetDeviceKeySpy).toHaveBeenCalledTimes(1); + // Act + const deviceKey = await deviceTrustCryptoService.getDeviceKey(mockUserId); - expect(deviceKey).not.toBeNull(); - expect(deviceKey).toBeInstanceOf(SymmetricCryptoKey); - expect(deviceKey).toEqual(existingDeviceKey); + // Assert + expect(deviceKey).toBeNull(); + }); + + it("returns the device key when there is an existing device key for the passed in user id", async () => { + // Arrange + secureStorageService.get.mockResolvedValue(existingDeviceKeyB64); + + // Act + const deviceKey = await deviceTrustCryptoService.getDeviceKey(mockUserId); + + // Assert + expect(deviceKey).not.toBeNull(); + expect(deviceKey).toBeInstanceOf(SymmetricCryptoKey); + expect(deviceKey).toEqual(existingDeviceKey); + }); + }); + + it("throws an error when no user id is passed in", async () => { + await expect(deviceTrustCryptoService.getDeviceKey(null)).rejects.toThrow( + "UserId is required. Cannot get device key.", + ); }); }); describe("setDeviceKey", () => { - it("sets the device key in the state service", async () => { - const stateSvcSetDeviceKeySpy = jest.spyOn(stateService, "setDeviceKey"); + describe("Secure Storage not supported", () => { + it("successfully sets the device key in state provider", async () => { + await stateProvider.setUserState(DEVICE_KEY, null, mockUserId); - const deviceKey = new SymmetricCryptoKey( + const newDeviceKey = new SymmetricCryptoKey( + new Uint8Array(deviceKeyBytesLength) as CsprngArray, + ) as DeviceKey; + + // TypeScript will allow calling private methods if the object is of type 'any' + // This is a hacky workaround, but it allows for cleaner tests + await (deviceTrustCryptoService as any).setDeviceKey(mockUserId, newDeviceKey); + + expect(stateProvider.mock.setUserState).toHaveBeenLastCalledWith( + DEVICE_KEY, + newDeviceKey.toJSON(), + mockUserId, + ); + }); + }); + describe("Secure Storage supported", () => { + beforeEach(() => { + const supportsSecureStorage = true; + deviceTrustCryptoService = createDeviceTrustCryptoService( + mockUserId, + supportsSecureStorage, + ); + }); + + it("successfully sets the device key in secure storage", async () => { + // Arrange + await stateProvider.setUserState(DEVICE_KEY, null, mockUserId); + + secureStorageService.get.mockResolvedValue(null); + + const newDeviceKey = new SymmetricCryptoKey( + new Uint8Array(deviceKeyBytesLength) as CsprngArray, + ) as DeviceKey; + + // Act + // TypeScript will allow calling private methods if the object is of type 'any' + // This is a hacky workaround, but it allows for cleaner tests + await (deviceTrustCryptoService as any).setDeviceKey(mockUserId, newDeviceKey); + + // Assert + expect(stateProvider.mock.setUserState).not.toHaveBeenCalledTimes(2); + expect(secureStorageService.save).toHaveBeenCalledWith( + deviceKeySecureStorageKey, + newDeviceKey, + secureStorageOptions, + ); + }); + }); + + it("throws an error when a null user id is passed in", async () => { + const newDeviceKey = new SymmetricCryptoKey( new Uint8Array(deviceKeyBytesLength) as CsprngArray, ) as DeviceKey; - // TypeScript will allow calling private methods if the object is of type 'any' - // This is a hacky workaround, but it allows for cleaner tests - await (deviceTrustCryptoService as any).setDeviceKey(deviceKey); - - expect(stateSvcSetDeviceKeySpy).toHaveBeenCalledTimes(1); - expect(stateSvcSetDeviceKeySpy).toHaveBeenCalledWith(deviceKey); + await expect( + (deviceTrustCryptoService as any).setDeviceKey(null, newDeviceKey), + ).rejects.toThrow("UserId is required. Cannot set device key."); }); }); @@ -300,7 +398,7 @@ describe("deviceTrustCryptoService", () => { }); it("calls the required methods with the correct arguments and returns a DeviceResponse", async () => { - const response = await deviceTrustCryptoService.trustDevice(); + const response = await deviceTrustCryptoService.trustDevice(mockUserId); expect(makeDeviceKeySpy).toHaveBeenCalledTimes(1); expect(rsaGenerateKeyPairSpy).toHaveBeenCalledTimes(1); @@ -331,7 +429,7 @@ describe("deviceTrustCryptoService", () => { // setup the spy to return null cryptoSvcGetUserKeySpy.mockResolvedValue(null); // check if the expected error is thrown - await expect(deviceTrustCryptoService.trustDevice()).rejects.toThrow( + await expect(deviceTrustCryptoService.trustDevice(mockUserId)).rejects.toThrow( "User symmetric key not found", ); @@ -341,7 +439,7 @@ describe("deviceTrustCryptoService", () => { // setup the spy to return undefined cryptoSvcGetUserKeySpy.mockResolvedValue(undefined); // check if the expected error is thrown - await expect(deviceTrustCryptoService.trustDevice()).rejects.toThrow( + await expect(deviceTrustCryptoService.trustDevice(mockUserId)).rejects.toThrow( "User symmetric key not found", ); }); @@ -381,7 +479,9 @@ describe("deviceTrustCryptoService", () => { it(`throws an error if ${method} fails`, async () => { const methodSpy = spy(); methodSpy.mockRejectedValue(new Error(errorText)); - await expect(deviceTrustCryptoService.trustDevice()).rejects.toThrow(errorText); + await expect(deviceTrustCryptoService.trustDevice(mockUserId)).rejects.toThrow( + errorText, + ); }); test.each([null, undefined])( @@ -389,11 +489,17 @@ describe("deviceTrustCryptoService", () => { async (invalidValue) => { const methodSpy = spy(); methodSpy.mockResolvedValue(invalidValue); - await expect(deviceTrustCryptoService.trustDevice()).rejects.toThrow(); + await expect(deviceTrustCryptoService.trustDevice(mockUserId)).rejects.toThrow(); }, ); }, ); + + it("throws an error when a null user id is passed in", async () => { + await expect(deviceTrustCryptoService.trustDevice(null)).rejects.toThrow( + "UserId is required. Cannot trust device.", + ); + }); }); describe("decryptUserKeyWithDeviceKey", () => { @@ -422,19 +528,26 @@ describe("deviceTrustCryptoService", () => { jest.clearAllMocks(); }); - it("returns null when device key isn't provided and isn't in state", async () => { - const getDeviceKeySpy = jest - .spyOn(deviceTrustCryptoService, "getDeviceKey") - .mockResolvedValue(null); + it("throws an error when a null user id is passed in", async () => { + await expect( + deviceTrustCryptoService.decryptUserKeyWithDeviceKey( + null, + mockEncryptedDevicePrivateKey, + mockEncryptedUserKey, + mockDeviceKey, + ), + ).rejects.toThrow("UserId is required. Cannot decrypt user key with device key."); + }); + it("returns null when device key isn't provided", async () => { const result = await deviceTrustCryptoService.decryptUserKeyWithDeviceKey( + mockUserId, mockEncryptedDevicePrivateKey, mockEncryptedUserKey, + mockDeviceKey, ); expect(result).toBeNull(); - - expect(getDeviceKeySpy).toHaveBeenCalledTimes(1); }); it("successfully returns the user key when provided keys (including device key) can decrypt it", async () => { @@ -446,6 +559,7 @@ describe("deviceTrustCryptoService", () => { .mockResolvedValue(new Uint8Array(userKeyBytesLength)); const result = await deviceTrustCryptoService.decryptUserKeyWithDeviceKey( + mockUserId, mockEncryptedDevicePrivateKey, mockEncryptedUserKey, mockDeviceKey, @@ -456,31 +570,6 @@ describe("deviceTrustCryptoService", () => { expect(rsaDecryptSpy).toHaveBeenCalledTimes(1); }); - it("successfully returns the user key when a device key is not provided (retrieves device key from state)", async () => { - const getDeviceKeySpy = jest - .spyOn(deviceTrustCryptoService, "getDeviceKey") - .mockResolvedValue(mockDeviceKey); - - const decryptToBytesSpy = jest - .spyOn(encryptService, "decryptToBytes") - .mockResolvedValue(new Uint8Array(userKeyBytesLength)); - const rsaDecryptSpy = jest - .spyOn(cryptoService, "rsaDecrypt") - .mockResolvedValue(new Uint8Array(userKeyBytesLength)); - - // Call without providing a device key - const result = await deviceTrustCryptoService.decryptUserKeyWithDeviceKey( - mockEncryptedDevicePrivateKey, - mockEncryptedUserKey, - ); - - expect(getDeviceKeySpy).toHaveBeenCalledTimes(1); - - expect(result).toEqual(mockUserKey); - expect(decryptToBytesSpy).toHaveBeenCalledTimes(1); - expect(rsaDecryptSpy).toHaveBeenCalledTimes(1); - }); - it("returns null and removes device key when the decryption fails", async () => { const decryptToBytesSpy = jest .spyOn(encryptService, "decryptToBytes") @@ -488,6 +577,7 @@ describe("deviceTrustCryptoService", () => { const setDeviceKeySpy = jest.spyOn(deviceTrustCryptoService as any, "setDeviceKey"); const result = await deviceTrustCryptoService.decryptUserKeyWithDeviceKey( + mockUserId, mockEncryptedDevicePrivateKey, mockEncryptedUserKey, mockDeviceKey, @@ -496,7 +586,7 @@ describe("deviceTrustCryptoService", () => { expect(result).toBeNull(); expect(decryptToBytesSpy).toHaveBeenCalledTimes(1); expect(setDeviceKeySpy).toHaveBeenCalledTimes(1); - expect(setDeviceKeySpy).toHaveBeenCalledWith(null); + expect(setDeviceKeySpy).toHaveBeenCalledWith(mockUserId, null); }); }); @@ -514,19 +604,28 @@ describe("deviceTrustCryptoService", () => { cryptoService.activeUserKey$ = of(fakeNewUserKey); }); - it("does an early exit when the current device is not a trusted device", async () => { - stateService.getDeviceKey.mockResolvedValue(null); + it("throws an error when a null user id is passed in", async () => { + await expect( + deviceTrustCryptoService.rotateDevicesTrust(null, fakeNewUserKey, ""), + ).rejects.toThrow("UserId is required. Cannot rotate device's trust."); + }); - await deviceTrustCryptoService.rotateDevicesTrust(fakeNewUserKey, ""); + it("does an early exit when the current device is not a trusted device", async () => { + const deviceKeyState: FakeActiveUserState = + stateProvider.activeUser.getFake(DEVICE_KEY); + deviceKeyState.nextState(null); + + await deviceTrustCryptoService.rotateDevicesTrust(mockUserId, fakeNewUserKey, ""); expect(devicesApiService.updateTrust).not.toHaveBeenCalled(); }); describe("is on a trusted device", () => { - beforeEach(() => { - stateService.getDeviceKey.mockResolvedValue( - new SymmetricCryptoKey(new Uint8Array(deviceKeyBytesLength)) as DeviceKey, - ); + beforeEach(async () => { + const mockDeviceKey = new SymmetricCryptoKey( + new Uint8Array(deviceKeyBytesLength), + ) as DeviceKey; + await stateProvider.setUserState(DEVICE_KEY, mockDeviceKey, mockUserId); }); it("rotates current device keys and calls api service when the current device is trusted", async () => { @@ -592,7 +691,11 @@ describe("deviceTrustCryptoService", () => { ); }); - await deviceTrustCryptoService.rotateDevicesTrust(fakeNewUserKey, "my_password_hash"); + await deviceTrustCryptoService.rotateDevicesTrust( + mockUserId, + fakeNewUserKey, + "my_password_hash", + ); expect(devicesApiService.updateTrust).toHaveBeenCalledWith( matches((updateTrustModel: UpdateDevicesTrustRequest) => { @@ -608,4 +711,32 @@ describe("deviceTrustCryptoService", () => { }); }); }); + + // Helpers + function createDeviceTrustCryptoService( + mockUserId: UserId | null, + supportsSecureStorage: boolean, + ) { + accountService = mockAccountServiceWith(mockUserId); + stateProvider = new FakeStateProvider(accountService); + + platformUtilsService.supportsSecureStorage.mockReturnValue(supportsSecureStorage); + + decryptionOptions.next({} as any); + userDecryptionOptionsService.userDecryptionOptions$ = decryptionOptions; + + return new DeviceTrustCryptoService( + keyGenerationService, + cryptoFunctionService, + cryptoService, + encryptService, + appIdService, + devicesApiService, + i18nService, + platformUtilsService, + stateProvider, + secureStorageService, + userDecryptionOptionsService, + ); + } }); diff --git a/libs/common/src/platform/abstractions/state.service.ts b/libs/common/src/platform/abstractions/state.service.ts index ab8b548951..9bc6d698a7 100644 --- a/libs/common/src/platform/abstractions/state.service.ts +++ b/libs/common/src/platform/abstractions/state.service.ts @@ -10,7 +10,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 } from "../../types/key"; +import { MasterKey } from "../../types/key"; import { CipherData } from "../../vault/models/data/cipher.data"; import { LocalData } from "../../vault/models/data/local.data"; import { CipherView } from "../../vault/models/view/cipher.view"; @@ -161,15 +161,11 @@ export abstract class StateService { setDecryptedSends: (value: SendView[], options?: StorageOptions) => Promise; getDuckDuckGoSharedKey: (options?: StorageOptions) => Promise; setDuckDuckGoSharedKey: (value: string, options?: StorageOptions) => Promise; - getDeviceKey: (options?: StorageOptions) => Promise; - setDeviceKey: (value: DeviceKey | null, options?: StorageOptions) => Promise; getAdminAuthRequest: (options?: StorageOptions) => Promise; setAdminAuthRequest: ( adminAuthRequest: AdminAuthRequestStorable, options?: StorageOptions, ) => Promise; - getShouldTrustDevice: (options?: StorageOptions) => Promise; - setShouldTrustDevice: (value: boolean, options?: StorageOptions) => Promise; getEmail: (options?: StorageOptions) => Promise; setEmail: (value: string, options?: StorageOptions) => Promise; getEmailVerified: (options?: StorageOptions) => Promise; diff --git a/libs/common/src/platform/models/domain/account-keys.spec.ts b/libs/common/src/platform/models/domain/account-keys.spec.ts index 7041acc5ba..4a96da1b48 100644 --- a/libs/common/src/platform/models/domain/account-keys.spec.ts +++ b/libs/common/src/platform/models/domain/account-keys.spec.ts @@ -1,6 +1,4 @@ import { makeStaticByteArray } from "../../../../spec"; -import { CsprngArray } from "../../../types/csprng"; -import { DeviceKey } from "../../../types/key"; import { Utils } from "../../misc/utils"; import { AccountKeys, EncryptionPair } from "./account"; @@ -24,23 +22,6 @@ describe("AccountKeys", () => { const json = JSON.stringify(keys); expect(json).toContain('"publicKey":"hello"'); }); - - // As the accountKeys.toJSON doesn't really serialize the device key - // this method just checks the persistence of the deviceKey - it("should persist deviceKey", () => { - // Arrange - const accountKeys = new AccountKeys(); - const deviceKeyBytesLength = 64; - accountKeys.deviceKey = new SymmetricCryptoKey( - new Uint8Array(deviceKeyBytesLength).buffer as CsprngArray, - ) as DeviceKey; - - // Act - const serializedKeys = accountKeys.toJSON(); - - // Assert - expect(serializedKeys.deviceKey).toEqual(accountKeys.deviceKey); - }); }); describe("fromJSON", () => { @@ -64,24 +45,5 @@ describe("AccountKeys", () => { } as any); expect(spy).toHaveBeenCalled(); }); - - it("should deserialize deviceKey", () => { - // Arrange - const expectedKeyB64 = - "ZJNnhx9BbJeb2EAq1hlMjqt6GFsg9G/GzoFf6SbPKsaiMhKGDcbHcwcyEg56Lh8lfilpZz4SRM6UA7oFCg+lSg=="; - - const symmetricCryptoKeyFromJsonSpy = jest.spyOn(SymmetricCryptoKey, "fromJSON"); - - // Act - const accountKeys = AccountKeys.fromJSON({ - deviceKey: { - keyB64: expectedKeyB64, - }, - } as any); - - // Assert - expect(symmetricCryptoKeyFromJsonSpy).toHaveBeenCalled(); - expect(accountKeys.deviceKey.keyB64).toEqual(expectedKeyB64); - }); }); }); diff --git a/libs/common/src/platform/models/domain/account.ts b/libs/common/src/platform/models/domain/account.ts index 61bb3eeac5..798a60600a 100644 --- a/libs/common/src/platform/models/domain/account.ts +++ b/libs/common/src/platform/models/domain/account.ts @@ -95,7 +95,6 @@ export class AccountData { export class AccountKeys { masterKey?: MasterKey; masterKeyEncryptedUserKey?: string; - deviceKey?: ReturnType; publicKey?: Uint8Array; /** @deprecated July 2023, left for migration purposes*/ @@ -125,7 +124,6 @@ export class AccountKeys { } return Object.assign(new AccountKeys(), obj, { masterKey: SymmetricCryptoKey.fromJSON(obj?.masterKey), - deviceKey: obj?.deviceKey, cryptoMasterKey: SymmetricCryptoKey.fromJSON(obj?.cryptoMasterKey), cryptoSymmetricKey: EncryptionPair.fromJSON( obj?.cryptoSymmetricKey, @@ -185,7 +183,6 @@ export class AccountSettings { vaultTimeout?: number; vaultTimeoutAction?: string = "lock"; approveLoginRequests?: boolean; - trustDeviceChoiceForDecryption?: boolean; /** @deprecated July 2023, left for migration purposes*/ pinProtected?: EncryptionPair = new EncryptionPair(); diff --git a/libs/common/src/platform/services/state.service.ts b/libs/common/src/platform/services/state.service.ts index 0e524b6c4b..57a2085ccf 100644 --- a/libs/common/src/platform/services/state.service.ts +++ b/libs/common/src/platform/services/state.service.ts @@ -14,7 +14,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 } from "../../types/key"; +import { MasterKey } from "../../types/key"; import { CipherData } from "../../vault/models/data/cipher.data"; import { LocalData } from "../../vault/models/data/local.data"; import { CipherView } from "../../vault/models/view/cipher.view"; @@ -650,39 +650,6 @@ export class StateService< : await this.secureStorageService.save(DDG_SHARED_KEY, value, options); } - async getDeviceKey(options?: StorageOptions): Promise { - options = this.reconcileOptions(options, await this.defaultOnDiskLocalOptions()); - - if (options?.userId == null) { - return null; - } - - const account = await this.getAccount(options); - - const existingDeviceKey = account?.keys?.deviceKey; - - // Must manually instantiate the SymmetricCryptoKey class from the JSON object - if (existingDeviceKey != null) { - return SymmetricCryptoKey.fromJSON(existingDeviceKey) as DeviceKey; - } else { - return null; - } - } - - async setDeviceKey(value: DeviceKey | null, options?: StorageOptions): Promise { - options = this.reconcileOptions(options, await this.defaultOnDiskLocalOptions()); - - if (options?.userId == null) { - return; - } - - const account = await this.getAccount(options); - - account.keys.deviceKey = value?.toJSON() ?? null; - - await this.saveAccount(account, options); - } - async getAdminAuthRequest(options?: StorageOptions): Promise { options = this.reconcileOptions(options, await this.defaultOnDiskLocalOptions()); @@ -714,31 +681,6 @@ export class StateService< await this.saveAccount(account, options); } - async getShouldTrustDevice(options?: StorageOptions): Promise { - options = this.reconcileOptions(options, await this.defaultOnDiskLocalOptions()); - - if (options?.userId == null) { - return null; - } - - const account = await this.getAccount(options); - - return account?.settings?.trustDeviceChoiceForDecryption ?? null; - } - - async setShouldTrustDevice(value: boolean, options?: StorageOptions): Promise { - options = this.reconcileOptions(options, await this.defaultOnDiskLocalOptions()); - if (options?.userId == null) { - return; - } - - const account = await this.getAccount(options); - - account.settings.trustDeviceChoiceForDecryption = value; - - await this.saveAccount(account, options); - } - async getEmail(options?: StorageOptions): Promise { return ( await this.getAccount(this.reconcileOptions(options, await this.defaultInMemoryOptions())) @@ -1633,7 +1575,6 @@ export class StateService< protected resetAccount(account: TAccount) { const persistentAccountInformation = { settings: account.settings, - keys: { deviceKey: account.keys.deviceKey }, adminAuthRequest: account.adminAuthRequest, }; return Object.assign(this.createAccount(), persistentAccountInformation); diff --git a/libs/common/src/platform/state/state-definitions.ts b/libs/common/src/platform/state/state-definitions.ts index 814bf0280f..466c3a2c11 100644 --- a/libs/common/src/platform/state/state-definitions.ts +++ b/libs/common/src/platform/state/state-definitions.ts @@ -48,6 +48,9 @@ export const TOKEN_DISK_LOCAL = new StateDefinition("tokenDiskLocal", "disk", { web: "disk-local", }); export const TOKEN_MEMORY = new StateDefinition("token", "memory"); +export const DEVICE_TRUST_DISK_LOCAL = new StateDefinition("deviceTrust", "disk", { + web: "disk-local", +}); export const USER_DECRYPTION_OPTIONS_DISK = new StateDefinition("userDecryptionOptions", "disk"); // Autofill diff --git a/libs/common/src/state-migrations/migrate.ts b/libs/common/src/state-migrations/migrate.ts index 0758d49f59..4e1a0529fc 100644 --- a/libs/common/src/state-migrations/migrate.ts +++ b/libs/common/src/state-migrations/migrate.ts @@ -49,6 +49,7 @@ import { AddKeyTypeToOrgKeysMigrator } from "./migrations/5-add-key-type-to-org- import { KeyConnectorMigrator } from "./migrations/50-move-key-connector-to-state-provider"; import { RememberedEmailMigrator } from "./migrations/51-move-remembered-email-to-state-providers"; import { DeleteInstalledVersion } from "./migrations/52-delete-installed-version"; +import { DeviceTrustCryptoServiceStateProviderMigrator } from "./migrations/53-migrate-device-trust-crypto-svc-to-state-providers"; import { RemoveLegacyEtmKeyMigrator } from "./migrations/6-remove-legacy-etm-key"; import { MoveBiometricAutoPromptToAccount } from "./migrations/7-move-biometric-auto-prompt-to-account"; import { MoveStateVersionMigrator } from "./migrations/8-move-state-version"; @@ -56,8 +57,7 @@ import { MoveBrowserSettingsToGlobal } from "./migrations/9-move-browser-setting import { MinVersionMigrator } from "./migrations/min-version"; export const MIN_VERSION = 3; -export const CURRENT_VERSION = 52; - +export const CURRENT_VERSION = 53; export type MinVersion = typeof MIN_VERSION; export function createMigrationBuilder() { @@ -111,7 +111,8 @@ export function createMigrationBuilder() { .with(AccountServerConfigMigrator, 48, 49) .with(KeyConnectorMigrator, 49, 50) .with(RememberedEmailMigrator, 50, 51) - .with(DeleteInstalledVersion, 51, CURRENT_VERSION); + .with(DeleteInstalledVersion, 51, 52) + .with(DeviceTrustCryptoServiceStateProviderMigrator, 52, CURRENT_VERSION); } export async function currentVersion( diff --git a/libs/common/src/state-migrations/migrations/53-migrate-device-trust-crypto-svc-to-state-providers.spec.ts b/libs/common/src/state-migrations/migrations/53-migrate-device-trust-crypto-svc-to-state-providers.spec.ts new file mode 100644 index 0000000000..79366a4716 --- /dev/null +++ b/libs/common/src/state-migrations/migrations/53-migrate-device-trust-crypto-svc-to-state-providers.spec.ts @@ -0,0 +1,171 @@ +import { MockProxy, any } from "jest-mock-extended"; + +import { MigrationHelper } from "../migration-helper"; +import { mockMigrationHelper } from "../migration-helper.spec"; + +import { + DEVICE_KEY, + DeviceTrustCryptoServiceStateProviderMigrator, + SHOULD_TRUST_DEVICE, +} from "./53-migrate-device-trust-crypto-svc-to-state-providers"; + +// Represents data in state service pre-migration +function preMigrationJson() { + return { + global: { + otherStuff: "otherStuff1", + }, + authenticatedAccounts: ["user1", "user2", "user3"], + user1: { + keys: { + deviceKey: { + keyB64: "user1_deviceKey", + }, + otherStuff: "overStuff2", + }, + settings: { + trustDeviceChoiceForDecryption: true, + otherStuff: "overStuff3", + }, + otherStuff: "otherStuff4", + }, + user2: { + keys: { + // no device key + otherStuff: "otherStuff5", + }, + settings: { + // no trust device choice + otherStuff: "overStuff6", + }, + otherStuff: "otherStuff7", + }, + }; +} + +function rollbackJSON() { + return { + // use pattern user_{userId}_{stateDefinitionName}_{keyDefinitionKey} for each user + // User1 migrated data + user_user1_deviceTrust_deviceKey: { + keyB64: "user1_deviceKey", + }, + user_user1_deviceTrust_shouldTrustDevice: true, + + // User2 does not have migrated data + + global: { + otherStuff: "otherStuff1", + }, + authenticatedAccounts: ["user1", "user2", "user3"], + user1: { + keys: { + otherStuff: "overStuff2", + }, + settings: { + otherStuff: "overStuff3", + }, + otherStuff: "otherStuff4", + }, + user2: { + keys: { + otherStuff: "otherStuff5", + }, + settings: { + otherStuff: "overStuff6", + }, + otherStuff: "otherStuff6", + }, + }; +} + +describe("DeviceTrustCryptoServiceStateProviderMigrator", () => { + let helper: MockProxy; + let sut: DeviceTrustCryptoServiceStateProviderMigrator; + + describe("migrate", () => { + beforeEach(() => { + helper = mockMigrationHelper(preMigrationJson(), 52); + sut = new DeviceTrustCryptoServiceStateProviderMigrator(52, 53); + }); + + // it should remove deviceKey and trustDeviceChoiceForDecryption from all accounts + it("should remove deviceKey and trustDeviceChoiceForDecryption from all accounts that have it", async () => { + await sut.migrate(helper); + expect(helper.set).toHaveBeenCalledWith("user1", { + keys: { + otherStuff: "overStuff2", + }, + settings: { + otherStuff: "overStuff3", + }, + otherStuff: "otherStuff4", + }); + + expect(helper.set).toHaveBeenCalledTimes(1); + expect(helper.set).not.toHaveBeenCalledWith("user2", any()); + expect(helper.set).not.toHaveBeenCalledWith("user3", any()); + }); + + it("should migrate deviceKey and trustDeviceChoiceForDecryption to state providers for accounts that have the data", async () => { + await sut.migrate(helper); + + expect(helper.setToUser).toHaveBeenCalledWith("user1", DEVICE_KEY, { + keyB64: "user1_deviceKey", + }); + expect(helper.setToUser).toHaveBeenCalledWith("user1", SHOULD_TRUST_DEVICE, true); + + expect(helper.setToUser).not.toHaveBeenCalledWith("user2", DEVICE_KEY, any()); + expect(helper.setToUser).not.toHaveBeenCalledWith("user2", SHOULD_TRUST_DEVICE, any()); + + expect(helper.setToUser).not.toHaveBeenCalledWith("user3", DEVICE_KEY, any()); + expect(helper.setToUser).not.toHaveBeenCalledWith("user3", SHOULD_TRUST_DEVICE, any()); + }); + }); + + describe("rollback", () => { + beforeEach(() => { + helper = mockMigrationHelper(rollbackJSON(), 53); + sut = new DeviceTrustCryptoServiceStateProviderMigrator(52, 53); + }); + + it("should null out newly migrated entries in state provider framework", async () => { + await sut.rollback(helper); + + expect(helper.setToUser).toHaveBeenCalledWith("user1", DEVICE_KEY, null); + expect(helper.setToUser).toHaveBeenCalledWith("user1", SHOULD_TRUST_DEVICE, null); + + expect(helper.setToUser).toHaveBeenCalledWith("user2", DEVICE_KEY, null); + expect(helper.setToUser).toHaveBeenCalledWith("user2", SHOULD_TRUST_DEVICE, null); + + expect(helper.setToUser).toHaveBeenCalledWith("user3", DEVICE_KEY, null); + expect(helper.setToUser).toHaveBeenCalledWith("user3", SHOULD_TRUST_DEVICE, null); + }); + + it("should add back deviceKey and trustDeviceChoiceForDecryption to all accounts", async () => { + await sut.rollback(helper); + + expect(helper.set).toHaveBeenCalledWith("user1", { + keys: { + deviceKey: { + keyB64: "user1_deviceKey", + }, + otherStuff: "overStuff2", + }, + settings: { + trustDeviceChoiceForDecryption: true, + otherStuff: "overStuff3", + }, + otherStuff: "otherStuff4", + }); + }); + + it("should not add data back if data wasn't migrated or acct doesn't exist", async () => { + await sut.rollback(helper); + + // no data to add back for user2 (acct exists but no migrated data) and user3 (no acct) + expect(helper.set).not.toHaveBeenCalledWith("user2", any()); + expect(helper.set).not.toHaveBeenCalledWith("user3", any()); + }); + }); +}); diff --git a/libs/common/src/state-migrations/migrations/53-migrate-device-trust-crypto-svc-to-state-providers.ts b/libs/common/src/state-migrations/migrations/53-migrate-device-trust-crypto-svc-to-state-providers.ts new file mode 100644 index 0000000000..e19c7b3fa5 --- /dev/null +++ b/libs/common/src/state-migrations/migrations/53-migrate-device-trust-crypto-svc-to-state-providers.ts @@ -0,0 +1,95 @@ +import { KeyDefinitionLike, MigrationHelper } from "../migration-helper"; +import { Migrator } from "../migrator"; + +// Types to represent data as it is stored in JSON +type DeviceKeyJsonType = { + keyB64: string; +}; + +type ExpectedAccountType = { + keys?: { + deviceKey?: DeviceKeyJsonType; + }; + settings?: { + trustDeviceChoiceForDecryption?: boolean; + }; +}; + +export const DEVICE_KEY: KeyDefinitionLike = { + key: "deviceKey", // matches KeyDefinition.key in DeviceTrustCryptoService + stateDefinition: { + name: "deviceTrust", // matches StateDefinition.name in StateDefinitions + }, +}; + +export const SHOULD_TRUST_DEVICE: KeyDefinitionLike = { + key: "shouldTrustDevice", + stateDefinition: { + name: "deviceTrust", + }, +}; + +export class DeviceTrustCryptoServiceStateProviderMigrator extends Migrator<52, 53> { + async migrate(helper: MigrationHelper): Promise { + const accounts = await helper.getAccounts(); + async function migrateAccount(userId: string, account: ExpectedAccountType): Promise { + let updatedAccount = false; + + // Migrate deviceKey + const existingDeviceKey = account?.keys?.deviceKey; + + if (existingDeviceKey != null) { + // Only migrate data that exists + await helper.setToUser(userId, DEVICE_KEY, existingDeviceKey); + delete account.keys.deviceKey; + updatedAccount = true; + } + + // Migrate shouldTrustDevice + const existingShouldTrustDevice = account?.settings?.trustDeviceChoiceForDecryption; + + if (existingShouldTrustDevice != null) { + await helper.setToUser(userId, SHOULD_TRUST_DEVICE, existingShouldTrustDevice); + delete account.settings.trustDeviceChoiceForDecryption; + updatedAccount = true; + } + + if (updatedAccount) { + // Save the migrated account + await helper.set(userId, account); + } + } + + await Promise.all([...accounts.map(({ userId, account }) => migrateAccount(userId, account))]); + } + + async rollback(helper: MigrationHelper): Promise { + const accounts = await helper.getAccounts(); + async function rollbackAccount(userId: string, account: ExpectedAccountType): Promise { + // Rollback deviceKey + const migratedDeviceKey: DeviceKeyJsonType = await helper.getFromUser(userId, DEVICE_KEY); + + if (account?.keys && migratedDeviceKey != null) { + account.keys.deviceKey = migratedDeviceKey; + await helper.set(userId, account); + } + + await helper.setToUser(userId, DEVICE_KEY, null); + + // Rollback shouldTrustDevice + const migratedShouldTrustDevice = await helper.getFromUser( + userId, + SHOULD_TRUST_DEVICE, + ); + + if (account?.settings && migratedShouldTrustDevice != null) { + account.settings.trustDeviceChoiceForDecryption = migratedShouldTrustDevice; + await helper.set(userId, account); + } + + await helper.setToUser(userId, SHOULD_TRUST_DEVICE, null); + } + + await Promise.all([...accounts.map(({ userId, account }) => rollbackAccount(userId, account))]); + } +}