[PM-22611] Require userid for masterKey methods on the key service (#15663)

* Require userId on targeted methods. * update method consumers * unit tests
2025-12-06 00:13:28 +00:00 · 2025-07-25 09:37:04 -05:00
parent b358d5663d
commit 2db31d1228
6 changed files with 233 additions and 88 deletions
--- a/libs/key-management/src/abstractions/key.service.ts
+++ b/libs/key-management/src/abstractions/key.service.ts
@@ -163,11 +163,14 @@ export abstract class KeyService {
   */
  abstract clearStoredUserKey(keySuffix: KeySuffixOptions, userId: string): Promise<void>;
  /**
-   * @throws Error when userId is null and no active user
+   * Retrieves the user's master key if it is in state, or derives it from the provided password
   * @param password The user's master password that will be used to derive a master key if one isn't found
   * @param userId The desired user
+   * @throws Error when userId is null/undefined.
+   * @throws Error when email or Kdf configuration cannot be found for the user.
+   * @returns The user's master key if it exists, or a newly derived master key.
   */
-  abstract getOrDeriveMasterKey(password: string, userId?: string): Promise<MasterKey>;
+  abstract getOrDeriveMasterKey(password: string, userId: UserId): Promise<MasterKey>;
  /**
   * Generates a master key from the provided password
   * @param password The user's master password
@@ -175,7 +178,7 @@ export abstract class KeyService {
   * @param KdfConfig The user's key derivation function configuration
   * @returns A master key derived from the provided password
   */
-  abstract makeMasterKey(password: string, email: string, KdfConfig: KdfConfig): Promise<MasterKey>;
+  abstract makeMasterKey(password: string, email: string, kdfConfig: KdfConfig): Promise<MasterKey>;
  /**
   * Encrypts the existing (or provided) user key with the
   * provided master key
@@ -191,24 +194,25 @@ export abstract class KeyService {
   * Creates a master password hash from the user's master password. Can
   * be used for local authentication or for server authentication depending
   * on the hashPurpose provided.
-   * @throws Error when password is null or key is null and no active user or active user have no master key
   * @param password The user's master password
   * @param key The user's master key or active's user master key.
-   * @param hashPurpose The iterations to use for the hash
+   * @param hashPurpose The iterations to use for the hash. Defaults to {@link HashPurpose.ServerAuthorization}.
+   * @throws Error when password is null/undefined or key is null/undefined.
   * @returns The user's master password hash
   */
  abstract hashMasterKey(
    password: string,
-    key: MasterKey | null,
+    key: MasterKey,
    hashPurpose?: HashPurpose,
  ): Promise<string>;
  /**
   * 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 masterKey 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
+   * @throws Error when master key is null/undefined.
+   * @returns True if the derived master password hash matches the stored
+   * key hash, false otherwise.
   */
  abstract compareKeyHash(
    masterPassword: string,
--- a/libs/key-management/src/key.service.spec.ts
+++ b/libs/key-management/src/key.service.spec.ts
@@ -18,7 +18,7 @@ import { KeyGenerationService } from "@bitwarden/common/platform/abstractions/ke
 import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
 import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
 import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
-import { KeySuffixOptions } from "@bitwarden/common/platform/enums";
+import { HashPurpose, KeySuffixOptions } from "@bitwarden/common/platform/enums";
 import { Encrypted } from "@bitwarden/common/platform/interfaces/encrypted";
 import { Utils } from "@bitwarden/common/platform/misc/utils";
 import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key";
@@ -47,6 +47,7 @@ import { UserKey, MasterKey } from "@bitwarden/common/types/key";
 import { KdfConfigService } from "./abstractions/kdf-config.service";
 import { UserPrivateKeyDecryptionFailedError } from "./abstractions/key.service";
 import { DefaultKeyService } from "./key.service";
+import { KdfConfig } from "./models/kdf-config";

 describe("keyService", () => {
  let keyService: DefaultKeyService;
@@ -817,55 +818,160 @@ describe("keyService", () => {
  });

  describe("getOrDeriveMasterKey", () => {
+    beforeEach(() => {
+      masterPasswordService.masterKeySubject.next(null);
+    });
+
+    test.each([null as unknown as UserId, undefined as unknown as UserId])(
+      "throws when the provided userId is %s",
+      async (userId) => {
+        await expect(keyService.getOrDeriveMasterKey("password", userId)).rejects.toThrow(
+          "User ID is required.",
+        );
+      },
+    );
+
    it("returns the master key if it is already available", async () => {
-      const getMasterKey = jest
-        .spyOn(masterPasswordService, "masterKey$")
-        .mockReturnValue(of("masterKey" as any));
+      const masterKey = makeSymmetricCryptoKey(32) as MasterKey;
+      masterPasswordService.masterKeySubject.next(masterKey);

      const result = await keyService.getOrDeriveMasterKey("password", mockUserId);

-      expect(getMasterKey).toHaveBeenCalledWith(mockUserId);
-      expect(result).toEqual("masterKey");
+      expect(kdfConfigService.getKdfConfig$).not.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));
+    it("throws an error if user's email is not available", async () => {
+      accountService.accounts$ = of({});

-      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");
+      await expect(keyService.getOrDeriveMasterKey("password", mockUserId)).rejects.toThrow(
+        "No email found for user " + mockUserId,
+      );
+      expect(kdfConfigService.getKdfConfig$).not.toHaveBeenCalled();
    });

    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",
      );
    });
+
+    it("derives the master key if it is not available", async () => {
+      keyGenerationService.deriveKeyFromPassword.mockReturnValue("mockMasterKey" as any);
+      kdfConfigService.getKdfConfig$.mockReturnValue(of("mockKdfConfig" as any));
+
+      const result = await keyService.getOrDeriveMasterKey("password", mockUserId);
+
+      expect(kdfConfigService.getKdfConfig$).toHaveBeenCalledWith(mockUserId);
+      expect(keyGenerationService.deriveKeyFromPassword).toHaveBeenCalledWith(
+        "password",
+        "email",
+        "mockKdfConfig",
+      );
+      expect(result).toEqual("mockMasterKey");
+    });
+  });
+
+  describe("makeMasterKey", () => {
+    const password = "testPassword";
+    let email = "test@example.com";
+    const masterKey = makeSymmetricCryptoKey(32) as MasterKey;
+    const kdfConfig = mock<KdfConfig>();
+
+    it("derives a master key from password and email", async () => {
+      keyGenerationService.deriveKeyFromPassword.mockResolvedValue(masterKey);
+
+      const result = await keyService.makeMasterKey(password, email, kdfConfig);
+
+      expect(result).toEqual(masterKey);
+    });
+
+    it("trims and lowercases the email for key generation call", async () => {
+      keyGenerationService.deriveKeyFromPassword.mockResolvedValue(masterKey);
+      email = "TEST@EXAMPLE.COM";
+
+      await keyService.makeMasterKey(password, email, kdfConfig);
+
+      expect(keyGenerationService.deriveKeyFromPassword).toHaveBeenCalledWith(
+        password,
+        email.trim().toLowerCase(),
+        kdfConfig,
+      );
+    });
+
+    it("should log the time taken to derive the master key", async () => {
+      keyGenerationService.deriveKeyFromPassword.mockResolvedValue(masterKey);
+      jest.spyOn(Date.prototype, "getTime").mockReturnValueOnce(1000).mockReturnValueOnce(1500);
+
+      await keyService.makeMasterKey(password, email, kdfConfig);
+
+      expect(logService.info).toHaveBeenCalledWith("[KeyService] Deriving master key took 500ms");
+    });
+  });
+
+  describe("hashMasterKey", () => {
+    const password = "testPassword";
+    const masterKey = makeSymmetricCryptoKey(32) as MasterKey;
+
+    test.each([null as unknown as string, undefined as unknown as string])(
+      "throws when the provided password is %s",
+      async (password) => {
+        await expect(keyService.hashMasterKey(password, masterKey)).rejects.toThrow(
+          "password is required.",
+        );
+      },
+    );
+
+    test.each([null as unknown as MasterKey, undefined as unknown as MasterKey])(
+      "throws when the provided key is %s",
+      async (key) => {
+        await expect(keyService.hashMasterKey("password", key)).rejects.toThrow("key is required.");
+      },
+    );
+
+    it("hashes master key with default iterations when no hashPurpose is provided", async () => {
+      const mockReturnedHashB64 = "bXlfaGFzaA==";
+      cryptoFunctionService.pbkdf2.mockResolvedValue(Utils.fromB64ToArray(mockReturnedHashB64));
+
+      const result = await keyService.hashMasterKey(password, masterKey);
+
+      expect(cryptoFunctionService.pbkdf2).toHaveBeenCalledWith(
+        masterKey.inner().encryptionKey,
+        password,
+        "sha256",
+        1,
+      );
+      expect(result).toBe(mockReturnedHashB64);
+    });
+
+    test.each([
+      [2, HashPurpose.LocalAuthorization],
+      [1, HashPurpose.ServerAuthorization],
+    ])(
+      "hashes master key with %s iterations when hashPurpose is %s",
+      async (expectedIterations, hashPurpose) => {
+        const mockReturnedHashB64 = "bXlfaGFzaA==";
+        cryptoFunctionService.pbkdf2.mockResolvedValue(Utils.fromB64ToArray(mockReturnedHashB64));
+
+        const result = await keyService.hashMasterKey(password, masterKey, hashPurpose);
+
+        expect(cryptoFunctionService.pbkdf2).toHaveBeenCalledWith(
+          masterKey.inner().encryptionKey,
+          password,
+          "sha256",
+          expectedIterations,
+        );
+        expect(result).toBe(mockReturnedHashB64);
+      },
+    );
  });

  describe("compareKeyHash", () => {
    type TestCase = {
      masterKey: MasterKey;
-      masterPassword: string | null;
+      masterPassword: string;
      storedMasterKeyHash: string | null;
      mockReturnedHash: string;
      expectedToMatch: boolean;
@@ -873,26 +979,33 @@ describe("keyService", () => {

    const data: TestCase[] = [
      {
-        masterKey: makeSymmetricCryptoKey(64),
+        masterKey: makeSymmetricCryptoKey(32),
        masterPassword: "my_master_password",
        storedMasterKeyHash: "bXlfaGFzaA==",
        mockReturnedHash: "bXlfaGFzaA==",
        expectedToMatch: true,
      },
      {
-        masterKey: makeSymmetricCryptoKey(64),
-        masterPassword: null,
+        masterKey: makeSymmetricCryptoKey(32),
+        masterPassword: null as unknown as string,
        storedMasterKeyHash: "bXlfaGFzaA==",
        mockReturnedHash: "bXlfaGFzaA==",
        expectedToMatch: false,
      },
      {
-        masterKey: makeSymmetricCryptoKey(64),
-        masterPassword: null,
+        masterKey: makeSymmetricCryptoKey(32),
+        masterPassword: null as unknown as string,
        storedMasterKeyHash: null,
        mockReturnedHash: "bXlfaGFzaA==",
        expectedToMatch: false,
      },
+      {
+        masterKey: makeSymmetricCryptoKey(32),
+        masterPassword: "my_master_password",
+        storedMasterKeyHash: "bXlfaGFzaA==",
+        mockReturnedHash: "zxccbXlfaGFzaA==",
+        expectedToMatch: false,
+      },
    ];

    it.each(data)(
@@ -907,7 +1020,7 @@ describe("keyService", () => {
        masterPasswordService.masterKeyHashSubject.next(storedMasterKeyHash);

        cryptoFunctionService.pbkdf2
-          .calledWith(masterKey.inner().encryptionKey, masterPassword as string, "sha256", 2)
+          .calledWith(masterKey.inner().encryptionKey, masterPassword, "sha256", 2)
          .mockResolvedValue(Utils.fromB64ToArray(mockReturnedHash));

        const actualDidMatch = await keyService.compareKeyHash(
@@ -919,6 +1032,38 @@ describe("keyService", () => {
        expect(actualDidMatch).toBe(expectedToMatch);
      },
    );
+
+    test.each([null as unknown as MasterKey, undefined as unknown as MasterKey])(
+      "throws an error if masterKey is %s",
+      async (masterKey) => {
+        await expect(
+          keyService.compareKeyHash("my_master_password", masterKey, mockUserId),
+        ).rejects.toThrow("'masterKey' is required to be non-null.");
+      },
+    );
+
+    test.each([null as unknown as string, undefined as unknown as string])(
+      "returns false when masterPassword is %s",
+      async (masterPassword) => {
+        const result = await keyService.compareKeyHash(
+          masterPassword,
+          makeSymmetricCryptoKey(32),
+          mockUserId,
+        );
+        expect(result).toBe(false);
+      },
+    );
+
+    it("returns false when storedMasterKeyHash is null", async () => {
+      masterPasswordService.masterKeyHashSubject.next(null);
+
+      const result = await keyService.compareKeyHash(
+        "my_master_password",
+        makeSymmetricCryptoKey(32),
+        mockUserId,
+      );
+      expect(result).toBe(false);
+    });
  });

  describe("userPrivateKey$", () => {
--- a/libs/key-management/src/key.service.ts
+++ b/libs/key-management/src/key.service.ts
@@ -259,28 +259,28 @@ export class DefaultKeyService implements KeyServiceAbstraction {
    }
  }

-  // TODO: Move to MasterPasswordService
-  async getOrDeriveMasterKey(password: string, userId?: UserId) {
-    const [resolvedUserId, email] = await firstValueFrom(
-      combineLatest([this.accountService.activeAccount$, this.accountService.accounts$]).pipe(
-        map(([activeAccount, accounts]) => {
-          userId ??= activeAccount?.id;
-          if (userId == null || accounts[userId] == null) {
-            throw new Error("No user found");
-          }
-          return [userId, accounts[userId].email];
-        }),
-      ),
-    );
-    const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(resolvedUserId));
+  async getOrDeriveMasterKey(password: string, userId: UserId): Promise<MasterKey> {
+    if (userId == null) {
+      throw new Error("User ID is required.");
+    }
+
+    const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(userId));
    if (masterKey != null) {
      return masterKey;
    }

-    const kdf = await firstValueFrom(this.kdfConfigService.getKdfConfig$(resolvedUserId));
-    if (kdf == null) {
-      throw new Error("No kdf found for user");
+    const email = await firstValueFrom(
+      this.accountService.accounts$.pipe(map((accounts) => accounts[userId]?.email)),
+    );
+    if (email == null) {
+      throw new Error("No email found for user " + userId);
    }
+
+    const kdf = await firstValueFrom(this.kdfConfigService.getKdfConfig$(userId));
+    if (kdf == null) {
+      throw new Error("No kdf found for user " + userId);
+    }
+
    return await this.makeMasterKey(password, email, kdf);
  }

@@ -289,14 +289,14 @@ export class DefaultKeyService implements KeyServiceAbstraction {
   *
   * @remarks
   * Does not validate the kdf config to ensure it satisfies the minimum requirements for the given kdf type.
-   * TODO: Move to MasterPasswordService
   */
-  async makeMasterKey(password: string, email: string, KdfConfig: KdfConfig): Promise<MasterKey> {
+  async makeMasterKey(password: string, email: string, kdfConfig: KdfConfig): Promise<MasterKey> {
    const start = new Date().getTime();
+    email = email.trim().toLowerCase();
    const masterKey = (await this.keyGenerationService.deriveKeyFromPassword(
      password,
      email,
-      KdfConfig,
+      kdfConfig,
    )) as MasterKey;
    const end = new Date().getTime();
    this.logService.info(`[KeyService] Deriving master key took ${end - start}ms`);
@@ -312,23 +312,16 @@ export class DefaultKeyService implements KeyServiceAbstraction {
    return await this.buildProtectedSymmetricKey(masterKey, userKey);
  }

-  // TODO: move to MasterPasswordService
  async hashMasterKey(
    password: string,
-    key: MasterKey | null,
+    key: MasterKey,
    hashPurpose?: HashPurpose,
  ): Promise<string> {
-    if (key == null) {
-      const userId = await firstValueFrom(this.stateProvider.activeUserId$);
-      if (userId == null) {
-        throw new Error("No active user found.");
-      }
-
-      key = await firstValueFrom(this.masterPasswordService.masterKey$(userId));
+    if (password == null) {
+      throw new Error("password is required.");
    }
-
-    if (password == null || key == null) {
-      throw new Error("Invalid parameters.");
+    if (key == null) {
+      throw new Error("key is required.");
    }

    const iterations = hashPurpose === HashPurpose.LocalAuthorization ? 2 : 1;
@@ -341,9 +334,8 @@ export class DefaultKeyService implements KeyServiceAbstraction {
    return Utils.fromBufferToB64(hash);
  }

-  // TODO: move to MasterPasswordService
  async compareKeyHash(
-    masterPassword: string | null,
+    masterPassword: string,
    masterKey: MasterKey,
    userId: UserId,
  ): Promise<boolean> {