Skip to content

Commit

Permalink
Auth/PM-5268 - DeviceTrustCryptoService state provider migration (#7882)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
JaredSnider-Bitwarden authored Apr 1, 2024
1 parent 94843bd commit c202c93
Show file tree
Hide file tree
Showing 32 changed files with 737 additions and 333 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -55,11 +59,12 @@ export type DeviceTrustCryptoServiceInitOptions = DeviceTrustCryptoServiceFactor
CryptoFunctionServiceInitOptions &
CryptoServiceInitOptions &
EncryptServiceInitOptions &
StateServiceInitOptions &
AppIdServiceInitOptions &
DevicesApiServiceInitOptions &
I18nServiceInitOptions &
PlatformUtilsServiceInitOptions &
StateProviderInitOptions &
SecureStorageServiceInitOptions &
UserDecryptionOptionsServiceInitOptions;

export function deviceTrustCryptoServiceFactory(
Expand All @@ -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),
),
);
Expand Down
3 changes: 3 additions & 0 deletions apps/browser/src/auth/popup/lock.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -62,6 +63,7 @@ export class LockComponent extends BaseLockComponent {
pinCryptoService: PinCryptoServiceAbstraction,
private routerService: BrowserRouterService,
biometricStateService: BiometricStateService,
accountService: AccountService,
) {
super(
router,
Expand All @@ -84,6 +86,7 @@ export class LockComponent extends BaseLockComponent {
userVerificationService,
pinCryptoService,
biometricStateService,
accountService,
);
this.successRoute = "/tabs/current";
this.isInitialLockScreen = (window as any).previousPopupUrl == null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -49,6 +50,7 @@ export class LoginViaAuthRequestComponent extends BaseLoginWithDeviceComponent {
deviceTrustCryptoService: DeviceTrustCryptoServiceAbstraction,
authRequestService: AuthRequestServiceAbstraction,
loginStrategyService: LoginStrategyServiceAbstraction,
accountService: AccountService,
private location: Location,
) {
super(
Expand All @@ -70,6 +72,7 @@ export class LoginViaAuthRequestComponent extends BaseLoginWithDeviceComponent {
deviceTrustCryptoService,
authRequestService,
loginStrategyService,
accountService,
);
super.onSuccessfulLogin = async () => {
await syncService.fullSync(true);
Expand Down
3 changes: 2 additions & 1 deletion apps/browser/src/background/main.background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);

Expand Down
3 changes: 2 additions & 1 deletion apps/cli/src/bw.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);

Expand Down
11 changes: 11 additions & 0 deletions apps/desktop/src/auth/lock.component.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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";
Expand All @@ -49,6 +53,9 @@ describe("LockComponent", () => {
let platformUtilsServiceMock: MockProxy<PlatformUtilsService>;
let activatedRouteMock: MockProxy<ActivatedRoute>;

const mockUserId = Utils.newGuid() as UserId;
const accountService: FakeAccountService = mockAccountServiceWith(mockUserId);

beforeEach(async () => {
stateServiceMock = mock<StateService>();
stateServiceMock.activeAccount$ = of(null);
Expand Down Expand Up @@ -147,6 +154,10 @@ describe("LockComponent", () => {
provide: BiometricStateService,
useValue: biometricStateService,
},
{
provide: AccountService,
useValue: accountService,
},
],
schemas: [NO_ERRORS_SCHEMA],
}).compileComponents();
Expand Down
3 changes: 3 additions & 0 deletions apps/desktop/src/auth/lock.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -59,6 +60,7 @@ export class LockComponent extends BaseLockComponent {
userVerificationService: UserVerificationService,
pinCryptoService: PinCryptoServiceAbstraction,
biometricStateService: BiometricStateService,
accountService: AccountService,
) {
super(
router,
Expand All @@ -81,6 +83,7 @@ export class LockComponent extends BaseLockComponent {
userVerificationService,
pinCryptoService,
biometricStateService,
accountService,
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -57,6 +58,7 @@ export class LoginViaAuthRequestComponent extends BaseLoginWithDeviceComponent {
deviceTrustCryptoService: DeviceTrustCryptoServiceAbstraction,
authRequestService: AuthRequestServiceAbstraction,
loginStrategyService: LoginStrategyServiceAbstraction,
accountService: AccountService,
private location: Location,
) {
super(
Expand All @@ -78,6 +80,7 @@ export class LoginViaAuthRequestComponent extends BaseLoginWithDeviceComponent {
deviceTrustCryptoService,
authRequestService,
loginStrategyService,
accountService,
);

super.onSuccessfulLogin = () => {
Expand Down
35 changes: 0 additions & 35 deletions apps/desktop/src/platform/services/electron-state.service.ts
Original file line number Diff line number Diff line change
@@ -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<GlobalState, Account> {
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<DeviceKey | null> {
options = this.reconcileOptions(options, await this.defaultSecureStorageOptions());
if (options?.userId == null) {
return;
}

const b64DeviceKey = await this.secureStorageService.get<string>(
`${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<void> {
options = this.reconcileOptions(options, await this.defaultSecureStorageOptions());
if (options?.userId == null) {
return;
}

await this.saveSecureStorageKey(this.partialKeys.deviceKey, value.keyB64, options);
}
}
10 changes: 7 additions & 3 deletions apps/web/jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@ module.exports = {
...sharedConfig,
preset: "jest-preset-angular",
setupFilesAfterEnv: ["<rootDir>/test.setup.ts"],
moduleNameMapper: pathsToModuleNameMapper(compilerOptions?.paths || {}, {
prefix: "<rootDir>/",
}),
moduleNameMapper: pathsToModuleNameMapper(
// lets us use @bitwarden/common/spec in web tests
{ "@bitwarden/common/spec": ["../../libs/common/spec"], ...(compilerOptions?.paths ?? {}) },
{
prefix: "<rootDir>/",
},
),
};
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -41,6 +44,9 @@ describe("KeyRotationService", () => {
let mockStateService: MockProxy<StateService>;
let mockConfigService: MockProxy<ConfigService>;

const mockUserId = Utils.newGuid() as UserId;
const mockAccountService: FakeAccountService = mockAccountServiceWith(mockUserId);

beforeAll(() => {
mockApiService = mock<UserKeyRotationApiService>();
mockCipherService = mock<CipherService>();
Expand All @@ -65,6 +71,7 @@ describe("KeyRotationService", () => {
mockCryptoService,
mockEncryptService,
mockStateService,
mockAccountService,
mockConfigService,
);
});
Expand Down
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -34,6 +35,7 @@ export class UserKeyRotationService {
private cryptoService: CryptoService,
private encryptService: EncryptService,
private stateService: StateService,
private accountService: AccountService,
private configService: ConfigService,
) {}

Expand Down Expand Up @@ -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<EncryptedString | null> {
Expand Down
3 changes: 3 additions & 0 deletions apps/web/src/app/auth/lock.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -47,6 +48,7 @@ export class LockComponent extends BaseLockComponent {
userVerificationService: UserVerificationService,
pinCryptoService: PinCryptoServiceAbstraction,
biometricStateService: BiometricStateService,
accountService: AccountService,
) {
super(
router,
Expand All @@ -69,6 +71,7 @@ export class LockComponent extends BaseLockComponent {
userVerificationService,
pinCryptoService,
biometricStateService,
accountService,
);
}

Expand Down
Loading

0 comments on commit c202c93

Please sign in to comment.