[PM-21611] Require userId on KeyService clear methods (#14788)

apps/browser/src/auth/popup/settings/account-security.component.ts

@@ -480,7 +480,8 @@ export class AccountSecurityComponent implements OnInit, OnDestroy {
       });
       await this.vaultNudgesService.dismissNudge(NudgeType.AccountSecurity, userId);
     } else {
-      await this.vaultTimeoutSettingsService.clear();
+      const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
+      await this.vaultTimeoutSettingsService.clear(userId);
     }
   }
 

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

@@ -874,7 +874,7 @@ export class ServiceContainer {
     const userId = await firstValueFrom(getUserId(this.accountService.activeAccount$));
     await Promise.all([
       this.eventUploadService.uploadEvents(userId as UserId),
-      this.keyService.clearKeys(),
+      this.keyService.clearKeys(userId),
       this.cipherService.clear(userId),
       this.folderService.clear(userId),
       this.collectionService.clear(userId),

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

@@ -506,7 +506,8 @@ export class SettingsComponent implements OnInit, OnDestroy {
         await this.updateRequirePasswordOnStart();
       }
 
-      await this.vaultTimeoutSettingsService.clear();
+      const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
+      await this.vaultTimeoutSettingsService.clear(userId);
     }
 
     this.messagingService.send("redrawMenu");

apps/web/src/app/app.component.ts

@@ -282,7 +282,7 @@ export class AppComponent implements OnDestroy, OnInit {
     );
 
     await Promise.all([
-      this.keyService.clearKeys(),
+      this.keyService.clearKeys(userId),
       this.cipherService.clear(userId),
       this.folderService.clear(userId),
       this.collectionService.clear(userId),

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

@@ -59,5 +59,5 @@ export abstract class VaultTimeoutSettingsService {
    */
   isBiometricLockSet: (userId?: string) => Promise<boolean>;
 
-  clear: (userId?: string) => Promise<void>;
+  clear: (userId: UserId) => Promise<void>;
 }

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

@@ -287,7 +287,7 @@ export class VaultTimeoutSettingsService implements VaultTimeoutSettingsServiceA
     return availableActions;
   }
 
-  async clear(userId?: string): Promise<void> {
+  async clear(userId: UserId): Promise<void> {
     await this.keyService.clearPinKeys(userId);
   }
 

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

@@ -370,8 +370,9 @@ export abstract class KeyService {
    * Note: This will remove the stored pin and as a result,
    * disable pin protection for the user
    * @param userId The desired user
+   * @throws Error when provided userId is null or undefined
    */
-  abstract clearPinKeys(userId?: string): Promise<void>;
+  abstract clearPinKeys(userId: UserId): Promise<void>;
   /**
    * @param keyMaterial The key material to derive the send key from
    * @returns A new send key
@@ -380,8 +381,9 @@ export abstract class KeyService {
   /**
    * Clears all of the user's keys from storage
    * @param userId The user's Id
+   * @throws Error when provided userId is null or undefined
    */
-  abstract clearKeys(userId?: string): Promise<any>;
+  abstract clearKeys(userId: UserId): Promise<void>;
   abstract randomNumber(min: number, max: number): Promise<number>;
   /**
    * Generates a new cipher key

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

@@ -1,5 +1,5 @@
 import { mock } from "jest-mock-extended";
-import { BehaviorSubject, bufferCount, firstValueFrom, lastValueFrom, of, take, tap } from "rxjs";
+import { BehaviorSubject, bufferCount, firstValueFrom, lastValueFrom, of, take } from "rxjs";
 
 import { PinServiceAbstraction } from "@bitwarden/auth/common";
 import { EncryptedOrganizationKeyData } from "@bitwarden/common/admin-console/models/data/encrypted-organization-key.data";
@@ -380,16 +380,12 @@ describe("keyService", () => {
   });
 
   describe("clearKeys", () => {
-    it("resolves active user id when called with no user id", async () => {
+    test.each([null as unknown as UserId, undefined as unknown as UserId])(
-      let callCount = 0;
+      "throws when the provided userId is %s",
-      stateProvider.activeUserId$ = stateProvider.activeUserId$.pipe(tap(() => callCount++));
+      async (userId) => {
-
+        await expect(keyService.clearKeys(userId)).rejects.toThrow("UserId is required");
-      await keyService.clearKeys();
+      },
-      expect(callCount).toBe(1);
+    );
-
-      // revert to the original state
-      accountService.activeAccount$ = accountService.activeAccountSubject.asObservable();
-    });
 
     describe.each([
       USER_ENCRYPTED_ORGANIZATION_KEYS,
@@ -397,14 +393,6 @@ describe("keyService", () => {
       USER_ENCRYPTED_PRIVATE_KEY,
       USER_KEY,
     ])("key removal", (key: UserKeyDefinition<unknown>) => {
-      it(`clears ${key.key} for active user when unspecified`, async () => {
-        await keyService.clearKeys();
-
-        const encryptedOrgKeyState = stateProvider.singleUser.getFake(mockUserId, key);
-        expect(encryptedOrgKeyState.nextMock).toHaveBeenCalledTimes(1);
-        expect(encryptedOrgKeyState.nextMock).toHaveBeenCalledWith(null);
-      });
-
       it(`clears ${key.key} for the specified user when specified`, async () => {
         const userId = "someOtherUser" as UserId;
         await keyService.clearKeys(userId);
@@ -416,6 +404,24 @@ describe("keyService", () => {
     });
   });
 
+  describe("clearPinKeys", () => {
+    test.each([null as unknown as UserId, undefined as unknown as UserId])(
+      "throws when the provided userId is %s",
+      async (userId) => {
+        await expect(keyService.clearPinKeys(userId)).rejects.toThrow("UserId is required");
+      },
+    );
+    it("calls pin service to clear", async () => {
+      const userId = "someOtherUser" as UserId;
+
+      await keyService.clearPinKeys(userId);
+
+      expect(pinService.clearPinKeyEncryptedUserKeyPersistent).toHaveBeenCalledWith(userId);
+      expect(pinService.clearPinKeyEncryptedUserKeyEphemeral).toHaveBeenCalledWith(userId);
+      expect(pinService.clearUserKeyEncryptedPin).toHaveBeenCalledWith(userId);
+    });
+  });
+
   describe("userPrivateKey$", () => {
     type SetupKeysParams = {
       makeMasterKey: boolean;

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

@@ -564,11 +564,9 @@ export class DefaultKeyService implements KeyServiceAbstraction {
     await this.stateProvider.setUserState(USER_ENCRYPTED_PRIVATE_KEY, null, userId);
   }
 
-  async clearPinKeys(userId?: UserId): Promise<void> {
+  async clearPinKeys(userId: UserId): Promise<void> {
-    userId ??= await firstValueFrom(this.stateProvider.activeUserId$);
-
     if (userId == null) {
-      throw new Error("Cannot clear PIN keys, no user Id resolved.");
+      throw new Error("UserId is required");
     }
 
     await this.pinService.clearPinKeyEncryptedUserKeyPersistent(userId);
@@ -588,11 +586,9 @@ export class DefaultKeyService implements KeyServiceAbstraction {
     return (await this.keyGenerationService.createKey(512)) as CipherKey;
   }
 
-  async clearKeys(userId?: UserId): Promise<any> {
+  async clearKeys(userId: UserId): Promise<void> {
-    userId ??= await firstValueFrom(this.stateProvider.activeUserId$);
-
     if (userId == null) {
-      throw new Error("Cannot clear keys, no user Id resolved.");
+      throw new Error("UserId is required");
     }
 
     await this.masterPasswordService.clearMasterKeyHash(userId);