[PM-22090] Delete password on Windows desktop throws incorrect error (#15070)

* delete password on Windows desktop throws incorrect error

* delete password on Windows desktop throws incorrect error

* napi documentation improvements

* napi documentation update

* better logging verbosity

* desktop native clippy errors

* unit test coverage

* napi TS documentation JS language friendly

* fixing merge conflicts

apps/desktop/src/key-management/biometrics/main-biometrics-ipc.listener.ts

@@ -55,7 +55,7 @@ export class MainBiometricsIPCListener {
             return;
         }
       } catch (e) {
-        this.logService.info(e);
+        this.logService.error("[Main Biometrics IPC Listener] %s failed", message.action, e);
       }
     });
   }

apps/desktop/src/key-management/biometrics/main-biometrics.service.ts

@@ -40,7 +40,7 @@ export class MainBiometricsService extends DesktopBiometricsService {
     } else if (platform === "darwin") {
       // eslint-disable-next-line
       const OsBiometricsServiceMac = require("./os-biometrics-mac.service").default;
-      this.osBiometricsService = new OsBiometricsServiceMac(this.i18nService);
+      this.osBiometricsService = new OsBiometricsServiceMac(this.i18nService, this.logService);
     } else if (platform === "linux") {
       // eslint-disable-next-line
       const OsBiometricsServiceLinux = require("./os-biometrics-linux.service").default;
@@ -48,6 +48,7 @@ export class MainBiometricsService extends DesktopBiometricsService {
         this.biometricStateService,
         this.encryptService,
         this.cryptoFunctionService,
+        this.logService,
       );
     } else {
       throw new Error("Unsupported platform");

apps/desktop/src/key-management/biometrics/os-biometrics-linux.service.spec.ts

@@ -0,0 +1,86 @@
+import { mock } from "jest-mock-extended";
+
+import { CryptoFunctionService } from "@bitwarden/common/key-management/crypto/abstractions/crypto-function.service";
+import { EncryptService } from "@bitwarden/common/key-management/crypto/abstractions/encrypt.service";
+import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
+import { UserId } from "@bitwarden/common/types/guid";
+import { passwords } from "@bitwarden/desktop-napi";
+import { BiometricStateService } from "@bitwarden/key-management";
+
+import OsBiometricsServiceLinux from "./os-biometrics-linux.service";
+
+jest.mock("@bitwarden/desktop-napi", () => ({
+  biometrics: {
+    setBiometricSecret: jest.fn(),
+    getBiometricSecret: jest.fn(),
+    deleteBiometricSecret: jest.fn(),
+    prompt: jest.fn(),
+    available: jest.fn(),
+    deriveKeyMaterial: jest.fn(),
+  },
+  passwords: {
+    deletePassword: jest.fn(),
+    getPassword: jest.fn(),
+    isAvailable: jest.fn(),
+    PASSWORD_NOT_FOUND: "Password not found",
+  },
+}));
+
+describe("OsBiometricsServiceLinux", () => {
+  let service: OsBiometricsServiceLinux;
+  let logService: LogService;
+
+  const mockUserId = "test-user-id" as UserId;
+
+  beforeEach(() => {
+    const biometricStateService = mock<BiometricStateService>();
+    const encryptService = mock<EncryptService>();
+    const cryptoFunctionService = mock<CryptoFunctionService>();
+    logService = mock<LogService>();
+    service = new OsBiometricsServiceLinux(
+      biometricStateService,
+      encryptService,
+      cryptoFunctionService,
+      logService,
+    );
+  });
+
+  afterEach(() => {
+    jest.clearAllMocks();
+  });
+
+  describe("deleteBiometricKey", () => {
+    const serviceName = "Bitwarden_biometric";
+    const keyName = "test-user-id_user_biometric";
+
+    it("should delete biometric key successfully", async () => {
+      await service.deleteBiometricKey(mockUserId);
+
+      expect(passwords.deletePassword).toHaveBeenCalledWith(serviceName, keyName);
+    });
+
+    it("should not throw error if key not found", async () => {
+      passwords.deletePassword = jest
+        .fn()
+        .mockRejectedValueOnce(new Error(passwords.PASSWORD_NOT_FOUND));
+
+      await service.deleteBiometricKey(mockUserId);
+
+      expect(passwords.deletePassword).toHaveBeenCalledWith(serviceName, keyName);
+      expect(logService.debug).toHaveBeenCalledWith(
+        "[OsBiometricService] Biometric key %s not found for service %s.",
+        keyName,
+        serviceName,
+      );
+    });
+
+    it("should throw error for unexpected errors", async () => {
+      const error = new Error("Unexpected error");
+      passwords.deletePassword = jest.fn().mockRejectedValueOnce(error);
+
+      await expect(service.deleteBiometricKey(mockUserId)).rejects.toThrow(error);
+
+      expect(passwords.deletePassword).toHaveBeenCalledWith(serviceName, keyName);
+    });
+  });
+});

apps/desktop/src/key-management/biometrics/os-biometrics-linux.service.ts

@@ -2,6 +2,7 @@ import { spawn } from "child_process";
 
 import { CryptoFunctionService } from "@bitwarden/common/key-management/crypto/abstractions/crypto-function.service";
 import { EncryptService } from "@bitwarden/common/key-management/crypto/abstractions/encrypt.service";
+import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
 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";
@@ -42,6 +43,7 @@ export default class OsBiometricsServiceLinux implements OsBiometricService {
     private biometricStateService: BiometricStateService,
     private encryptService: EncryptService,
     private cryptoFunctionService: CryptoFunctionService,
+    private logService: LogService,
   ) {}
   private _iv: string | null = null;
   // Use getKeyMaterial helper instead of direct access
@@ -62,7 +64,19 @@ export default class OsBiometricsServiceLinux implements OsBiometricService {
     );
   }
   async deleteBiometricKey(userId: UserId): Promise<void> {
-    await passwords.deletePassword(SERVICE, getLookupKeyForUser(userId));
+    try {
+      await passwords.deletePassword(SERVICE, getLookupKeyForUser(userId));
+    } catch (e) {
+      if (e instanceof Error && e.message === passwords.PASSWORD_NOT_FOUND) {
+        this.logService.debug(
+          "[OsBiometricService] Biometric key %s not found for service %s.",
+          getLookupKeyForUser(userId),
+          SERVICE,
+        );
+      } else {
+        throw e;
+      }
+    }
   }
 
   async getBiometricKey(userId: UserId): Promise<SymmetricCryptoKey | null> {

apps/desktop/src/key-management/biometrics/os-biometrics-mac.service.spec.ts

@@ -0,0 +1,78 @@
+import { mock } from "jest-mock-extended";
+
+import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
+import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
+import { UserId } from "@bitwarden/common/types/guid";
+import { passwords } from "@bitwarden/desktop-napi";
+
+import OsBiometricsServiceMac from "./os-biometrics-mac.service";
+
+jest.mock("@bitwarden/desktop-napi", () => ({
+  biometrics: {
+    setBiometricSecret: jest.fn(),
+    getBiometricSecret: jest.fn(),
+    deleteBiometricSecret: jest.fn(),
+    prompt: jest.fn(),
+    available: jest.fn(),
+    deriveKeyMaterial: jest.fn(),
+  },
+  passwords: {
+    deletePassword: jest.fn(),
+    getPassword: jest.fn(),
+    isAvailable: jest.fn(),
+    PASSWORD_NOT_FOUND: "Password not found",
+  },
+}));
+
+describe("OsBiometricsServiceMac", () => {
+  let service: OsBiometricsServiceMac;
+  let i18nService: I18nService;
+  let logService: LogService;
+
+  const mockUserId = "test-user-id" as UserId;
+
+  beforeEach(() => {
+    i18nService = mock<I18nService>();
+    logService = mock<LogService>();
+    service = new OsBiometricsServiceMac(i18nService, logService);
+  });
+
+  afterEach(() => {
+    jest.clearAllMocks();
+  });
+
+  describe("deleteBiometricKey", () => {
+    const serviceName = "Bitwarden_biometric";
+    const keyName = "test-user-id_user_biometric";
+
+    it("should delete biometric key successfully", async () => {
+      await service.deleteBiometricKey(mockUserId);
+
+      expect(passwords.deletePassword).toHaveBeenCalledWith(serviceName, keyName);
+    });
+
+    it("should not throw error if key not found", async () => {
+      passwords.deletePassword = jest
+        .fn()
+        .mockRejectedValueOnce(new Error(passwords.PASSWORD_NOT_FOUND));
+
+      await service.deleteBiometricKey(mockUserId);
+
+      expect(passwords.deletePassword).toHaveBeenCalledWith(serviceName, keyName);
+      expect(logService.debug).toHaveBeenCalledWith(
+        "[OsBiometricService] Biometric key %s not found for service %s.",
+        keyName,
+        serviceName,
+      );
+    });
+
+    it("should throw error for unexpected errors", async () => {
+      const error = new Error("Unexpected error");
+      passwords.deletePassword = jest.fn().mockRejectedValueOnce(error);
+
+      await expect(service.deleteBiometricKey(mockUserId)).rejects.toThrow(error);
+
+      expect(passwords.deletePassword).toHaveBeenCalledWith(serviceName, keyName);
+    });
+  });
+});

apps/desktop/src/key-management/biometrics/os-biometrics-mac.service.ts

@@ -1,6 +1,7 @@
 import { systemPreferences } from "electron";
 
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
+import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
 import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key";
 import { UserId } from "@bitwarden/common/types/guid";
 import { passwords } from "@bitwarden/desktop-napi";
@@ -14,7 +15,10 @@ function getLookupKeyForUser(userId: UserId): string {
 }
 
 export default class OsBiometricsServiceMac implements OsBiometricService {
-  constructor(private i18nservice: I18nService) {}
+  constructor(
+    private i18nservice: I18nService,
+    private logService: LogService,
+  ) {}
 
   async supportsBiometrics(): Promise<boolean> {
     return systemPreferences.canPromptTouchID();
@@ -52,7 +56,19 @@ export default class OsBiometricsServiceMac implements OsBiometricService {
   }
 
   async deleteBiometricKey(user: UserId): Promise<void> {
-    return await passwords.deletePassword(SERVICE, getLookupKeyForUser(user));
+    try {
+      return await passwords.deletePassword(SERVICE, getLookupKeyForUser(user));
+    } catch (e) {
+      if (e instanceof Error && e.message === passwords.PASSWORD_NOT_FOUND) {
+        this.logService.debug(
+          "[OsBiometricService] Biometric key %s not found for service %s.",
+          getLookupKeyForUser(user),
+          SERVICE,
+        );
+      } else {
+        throw e;
+      }
+    }
   }
 
   private async valueUpToDate(user: UserId, key: SymmetricCryptoKey): Promise<boolean> {

apps/desktop/src/key-management/biometrics/os-biometrics-windows.service.spec.ts

@@ -6,8 +6,11 @@ import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.servic
 import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
 import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key";
 import { UserId } from "@bitwarden/common/types/guid";
+import { passwords } from "@bitwarden/desktop-napi";
 import { BiometricsStatus, BiometricStateService } from "@bitwarden/key-management";
 
+import { WindowMain } from "../../main/window.main";
+
 import OsBiometricsServiceWindows from "./os-biometrics-windows.service";
 
 jest.mock("@bitwarden/desktop-napi", () => ({
@@ -15,28 +18,37 @@ jest.mock("@bitwarden/desktop-napi", () => ({
     available: jest.fn(),
     setBiometricSecret: jest.fn(),
     getBiometricSecret: jest.fn(),
-    deriveKeyMaterial: jest.fn(),
+    deleteBiometricSecret: jest.fn(),
     prompt: jest.fn(),
+    deriveKeyMaterial: jest.fn(),
   },
   passwords: {
     getPassword: jest.fn(),
     deletePassword: jest.fn(),
+    isAvailable: jest.fn(),
+    PASSWORD_NOT_FOUND: "Password not found",
   },
 }));
 
 describe("OsBiometricsServiceWindows", () => {
   let service: OsBiometricsServiceWindows;
+  let i18nService: I18nService;
+  let windowMain: WindowMain;
+  let logService: LogService;
   let biometricStateService: BiometricStateService;
 
+  const mockUserId = "test-user-id" as UserId;
+
   beforeEach(() => {
-    const i18nService = mock<I18nService>();
-    const logService = mock<LogService>();
+    i18nService = mock<I18nService>();
+    windowMain = mock<WindowMain>();
+    logService = mock<LogService>();
     biometricStateService = mock<BiometricStateService>();
     const encryptionService = mock<EncryptService>();
     const cryptoFunctionService = mock<CryptoFunctionService>();
     service = new OsBiometricsServiceWindows(
       i18nService,
-      null,
+      windowMain,
       logService,
       biometricStateService,
       encryptionService,
@@ -81,7 +93,7 @@ describe("OsBiometricsServiceWindows", () => {
       cryptoFunctionService = mock<CryptoFunctionService>();
       service = new OsBiometricsServiceWindows(
         mock<I18nService>(),
-        null,
+        windowMain,
         mock<LogService>(),
         biometricStateService,
         encryptionService,
@@ -140,4 +152,97 @@ describe("OsBiometricsServiceWindows", () => {
       expect((service as any).clientKeyHalves.get(userId.toString())).toBeNull();
     });
   });
+
+  describe("deleteBiometricKey", () => {
+    const serviceName = "Bitwarden_biometric";
+    const keyName = "test-user-id_user_biometric";
+    const witnessKeyName = "test-user-id_user_biometric_witness";
+
+    it("should delete biometric key successfully", async () => {
+      await service.deleteBiometricKey(mockUserId);
+
+      expect(passwords.deletePassword).toHaveBeenCalledWith(serviceName, keyName);
+      expect(passwords.deletePassword).toHaveBeenCalledWith(serviceName, witnessKeyName);
+    });
+
+    it.each([
+      [false, false],
+      [false, true],
+      [true, false],
+    ])(
+      "should not throw error if key found: %s and witness key found: %s",
+      async (keyFound, witnessKeyFound) => {
+        passwords.deletePassword = jest.fn().mockImplementation((_, account) => {
+          if (account === keyName) {
+            if (!keyFound) {
+              throw new Error(passwords.PASSWORD_NOT_FOUND);
+            }
+            return Promise.resolve();
+          }
+          if (account === witnessKeyName) {
+            if (!witnessKeyFound) {
+              throw new Error(passwords.PASSWORD_NOT_FOUND);
+            }
+            return Promise.resolve();
+          }
+          throw new Error("Unexpected key");
+        });
+
+        await service.deleteBiometricKey(mockUserId);
+
+        expect(passwords.deletePassword).toHaveBeenCalledWith(serviceName, keyName);
+        expect(passwords.deletePassword).toHaveBeenCalledWith(serviceName, witnessKeyName);
+        if (!keyFound) {
+          expect(logService.debug).toHaveBeenCalledWith(
+            "[OsBiometricService] Biometric key %s not found for service %s.",
+            keyName,
+            serviceName,
+          );
+        }
+        if (!witnessKeyFound) {
+          expect(logService.debug).toHaveBeenCalledWith(
+            "[OsBiometricService] Biometric witness key %s not found for service %s.",
+            witnessKeyName,
+            serviceName,
+          );
+        }
+      },
+    );
+
+    it("should throw error when deletePassword for key throws unexpected errors", async () => {
+      const error = new Error("Unexpected error");
+      passwords.deletePassword = jest.fn().mockImplementation((_, account) => {
+        if (account === keyName) {
+          throw error;
+        }
+        if (account === witnessKeyName) {
+          return Promise.resolve();
+        }
+        throw new Error("Unexpected key");
+      });
+
+      await expect(service.deleteBiometricKey(mockUserId)).rejects.toThrow(error);
+
+      expect(passwords.deletePassword).toHaveBeenCalledWith(serviceName, keyName);
+      expect(passwords.deletePassword).not.toHaveBeenCalledWith(serviceName, witnessKeyName);
+    });
+
+    it("should throw error when deletePassword for witness key throws unexpected errors", async () => {
+      const error = new Error("Unexpected error");
+      passwords.deletePassword = jest.fn().mockImplementation((_, account) => {
+        if (account === keyName) {
+          return Promise.resolve();
+        }
+        if (account === witnessKeyName) {
+          throw error;
+        }
+        throw new Error("Unexpected key");
+      });
+
+      await expect(service.deleteBiometricKey(mockUserId)).rejects.toThrow(error);
+
+      expect(passwords.deletePassword).toHaveBeenCalledWith(serviceName, keyName);
+      expect(passwords.deletePassword).toHaveBeenCalledWith(serviceName, witnessKeyName);
+    });
+  });
 });

apps/desktop/src/key-management/biometrics/os-biometrics-windows.service.ts

@@ -116,8 +116,32 @@ export default class OsBiometricsServiceWindows implements OsBiometricService {
   }
 
   async deleteBiometricKey(userId: UserId): Promise<void> {
-    await passwords.deletePassword(SERVICE, getLookupKeyForUser(userId));
-    await passwords.deletePassword(SERVICE, getLookupKeyForUser(userId) + KEY_WITNESS_SUFFIX);
+    try {
+      await passwords.deletePassword(SERVICE, getLookupKeyForUser(userId));
+    } catch (e) {
+      if (e instanceof Error && e.message === passwords.PASSWORD_NOT_FOUND) {
+        this.logService.debug(
+          "[OsBiometricService] Biometric key %s not found for service %s.",
+          getLookupKeyForUser(userId),
+          SERVICE,
+        );
+      } else {
+        throw e;
+      }
+    }
+    try {
+      await passwords.deletePassword(SERVICE, getLookupKeyForUser(userId) + KEY_WITNESS_SUFFIX);
+    } catch (e) {
+      if (e instanceof Error && e.message === passwords.PASSWORD_NOT_FOUND) {
+        this.logService.debug(
+          "[OsBiometricService] Biometric witness key %s not found for service %s.",
+          getLookupKeyForUser(userId) + KEY_WITNESS_SUFFIX,
+          SERVICE,
+        );
+      } else {
+        throw e;
+      }
+    }
   }
 
   async authenticateBiometric(): Promise<boolean> {
@@ -227,8 +251,19 @@ export default class OsBiometricsServiceWindows implements OsBiometricService {
         storageKey + KEY_WITNESS_SUFFIX,
         witnessKeyMaterial,
       );
-    } catch {
-      this.logService.debug("Error retrieving witness key, assuming value is not up to date.");
+    } catch (e) {
+      if (e instanceof Error && e.message === passwords.PASSWORD_NOT_FOUND) {
+        this.logService.debug(
+          "[OsBiometricService] Biometric witness key %s not found for service %s, value is not up to date.",
+          storageKey + KEY_WITNESS_SUFFIX,
+          service,
+        );
+      } else {
+        this.logService.error(
+          "[OsBiometricService] Error retrieving witness key, assuming value is not up to date.",
+          e,
+        );
+      }
       return false;
     }
 

apps/desktop/src/platform/main/desktop-credential-storage-listener.ts

@@ -35,13 +35,13 @@ export class DesktopCredentialStorageListener {
         }
         return val;
       } catch (e) {
-        if (
-          e.message === "Password not found." ||
-          e.message === "The specified item could not be found in the keychain."
-        ) {
+        if (e instanceof Error && e.message === passwords.PASSWORD_NOT_FOUND) {
+          if (message.action === "hasPassword") {
+            return false;
+          }
           return null;
         }
-        this.logService.info(e);
+        this.logService.error("[Credential Storage Listener] %s failed", message.action, e);
       }
     });
   }