From 1536f93dd3c76b97e38cc2344b6ef997a41205c8 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Wed, 12 Nov 2025 13:09:44 -0800 Subject: [PATCH] add missing authRequestUserId error handling and update tests --- ...ion-auth-request-answering.service.spec.ts | 108 ++++++++++-------- ...xtension-auth-request-answering.service.ts | 5 +- ...top-auth-request-answering.service.spec.ts | 41 ++++--- .../desktop-auth-request-answering.service.ts | 4 + 4 files changed, 94 insertions(+), 64 deletions(-) diff --git a/apps/browser/src/auth/services/auth-request-answering/extension-auth-request-answering.service.spec.ts b/apps/browser/src/auth/services/auth-request-answering/extension-auth-request-answering.service.spec.ts index 377b1cd1882..9ca2522939f 100644 --- a/apps/browser/src/auth/services/auth-request-answering/extension-auth-request-answering.service.spec.ts +++ b/apps/browser/src/auth/services/auth-request-answering/extension-auth-request-answering.service.spec.ts @@ -85,12 +85,20 @@ describe("ExtensionAuthRequestAnsweringService", () => { }); describe("receivedPendingAuthRequest()", () => { + it("should throw if authRequestUserId not given", async () => { + // Act + const promise = sut.receivedPendingAuthRequest(undefined, authRequestId); + + // Assert + await expect(promise).rejects.toThrow("authRequestUserId required"); + }); + it("should throw if authRequestId not given", async () => { // Act const promise = sut.receivedPendingAuthRequest(userId, undefined); // Assert - await expect(promise).rejects.toThrow("authRequestId not found."); + await expect(promise).rejects.toThrow("authRequestId required"); }); it("should add a pending marker for the user to state", async () => { @@ -102,63 +110,69 @@ describe("ExtensionAuthRequestAnsweringService", () => { expect(pendingAuthRequestsState.add).toHaveBeenCalledWith(userId); }); - it("should send an 'openLoginApproval' message if the popup is open and the user is Unlocked, active, and not required to set/change their master password", async () => { - // Arrange - platformUtilsService.isPopupOpen.mockResolvedValue(true); - authService.activeAccountStatus$ = of(AuthenticationStatus.Unlocked); + describe("given the active user is the intended recipient of the auth request, unlocked, and not required to set/change their master password", () => { + describe("given the popup is open", () => { + it("should send an 'openLoginApproval' message", async () => { + // Arrange + platformUtilsService.isPopupOpen.mockResolvedValue(true); + authService.activeAccountStatus$ = of(AuthenticationStatus.Unlocked); - // Act - await sut.receivedPendingAuthRequest(userId, authRequestId); + // Act + await sut.receivedPendingAuthRequest(userId, authRequestId); - // Assert - expect(messagingService.send).toHaveBeenCalledTimes(1); - expect(messagingService.send).toHaveBeenCalledWith("openLoginApproval", { - notificationId: authRequestId, + // Assert + expect(messagingService.send).toHaveBeenCalledTimes(1); + expect(messagingService.send).toHaveBeenCalledWith("openLoginApproval", { + notificationId: authRequestId, + }); + }); + + it("should not create a system notification", async () => { + // Arrange + platformUtilsService.isPopupOpen.mockResolvedValue(true); + authService.activeAccountStatus$ = of(AuthenticationStatus.Unlocked); + + // Act + await sut.receivedPendingAuthRequest(userId, authRequestId); + + // Assert + expect(systemNotificationsService.create).not.toHaveBeenCalled(); + }); }); - }); - it("should not send an 'openLoginApproval' message if the popup is closed", async () => { - // Arrange - platformUtilsService.isPopupOpen.mockResolvedValue(false); - authService.activeAccountStatus$ = of(AuthenticationStatus.Unlocked); + describe("given the popup is closed", () => { + it("should not send an 'openLoginApproval' message", async () => { + // Arrange + platformUtilsService.isPopupOpen.mockResolvedValue(false); + authService.activeAccountStatus$ = of(AuthenticationStatus.Unlocked); - // Act - await sut.receivedPendingAuthRequest(userId, authRequestId); + // Act + await sut.receivedPendingAuthRequest(userId, authRequestId); - // Assert - expect(messagingService.send).not.toHaveBeenCalled(); - }); + // Assert + expect(messagingService.send).not.toHaveBeenCalled(); + }); - it("should create a system notification if the popup is closed", async () => { - // Arrange - platformUtilsService.isPopupOpen.mockResolvedValue(false); - authService.activeAccountStatus$ = of(AuthenticationStatus.Unlocked); + it("should create a system notification", async () => { + // Arrange + platformUtilsService.isPopupOpen.mockResolvedValue(false); + authService.activeAccountStatus$ = of(AuthenticationStatus.Unlocked); - // Act - await sut.receivedPendingAuthRequest(userId, authRequestId); + // Act + await sut.receivedPendingAuthRequest(userId, authRequestId); - // Assert - expect(i18nService.t).toHaveBeenCalledWith("accountAccessRequested"); - expect(i18nService.t).toHaveBeenCalledWith("confirmAccessAttempt", "user@example.com"); - expect(systemNotificationsService.create).toHaveBeenCalledWith({ - id: `${AuthServerNotificationTags.AuthRequest}_${authRequestId}`, - title: "accountAccessRequested", - body: "confirmAccessAttempt:user@example.com", - buttons: [], + // Assert + expect(i18nService.t).toHaveBeenCalledWith("accountAccessRequested"); + expect(i18nService.t).toHaveBeenCalledWith("confirmAccessAttempt", "user@example.com"); + expect(systemNotificationsService.create).toHaveBeenCalledWith({ + id: `${AuthServerNotificationTags.AuthRequest}_${authRequestId}`, + title: "accountAccessRequested", + body: "confirmAccessAttempt:user@example.com", + buttons: [], + }); + }); }); }); - - it("should not create a system notification if the popup is open and the user is Unlocked, active, and not required to set/change their master password", async () => { - // Arrange - platformUtilsService.isPopupOpen.mockResolvedValue(true); - authService.activeAccountStatus$ = of(AuthenticationStatus.Unlocked); - - // Act - await sut.receivedPendingAuthRequest(userId, authRequestId); - - // Assert - expect(systemNotificationsService.create).not.toHaveBeenCalled(); - }); }); describe("activeUserMeetsConditionsToShowApprovalDialog()", () => { diff --git a/apps/browser/src/auth/services/auth-request-answering/extension-auth-request-answering.service.ts b/apps/browser/src/auth/services/auth-request-answering/extension-auth-request-answering.service.ts index c4a6fd1cc7a..988de685978 100644 --- a/apps/browser/src/auth/services/auth-request-answering/extension-auth-request-answering.service.ts +++ b/apps/browser/src/auth/services/auth-request-answering/extension-auth-request-answering.service.ts @@ -48,8 +48,11 @@ export class ExtensionAuthRequestAnsweringService authRequestUserId: UserId, authRequestId: string, ): Promise { + if (!authRequestUserId) { + throw new Error("authRequestUserId required"); + } if (!authRequestId) { - throw new Error("authRequestId not found."); + throw new Error("authRequestId required"); } // Always persist the pending marker for this user to global state. diff --git a/apps/desktop/src/auth/services/auth-request-answering/desktop-auth-request-answering.service.spec.ts b/apps/desktop/src/auth/services/auth-request-answering/desktop-auth-request-answering.service.spec.ts index e9d0b1876e8..aa2f17b8dee 100644 --- a/apps/desktop/src/auth/services/auth-request-answering/desktop-auth-request-answering.service.spec.ts +++ b/apps/desktop/src/auth/services/auth-request-answering/desktop-auth-request-answering.service.spec.ts @@ -76,6 +76,14 @@ describe("DesktopAuthRequestAnsweringService", () => { }); describe("receivedPendingAuthRequest()", () => { + it("should throw if authRequestUserId not given", async () => { + // Act + const promise = sut.receivedPendingAuthRequest(undefined, undefined); + + // Assert + await expect(promise).rejects.toThrow("authRequestUserId required"); + }); + it("should add a pending marker for the user to state", async () => { // Act await sut.receivedPendingAuthRequest(userId, authRequestId); @@ -85,7 +93,7 @@ describe("DesktopAuthRequestAnsweringService", () => { expect(pendingAuthRequestsState.add).toHaveBeenCalledWith(userId); }); - describe("given the user is Unlocked, active, and not required to set/change their master password", () => { + describe("given the active user is the intended recipient of the auth request, unlocked, and not required to set/change their master password", () => { describe("given the Desktop window is visible", () => { it("should send an 'openLoginApproval' message", async () => { // Arrange @@ -149,7 +157,7 @@ describe("DesktopAuthRequestAnsweringService", () => { }); }); - describe("given the user is Locked", () => { + describe("given the active user is Locked", () => { it("should NOT send an 'openLoginApproval' message", async () => { // Arrange (global as any).ipc.platform.isWindowVisible.mockResolvedValue(true); @@ -179,21 +187,27 @@ describe("DesktopAuthRequestAnsweringService", () => { }); }); - describe("given the user is NOT the active user", () => { - it("should NOT send an 'openLoginApproval' message", async () => { - // Arrange + describe("given the active user is not the intended recipient of the auth request", () => { + beforeEach(() => { + // Different active user for these tests const differentUserId = "different-user-id" as UserId; - (global as any).ipc.platform.isWindowVisible.mockResolvedValue(true); - authService.activeAccountStatus$ = of(AuthenticationStatus.Unlocked); accountService.activeAccount$ = of({ id: differentUserId, email: "different@example.com", emailVerified: true, name: "Different User", }); + }); + + it("should NOT send an 'openLoginApproval' message", async () => { + // Arrange + (global as any).ipc.platform.isWindowVisible.mockResolvedValue(true); + authService.activeAccountStatus$ = of(AuthenticationStatus.Unlocked); // Act - await sut.receivedPendingAuthRequest(userId, authRequestId); + // Pass in userId, not differentUserId (the active user), to mimic an auth + // request coming in for a user who is not the active user + await sut.receivedPendingAuthRequest(userId, authRequestId); // pass in userId, not differentUserId // Assert expect(messagingService.send).not.toHaveBeenCalled(); @@ -201,17 +215,12 @@ describe("DesktopAuthRequestAnsweringService", () => { it("should create a system notification", async () => { // Arrange - const differentUserId = "different-user-id" as UserId; (global as any).ipc.platform.isWindowVisible.mockResolvedValue(true); authService.activeAccountStatus$ = of(AuthenticationStatus.Unlocked); - accountService.activeAccount$ = of({ - id: differentUserId, - email: "different@example.com", - emailVerified: true, - name: "Different User", - }); // Act + // Pass in userId, not differentUserId (the active user), to mimic an auth + // request coming in for a user who is not the active user await sut.receivedPendingAuthRequest(userId, authRequestId); // Assert @@ -223,7 +232,7 @@ describe("DesktopAuthRequestAnsweringService", () => { }); }); - describe("given the user is required to set/change their master password", () => { + describe("given the active user is required to set/change their master password", () => { it("should NOT send an 'openLoginApproval' message", async () => { // Arrange (global as any).ipc.platform.isWindowVisible.mockResolvedValue(true); diff --git a/apps/desktop/src/auth/services/auth-request-answering/desktop-auth-request-answering.service.ts b/apps/desktop/src/auth/services/auth-request-answering/desktop-auth-request-answering.service.ts index 1b928746d80..3b2602660fe 100644 --- a/apps/desktop/src/auth/services/auth-request-answering/desktop-auth-request-answering.service.ts +++ b/apps/desktop/src/auth/services/auth-request-answering/desktop-auth-request-answering.service.ts @@ -43,6 +43,10 @@ export class DesktopAuthRequestAnsweringService authRequestUserId: UserId, authRequestId: string, ): Promise { + if (!authRequestUserId) { + throw new Error("authRequestUserId required"); + } + // Always persist the pending marker for this user to global state. await this.pendingAuthRequestsState.add(authRequestUserId);