[PM-13673] Require UserId In CompareHash Method (#11568)

* Require UserId In CompareHash Method

* Throw on null-ish 'masterKey'

* Update Test

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

@@ -216,7 +216,7 @@ describe("UserVerificationService", () => {
       });
 
       it("returns if verification is successful", async () => {
-        keyService.compareAndUpdateKeyHash.mockResolvedValueOnce(true);
+        keyService.compareKeyHash.mockResolvedValueOnce(true);
 
         const result = await sut.verifyUserByMasterPassword(
           {
@@ -227,7 +227,7 @@ describe("UserVerificationService", () => {
           "email",
         );
 
-        expect(keyService.compareAndUpdateKeyHash).toHaveBeenCalled();
+        expect(keyService.compareKeyHash).toHaveBeenCalled();
         expect(masterPasswordService.setMasterKeyHash).toHaveBeenCalledWith(
           "localHash",
           mockUserId,
@@ -240,7 +240,7 @@ describe("UserVerificationService", () => {
       });
 
       it("throws if verification fails", async () => {
-        keyService.compareAndUpdateKeyHash.mockResolvedValueOnce(false);
+        keyService.compareKeyHash.mockResolvedValueOnce(false);
 
         await expect(
           sut.verifyUserByMasterPassword(
@@ -253,7 +253,7 @@ describe("UserVerificationService", () => {
           ),
         ).rejects.toThrow("Invalid master password");
 
-        expect(keyService.compareAndUpdateKeyHash).toHaveBeenCalled();
+        expect(keyService.compareKeyHash).toHaveBeenCalled();
         expect(masterPasswordService.setMasterKeyHash).not.toHaveBeenCalledWith();
         expect(masterPasswordService.setMasterKey).not.toHaveBeenCalledWith();
       });
@@ -285,7 +285,7 @@ describe("UserVerificationService", () => {
           "email",
         );
 
-        expect(keyService.compareAndUpdateKeyHash).not.toHaveBeenCalled();
+        expect(keyService.compareKeyHash).not.toHaveBeenCalled();
         expect(masterPasswordService.setMasterKeyHash).toHaveBeenCalledWith(
           "localHash",
           mockUserId,
@@ -318,7 +318,7 @@ describe("UserVerificationService", () => {
           ),
         ).rejects.toThrow("Invalid master password");
 
-        expect(keyService.compareAndUpdateKeyHash).not.toHaveBeenCalled();
+        expect(keyService.compareKeyHash).not.toHaveBeenCalled();
         expect(masterPasswordService.setMasterKeyHash).not.toHaveBeenCalledWith();
         expect(masterPasswordService.setMasterKey).not.toHaveBeenCalledWith();
       });

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

@@ -206,9 +206,10 @@ export class UserVerificationService implements UserVerificationServiceAbstracti
     let policyOptions: MasterPasswordPolicyResponse | null;
     // Client-side verification
     if (await this.hasMasterPasswordAndMasterKeyHash(userId)) {
-      const passwordValid = await this.keyService.compareAndUpdateKeyHash(
+      const passwordValid = await this.keyService.compareKeyHash(
         verification.secret,
         masterKey,
+        userId,
       );
       if (!passwordValid) {
         throw new Error(this.i18nService.t("invalidMasterPassword"));

libs/key-management/src/abstractions/key.service.ts

@@ -204,14 +204,18 @@ export abstract class KeyService {
     hashPurpose?: HashPurpose,
   ): Promise<string>;
   /**
-   * Compares the provided master password to the stored password hash and server password hash.
-   * Updates the stored hash if outdated.
+   * Compares the provided master password to the stored password hash.
    * @param masterPassword The user's master password
    * @param key The user's master key
+   * @param userId The id of the user to do the operation for.
    * @returns True if the provided master password matches either the stored
    * key hash or the server key hash
    */
-  abstract compareAndUpdateKeyHash(masterPassword: string, masterKey: MasterKey): Promise<boolean>;
+  abstract compareKeyHash(
+    masterPassword: string,
+    masterKey: MasterKey,
+    userId: UserId,
+  ): Promise<boolean>;
   /**
    * Stores the encrypted organization keys and clears any decrypted
    * organization keys currently in memory

libs/key-management/src/key.service.spec.ts

@@ -733,4 +733,63 @@ describe("keyService", () => {
       });
     });
   });
+
+  describe("compareKeyHash", () => {
+    type TestCase = {
+      masterKey: MasterKey;
+      masterPassword: string | null;
+      storedMasterKeyHash: string;
+      mockReturnedHash: string;
+      expectedToMatch: boolean;
+    };
+
+    const data: TestCase[] = [
+      {
+        masterKey: makeSymmetricCryptoKey(64),
+        masterPassword: "my_master_password",
+        storedMasterKeyHash: "bXlfaGFzaA==",
+        mockReturnedHash: "bXlfaGFzaA==",
+        expectedToMatch: true,
+      },
+      {
+        masterKey: makeSymmetricCryptoKey(64),
+        masterPassword: null,
+        storedMasterKeyHash: "bXlfaGFzaA==",
+        mockReturnedHash: "bXlfaGFzaA==",
+        expectedToMatch: false,
+      },
+      {
+        masterKey: makeSymmetricCryptoKey(64),
+        masterPassword: null,
+        storedMasterKeyHash: null,
+        mockReturnedHash: "bXlfaGFzaA==",
+        expectedToMatch: false,
+      },
+    ];
+
+    it.each(data)(
+      "returns expected match value when calculated hash equals stored hash",
+      async ({
+        masterKey,
+        masterPassword,
+        storedMasterKeyHash,
+        mockReturnedHash,
+        expectedToMatch,
+      }) => {
+        masterPasswordService.masterKeyHashSubject.next(storedMasterKeyHash);
+
+        cryptoFunctionService.pbkdf2
+          .calledWith(masterKey.key, masterPassword, "sha256", 2)
+          .mockResolvedValue(Utils.fromB64ToArray(mockReturnedHash));
+
+        const actualDidMatch = await keyService.compareKeyHash(
+          masterPassword,
+          masterKey,
+          mockUserId,
+        );
+
+        expect(actualDidMatch).toBe(expectedToMatch);
+      },
+    );
+  });
 });

libs/key-management/src/key.service.ts

@@ -319,36 +319,45 @@ export class DefaultKeyService implements KeyServiceAbstraction {
   }
 
   // TODO: move to MasterPasswordService
-  async compareAndUpdateKeyHash(masterPassword: string, masterKey: MasterKey): Promise<boolean> {
-    const userId = await firstValueFrom(this.stateProvider.activeUserId$);
+  async compareKeyHash(
+    masterPassword: string,
+    masterKey: MasterKey,
+    userId: UserId,
+  ): Promise<boolean> {
+    if (masterKey == null) {
+      throw new Error("'masterKey' is required to be non-null.");
+    }
+
+    if (masterPassword == null) {
+      // If they don't give us a master password, we can't hash it, and therefore
+      // it will never match what we have stored.
+      return false;
+    }
+
+    // Retrieve the current password hash
     const storedPasswordHash = await firstValueFrom(
       this.masterPasswordService.masterKeyHash$(userId),
     );
-    if (masterPassword != null && storedPasswordHash != null) {
+
+    if (storedPasswordHash == null) {
+      return false;
+    }
+
+    // Hash the key for local use
     const localKeyHash = await this.hashMasterKey(
       masterPassword,
       masterKey,
       HashPurpose.LocalAuthorization,
     );
-      if (localKeyHash != null && storedPasswordHash === localKeyHash) {
-        return true;
-      }
-
-      // TODO: remove serverKeyHash check in 1-2 releases after everyone's keyHash has been updated
-      const serverKeyHash = await this.hashMasterKey(
-        masterPassword,
-        masterKey,
-        HashPurpose.ServerAuthorization,
-      );
-      if (serverKeyHash != null && storedPasswordHash === serverKeyHash) {
-        await this.masterPasswordService.setMasterKeyHash(localKeyHash, userId);
-        return true;
-      }
-    }
 
+    // Check if the stored hash is already equal to the hash we create locally
+    if (localKeyHash == null || storedPasswordHash !== localKeyHash) {
       return false;
     }
 
+    return true;
+  }
+
   async setOrgKeys(
     orgs: ProfileOrganizationResponse[],
     providerOrgs: ProfileProviderOrganizationResponse[],

libs/vault/src/components/password-reprompt.component.ts

@@ -1,8 +1,10 @@
 import { DialogRef } from "@angular/cdk/dialog";
 import { Component } from "@angular/core";
 import { FormBuilder, ReactiveFormsModule, Validators } from "@angular/forms";
+import { firstValueFrom, map } from "rxjs";
 
 import { JslibModule } from "@bitwarden/angular/jslib.module";
+import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
 import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
 import {
@@ -43,16 +45,25 @@ export class PasswordRepromptComponent {
     protected i18nService: I18nService,
     protected formBuilder: FormBuilder,
     protected dialogRef: DialogRef,
+    protected accountService: AccountService,
   ) {}
 
   submit = async () => {
+    const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(map((a) => a?.id)));
+
+    if (userId == null) {
+      throw new Error("An active user is expected while doing password reprompt.");
+    }
+
     const storedMasterKey = await this.keyService.getOrDeriveMasterKey(
       this.formGroup.value.masterPassword,
+      userId,
     );
     if (
-      !(await this.keyService.compareAndUpdateKeyHash(
+      !(await this.keyService.compareKeyHash(
         this.formGroup.value.masterPassword,
         storedMasterKey,
+        userId,
       ))
     ) {
       this.platformUtilsService.showToast(