diff --git a/apps/browser/src/auth/popup/settings/account-security.component.ts b/apps/browser/src/auth/popup/settings/account-security.component.ts index d7b8aa573d4..26a805b3624 100644 --- a/apps/browser/src/auth/popup/settings/account-security.component.ts +++ b/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); } } diff --git a/apps/cli/src/service-container/service-container.ts b/apps/cli/src/service-container/service-container.ts index cdf6c4bbfda..bc5db09da26 100644 --- a/apps/cli/src/service-container/service-container.ts +++ b/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), diff --git a/apps/desktop/src/app/accounts/settings.component.ts b/apps/desktop/src/app/accounts/settings.component.ts index 83c982fbaba..fd0585e805e 100644 --- a/apps/desktop/src/app/accounts/settings.component.ts +++ b/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"); diff --git a/apps/web/src/app/app.component.ts b/apps/web/src/app/app.component.ts index cac0487d05d..3de9bf0a8c8 100644 --- a/apps/web/src/app/app.component.ts +++ b/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), diff --git a/libs/common/src/key-management/vault-timeout/abstractions/vault-timeout-settings.service.ts b/libs/common/src/key-management/vault-timeout/abstractions/vault-timeout-settings.service.ts index 7094a2c2f83..9ff362e4009 100644 --- a/libs/common/src/key-management/vault-timeout/abstractions/vault-timeout-settings.service.ts +++ b/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; - clear: (userId?: string) => Promise; + clear: (userId: UserId) => Promise; } diff --git a/libs/common/src/key-management/vault-timeout/services/vault-timeout-settings.service.ts b/libs/common/src/key-management/vault-timeout/services/vault-timeout-settings.service.ts index 0716bf0bb93..07b8a8c297d 100644 --- a/libs/common/src/key-management/vault-timeout/services/vault-timeout-settings.service.ts +++ b/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 { + async clear(userId: UserId): Promise { await this.keyService.clearPinKeys(userId); } diff --git a/libs/key-management/src/abstractions/key.service.ts b/libs/key-management/src/abstractions/key.service.ts index d67fec4c98e..95b79890c6a 100644 --- a/libs/key-management/src/abstractions/key.service.ts +++ b/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; + abstract clearPinKeys(userId: UserId): Promise; /** * @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; + abstract clearKeys(userId: UserId): Promise; abstract randomNumber(min: number, max: number): Promise; /** * Generates a new cipher key diff --git a/libs/key-management/src/key.service.spec.ts b/libs/key-management/src/key.service.spec.ts index 25d8aff99fb..f1f1286dfc5 100644 --- a/libs/key-management/src/key.service.spec.ts +++ b/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 () => { - let callCount = 0; - stateProvider.activeUserId$ = stateProvider.activeUserId$.pipe(tap(() => callCount++)); - - await keyService.clearKeys(); - expect(callCount).toBe(1); - - // revert to the original state - accountService.activeAccount$ = accountService.activeAccountSubject.asObservable(); - }); + test.each([null as unknown as UserId, undefined as unknown as UserId])( + "throws when the provided userId is %s", + async (userId) => { + await expect(keyService.clearKeys(userId)).rejects.toThrow("UserId is required"); + }, + ); describe.each([ USER_ENCRYPTED_ORGANIZATION_KEYS, @@ -397,14 +393,6 @@ describe("keyService", () => { USER_ENCRYPTED_PRIVATE_KEY, USER_KEY, ])("key removal", (key: UserKeyDefinition) => { - 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; diff --git a/libs/key-management/src/key.service.ts b/libs/key-management/src/key.service.ts index 849b9db6a50..9372dafd3ea 100644 --- a/libs/key-management/src/key.service.ts +++ b/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 { - userId ??= await firstValueFrom(this.stateProvider.activeUserId$); - + async clearPinKeys(userId: UserId): Promise { 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 { - userId ??= await firstValueFrom(this.stateProvider.activeUserId$); - + async clearKeys(userId: UserId): Promise { if (userId == null) { - throw new Error("Cannot clear keys, no user Id resolved."); + throw new Error("UserId is required"); } await this.masterPasswordService.clearMasterKeyHash(userId);