[PM-21705] Require userID for refreshAdditionalKeys() on key-service (#14810)

* Require userID for refreshAdditionalKeys()

* Add error handling to desktop Unlock settings

* Add more unit test coverage

apps/desktop/src/app/accounts/settings.component.spec.ts

@@ -22,17 +22,20 @@ import {
 import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.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 { ValidationService } from "@bitwarden/common/platform/abstractions/validation.service";
 import { ThemeType } from "@bitwarden/common/platform/enums";
 import { MessageSender } from "@bitwarden/common/platform/messaging";
 import { Utils } from "@bitwarden/common/platform/misc/utils";
 import { ThemeStateService } from "@bitwarden/common/platform/theming/theme-state.service";
 import { FakeAccountService, mockAccountServiceWith } from "@bitwarden/common/spec";
 import { UserId } from "@bitwarden/common/types/guid";
-import { DialogService } from "@bitwarden/components";
+import { DialogRef, DialogService } from "@bitwarden/components";
 import { BiometricStateService, BiometricsStatus, KeyService } from "@bitwarden/key-management";
 
+import { SetPinComponent } from "../../auth/components/set-pin.component";
 import { SshAgentPromptType } from "../../autofill/models/ssh-agent-setting";
 import { DesktopAutofillSettingsService } from "../../autofill/services/desktop-autofill-settings.service";
 import { DesktopBiometricsService } from "../../key-management/biometrics/desktop.biometrics.service";
@@ -60,6 +63,11 @@ describe("SettingsComponent", () => {
   const pinServiceAbstraction = mock<PinServiceAbstraction>();
   const desktopBiometricsService = mock<DesktopBiometricsService>();
   const platformUtilsService = mock<PlatformUtilsService>();
+  const logService = mock<LogService>();
+  const validationService = mock<ValidationService>();
+  const messagingService = mock<MessagingService>();
+  const keyService = mock<KeyService>();
+  const dialogService = mock<DialogService>();
 
   beforeEach(async () => {
     originalIpc = (global as any).ipc;
@@ -95,15 +103,15 @@ describe("SettingsComponent", () => {
         { provide: DesktopBiometricsService, useValue: desktopBiometricsService },
         { provide: DesktopSettingsService, useValue: desktopSettingsService },
         { provide: DomainSettingsService, useValue: domainSettingsService },
-        { provide: DialogService, useValue: mock<DialogService>() },
+        { provide: DialogService, useValue: dialogService },
         { provide: I18nService, useValue: i18nService },
-        { provide: LogService, useValue: mock<LogService>() },
+        { provide: LogService, useValue: logService },
         { provide: MessageSender, useValue: mock<MessageSender>() },
         {
           provide: NativeMessagingManifestService,
           useValue: mock<NativeMessagingManifestService>(),
         },
-        { provide: KeyService, useValue: mock<KeyService>() },
+        { provide: KeyService, useValue: keyService },
         { provide: PinServiceAbstraction, useValue: pinServiceAbstraction },
         { provide: PlatformUtilsService, useValue: platformUtilsService },
         { provide: PolicyService, useValue: policyService },
@@ -111,6 +119,8 @@ describe("SettingsComponent", () => {
         { provide: ThemeStateService, useValue: themeStateService },
         { provide: UserVerificationService, useValue: mock<UserVerificationService>() },
         { provide: VaultTimeoutSettingsService, useValue: vaultTimeoutSettingsService },
+        { provide: ValidationService, useValue: validationService },
+        { provide: MessagingService, useValue: messagingService },
       ],
       schemas: [NO_ERRORS_SCHEMA],
     }).compileComponents();
@@ -324,4 +334,261 @@ describe("SettingsComponent", () => {
       expect(textNodes).toContain("Require password on app start");
     });
   });
+
+  describe("updatePinHandler", () => {
+    afterEach(() => {
+      jest.resetAllMocks();
+    });
+
+    test.each([true, false])(`handles thrown errors when updated pin to %s`, async (update) => {
+      const error = new Error("Test error");
+      jest.spyOn(component, "updatePin").mockRejectedValue(error);
+
+      await component.ngOnInit();
+      await component.updatePinHandler(update);
+
+      expect(logService.error).toHaveBeenCalled();
+      expect(component.form.controls.pin.value).toBe(!update);
+      expect(validationService.showError).toHaveBeenCalledWith(error);
+      expect(messagingService.send).toHaveBeenCalledWith("redrawMenu");
+    });
+
+    describe("when updating to true", () => {
+      it("sets pin form control to false when the PIN dialog is cancelled", async () => {
+        jest.spyOn(SetPinComponent, "open").mockReturnValue(null);
+
+        await component.ngOnInit();
+        await component.updatePinHandler(true);
+
+        expect(component.form.controls.pin.value).toBe(false);
+        expect(vaultTimeoutSettingsService.clear).not.toHaveBeenCalled();
+        expect(messagingService.send).toHaveBeenCalledWith("redrawMenu");
+      });
+
+      test.each([true, false])(
+        `sets the pin form control to the dialog result`,
+        async (dialogResult) => {
+          const mockDialogRef = {
+            closed: of(dialogResult),
+          } as DialogRef<boolean>;
+          jest.spyOn(SetPinComponent, "open").mockReturnValue(mockDialogRef);
+
+          await component.ngOnInit();
+          await component.updatePinHandler(true);
+
+          expect(component.form.controls.pin.value).toBe(dialogResult);
+          expect(vaultTimeoutSettingsService.clear).not.toHaveBeenCalled();
+          expect(messagingService.send).toHaveBeenCalledWith("redrawMenu");
+        },
+      );
+    });
+
+    describe("when updating to false", () => {
+      let updateRequirePasswordOnStartSpy: jest.SpyInstance;
+
+      beforeEach(() => {
+        updateRequirePasswordOnStartSpy = jest
+          .spyOn(component, "updateRequirePasswordOnStart")
+          .mockImplementation(() => Promise.resolve());
+      });
+
+      it("updates requires password on start when the user doesn't have a MP and has requirePasswordOnStart on", async () => {
+        await component.ngOnInit();
+        component.form.controls.requirePasswordOnStart.setValue(true, { emitEvent: false });
+        component.userHasMasterPassword = false;
+        await component.updatePinHandler(false);
+
+        expect(component.form.controls.pin.value).toBe(false);
+        expect(component.form.controls.requirePasswordOnStart.value).toBe(false);
+        expect(updateRequirePasswordOnStartSpy).toHaveBeenCalled();
+        expect(vaultTimeoutSettingsService.clear).toHaveBeenCalled();
+        expect(messagingService.send).toHaveBeenCalledWith("redrawMenu");
+      });
+
+      test.each([
+        [true, true],
+        [false, true],
+        [false, false],
+      ])(
+        `doesn't updates requires password on start when the user's requirePasswordOnStart is %s and userHasMasterPassword is %s`,
+        async (requirePasswordOnStart, userHasMasterPassword) => {
+          await component.ngOnInit();
+          component.form.controls.requirePasswordOnStart.setValue(requirePasswordOnStart, {
+            emitEvent: false,
+          });
+          component.userHasMasterPassword = userHasMasterPassword;
+          await component.updatePinHandler(false);
+
+          expect(component.form.controls.pin.value).toBe(false);
+          expect(component.form.controls.requirePasswordOnStart.value).toBe(requirePasswordOnStart);
+          expect(updateRequirePasswordOnStartSpy).not.toHaveBeenCalled();
+          expect(vaultTimeoutSettingsService.clear).toHaveBeenCalled();
+          expect(messagingService.send).toHaveBeenCalledWith("redrawMenu");
+        },
+      );
+    });
+  });
+
+  describe("updateBiometricHandler", () => {
+    afterEach(() => {
+      jest.resetAllMocks();
+    });
+
+    test.each([true, false])(
+      `handles thrown errors when updated biometrics to %s`,
+      async (update) => {
+        const error = new Error("Test error");
+        jest.spyOn(component, "updateBiometric").mockRejectedValue(error);
+
+        await component.ngOnInit();
+        await component.updateBiometricHandler(update);
+
+        expect(logService.error).toHaveBeenCalled();
+        expect(component.form.controls.biometric.value).toBe(false);
+        expect(validationService.showError).toHaveBeenCalledWith(error);
+        expect(messagingService.send).toHaveBeenCalledWith("redrawMenu");
+      },
+    );
+
+    describe("when updating to true", () => {
+      beforeEach(async () => {
+        await component.ngOnInit();
+        component.supportsBiometric = true;
+      });
+
+      it("calls services to clear biometrics when supportsBiometric is false", async () => {
+        component.supportsBiometric = false;
+        await component.updateBiometricHandler(true);
+
+        expect(component.form.controls.biometric.value).toBe(false);
+        expect(biometricStateService.setBiometricUnlockEnabled).toHaveBeenLastCalledWith(false);
+        expect(keyService.refreshAdditionalKeys).toHaveBeenCalled();
+        expect(messagingService.send).toHaveBeenCalledWith("redrawMenu");
+      });
+
+      test.each([true, false])(
+        `launches a dialog and exits when man setup is needed, dialog result is %s`,
+        async (dialogResult) => {
+          dialogService.openSimpleDialog.mockResolvedValue(dialogResult);
+          desktopBiometricsService.getBiometricsStatus.mockResolvedValue(
+            BiometricsStatus.ManualSetupNeeded,
+          );
+
+          await component.updateBiometricHandler(true);
+
+          expect(biometricStateService.setBiometricUnlockEnabled).not.toHaveBeenCalled();
+          expect(keyService.refreshAdditionalKeys).not.toHaveBeenCalled();
+          expect(messagingService.send).toHaveBeenCalledWith("redrawMenu");
+
+          if (dialogResult) {
+            expect(platformUtilsService.launchUri).toHaveBeenCalledWith(
+              "https://bitwarden.com/help/biometrics/",
+            );
+          } else {
+            expect(platformUtilsService.launchUri).not.toHaveBeenCalled();
+          }
+        },
+      );
+
+      it("sets up biometrics when auto setup is needed", async () => {
+        desktopBiometricsService.getBiometricsStatus.mockResolvedValue(
+          BiometricsStatus.AutoSetupNeeded,
+        );
+        desktopBiometricsService.getBiometricsStatusForUser.mockResolvedValue(
+          BiometricsStatus.Available,
+        );
+
+        await component.updateBiometricHandler(true);
+
+        expect(desktopBiometricsService.setupBiometrics).toHaveBeenCalled();
+        expect(biometricStateService.setBiometricUnlockEnabled).toHaveBeenCalledWith(true);
+        expect(component.form.controls.biometric.value).toBe(true);
+        expect(keyService.refreshAdditionalKeys).toHaveBeenCalledWith(mockUserId);
+        expect(messagingService.send).toHaveBeenCalledWith("redrawMenu");
+      });
+
+      it("handles windows case", async () => {
+        desktopBiometricsService.getBiometricsStatus.mockResolvedValue(BiometricsStatus.Available);
+        desktopBiometricsService.getBiometricsStatusForUser.mockResolvedValue(
+          BiometricsStatus.Available,
+        );
+
+        component.isWindows = true;
+        component.isLinux = false;
+        await component.updateBiometricHandler(true);
+
+        expect(biometricStateService.setBiometricUnlockEnabled).toHaveBeenCalledWith(true);
+        expect(component.form.controls.requirePasswordOnStart.value).toBe(true);
+        expect(component.form.controls.autoPromptBiometrics.value).toBe(false);
+        expect(biometricStateService.setPromptAutomatically).toHaveBeenCalledWith(false);
+        expect(biometricStateService.setRequirePasswordOnStart).toHaveBeenCalledWith(true);
+        expect(biometricStateService.setDismissedRequirePasswordOnStartCallout).toHaveBeenCalled();
+        expect(keyService.refreshAdditionalKeys).toHaveBeenCalledWith(mockUserId);
+        expect(component.form.controls.biometric.value).toBe(true);
+        expect(messagingService.send).toHaveBeenCalledWith("redrawMenu");
+      });
+
+      it("handles linux case", async () => {
+        desktopBiometricsService.getBiometricsStatus.mockResolvedValue(BiometricsStatus.Available);
+        desktopBiometricsService.getBiometricsStatusForUser.mockResolvedValue(
+          BiometricsStatus.Available,
+        );
+
+        component.isWindows = false;
+        component.isLinux = true;
+        await component.updateBiometricHandler(true);
+
+        expect(biometricStateService.setBiometricUnlockEnabled).toHaveBeenCalledWith(true);
+        expect(component.form.controls.requirePasswordOnStart.value).toBe(true);
+        expect(component.form.controls.autoPromptBiometrics.value).toBe(false);
+        expect(biometricStateService.setPromptAutomatically).toHaveBeenCalledWith(false);
+        expect(biometricStateService.setRequirePasswordOnStart).toHaveBeenCalledWith(true);
+        expect(biometricStateService.setDismissedRequirePasswordOnStartCallout).toHaveBeenCalled();
+        expect(keyService.refreshAdditionalKeys).toHaveBeenCalledWith(mockUserId);
+        expect(component.form.controls.biometric.value).toBe(true);
+        expect(messagingService.send).toHaveBeenCalledWith("redrawMenu");
+      });
+
+      test.each([
+        BiometricsStatus.UnlockNeeded,
+        BiometricsStatus.HardwareUnavailable,
+        BiometricsStatus.AutoSetupNeeded,
+        BiometricsStatus.ManualSetupNeeded,
+        BiometricsStatus.PlatformUnsupported,
+        BiometricsStatus.DesktopDisconnected,
+        BiometricsStatus.NotEnabledLocally,
+        BiometricsStatus.NotEnabledInConnectedDesktopApp,
+        BiometricsStatus.NativeMessagingPermissionMissing,
+      ])(
+        `disables biometric when biometrics status check for the user returns %s`,
+        async (status) => {
+          desktopBiometricsService.getBiometricsStatus.mockResolvedValue(
+            BiometricsStatus.Available,
+          );
+          desktopBiometricsService.getBiometricsStatusForUser.mockResolvedValue(status);
+
+          await component.updateBiometricHandler(true);
+
+          expect(keyService.refreshAdditionalKeys).toHaveBeenCalledWith(mockUserId);
+          expect(component.form.controls.biometric.value).toBe(false);
+          expect(biometricStateService.setBiometricUnlockEnabled).toHaveBeenCalledWith(true);
+          expect(biometricStateService.setBiometricUnlockEnabled).toHaveBeenCalledTimes(2);
+          expect(biometricStateService.setBiometricUnlockEnabled).toHaveBeenLastCalledWith(false);
+          expect(messagingService.send).toHaveBeenCalledWith("redrawMenu");
+        },
+      );
+    });
+
+    describe("when updating to false", () => {
+      it("calls services to clear biometrics", async () => {
+        await component.ngOnInit();
+        await component.updateBiometricHandler(false);
+
+        expect(component.form.controls.biometric.value).toBe(false);
+        expect(biometricStateService.setBiometricUnlockEnabled).toHaveBeenLastCalledWith(false);
+        expect(keyService.refreshAdditionalKeys).toHaveBeenCalled();
+        expect(messagingService.send).toHaveBeenCalledWith("redrawMenu");
+      });
+    });
+  });
 });

apps/desktop/src/app/accounts/settings.component.ts

@@ -37,6 +37,7 @@ 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 { ValidationService } from "@bitwarden/common/platform/abstractions/validation.service";
 import { Theme, ThemeTypes } from "@bitwarden/common/platform/enums/theme-type.enum";
 import { Utils } from "@bitwarden/common/platform/misc/utils";
 import { ThemeStateService } from "@bitwarden/common/platform/theming/theme-state.service";
@@ -162,6 +163,7 @@ export class SettingsComponent implements OnInit, OnDestroy {
     private logService: LogService,
     private nativeMessagingManifestService: NativeMessagingManifestService,
     private configService: ConfigService,
+    private validationService: ValidationService,
   ) {
     const isMac = this.platformUtilsService.getDevice() === DeviceType.MacOsDesktop;
 
@@ -379,7 +381,7 @@ export class SettingsComponent implements OnInit, OnDestroy {
     this.form.controls.pin.valueChanges
       .pipe(
         concatMap(async (value) => {
-          await this.updatePin(value);
+          await this.updatePinHandler(value);
           this.refreshTimeoutSettings$.next();
         }),
         takeUntil(this.destroy$),
@@ -389,7 +391,7 @@ export class SettingsComponent implements OnInit, OnDestroy {
     this.form.controls.biometric.valueChanges
       .pipe(
         concatMap(async (enabled) => {
-          await this.updateBiometric(enabled);
+          await this.updateBiometricHandler(enabled);
           this.refreshTimeoutSettings$.next();
         }),
         takeUntil(this.destroy$),
@@ -485,6 +487,18 @@ export class SettingsComponent implements OnInit, OnDestroy {
     );
   }
 
+  async updatePinHandler(value: boolean) {
+    try {
+      await this.updatePin(value);
+    } catch (error) {
+      this.logService.error("Error updating unlock with PIN: ", error);
+      this.form.controls.pin.setValue(!value, { emitEvent: false });
+      this.validationService.showError(error);
+    } finally {
+      this.messagingService.send("redrawMenu");
+    }
+  }
+
   async updatePin(value: boolean) {
     if (value) {
       const dialogRef = SetPinComponent.open(this.dialogService);
@@ -509,8 +523,18 @@ export class SettingsComponent implements OnInit, OnDestroy {
       const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
       await this.vaultTimeoutSettingsService.clear(userId);
     }
+  }
 
-    this.messagingService.send("redrawMenu");
+  async updateBiometricHandler(value: boolean) {
+    try {
+      await this.updateBiometric(value);
+    } catch (error) {
+      this.logService.error("Error updating unlock with biometrics: ", error);
+      this.form.controls.biometric.setValue(false, { emitEvent: false });
+      this.validationService.showError(error);
+    } finally {
+      this.messagingService.send("redrawMenu");
+    }
   }
 
   async updateBiometric(enabled: boolean) {
@@ -519,61 +543,55 @@ export class SettingsComponent implements OnInit, OnDestroy {
     // The bug should resolve itself once the angular issue is resolved.
     // See: https://github.com/angular/angular/issues/13063
 
-    try {
-      if (!enabled || !this.supportsBiometric) {
-        this.form.controls.biometric.setValue(false, { emitEvent: false });
-        await this.biometricStateService.setBiometricUnlockEnabled(false);
-        await this.keyService.refreshAdditionalKeys();
-        return;
-      }
+    const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
+    if (!enabled || !this.supportsBiometric) {
+      this.form.controls.biometric.setValue(false, { emitEvent: false });
+      await this.biometricStateService.setBiometricUnlockEnabled(false);
+      await this.keyService.refreshAdditionalKeys(activeUserId);
+      return;
+    }
 
-      const status = await this.biometricsService.getBiometricsStatus();
+    const status = await this.biometricsService.getBiometricsStatus();
 
-      if (status === BiometricsStatus.AutoSetupNeeded) {
-        await this.biometricsService.setupBiometrics();
-      } else if (status === BiometricsStatus.ManualSetupNeeded) {
-        const confirmed = await this.dialogService.openSimpleDialog({
-          title: { key: "biometricsManualSetupTitle" },
-          content: { key: "biometricsManualSetupDesc" },
-          type: "warning",
-        });
-        if (confirmed) {
-          this.platformUtilsService.launchUri("https://bitwarden.com/help/biometrics/");
-        }
-        return;
+    if (status === BiometricsStatus.AutoSetupNeeded) {
+      await this.biometricsService.setupBiometrics();
+    } else if (status === BiometricsStatus.ManualSetupNeeded) {
+      const confirmed = await this.dialogService.openSimpleDialog({
+        title: { key: "biometricsManualSetupTitle" },
+        content: { key: "biometricsManualSetupDesc" },
+        type: "warning",
+      });
+      if (confirmed) {
+        this.platformUtilsService.launchUri("https://bitwarden.com/help/biometrics/");
       }
+      return;
+    }
 
-      await this.biometricStateService.setBiometricUnlockEnabled(true);
-      if (this.isWindows) {
-        // Recommended settings for Windows Hello
-        this.form.controls.requirePasswordOnStart.setValue(true);
-        this.form.controls.autoPromptBiometrics.setValue(false);
-        await this.biometricStateService.setPromptAutomatically(false);
-        await this.biometricStateService.setRequirePasswordOnStart(true);
-        await this.biometricStateService.setDismissedRequirePasswordOnStartCallout();
-      } else if (this.isLinux) {
-        // Similar to Windows
-        this.form.controls.requirePasswordOnStart.setValue(true);
-        this.form.controls.autoPromptBiometrics.setValue(false);
-        await this.biometricStateService.setPromptAutomatically(false);
-        await this.biometricStateService.setRequirePasswordOnStart(true);
-        await this.biometricStateService.setDismissedRequirePasswordOnStartCallout();
-      }
-      await this.keyService.refreshAdditionalKeys();
+    await this.biometricStateService.setBiometricUnlockEnabled(true);
+    if (this.isWindows) {
+      // Recommended settings for Windows Hello
+      this.form.controls.requirePasswordOnStart.setValue(true);
+      this.form.controls.autoPromptBiometrics.setValue(false);
+      await this.biometricStateService.setPromptAutomatically(false);
+      await this.biometricStateService.setRequirePasswordOnStart(true);
+      await this.biometricStateService.setDismissedRequirePasswordOnStartCallout();
+    } else if (this.isLinux) {
+      // Similar to Windows
+      this.form.controls.requirePasswordOnStart.setValue(true);
+      this.form.controls.autoPromptBiometrics.setValue(false);
+      await this.biometricStateService.setPromptAutomatically(false);
+      await this.biometricStateService.setRequirePasswordOnStart(true);
+      await this.biometricStateService.setDismissedRequirePasswordOnStartCallout();
+    }
+    await this.keyService.refreshAdditionalKeys(activeUserId);
 
-      const activeUserId = await firstValueFrom(
-        this.accountService.activeAccount$.pipe(map((a) => a?.id)),
-      );
-      // Validate the key is stored in case biometrics fail.
-      const biometricSet =
-        (await this.biometricsService.getBiometricsStatusForUser(activeUserId)) ===
-        BiometricsStatus.Available;
-      this.form.controls.biometric.setValue(biometricSet, { emitEvent: false });
-      if (!biometricSet) {
-        await this.biometricStateService.setBiometricUnlockEnabled(false);
-      }
-    } finally {
-      this.messagingService.send("redrawMenu");
+    // Validate the key is stored in case biometrics fail.
+    const biometricSet =
+      (await this.biometricsService.getBiometricsStatusForUser(activeUserId)) ===
+      BiometricsStatus.Available;
+    this.form.controls.biometric.setValue(biometricSet, { emitEvent: false });
+    if (!biometricSet) {
+      await this.biometricStateService.setBiometricUnlockEnabled(false);
     }
   }
 
@@ -599,7 +617,8 @@ export class SettingsComponent implements OnInit, OnDestroy {
       await this.biometricStateService.setRequirePasswordOnStart(false);
     }
     await this.biometricStateService.setDismissedRequirePasswordOnStartCallout();
-    await this.keyService.refreshAdditionalKeys();
+    const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
+    await this.keyService.refreshAdditionalKeys(userId);
   }
 
   async saveFavicons() {