From 09fb74679dd42c9794fe6df2677f14bc9e7dedf6 Mon Sep 17 00:00:00 2001 From: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> Date: Wed, 9 Jul 2025 11:53:16 -0500 Subject: [PATCH] [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 --- .../extension-lock-component.service.spec.ts | 8 ---- apps/cli/src/oss-serve-configurator.ts | 8 +++- .../src/tools/send/commands/get.command.ts | 6 ++- .../src/tools/send/commands/list.command.ts | 6 ++- apps/cli/src/tools/send/send.program.ts | 3 ++ .../key-management/electron-key.service.ts | 4 -- .../login-strategies/sso-login.strategy.ts | 2 +- .../send/services/send.service.abstraction.ts | 2 +- .../tools/send/services/send.service.spec.ts | 17 +++++-- .../src/tools/send/services/send.service.ts | 4 +- .../src/lock/components/lock.component.ts | 2 +- .../src/abstractions/key.service.ts | 19 ++------ libs/key-management/src/key.service.spec.ts | 45 +++++++++---------- libs/key-management/src/key.service.ts | 15 +------ 14 files changed, 66 insertions(+), 75 deletions(-) diff --git a/apps/browser/src/key-management/lock/services/extension-lock-component.service.spec.ts b/apps/browser/src/key-management/lock/services/extension-lock-component.service.spec.ts index 86781474b6..0ae0997fe4 100644 --- a/apps/browser/src/key-management/lock/services/extension-lock-component.service.spec.ts +++ b/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; let pinService: MockProxy; let vaultTimeoutSettingsService: MockProxy; - let keyService: MockProxy; let routerService: MockProxy; let biometricStateService: MockProxy; @@ -45,7 +43,6 @@ describe("ExtensionLockComponentService", () => { biometricsService = mock(); pinService = mock(); vaultTimeoutSettingsService = mock(); - keyService = mock(); routerService = mock(); biometricStateService = mock(); @@ -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, ); diff --git a/apps/cli/src/oss-serve-configurator.ts b/apps/cli/src/oss-serve-configurator.ts index 875b8cc750..14e6ace3b3 100644 --- a/apps/cli/src/oss-serve-configurator.ts +++ b/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.")); diff --git a/apps/cli/src/tools/send/commands/get.command.ts b/apps/cli/src/tools/send/commands/get.command.ts index 1b3a8f6c50..2d6cc93c78 100644 --- a/apps/cli/src/tools/send/commands/get.command.ts +++ b/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; diff --git a/apps/cli/src/tools/send/commands/list.command.ts b/apps/cli/src/tools/send/commands/list.command.ts index f611cb3f5d..d3cb73e9b2 100644 --- a/apps/cli/src/tools/send/commands/list.command.ts +++ b/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): Promise { - 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() !== "") { diff --git a/apps/cli/src/tools/send/send.program.ts b/apps/cli/src/tools/send/send.program.ts index 6af714cb78..cbeda188a9 100644 --- a/apps/cli/src/tools/send/send.program.ts +++ b/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, diff --git a/apps/desktop/src/key-management/electron-key.service.ts b/apps/desktop/src/key-management/electron-key.service.ts index 8a6fbfa085..0f5555167c 100644 --- a/apps/desktop/src/key-management/electron-key.service.ts +++ b/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 { - return super.hasUserKeyStored(keySuffix, userId); - } - override async clearStoredUserKey(keySuffix: KeySuffixOptions, userId: UserId): Promise { await super.clearStoredUserKey(keySuffix, userId); } diff --git a/libs/auth/src/common/login-strategies/sso-login.strategy.ts b/libs/auth/src/common/login-strategies/sso-login.strategy.ts index a48ffd0950..8b60e42f03 100644 --- a/libs/auth/src/common/login-strategies/sso-login.strategy.ts +++ b/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); diff --git a/libs/common/src/tools/send/services/send.service.abstraction.ts b/libs/common/src/tools/send/services/send.service.abstraction.ts index 0cf951e419..f586e39a75 100644 --- a/libs/common/src/tools/send/services/send.service.abstraction.ts +++ b/libs/common/src/tools/send/services/send.service.abstraction.ts @@ -54,7 +54,7 @@ export abstract class SendService implements UserKeyRotationDataProvider Promise; + getAllDecryptedFromState: (userId: UserId) => Promise; } export abstract class InternalSendService extends SendService { diff --git a/libs/common/src/tools/send/services/send.service.spec.ts b/libs/common/src/tools/send/services/send.service.spec.ts index 777bc54f29..d2a2d5dd9b 100644 --- a/libs/common/src/tools/send/services/send.service.spec.ts +++ b/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", () => { diff --git a/libs/common/src/tools/send/services/send.service.ts b/libs/common/src/tools/send/services/send.service.ts index 2556fa2e90..623ab7c4a7 100644 --- a/libs/common/src/tools/send/services/send.service.ts +++ b/libs/common/src/tools/send/services/send.service.ts @@ -199,14 +199,14 @@ export class SendService implements InternalSendServiceAbstraction { return response; } - async getAllDecryptedFromState(): Promise { + async getAllDecryptedFromState(userId: UserId): Promise { 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."); } diff --git a/libs/key-management-ui/src/lock/components/lock.component.ts b/libs/key-management-ui/src/lock/components/lock.component.ts index cd731629b4..3d5951a5ac 100644 --- a/libs/key-management-ui/src/lock/components/lock.component.ts +++ b/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; } diff --git a/libs/key-management/src/abstractions/key.service.ts b/libs/key-management/src/abstractions/key.service.ts index 452d3e0243..bbe0dd50f3 100644 --- a/libs/key-management/src/abstractions/key.service.ts +++ b/libs/key-management/src/abstractions/key.service.ts @@ -138,24 +138,13 @@ export abstract class KeyService { userId: string, ): Promise; - /** - * 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; /** * 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; - /** - * @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; + abstract hasUserKey(userId: UserId): Promise; + /** * Generates a new user key * @throws Error when master key is null and there is no active user diff --git a/libs/key-management/src/key.service.spec.ts b/libs/key-management/src/key.service.spec.ts index 1fc998dc13..7d7ac99898 100644 --- a/libs/key-management/src/key.service.spec.ts +++ b/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; 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 = [ diff --git a/libs/key-management/src/key.service.ts b/libs/key-management/src/key.service.ts index eae52a2ba8..a8e75123b8 100644 --- a/libs/key-management/src/key.service.ts +++ b/libs/key-management/src/key.service.ts @@ -198,16 +198,7 @@ export class DefaultKeyService implements KeyServiceAbstraction { return userKey; } - async hasUserKey(userId?: UserId): Promise { - userId ??= await firstValueFrom(this.stateProvider.activeUserId$); - if (userId == null) { - return false; - } - return await this.hasUserKeyInMemory(userId); - } - - async hasUserKeyInMemory(userId?: UserId): Promise { - userId ??= await firstValueFrom(this.stateProvider.activeUserId$); + async hasUserKey(userId: UserId): Promise { 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 { - 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$);