From a3c9a42d13263537bdc3e8f53ac9c608c3e3d8cd Mon Sep 17 00:00:00 2001 From: Jake Fink Date: Tue, 1 Apr 2025 11:54:13 -0400 Subject: [PATCH] [PM-19172] Passes UserId through change email component and adds tests (#13686) * add tests and pass userid * add tests for getOrDeriveMasterKey * remove extra coalesce for token --- .../account/change-email.component.spec.ts | 197 ++++++++++++++++++ .../account/change-email.component.ts | 79 ++++--- libs/key-management/src/key.service.spec.ts | 46 ++++ libs/key-management/src/key.service.ts | 13 +- 4 files changed, 302 insertions(+), 33 deletions(-) create mode 100644 apps/web/src/app/auth/settings/account/change-email.component.spec.ts diff --git a/apps/web/src/app/auth/settings/account/change-email.component.spec.ts b/apps/web/src/app/auth/settings/account/change-email.component.spec.ts new file mode 100644 index 00000000000..838a50b5c2e --- /dev/null +++ b/apps/web/src/app/auth/settings/account/change-email.component.spec.ts @@ -0,0 +1,197 @@ +import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { FormBuilder, ReactiveFormsModule } from "@angular/forms"; +import mock, { MockProxy } from "jest-mock-extended/lib/Mock"; +import { firstValueFrom, of } from "rxjs"; + +import { ApiService } from "@bitwarden/common/abstractions/api.service"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { TwoFactorProviderType } from "@bitwarden/common/auth/enums/two-factor-provider-type"; +import { TwoFactorProviderResponse } from "@bitwarden/common/auth/models/response/two-factor-provider.response"; +import { ListResponse } from "@bitwarden/common/models/response/list.response"; +import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; +import { FakeAccountService, mockAccountServiceWith } from "@bitwarden/common/spec"; +import { UserId } from "@bitwarden/common/types/guid"; +import { ToastService } from "@bitwarden/components"; +import { KdfConfigService, KeyService } from "@bitwarden/key-management"; +import { ChangeEmailComponent } from "@bitwarden/web-vault/app/auth/settings/account/change-email.component"; +import { SharedModule } from "@bitwarden/web-vault/app/shared"; + +describe("ChangeEmailComponent", () => { + let component: ChangeEmailComponent; + let fixture: ComponentFixture; + + let apiService: MockProxy; + let accountService: FakeAccountService; + let keyService: MockProxy; + let kdfConfigService: MockProxy; + + beforeEach(async () => { + apiService = mock(); + keyService = mock(); + kdfConfigService = mock(); + accountService = mockAccountServiceWith("UserId" as UserId); + + await TestBed.configureTestingModule({ + declarations: [ChangeEmailComponent], + imports: [ReactiveFormsModule, SharedModule], + providers: [ + { provide: AccountService, useValue: accountService }, + { provide: ApiService, useValue: apiService }, + { provide: I18nService, useValue: { t: (key: string) => key } }, + { provide: KeyService, useValue: keyService }, + { provide: MessagingService, useValue: mock() }, + { provide: KdfConfigService, useValue: kdfConfigService }, + { provide: ToastService, useValue: mock() }, + { provide: FormBuilder, useClass: FormBuilder }, + ], + }).compileComponents(); + + fixture = TestBed.createComponent(ChangeEmailComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + }); + + it("creates component", () => { + expect(component).toBeTruthy(); + }); + + describe("ngOnInit", () => { + beforeEach(() => { + apiService.getTwoFactorProviders.mockResolvedValue({ + data: [{ type: TwoFactorProviderType.Email, enabled: true } as TwoFactorProviderResponse], + } as ListResponse); + }); + + it("initializes userId", async () => { + await component.ngOnInit(); + expect(component.userId).toBe("UserId"); + }); + + it("errors if there is no active user", async () => { + // clear active account + await firstValueFrom(accountService.activeAccount$); + accountService.activeAccountSubject.next(null); + + await expect(() => component.ngOnInit()).rejects.toThrow("Null or undefined account"); + }); + + it("initializes showTwoFactorEmailWarning", async () => { + await component.ngOnInit(); + expect(component.showTwoFactorEmailWarning).toBe(true); + }); + }); + + describe("submit", () => { + beforeEach(() => { + component.formGroup.controls.step1.setValue({ + masterPassword: "password", + newEmail: "test@example.com", + }); + + keyService.getOrDeriveMasterKey + .calledWith("password", "UserId") + .mockResolvedValue("getOrDeriveMasterKey" as any); + keyService.hashMasterKey + .calledWith("password", "getOrDeriveMasterKey" as any) + .mockResolvedValue("existingHash"); + }); + + it("throws if userId is null on submit", async () => { + component.userId = undefined; + + await expect(component.submit()).rejects.toThrow("Can't find user"); + }); + + describe("step 1", () => { + it("does not submit if step 1 is invalid", async () => { + component.formGroup.controls.step1.setValue({ + masterPassword: "", + newEmail: "", + }); + + await component.submit(); + + expect(apiService.postEmailToken).not.toHaveBeenCalled(); + }); + + it("sends email token in step 1 if tokenSent is false", async () => { + await component.submit(); + + expect(apiService.postEmailToken).toHaveBeenCalledWith({ + newEmail: "test@example.com", + masterPasswordHash: "existingHash", + }); + // should activate step 2 + expect(component.tokenSent).toBe(true); + expect(component.formGroup.controls.step1.disabled).toBe(true); + expect(component.formGroup.controls.token.enabled).toBe(true); + }); + }); + + describe("step 2", () => { + beforeEach(() => { + component.tokenSent = true; + component.formGroup.controls.step1.disable(); + component.formGroup.controls.token.enable(); + component.formGroup.controls.token.setValue("token"); + + kdfConfigService.getKdfConfig$ + .calledWith("UserId" as any) + .mockReturnValue(of("kdfConfig" as any)); + keyService.userKey$.calledWith("UserId" as any).mockReturnValue(of("userKey" as any)); + + keyService.makeMasterKey + .calledWith("password", "test@example.com", "kdfConfig" as any) + .mockResolvedValue("newMasterKey" as any); + keyService.hashMasterKey + .calledWith("password", "newMasterKey" as any) + .mockResolvedValue("newMasterKeyHash"); + + // Important: make sure this is called with new master key, not existing + keyService.encryptUserKeyWithMasterKey + .calledWith("newMasterKey" as any, "userKey" as any) + .mockResolvedValue(["userKey" as any, { encryptedString: "newEncryptedUserKey" } as any]); + }); + + it("does not post email if token is missing on submit", async () => { + component.formGroup.controls.token.setValue(""); + + await component.submit(); + + expect(apiService.postEmail).not.toHaveBeenCalled(); + }); + + it("throws if kdfConfig is missing on submit", async () => { + kdfConfigService.getKdfConfig$.mockReturnValue(of(null)); + + await expect(component.submit()).rejects.toThrow("Missing kdf config"); + }); + + it("throws if userKey can't be found", async () => { + keyService.userKey$.mockReturnValue(of(null)); + + await expect(component.submit()).rejects.toThrow("Can't find UserKey"); + }); + + it("throws if encryptedUserKey is missing", async () => { + keyService.encryptUserKeyWithMasterKey.mockResolvedValue(["userKey" as any, null as any]); + + await expect(component.submit()).rejects.toThrow("Missing Encrypted User Key"); + }); + + it("submits if step 2 is valid", async () => { + await component.submit(); + + // validate that hashes are correct + expect(apiService.postEmail).toHaveBeenCalledWith({ + masterPasswordHash: "existingHash", + newMasterPasswordHash: "newMasterKeyHash", + token: "token", + newEmail: "test@example.com", + key: "newEncryptedUserKey", + }); + }); + }); + }); +}); diff --git a/apps/web/src/app/auth/settings/account/change-email.component.ts b/apps/web/src/app/auth/settings/account/change-email.component.ts index a49c0b5a2f8..caf7e0933b0 100644 --- a/apps/web/src/app/auth/settings/account/change-email.component.ts +++ b/apps/web/src/app/auth/settings/account/change-email.component.ts @@ -1,17 +1,16 @@ -// FIXME: Update this file to be type safe and remove this and next line -// @ts-strict-ignore import { Component, OnInit } from "@angular/core"; import { FormBuilder, Validators } from "@angular/forms"; +import { firstValueFrom } from "rxjs"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { TwoFactorProviderType } from "@bitwarden/common/auth/enums/two-factor-provider-type"; import { EmailTokenRequest } from "@bitwarden/common/auth/models/request/email-token.request"; import { EmailRequest } from "@bitwarden/common/auth/models/request/email.request"; +import { getUserId } from "@bitwarden/common/auth/services/account.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; -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 { ToastService } from "@bitwarden/components"; import { KdfConfigService, KeyService } from "@bitwarden/key-management"; @@ -22,8 +21,9 @@ import { KdfConfigService, KeyService } from "@bitwarden/key-management"; export class ChangeEmailComponent implements OnInit { tokenSent = false; showTwoFactorEmailWarning = false; + userId: UserId | undefined; - protected formGroup = this.formBuilder.group({ + formGroup = this.formBuilder.group({ step1: this.formBuilder.group({ masterPassword: ["", [Validators.required]], newEmail: ["", [Validators.required, Validators.email]], @@ -32,26 +32,30 @@ export class ChangeEmailComponent implements OnInit { }); constructor( + private accountService: AccountService, private apiService: ApiService, private i18nService: I18nService, - private platformUtilsService: PlatformUtilsService, private keyService: KeyService, private messagingService: MessagingService, - private logService: LogService, - private stateService: StateService, private formBuilder: FormBuilder, private kdfConfigService: KdfConfigService, private toastService: ToastService, ) {} async ngOnInit() { + this.userId = await firstValueFrom(getUserId(this.accountService.activeAccount$)); + const twoFactorProviders = await this.apiService.getTwoFactorProviders(); this.showTwoFactorEmailWarning = twoFactorProviders.data.some( (p) => p.type === TwoFactorProviderType.Email && p.enabled, ); } - protected submit = async () => { + submit = async () => { + if (this.userId == null) { + throw new Error("Can't find user"); + } + // This form has multiple steps, so we need to mark all the groups as touched. this.formGroup.controls.step1.markAllAsTouched(); @@ -65,37 +69,54 @@ export class ChangeEmailComponent implements OnInit { } const step1Value = this.formGroup.controls.step1.value; - const newEmail = step1Value.newEmail.trim().toLowerCase(); + const newEmail = step1Value.newEmail?.trim().toLowerCase(); + const masterPassword = step1Value.masterPassword; + + if (newEmail == null || masterPassword == null) { + throw new Error("Missing email or password"); + } + + const existingHash = await this.keyService.hashMasterKey( + masterPassword, + await this.keyService.getOrDeriveMasterKey(masterPassword, this.userId), + ); if (!this.tokenSent) { const request = new EmailTokenRequest(); request.newEmail = newEmail; - request.masterPasswordHash = await this.keyService.hashMasterKey( - step1Value.masterPassword, - await this.keyService.getOrDeriveMasterKey(step1Value.masterPassword), - ); + request.masterPasswordHash = existingHash; await this.apiService.postEmailToken(request); this.activateStep2(); } else { + const token = this.formGroup.value.token; + if (token == null) { + throw new Error("Missing token"); + } const request = new EmailRequest(); - request.token = this.formGroup.value.token; + request.token = token; request.newEmail = newEmail; - request.masterPasswordHash = await this.keyService.hashMasterKey( - step1Value.masterPassword, - await this.keyService.getOrDeriveMasterKey(step1Value.masterPassword), - ); - const kdfConfig = await this.kdfConfigService.getKdfConfig(); - const newMasterKey = await this.keyService.makeMasterKey( - step1Value.masterPassword, - newEmail, - kdfConfig, - ); + request.masterPasswordHash = existingHash; + + const kdfConfig = await firstValueFrom(this.kdfConfigService.getKdfConfig$(this.userId)); + if (kdfConfig == null) { + throw new Error("Missing kdf config"); + } + const newMasterKey = await this.keyService.makeMasterKey(masterPassword, newEmail, kdfConfig); request.newMasterPasswordHash = await this.keyService.hashMasterKey( - step1Value.masterPassword, + masterPassword, newMasterKey, ); - const newUserKey = await this.keyService.encryptUserKeyWithMasterKey(newMasterKey); - request.key = newUserKey[1].encryptedString; + + const userKey = await firstValueFrom(this.keyService.userKey$(this.userId)); + if (userKey == null) { + throw new Error("Can't find UserKey"); + } + const newUserKey = await this.keyService.encryptUserKeyWithMasterKey(newMasterKey, userKey); + const encryptedUserKey = newUserKey[1]?.encryptedString; + if (encryptedUserKey == null) { + throw new Error("Missing Encrypted User Key"); + } + request.key = encryptedUserKey; await this.apiService.postEmail(request); this.reset(); diff --git a/libs/key-management/src/key.service.spec.ts b/libs/key-management/src/key.service.spec.ts index a0afc160794..f3264be69de 100644 --- a/libs/key-management/src/key.service.spec.ts +++ b/libs/key-management/src/key.service.spec.ts @@ -736,6 +736,52 @@ describe("keyService", () => { }); }); + describe("getOrDeriveMasterKey", () => { + it("returns the master key if it is already available", async () => { + const getMasterKey = jest + .spyOn(masterPasswordService, "masterKey$") + .mockReturnValue(of("masterKey" as any)); + + const result = await keyService.getOrDeriveMasterKey("password", mockUserId); + + expect(getMasterKey).toHaveBeenCalledWith(mockUserId); + expect(result).toEqual("masterKey"); + }); + + it("derives the master key if it is not available", async () => { + const getMasterKey = jest + .spyOn(masterPasswordService, "masterKey$") + .mockReturnValue(of(null as any)); + + const deriveKeyFromPassword = jest + .spyOn(keyGenerationService, "deriveKeyFromPassword") + .mockResolvedValue("mockMasterKey" as any); + + kdfConfigService.getKdfConfig$.mockReturnValue(of("mockKdfConfig" as any)); + + const result = await keyService.getOrDeriveMasterKey("password", mockUserId); + + expect(getMasterKey).toHaveBeenCalledWith(mockUserId); + expect(deriveKeyFromPassword).toHaveBeenCalledWith("password", "email", "mockKdfConfig"); + expect(result).toEqual("mockMasterKey"); + }); + + it("throws an error if no user is found", async () => { + accountService.activeAccountSubject.next(null); + + await expect(keyService.getOrDeriveMasterKey("password")).rejects.toThrow("No user found"); + }); + + it("throws an error if no kdf config is found", async () => { + jest.spyOn(masterPasswordService, "masterKey$").mockReturnValue(of(null as any)); + kdfConfigService.getKdfConfig$.mockReturnValue(of(null)); + + await expect(keyService.getOrDeriveMasterKey("password", mockUserId)).rejects.toThrow( + "No kdf found for user", + ); + }); + }); + describe("compareKeyHash", () => { type TestCase = { masterKey: MasterKey; diff --git a/libs/key-management/src/key.service.ts b/libs/key-management/src/key.service.ts index 870513f3913..5c9c0a01da6 100644 --- a/libs/key-management/src/key.service.ts +++ b/libs/key-management/src/key.service.ts @@ -287,10 +287,15 @@ export class DefaultKeyService implements KeyServiceAbstraction { ), ); const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(resolvedUserId)); - return ( - masterKey || - (await this.makeMasterKey(password, email, await this.kdfConfigService.getKdfConfig())) - ); + if (masterKey != null) { + return masterKey; + } + + const kdf = await firstValueFrom(this.kdfConfigService.getKdfConfig$(resolvedUserId)); + if (kdf == null) { + throw new Error("No kdf found for user"); + } + return await this.makeMasterKey(password, email, kdf); } /**