[PM-27515] Fix 2fa settings not working after KDF change (#17070)

* Always derive authentication data fresh for UV

* Cleanup

* Add tests

* Fix remote UV

* Fix test

* Fix test

* Address feedback

* Fix build

* Update libs/common/src/auth/types/verification.ts

Co-authored-by: Maciej Zieniuk <167752252+mzieniukbw@users.noreply.github.com>

* Remove unused var

* Fix relative import

* Fix types

---------

Co-authored-by: Maciej Zieniuk <167752252+mzieniukbw@users.noreply.github.com>

apps/web/src/app/auth/settings/two-factor/two-factor-setup-method-base.component.ts

@@ -27,7 +27,7 @@ export abstract class TwoFactorSetupMethodBaseComponent {
   enabled = false;
   authed = false;
 
-  protected hashedSecret: string | undefined;
+  protected secret: string | undefined;
   protected verificationType: VerificationType | undefined;
   protected componentName = "";
 
@@ -42,7 +42,7 @@ export abstract class TwoFactorSetupMethodBaseComponent {
   ) {}
 
   protected auth(authResponse: AuthResponseBase) {
-    this.hashedSecret = authResponse.secret;
+    this.secret = authResponse.secret;
     this.verificationType = authResponse.verificationType;
     this.authed = true;
   }
@@ -132,12 +132,12 @@ export abstract class TwoFactorSetupMethodBaseComponent {
   protected async buildRequestModel<T extends SecretVerificationRequest>(
     requestClass: new () => T,
   ) {
-    if (this.hashedSecret === undefined || this.verificationType === undefined) {
+    if (this.secret === undefined || this.verificationType === undefined) {
       throw new Error("User verification data is missing");
     }
     return this.userVerificationService.buildRequest(
       {
-        secret: this.hashedSecret,
+        secret: this.secret,
         type: this.verificationType,
       },
       requestClass,

apps/web/src/app/auth/settings/two-factor/two-factor-verify.component.ts

@@ -1,15 +1,14 @@
-import { Component, EventEmitter, Inject, Output } from "@angular/core";
+import { Component, Inject } from "@angular/core";
 import { FormControl, FormGroup, ReactiveFormsModule } from "@angular/forms";
 
 import { UserVerificationFormInputComponent } from "@bitwarden/auth/angular";
 import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction";
 import { TwoFactorProviderType } from "@bitwarden/common/auth/enums/two-factor-provider-type";
-import { VerificationType } from "@bitwarden/common/auth/enums/verification-type";
 import { SecretVerificationRequest } from "@bitwarden/common/auth/models/request/secret-verification.request";
 import { TwoFactorApiService } from "@bitwarden/common/auth/two-factor";
 import { AuthResponse } from "@bitwarden/common/auth/types/auth-response";
 import { TwoFactorResponse } from "@bitwarden/common/auth/types/two-factor-response";
-import { Verification } from "@bitwarden/common/auth/types/verification";
+import { VerificationWithSecret } from "@bitwarden/common/auth/types/verification";
 import { ErrorResponse } from "@bitwarden/common/models/response/error.response";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
 import {
@@ -45,14 +44,10 @@ type TwoFactorVerifyDialogData = {
 export class TwoFactorVerifyComponent {
   type: TwoFactorProviderType;
   organizationId: string;
-  // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals
-  // eslint-disable-next-line @angular-eslint/prefer-output-emitter-ref
-  @Output() onAuthed = new EventEmitter<AuthResponse<TwoFactorResponse>>();
-
   formPromise: Promise<TwoFactorResponse> | undefined;
 
   protected formGroup = new FormGroup({
-    secret: new FormControl<Verification | null>(null),
+    secret: new FormControl<VerificationWithSecret | null>(null),
   });
   invalidSecret: boolean = false;
 
@@ -69,24 +64,19 @@ export class TwoFactorVerifyComponent {
 
   submit = async () => {
     try {
-      let hashedSecret = "";
       if (!this.formGroup.value.secret) {
         throw new Error("Secret is required");
       }
 
       const secret = this.formGroup.value.secret!;
       this.formPromise = this.userVerificationService.buildRequest(secret).then((request) => {
-        hashedSecret =
-          secret.type === VerificationType.MasterPassword
-            ? request.masterPasswordHash
-            : request.otp;
         return this.apiCall(request);
       });
 
       const response = await this.formPromise;
       this.dialogRef.close({
         response: response,
-        secret: hashedSecret,
+        secret: secret.secret,
         verificationType: secret.type,
       });
     } catch (e) {

libs/common/src/auth/services/user-verification/user-verification.service.spec.ts

@@ -191,6 +191,140 @@ describe("UserVerificationService", () => {
     });
   });
 
+  describe("buildRequest", () => {
+    beforeEach(() => {
+      accountService = mockAccountServiceWith(mockUserId);
+      i18nService.t
+        .calledWith("verificationCodeRequired")
+        .mockReturnValue("Verification code is required");
+      i18nService.t
+        .calledWith("masterPasswordRequired")
+        .mockReturnValue("Master Password is required");
+    });
+
+    describe("OTP verification", () => {
+      it("should build request with OTP secret", async () => {
+        const verification = {
+          type: VerificationType.OTP,
+          secret: "123456",
+        } as any;
+
+        const result = await sut.buildRequest(verification);
+
+        expect(result.otp).toBe("123456");
+      });
+
+      it("should throw if OTP secret is empty", async () => {
+        const verification = {
+          type: VerificationType.OTP,
+          secret: "",
+        } as any;
+
+        await expect(sut.buildRequest(verification)).rejects.toThrow(
+          "Verification code is required",
+        );
+      });
+
+      it("should throw if OTP secret is null", async () => {
+        const verification = {
+          type: VerificationType.OTP,
+          secret: null,
+        } as any;
+
+        await expect(sut.buildRequest(verification)).rejects.toThrow(
+          "Verification code is required",
+        );
+      });
+    });
+
+    describe("Master password verification", () => {
+      beforeEach(() => {
+        kdfConfigService.getKdfConfig.mockResolvedValue("kdfConfig" as unknown as KdfConfig);
+        masterPasswordService.saltForUser$.mockReturnValue(of("salt" as any));
+        masterPasswordService.makeMasterPasswordAuthenticationData.mockResolvedValue({
+          masterPasswordAuthenticationHash: "hash",
+        } as any);
+      });
+
+      it("should build request with master password secret", async () => {
+        const verification = {
+          type: VerificationType.MasterPassword,
+          secret: "password123",
+        } as any;
+
+        const result = await sut.buildRequest(verification);
+
+        expect(result.masterPasswordHash).toBe("hash");
+      });
+
+      it("should use default SecretVerificationRequest if no custom class provided", async () => {
+        const verification = {
+          type: VerificationType.MasterPassword,
+          secret: "password123",
+        } as any;
+
+        const result = await sut.buildRequest(verification);
+
+        expect(result).toHaveProperty("masterPasswordHash");
+      });
+
+      it("should get KDF config for the active user", async () => {
+        const verification = {
+          type: VerificationType.MasterPassword,
+          secret: "password123",
+        } as any;
+
+        await sut.buildRequest(verification);
+
+        expect(kdfConfigService.getKdfConfig).toHaveBeenCalledWith(mockUserId);
+      });
+
+      it("should get salt for the active user", async () => {
+        const verification = {
+          type: VerificationType.MasterPassword,
+          secret: "password123",
+        } as any;
+
+        await sut.buildRequest(verification);
+
+        expect(masterPasswordService.saltForUser$).toHaveBeenCalledWith(mockUserId);
+      });
+
+      it("should call makeMasterPasswordAuthenticationData with correct parameters", async () => {
+        const verification = {
+          type: VerificationType.MasterPassword,
+          secret: "password123",
+        } as any;
+
+        await sut.buildRequest(verification);
+
+        expect(masterPasswordService.makeMasterPasswordAuthenticationData).toHaveBeenCalledWith(
+          "password123",
+          "kdfConfig",
+          "salt",
+        );
+      });
+
+      it("should throw if master password secret is empty", async () => {
+        const verification = {
+          type: VerificationType.MasterPassword,
+          secret: "",
+        } as any;
+
+        await expect(sut.buildRequest(verification)).rejects.toThrow("Master Password is required");
+      });
+
+      it("should throw if master password secret is null", async () => {
+        const verification = {
+          type: VerificationType.MasterPassword,
+          secret: null,
+        } as any;
+
+        await expect(sut.buildRequest(verification)).rejects.toThrow("Master Password is required");
+      });
+    });
+  });
+
   describe("verifyUserByMasterPassword", () => {
     beforeAll(() => {
       i18nService.t.calledWith("invalidMasterPassword").mockReturnValue("Invalid master password");
@@ -228,7 +362,6 @@ describe("UserVerificationService", () => {
         expect(result).toEqual({
           policyOptions: null,
           masterKey: "masterKey",
-          kdfConfig: "kdfConfig",
           email: "email",
         });
       });
@@ -288,7 +421,6 @@ describe("UserVerificationService", () => {
         expect(result).toEqual({
           policyOptions: "MasterPasswordPolicyOptions",
           masterKey: "masterKey",
-          kdfConfig: "kdfConfig",
           email: "email",
         });
       });

libs/common/src/auth/services/user-verification/user-verification.service.ts

@@ -37,6 +37,7 @@ import {
   VerificationWithSecret,
   verificationHasSecret,
 } from "../../types/verification";
+import { getUserId } from "../account.service";
 
 /**
  * Used for general-purpose user verification throughout the app.
@@ -101,7 +102,6 @@ export class UserVerificationService implements UserVerificationServiceAbstracti
   async buildRequest<T extends SecretVerificationRequest>(
     verification: ServerSideVerification,
     requestClass?: new () => T,
-    alreadyHashed?: boolean,
   ) {
     this.validateSecretInput(verification);
 
@@ -111,20 +111,17 @@ export class UserVerificationService implements UserVerificationServiceAbstracti
     if (verification.type === VerificationType.OTP) {
       request.otp = verification.secret;
     } else {
-      const [userId, email] = await firstValueFrom(
-        this.accountService.activeAccount$.pipe(map((a) => [a?.id, a?.email])),
-      );
-      let masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(userId));
-      if (!masterKey && !alreadyHashed) {
-        masterKey = await this.keyService.makeMasterKey(
+      const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
+      const kdf = await this.kdfConfigService.getKdfConfig(userId as UserId);
+      const salt = await firstValueFrom(this.masterPasswordService.saltForUser$(userId as UserId));
+
+      const authenticationData =
+        await this.masterPasswordService.makeMasterPasswordAuthenticationData(
           verification.secret,
-          email,
-          await this.kdfConfigService.getKdfConfig(userId),
+          kdf,
+          salt,
         );
-      }
-      request.masterPasswordHash = alreadyHashed
-        ? verification.secret
-        : await this.keyService.hashMasterKey(verification.secret, masterKey);
+      request.authenticateWith(authenticationData);
     }
 
     return request;
@@ -239,7 +236,7 @@ export class UserVerificationService implements UserVerificationServiceAbstracti
     );
     await this.masterPasswordService.setMasterKeyHash(localKeyHash, userId);
     await this.masterPasswordService.setMasterKey(masterKey, userId);
-    return { policyOptions, masterKey, kdfConfig, email };
+    return { policyOptions, masterKey, email };
   }
 
   private async verifyUserByPIN(verification: PinVerification, userId: UserId): Promise<boolean> {

libs/common/src/auth/types/verification.ts

@@ -1,13 +1,13 @@
-// This import has been flagged as unallowed for this class. It may be involved in a circular dependency loop.
-// eslint-disable-next-line no-restricted-imports
-import { KdfConfig } from "@bitwarden/key-management";
-
 import { MasterKey } from "../../types/key";
 import { VerificationType } from "../enums/verification-type";
 import { MasterPasswordPolicyResponse } from "../models/response/master-password-policy.response";
 
 export type OtpVerification = { type: VerificationType.OTP; secret: string };
-export type MasterPasswordVerification = { type: VerificationType.MasterPassword; secret: string };
+export type MasterPasswordVerification = {
+  type: VerificationType.MasterPassword;
+  /** Secret here means the master password, *NOT* a hash of it */
+  secret: string;
+};
 export type PinVerification = { type: VerificationType.PIN; secret: string };
 export type BiometricsVerification = { type: VerificationType.Biometrics };
 
@@ -25,8 +25,8 @@ export function verificationHasSecret(
 export type ServerSideVerification = OtpVerification | MasterPasswordVerification;
 
 export type MasterPasswordVerificationResponse = {
+  /** @deprecated */
   masterKey: MasterKey;
-  kdfConfig: KdfConfig;
   email: string;
   policyOptions: MasterPasswordPolicyResponse | null;
 };

libs/key-management-ui/src/lock/components/lock.component.spec.ts

@@ -50,7 +50,6 @@ import {
   BiometricsStatus,
   BiometricStateService,
   KeyService,
-  PBKDF2KdfConfig,
   UserAsymmetricKeysRegenerationService,
 } from "@bitwarden/key-management";
 
@@ -494,7 +493,6 @@ describe("LockComponent", () => {
     const mockMasterKey = new SymmetricCryptoKey(new Uint8Array(64)) as MasterKey;
     const masterPasswordVerificationResponse: MasterPasswordVerificationResponse = {
       masterKey: mockMasterKey,
-      kdfConfig: new PBKDF2KdfConfig(600_001),
       email: "test-email@example.com",
       policyOptions: null,
     };