[PM-22133] Require userID for clearStoredUserKey (#14973)

apps/browser/src/background/main.background.ts

@@ -437,7 +437,7 @@ export default class MainBackground {
 
   constructor() {
     // Services
-    const lockedCallback = async (userId?: string) => {
+    const lockedCallback = async (userId: UserId) => {
       await this.refreshBadge();
       await this.refreshMenu(true);
       if (this.systemService != null) {

apps/cli/src/service-container/service-container.ts

@@ -715,8 +715,8 @@ export class ServiceContainer {
 
     this.folderApiService = new FolderApiService(this.folderService, this.apiService);
 
-    const lockedCallback = async (userId?: string) =>
+    const lockedCallback = async (userId: UserId) =>
-      await this.keyService.clearStoredUserKey(KeySuffixOptions.Auto);
+      await this.keyService.clearStoredUserKey(KeySuffixOptions.Auto, userId);
 
     this.userVerificationApiService = new UserVerificationApiService(this.apiService);
 

apps/desktop/src/key-management/electron-key.service.ts

@@ -57,7 +57,7 @@ export class ElectronKeyService extends DefaultKeyService {
     return super.hasUserKeyStored(keySuffix, userId);
   }
 
-  override async clearStoredUserKey(keySuffix: KeySuffixOptions, userId?: UserId): Promise<void> {
+  override async clearStoredUserKey(keySuffix: KeySuffixOptions, userId: UserId): Promise<void> {
     await super.clearStoredUserKey(keySuffix, userId);
   }
 

libs/common/src/key-management/vault-timeout/services/vault-timeout.service.spec.ts

@@ -417,16 +417,12 @@ describe("VaultTimeoutService", () => {
       expect(stateEventRunnerService.handleEvent).toHaveBeenCalledWith("lock", "user1");
     });
 
-    it("should call locked callback if no user passed into lock", async () => {
+    it("should call locked callback with the locking user if no userID is passed in.", async () => {
       setupLock();
 
       await vaultTimeoutService.lock();
 
-      // Currently these pass `undefined` (or what they were given) as the userId back
+      expect(lockedCallback).toHaveBeenCalledWith("user1");
-      // but we could change this to give the user that was locked (active) to these methods
-      // so they don't have to get it their own way, but that is a behavioral change that needs
-      // to be tested.
-      expect(lockedCallback).toHaveBeenCalledWith(undefined);
     });
 
     it("should call state event runner with user passed into lock", async () => {

libs/common/src/key-management/vault-timeout/services/vault-timeout.service.ts

@@ -49,7 +49,7 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction {
     private taskSchedulerService: TaskSchedulerService,
     protected logService: LogService,
     private biometricService: BiometricsService,
-    private lockedCallback: (userId?: string) => Promise<void> = null,
+    private lockedCallback: (userId: UserId) => Promise<void> = null,
     private loggedOutCallback: (
       logoutReason: LogoutReason,
       userId?: string,
@@ -166,7 +166,7 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction {
     this.messagingService.send("locked", { userId: lockingUserId });
 
     if (this.lockedCallback != null) {
-      await this.lockedCallback(userId);
+      await this.lockedCallback(lockingUserId);
     }
   }
 

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

@@ -167,8 +167,9 @@ export abstract class KeyService {
    * Clears the user's stored version of the user key
    * @param keySuffix The desired version of the key to clear
    * @param userId The desired user
+   * @throws Error when userId is null or undefined.
    */
-  abstract clearStoredUserKey(keySuffix: KeySuffixOptions, userId?: string): Promise<void>;
+  abstract clearStoredUserKey(keySuffix: KeySuffixOptions, userId: string): Promise<void>;
   /**
    * Stores the master key encrypted user key
    * @throws Error when userId is null and there is no active user.

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

@@ -410,6 +410,45 @@ describe("keyService", () => {
     });
   });
 
+  describe("clearStoredUserKey", () => {
+    describe("input validation", () => {
+      const invalidUserIdTestCases = [
+        { keySuffix: KeySuffixOptions.Auto, userId: null as unknown as UserId },
+        { keySuffix: KeySuffixOptions.Auto, userId: undefined as unknown as UserId },
+        { keySuffix: KeySuffixOptions.Pin, userId: null as unknown as UserId },
+        { keySuffix: KeySuffixOptions.Pin, userId: undefined as unknown as UserId },
+      ];
+      test.each(invalidUserIdTestCases)(
+        "throws when keySuffix is $keySuffix and userId is $userId",
+        async ({ keySuffix, userId }) => {
+          await expect(keyService.clearStoredUserKey(keySuffix, userId)).rejects.toThrow(
+            "UserId is required",
+          );
+        },
+      );
+    });
+
+    describe("with Auto key suffix", () => {
+      it("UserKeyAutoUnlock is cleared and pin keys are not cleared", async () => {
+        await keyService.clearStoredUserKey(KeySuffixOptions.Auto, mockUserId);
+
+        expect(stateService.setUserKeyAutoUnlock).toHaveBeenCalledWith(null, {
+          userId: mockUserId,
+        });
+        expect(pinService.clearPinKeyEncryptedUserKeyEphemeral).not.toHaveBeenCalled();
+      });
+    });
+
+    describe("with PIN key suffix", () => {
+      it("pin keys are cleared and user key auto unlock not", async () => {
+        await keyService.clearStoredUserKey(KeySuffixOptions.Pin, mockUserId);
+
+        expect(stateService.setUserKeyAutoUnlock).not.toHaveBeenCalled();
+        expect(pinService.clearPinKeyEncryptedUserKeyEphemeral).toHaveBeenCalledWith(mockUserId);
+      });
+    });
+  });
+
   describe("clearKeys", () => {
     test.each([null as unknown as UserId, undefined as unknown as UserId])(
       "throws when the provided userId is %s",

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

@@ -250,16 +250,16 @@ export class DefaultKeyService implements KeyServiceAbstraction {
     await this.clearAllStoredUserKeys(userId);
   }
 
-  async clearStoredUserKey(keySuffix: KeySuffixOptions, userId?: UserId): Promise<void> {
+  async clearStoredUserKey(keySuffix: KeySuffixOptions, userId: UserId): Promise<void> {
-    if (keySuffix === KeySuffixOptions.Auto) {
+    if (userId == null) {
-      // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
+      throw new Error("UserId is required");
-      // eslint-disable-next-line @typescript-eslint/no-floating-promises
-      this.stateService.setUserKeyAutoUnlock(null, { userId: userId });
     }
-    if (keySuffix === KeySuffixOptions.Pin && userId != null) {
+
-      // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
+    if (keySuffix === KeySuffixOptions.Auto) {
-      // eslint-disable-next-line @typescript-eslint/no-floating-promises
+      await this.stateService.setUserKeyAutoUnlock(null, { userId: userId });
-      this.pinService.clearPinKeyEncryptedUserKeyEphemeral(userId);
+    }
+    if (keySuffix === KeySuffixOptions.Pin) {
+      await this.pinService.clearPinKeyEncryptedUserKeyEphemeral(userId);
     }
   }