[PM-21912] Require userID for KeyService's hasUserKey (#14890)

* Update keyService hasUserKey to require userId and remove unused/duplicate methods

* Update lock component consumer

* Update send commands to pass in userId

* update SSO login to pass in userID

* Update bw serve to pass in userID

* remove unneeded method from electron-key.service

apps/browser/src/key-management/lock/services/extension-lock-component.service.spec.ts

@@ -12,7 +12,6 @@ import { VaultTimeoutSettingsService } from "@bitwarden/common/key-management/va
 import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
 import { UserId } from "@bitwarden/common/types/guid";
 import {
-  KeyService,
   BiometricsService,
   BiometricsStatus,
   BiometricStateService,
@@ -35,7 +34,6 @@ describe("ExtensionLockComponentService", () => {
   let biometricsService: MockProxy<BiometricsService>;
   let pinService: MockProxy<PinServiceAbstraction>;
   let vaultTimeoutSettingsService: MockProxy<VaultTimeoutSettingsService>;
-  let keyService: MockProxy<KeyService>;
   let routerService: MockProxy<BrowserRouterService>;
   let biometricStateService: MockProxy<BiometricStateService>;
 
@@ -45,7 +43,6 @@ describe("ExtensionLockComponentService", () => {
     biometricsService = mock<BiometricsService>();
     pinService = mock<PinServiceAbstraction>();
     vaultTimeoutSettingsService = mock<VaultTimeoutSettingsService>();
-    keyService = mock<KeyService>();
     routerService = mock<BrowserRouterService>();
     biometricStateService = mock<BiometricStateService>();
 
@@ -72,10 +69,6 @@ describe("ExtensionLockComponentService", () => {
           provide: VaultTimeoutSettingsService,
           useValue: vaultTimeoutSettingsService,
         },
-        {
-          provide: KeyService,
-          useValue: keyService,
-        },
         {
           provide: BrowserRouterService,
           useValue: routerService,
@@ -375,7 +368,6 @@ describe("ExtensionLockComponentService", () => {
       vaultTimeoutSettingsService.isBiometricLockSet.mockResolvedValue(
         mockInputs.hasBiometricEncryptedUserKeyStored,
       );
-      keyService.hasUserKeyStored.mockResolvedValue(mockInputs.hasBiometricEncryptedUserKeyStored);
       platformUtilsService.supportsSecureStorage.mockReturnValue(
         mockInputs.platformSupportsSecureStorage,
       );

apps/cli/src/oss-serve-configurator.ts

@@ -3,6 +3,7 @@
 import * as koaMulter from "@koa/multer";
 import * as koaRouter from "@koa/router";
 import * as koa from "koa";
+import { firstValueFrom, map } from "rxjs";
 
 import { ConfirmCommand } from "./admin-console/commands/confirm.command";
 import { ShareCommand } from "./admin-console/commands/share.command";
@@ -170,6 +171,7 @@ export class OssServeConfigurator {
       this.serviceContainer.searchService,
       this.serviceContainer.encryptService,
       this.serviceContainer.apiService,
+      this.serviceContainer.accountService,
     );
     this.sendEditCommand = new SendEditCommand(
       this.serviceContainer.sendService,
@@ -182,6 +184,7 @@ export class OssServeConfigurator {
       this.serviceContainer.sendService,
       this.serviceContainer.environmentService,
       this.serviceContainer.searchService,
+      this.serviceContainer.accountService,
     );
     this.sendRemovePasswordCommand = new SendRemovePasswordCommand(
       this.serviceContainer.sendService,
@@ -414,7 +417,10 @@ export class OssServeConfigurator {
       this.processResponse(res, Response.error("You are not logged in."));
       return true;
     }
-    if (await this.serviceContainer.keyService.hasUserKey()) {
+    const userId = await firstValueFrom(
+      this.serviceContainer.accountService.activeAccount$.pipe(map((account) => account?.id)),
+    );
+    if (await this.serviceContainer.keyService.hasUserKey(userId)) {
       return false;
     }
     this.processResponse(res, Response.error("Vault is locked."));

apps/cli/src/tools/send/commands/get.command.ts

@@ -4,6 +4,8 @@ import { OptionValues } from "commander";
 import { firstValueFrom } from "rxjs";
 
 import { ApiService } from "@bitwarden/common/abstractions/api.service";
+import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+import { getUserId } from "@bitwarden/common/auth/services/account.service";
 import { EncryptService } from "@bitwarden/common/key-management/crypto/abstractions/encrypt.service";
 import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service";
 import { Utils } from "@bitwarden/common/platform/misc/utils";
@@ -22,6 +24,7 @@ export class SendGetCommand extends DownloadCommand {
     private searchService: SearchService,
     encryptService: EncryptService,
     apiService: ApiService,
+    private accountService: AccountService,
   ) {
     super(encryptService, apiService);
   }
@@ -77,7 +80,8 @@ export class SendGetCommand extends DownloadCommand {
         return await send.decrypt();
       }
     } else if (id.trim() !== "") {
-      let sends = await this.sendService.getAllDecryptedFromState();
+      const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
+      let sends = await this.sendService.getAllDecryptedFromState(activeUserId);
       sends = this.searchService.searchSends(sends, id);
       if (sends.length > 1) {
         return sends;

apps/cli/src/tools/send/commands/list.command.ts

@@ -1,5 +1,7 @@
 import { firstValueFrom } from "rxjs";
 
+import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+import { getUserId } from "@bitwarden/common/auth/services/account.service";
 import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service";
 import { SendService } from "@bitwarden/common/tools/send/services/send.service.abstraction";
 import { SearchService } from "@bitwarden/common/vault/abstractions/search.service";
@@ -13,10 +15,12 @@ export class SendListCommand {
     private sendService: SendService,
     private environmentService: EnvironmentService,
     private searchService: SearchService,
+    private accountService: AccountService,
   ) {}
 
   async run(cmdOptions: Record<string, any>): Promise<Response> {
-    let sends = await this.sendService.getAllDecryptedFromState();
+    const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
+    let sends = await this.sendService.getAllDecryptedFromState(activeUserId);
 
     const normalizedOptions = new Options(cmdOptions);
     if (normalizedOptions.search != null && normalizedOptions.search.trim() !== "") {

apps/cli/src/tools/send/send.program.ts

@@ -128,6 +128,7 @@ export class SendProgram extends BaseProgram {
           this.serviceContainer.sendService,
           this.serviceContainer.environmentService,
           this.serviceContainer.searchService,
+          this.serviceContainer.accountService,
         );
         const response = await cmd.run(options);
         this.processResponse(response);
@@ -193,6 +194,7 @@ export class SendProgram extends BaseProgram {
           this.serviceContainer.searchService,
           this.serviceContainer.encryptService,
           this.serviceContainer.apiService,
+          this.serviceContainer.accountService,
         );
         const response = await cmd.run(id, options);
         this.processResponse(response);
@@ -253,6 +255,7 @@ export class SendProgram extends BaseProgram {
           this.serviceContainer.searchService,
           this.serviceContainer.encryptService,
           this.serviceContainer.apiService,
+          this.serviceContainer.accountService,
         );
         const cmd = new SendEditCommand(
           this.serviceContainer.sendService,

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

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

libs/auth/src/common/login-strategies/sso-login.strategy.ts

@@ -266,7 +266,7 @@ export class SsoLoginStrategy extends LoginStrategy {
         );
       }
 
-      if (await this.keyService.hasUserKey()) {
+      if (await this.keyService.hasUserKey(userId)) {
         // Now that we have a decrypted user key in memory, we can check if we
         // need to establish trust on the current device
         await this.deviceTrustService.trustDeviceIfRequired(userId);

libs/common/src/tools/send/services/send.service.abstraction.ts

@@ -54,7 +54,7 @@ export abstract class SendService implements UserKeyRotationDataProvider<SendWit
   /**
    * @deprecated Only use in CLI
    */
-  getAllDecryptedFromState: () => Promise<SendView[]>;
+  getAllDecryptedFromState: (userId: UserId) => Promise<SendView[]>;
 }
 
 export abstract class InternalSendService extends SendService {

libs/common/src/tools/send/services/send.service.spec.ts

@@ -467,10 +467,21 @@ describe("SendService", () => {
     });
   });
 
-  it("getAllDecryptedFromState", async () => {
-    const sends = await sendService.getAllDecryptedFromState();
+  describe("getAllDecryptedFromState", () => {
+    it("returns already decrypted sends in state", async () => {
+      const sends = await sendService.getAllDecryptedFromState(mockUserId);
 
-    expect(sends[0]).toMatchObject(testSendViewData("1", "Test Send"));
+      expect(sends[0]).toMatchObject(testSendViewData("1", "Test Send"));
+    });
+
+    it("throws if no decrypted sends in state and there is no userKey", async () => {
+      decryptedState.nextState(null);
+      keyService.hasUserKey.mockResolvedValue(false);
+
+      await expect(sendService.getAllDecryptedFromState(mockUserId)).rejects.toThrow(
+        "No user key found.",
+      );
+    });
   });
 
   describe("getRotatedData", () => {

libs/common/src/tools/send/services/send.service.ts

@@ -199,14 +199,14 @@ export class SendService implements InternalSendServiceAbstraction {
     return response;
   }
 
-  async getAllDecryptedFromState(): Promise<SendView[]> {
+  async getAllDecryptedFromState(userId: UserId): Promise<SendView[]> {
     let decSends = await this.stateProvider.getDecryptedSends();
     if (decSends != null) {
       return decSends;
     }
 
     decSends = [];
-    const hasKey = await this.keyService.hasUserKey();
+    const hasKey = await this.keyService.hasUserKey(userId);
     if (!hasKey) {
       throw new Error("No user key found.");
     }

libs/key-management-ui/src/lock/components/lock.component.ts

@@ -249,7 +249,7 @@ export class LockComponent implements OnInit, OnDestroy {
 
   private async handleActiveAccountChange(activeAccount: Account) {
     // this account may be unlocked, prevent any prompts so we can redirect to vault
-    if (await this.keyService.hasUserKeyInMemory(activeAccount.id)) {
+    if (await this.keyService.hasUserKey(activeAccount.id)) {
       return;
     }
 

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

@@ -138,24 +138,13 @@ export abstract class KeyService {
     userId: string,
   ): Promise<UserKey | null>;
 
-  /**
-   * Determines whether the user key is available for the given user.
-   * @param userId The desired user. If not provided, the active user will be used. If no active user exists, the method will return false.
-   * @returns True if the user key is available
-   */
-  abstract hasUserKey(userId?: UserId): Promise<boolean>;
   /**
    * Determines whether the user key is available for the given user in memory.
-   * @param userId The desired user. If not provided, the active user will be used. If no active user exists, the method will return false.
-   * @returns True if the user key is available
+   * @param userId The desired user. If null or undefined, will return false.
+   * @returns True if the user key is available, returns false otherwise.
    */
-  abstract hasUserKeyInMemory(userId?: string): Promise<boolean>;
-  /**
-   * @param keySuffix The desired version of the user's key to check
-   * @param userId The desired user
-   * @returns True if the provided version of the user key is stored
-   */
-  abstract hasUserKeyStored(keySuffix: KeySuffixOptions, userId?: string): Promise<boolean>;
+  abstract hasUserKey(userId: UserId): Promise<boolean>;
+
   /**
    * Generates a new user key
    * @throws Error when master key is null and there is no active user

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

@@ -148,39 +148,25 @@ describe("keyService", () => {
     });
   });
 
-  describe.each(["hasUserKey", "hasUserKeyInMemory"])(`%s`, (methodName: string) => {
+  describe("hasUserKey", () => {
     let mockUserKey: UserKey;
-    let method: (userId?: UserId) => Promise<boolean>;
 
     beforeEach(() => {
       const mockRandomBytes = new Uint8Array(64) as CsprngArray;
       mockUserKey = new SymmetricCryptoKey(mockRandomBytes) as UserKey;
-      method =
-        methodName === "hasUserKey"
-          ? keyService.hasUserKey.bind(keyService)
-          : keyService.hasUserKeyInMemory.bind(keyService);
     });
 
+    test.each([null as unknown as UserId, undefined as unknown as UserId])(
+      "returns false when userId is %s",
+      async (userId) => {
+        expect(await keyService.hasUserKey(userId)).toBe(false);
+      },
+    );
+
     it.each([true, false])("returns %s if the user key is set", async (hasKey) => {
       stateProvider.singleUser.getFake(mockUserId, USER_KEY).nextState(hasKey ? mockUserKey : null);
-      expect(await method(mockUserId)).toBe(hasKey);
+      expect(await keyService.hasUserKey(mockUserId)).toBe(hasKey);
     });
-
-    it("returns false when no active userId is set", async () => {
-      accountService.activeAccountSubject.next(null);
-      expect(await method()).toBe(false);
-    });
-
-    it.each([true, false])(
-      "resolves %s for active user id when none is provided",
-      async (hasKey) => {
-        stateProvider.activeUserId$ = of(mockUserId);
-        stateProvider.singleUser
-          .getFake(mockUserId, USER_KEY)
-          .nextState(hasKey ? mockUserKey : null);
-        expect(await method()).toBe(hasKey);
-      },
-    );
   });
 
   describe("getUserKeyWithLegacySupport", () => {
@@ -410,6 +396,19 @@ describe("keyService", () => {
     });
   });
 
+  describe("makeSendKey", () => {
+    const mockRandomBytes = new Uint8Array(16) as CsprngArray;
+    it("calls keyGenerationService with expected hard coded parameters", async () => {
+      await keyService.makeSendKey(mockRandomBytes);
+
+      expect(keyGenerationService.deriveKeyFromMaterial).toHaveBeenCalledWith(
+        mockRandomBytes,
+        "bitwarden-send",
+        "send",
+      );
+    });
+  });
+
   describe("clearStoredUserKey", () => {
     describe("input validation", () => {
       const invalidUserIdTestCases = [

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

@@ -198,16 +198,7 @@ export class DefaultKeyService implements KeyServiceAbstraction {
     return userKey;
   }
 
-  async hasUserKey(userId?: UserId): Promise<boolean> {
-    userId ??= await firstValueFrom(this.stateProvider.activeUserId$);
-    if (userId == null) {
-      return false;
-    }
-    return await this.hasUserKeyInMemory(userId);
-  }
-
-  async hasUserKeyInMemory(userId?: UserId): Promise<boolean> {
-    userId ??= await firstValueFrom(this.stateProvider.activeUserId$);
+  async hasUserKey(userId: UserId): Promise<boolean> {
     if (userId == null) {
       return false;
     }
@@ -215,10 +206,6 @@ export class DefaultKeyService implements KeyServiceAbstraction {
     return (await firstValueFrom(this.stateProvider.getUserState$(USER_KEY, userId))) != null;
   }
 
-  async hasUserKeyStored(keySuffix: KeySuffixOptions, userId?: UserId): Promise<boolean> {
-    return (await this.getKeyFromStorage(keySuffix, userId)) != null;
-  }
-
   async makeUserKey(masterKey: MasterKey | null): Promise<[UserKey, EncString]> {
     if (masterKey == null) {
       const userId = await firstValueFrom(this.stateProvider.activeUserId$);