diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index ac2d2d4116a..e7b825e6ce2 100644 --- a/apps/browser/src/background/main.background.ts +++ b/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) { diff --git a/apps/cli/src/service-container/service-container.ts b/apps/cli/src/service-container/service-container.ts index b934b430370..05437e3e3d3 100644 --- a/apps/cli/src/service-container/service-container.ts +++ b/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) => - await this.keyService.clearStoredUserKey(KeySuffixOptions.Auto); + const lockedCallback = async (userId: UserId) => + await this.keyService.clearStoredUserKey(KeySuffixOptions.Auto, userId); this.userVerificationApiService = new UserVerificationApiService(this.apiService); diff --git a/apps/desktop/src/key-management/electron-key.service.ts b/apps/desktop/src/key-management/electron-key.service.ts index 2941276720c..d31e717e7a5 100644 --- a/apps/desktop/src/key-management/electron-key.service.ts +++ b/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 { + override async clearStoredUserKey(keySuffix: KeySuffixOptions, userId: UserId): Promise { await super.clearStoredUserKey(keySuffix, userId); } diff --git a/libs/common/src/key-management/vault-timeout/services/vault-timeout.service.spec.ts b/libs/common/src/key-management/vault-timeout/services/vault-timeout.service.spec.ts index b17e85ca9c4..5ce7e37778d 100644 --- a/libs/common/src/key-management/vault-timeout/services/vault-timeout.service.spec.ts +++ b/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 - // 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); + expect(lockedCallback).toHaveBeenCalledWith("user1"); }); it("should call state event runner with user passed into lock", async () => { diff --git a/libs/common/src/key-management/vault-timeout/services/vault-timeout.service.ts b/libs/common/src/key-management/vault-timeout/services/vault-timeout.service.ts index 131f826fd33..04769567db2 100644 --- a/libs/common/src/key-management/vault-timeout/services/vault-timeout.service.ts +++ b/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 = null, + private lockedCallback: (userId: UserId) => Promise = 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); } } diff --git a/libs/key-management/src/abstractions/key.service.ts b/libs/key-management/src/abstractions/key.service.ts index 4a3fca16515..965c6858470 100644 --- a/libs/key-management/src/abstractions/key.service.ts +++ b/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; + abstract clearStoredUserKey(keySuffix: KeySuffixOptions, userId: string): Promise; /** * Stores the master key encrypted user key * @throws Error when userId 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 7d30af23372..1fc998dc131 100644 --- a/libs/key-management/src/key.service.spec.ts +++ b/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", diff --git a/libs/key-management/src/key.service.ts b/libs/key-management/src/key.service.ts index 6cbb1fbcc03..a872e89cb82 100644 --- a/libs/key-management/src/key.service.ts +++ b/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 { - if (keySuffix === KeySuffixOptions.Auto) { - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.stateService.setUserKeyAutoUnlock(null, { userId: userId }); + async clearStoredUserKey(keySuffix: KeySuffixOptions, userId: UserId): Promise { + if (userId == null) { + throw new Error("UserId is required"); } - 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. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.pinService.clearPinKeyEncryptedUserKeyEphemeral(userId); + + if (keySuffix === KeySuffixOptions.Auto) { + await this.stateService.setUserKeyAutoUnlock(null, { userId: userId }); + } + if (keySuffix === KeySuffixOptions.Pin) { + await this.pinService.clearPinKeyEncryptedUserKeyEphemeral(userId); } }