From bfb0b874ed70060bcb58aecc21be3a2e3eb35dd5 Mon Sep 17 00:00:00 2001 From: Jared Snider <116684653+JaredSnider-Bitwarden@users.noreply.github.com> Date: Fri, 13 Jun 2025 13:22:04 -0400 Subject: [PATCH] fix(Multi-Account-Logout: [Auth/PM-19555] Fix multi account logout on lock screens not redirecting properly (#14630) * PM-19555 - LogoutService - build abstraction, default, and extension service and register with service modules * PM-19555 - Lock Comp - use logoutService * PM-19555 - LoginDecryptionOptions - Use logout service which removed need for extension-login-decryption-options.service * PM-19555 - AccountSwitcher logic update - (1) Use logout service + redirect guard routing (2) Remove logout method from account switcher service (3) use new NewActiveUser type * PM-19555 - Extension - Acct Switcher comp - clean up TODOs * PM-19555 - Add TODOs for remaining tech debt * PM-19555 - Add tests for new logout services. * PM-19555 - Extension - LoginInitiated - show acct switcher b/c user is AuthN * PM-19555 - Add TODO to replace LogoutCallback with LogoutService * PM-19555 WIP * PM-19555 - Extension App Comp - account switching to account in TDE locked state works now. * PM-19555 - Extension App Comp - add docs * PM-19555 - Extension App Comp - add early return * PM-19555 - Desktop App Comp - add handling for TDE lock case to switch account logic. * PM-19555 - Extension - Account Component - if account unlocked go to vault * PM-19555 - Per PR feedback, clean up unnecessary nullish coalescing operator. * PM-19555 - Extension - AppComponent - fix everHadUserKey merge issue * PM-19555 - PR feedback - refactor switchAccount and locked message handling on browser & desktop to require user id. I audited all callsites for both to ensure this *shouldn't* error. --- .../account-switcher.component.ts | 12 +-- .../account-switching/account.component.ts | 9 +- .../services/account-switcher.service.spec.ts | 31 ------ .../services/account-switcher.service.ts | 27 +---- ...n-login-decryption-options.service.spec.ts | 64 ----------- ...ension-login-decryption-options.service.ts | 39 ------- .../logout/extension-logout.service.spec.ts | 101 ++++++++++++++++++ .../popup/logout/extension-logout.service.ts | 39 +++++++ .../browser/src/background/main.background.ts | 1 + apps/browser/src/popup/app-routing.module.ts | 3 +- apps/browser/src/popup/app.component.ts | 48 +++++++-- .../src/popup/services/services.module.ts | 16 +-- apps/desktop/src/app/app.component.ts | 35 +++++- .../src/services/jslib-services.module.ts | 8 ++ .../login-decryption-options.component.ts | 18 ++-- .../login-decryption-options.service.ts | 4 - libs/auth/src/common/abstractions/index.ts | 1 + .../src/common/abstractions/logout.service.ts | 19 ++++ libs/auth/src/common/services/index.ts | 1 + .../logout/default-logout.service.spec.ts | 40 +++++++ .../services/logout/default-logout.service.ts | 14 +++ .../device-trust.service.implementation.ts | 8 +- .../src/lock/components/lock.component.ts | 7 +- 23 files changed, 334 insertions(+), 211 deletions(-) delete mode 100644 apps/browser/src/auth/popup/login-decryption-options/extension-login-decryption-options.service.spec.ts delete mode 100644 apps/browser/src/auth/popup/login-decryption-options/extension-login-decryption-options.service.ts create mode 100644 apps/browser/src/auth/popup/logout/extension-logout.service.spec.ts create mode 100644 apps/browser/src/auth/popup/logout/extension-logout.service.ts create mode 100644 libs/auth/src/common/abstractions/logout.service.ts create mode 100644 libs/auth/src/common/services/logout/default-logout.service.spec.ts create mode 100644 libs/auth/src/common/services/logout/default-logout.service.ts diff --git a/apps/browser/src/auth/popup/account-switching/account-switcher.component.ts b/apps/browser/src/auth/popup/account-switching/account-switcher.component.ts index 3c94fbeef70..48fd57431a2 100644 --- a/apps/browser/src/auth/popup/account-switching/account-switcher.component.ts +++ b/apps/browser/src/auth/popup/account-switching/account-switcher.component.ts @@ -4,7 +4,7 @@ import { Router } from "@angular/router"; import { Subject, firstValueFrom, map, of, startWith, switchMap } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; -import { LockService } from "@bitwarden/auth/common"; +import { LockService, LogoutService } from "@bitwarden/auth/common"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; @@ -69,6 +69,7 @@ export class AccountSwitcherComponent implements OnInit, OnDestroy { private vaultTimeoutSettingsService: VaultTimeoutSettingsService, private authService: AuthService, private lockService: LockService, + private logoutService: LogoutService, ) {} get accountLimit() { @@ -140,12 +141,9 @@ export class AccountSwitcherComponent implements OnInit, OnDestroy { }); if (confirmed) { - const result = await this.accountSwitcherService.logoutAccount(userId); - // unlocked logout responses need to be navigated out of the account switcher. - // other responses will be handled by background and app.component - if (result?.status === AuthenticationStatus.Unlocked) { - this.location.back(); - } + await this.logoutService.logout(userId); + // navigate to root so redirect guard can properly route next active user or null user to correct page + await this.router.navigate(["/"]); } this.loading = false; } diff --git a/apps/browser/src/auth/popup/account-switching/account.component.ts b/apps/browser/src/auth/popup/account-switching/account.component.ts index cdd2656fdc1..c060d9161ef 100644 --- a/apps/browser/src/auth/popup/account-switching/account.component.ts +++ b/apps/browser/src/auth/popup/account-switching/account.component.ts @@ -1,7 +1,8 @@ // FIXME: Update this file to be type safe and remove this and next line // @ts-strict-ignore -import { CommonModule, Location } from "@angular/common"; +import { CommonModule } from "@angular/common"; import { Component, EventEmitter, Input, Output } from "@angular/core"; +import { Router } from "@angular/router"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; @@ -23,7 +24,7 @@ export class AccountComponent { constructor( private accountSwitcherService: AccountSwitcherService, - private location: Location, + private router: Router, private i18nService: I18nService, private logService: LogService, private biometricsService: BiometricsService, @@ -44,8 +45,8 @@ export class AccountComponent { // Navigate out of account switching for unlocked accounts // locked or logged out account statuses are handled by background and app.component - if (result?.status === AuthenticationStatus.Unlocked) { - this.location.back(); + if (result?.authenticationStatus === AuthenticationStatus.Unlocked) { + await this.router.navigate(["vault"]); await this.biometricsService.setShouldAutopromptNow(false); } else { await this.biometricsService.setShouldAutopromptNow(true); diff --git a/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.spec.ts b/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.spec.ts index 1fdd0b1ecf2..4bacd453803 100644 --- a/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.spec.ts +++ b/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.spec.ts @@ -207,35 +207,4 @@ describe("AccountSwitcherService", () => { expect(removeListenerSpy).toBeCalledTimes(1); }); }); - - describe("logout", () => { - const userId1 = "1" as UserId; - const userId2 = "2" as UserId; - it("initiates logout", async () => { - let listener: ( - message: { command: string; userId: UserId; status: AuthenticationStatus }, - sender: unknown, - sendResponse: unknown, - ) => void; - jest.spyOn(chrome.runtime.onMessage, "addListener").mockImplementation((addedListener) => { - listener = addedListener; - }); - - const removeListenerSpy = jest.spyOn(chrome.runtime.onMessage, "removeListener"); - - const logoutPromise = accountSwitcherService.logoutAccount(userId1); - - listener( - { command: "switchAccountFinish", userId: userId2, status: AuthenticationStatus.Unlocked }, - undefined, - undefined, - ); - - const result = await logoutPromise; - - expect(messagingService.send).toHaveBeenCalledWith("logout", { userId: userId1 }); - expect(result).toEqual({ newUserId: userId2, status: AuthenticationStatus.Unlocked }); - expect(removeListenerSpy).toBeCalledTimes(1); - }); - }); }); diff --git a/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.ts b/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.ts index bfed7dc1408..7bb12fc260d 100644 --- a/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.ts +++ b/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.ts @@ -12,6 +12,7 @@ import { timeout, } from "rxjs"; +import { NewActiveUser } from "@bitwarden/auth/common"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AvatarService } from "@bitwarden/common/auth/abstractions/avatar.service"; @@ -43,7 +44,7 @@ export class AccountSwitcherService { SPECIAL_ADD_ACCOUNT_ID = "addAccount"; availableAccounts$: Observable; - switchAccountFinished$: Observable<{ userId: UserId; status: AuthenticationStatus }>; + switchAccountFinished$: Observable; constructor( private accountService: AccountService, @@ -118,7 +119,7 @@ export class AccountSwitcherService { [message: { command: string; userId: UserId; status: AuthenticationStatus }] >(chrome.runtime.onMessage).pipe( filter(([message]) => message.command === "switchAccountFinish"), - map(([message]) => ({ userId: message.userId, status: message.status })), + map(([message]) => ({ userId: message.userId, authenticationStatus: message.status })), ); } @@ -143,29 +144,9 @@ export class AccountSwitcherService { return await switchAccountFinishedPromise; } - /** - * - * @param userId the user id to logout - * @returns the userId and status of the that has been switch to due to the logout. null on errors. - */ - async logoutAccount( - userId: UserId, - ): Promise<{ newUserId: UserId; status: AuthenticationStatus } | null> { - // logout creates an account switch to the next up user, which may be null - const switchPromise = this.listenForSwitchAccountFinish(null); - - await this.messagingService.send("logout", { userId }); - - // wait for account switch to happen, the result will be the new user id and status - const result = await switchPromise; - return { newUserId: result.userId, status: result.status }; - } - // Listens for the switchAccountFinish message and returns the userId from the message // Optionally filters switchAccountFinish to an expected userId - private listenForSwitchAccountFinish( - expectedUserId: UserId | null, - ): Promise<{ userId: UserId; status: AuthenticationStatus } | null> { + listenForSwitchAccountFinish(expectedUserId: UserId | null): Promise { return firstValueFrom( this.switchAccountFinished$.pipe( filter(({ userId }) => (expectedUserId ? userId === expectedUserId : true)), diff --git a/apps/browser/src/auth/popup/login-decryption-options/extension-login-decryption-options.service.spec.ts b/apps/browser/src/auth/popup/login-decryption-options/extension-login-decryption-options.service.spec.ts deleted file mode 100644 index 8f3199cdfce..00000000000 --- a/apps/browser/src/auth/popup/login-decryption-options/extension-login-decryption-options.service.spec.ts +++ /dev/null @@ -1,64 +0,0 @@ -import { Router } from "@angular/router"; -import { MockProxy, mock } from "jest-mock-extended"; -import { BehaviorSubject } from "rxjs"; - -import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; - -import { postLogoutMessageListener$ } from "../utils/post-logout-message-listener"; - -import { ExtensionLoginDecryptionOptionsService } from "./extension-login-decryption-options.service"; - -// Mock the module providing postLogoutMessageListener$ -jest.mock("../utils/post-logout-message-listener", () => { - return { - postLogoutMessageListener$: new BehaviorSubject(""), // Replace with mock subject - }; -}); - -describe("ExtensionLoginDecryptionOptionsService", () => { - let service: ExtensionLoginDecryptionOptionsService; - - let messagingService: MockProxy; - let router: MockProxy; - let postLogoutMessageSubject: BehaviorSubject; - - beforeEach(() => { - messagingService = mock(); - router = mock(); - - // Cast postLogoutMessageListener$ to BehaviorSubject for dynamic control - postLogoutMessageSubject = postLogoutMessageListener$ as BehaviorSubject; - - service = new ExtensionLoginDecryptionOptionsService(messagingService, router); - }); - - it("should instantiate the service", () => { - expect(service).not.toBeFalsy(); - }); - - describe("logOut()", () => { - it("should send a logout message", async () => { - postLogoutMessageSubject.next("switchAccountFinish"); - - await service.logOut(); - - expect(messagingService.send).toHaveBeenCalledWith("logout"); - }); - - it("should navigate to root on 'switchAccountFinish'", async () => { - postLogoutMessageSubject.next("switchAccountFinish"); - - await service.logOut(); - - expect(router.navigate).toHaveBeenCalledWith(["/"]); - }); - - it("should not navigate for 'doneLoggingOut'", async () => { - postLogoutMessageSubject.next("doneLoggingOut"); - - await service.logOut(); - - expect(router.navigate).not.toHaveBeenCalled(); - }); - }); -}); diff --git a/apps/browser/src/auth/popup/login-decryption-options/extension-login-decryption-options.service.ts b/apps/browser/src/auth/popup/login-decryption-options/extension-login-decryption-options.service.ts deleted file mode 100644 index 3e591e08ac1..00000000000 --- a/apps/browser/src/auth/popup/login-decryption-options/extension-login-decryption-options.service.ts +++ /dev/null @@ -1,39 +0,0 @@ -import { Router } from "@angular/router"; -import { firstValueFrom } from "rxjs"; - -import { - DefaultLoginDecryptionOptionsService, - LoginDecryptionOptionsService, -} from "@bitwarden/auth/angular"; -import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; - -import { postLogoutMessageListener$ } from "../utils/post-logout-message-listener"; - -export class ExtensionLoginDecryptionOptionsService - extends DefaultLoginDecryptionOptionsService - implements LoginDecryptionOptionsService -{ - constructor( - protected messagingService: MessagingService, - private router: Router, - ) { - super(messagingService); - } - - override async logOut(): Promise { - // start listening for "switchAccountFinish" or "doneLoggingOut" - const messagePromise = firstValueFrom(postLogoutMessageListener$); - - // 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 - super.logOut(); - - // wait for messages - const command = await messagePromise; - - // doneLoggingOut already has a message handler that will navigate us - if (command === "switchAccountFinish") { - await this.router.navigate(["/"]); - } - } -} diff --git a/apps/browser/src/auth/popup/logout/extension-logout.service.spec.ts b/apps/browser/src/auth/popup/logout/extension-logout.service.spec.ts new file mode 100644 index 00000000000..7ab7742c1c3 --- /dev/null +++ b/apps/browser/src/auth/popup/logout/extension-logout.service.spec.ts @@ -0,0 +1,101 @@ +import { MockProxy, mock } from "jest-mock-extended"; + +import { LogoutReason, LogoutService } from "@bitwarden/auth/common"; +import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; +import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; +import { UserId } from "@bitwarden/common/types/guid"; + +import { AccountSwitcherService } from "../account-switching/services/account-switcher.service"; + +import { ExtensionLogoutService } from "./extension-logout.service"; + +describe("ExtensionLogoutService", () => { + let logoutService: LogoutService; + let messagingService: MockProxy; + let accountSwitcherService: MockProxy; + + let primaryUserId: UserId; + let secondaryUserId: UserId; + let logoutReason: LogoutReason; + + beforeEach(() => { + primaryUserId = "1" as UserId; + secondaryUserId = "2" as UserId; + logoutReason = "vaultTimeout"; + + messagingService = mock(); + accountSwitcherService = mock(); + logoutService = new ExtensionLogoutService(messagingService, accountSwitcherService); + }); + + it("instantiates", () => { + expect(logoutService).not.toBeFalsy(); + }); + + describe("logout", () => { + describe("No new active user", () => { + beforeEach(() => { + accountSwitcherService.listenForSwitchAccountFinish.mockResolvedValue(null); + }); + + it("sends logout message without a logout reason when not provided", async () => { + const result = await logoutService.logout(primaryUserId); + + expect(accountSwitcherService.listenForSwitchAccountFinish).toHaveBeenCalledTimes(1); + expect(messagingService.send).toHaveBeenCalledWith("logout", { userId: primaryUserId }); + + expect(result).toBeUndefined(); + }); + + it("sends logout message with a logout reason when provided", async () => { + const result = await logoutService.logout(primaryUserId, logoutReason); + + expect(accountSwitcherService.listenForSwitchAccountFinish).toHaveBeenCalledTimes(1); + expect(messagingService.send).toHaveBeenCalledWith("logout", { + userId: primaryUserId, + logoutReason, + }); + expect(result).toBeUndefined(); + }); + }); + + describe("New active user", () => { + const newActiveUserAuthenticationStatus = AuthenticationStatus.Unlocked; + + beforeEach(() => { + accountSwitcherService.listenForSwitchAccountFinish.mockResolvedValue({ + userId: secondaryUserId, + authenticationStatus: newActiveUserAuthenticationStatus, + }); + }); + + it("sends logout message without a logout reason when not provided and returns the new active user", async () => { + const result = await logoutService.logout(primaryUserId); + + expect(accountSwitcherService.listenForSwitchAccountFinish).toHaveBeenCalledTimes(1); + + expect(messagingService.send).toHaveBeenCalledWith("logout", { userId: primaryUserId }); + + expect(result).toEqual({ + userId: secondaryUserId, + authenticationStatus: newActiveUserAuthenticationStatus, + }); + }); + + it("sends logout message with a logout reason when provided and returns the new active user", async () => { + const result = await logoutService.logout(primaryUserId, logoutReason); + + expect(accountSwitcherService.listenForSwitchAccountFinish).toHaveBeenCalledTimes(1); + + expect(messagingService.send).toHaveBeenCalledWith("logout", { + userId: primaryUserId, + logoutReason, + }); + expect(result).toEqual({ + userId: secondaryUserId, + authenticationStatus: newActiveUserAuthenticationStatus, + }); + }); + }); + }); +}); diff --git a/apps/browser/src/auth/popup/logout/extension-logout.service.ts b/apps/browser/src/auth/popup/logout/extension-logout.service.ts new file mode 100644 index 00000000000..c43c18f157a --- /dev/null +++ b/apps/browser/src/auth/popup/logout/extension-logout.service.ts @@ -0,0 +1,39 @@ +import { + DefaultLogoutService, + LogoutReason, + LogoutService, + NewActiveUser, +} from "@bitwarden/auth/common"; +import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; +import { UserId } from "@bitwarden/common/types/guid"; + +import { AccountSwitcherService } from "../account-switching/services/account-switcher.service"; + +export class ExtensionLogoutService extends DefaultLogoutService implements LogoutService { + constructor( + protected messagingService: MessagingService, + private accountSwitcherService: AccountSwitcherService, + ) { + super(messagingService); + } + + override async logout( + userId: UserId, + logoutReason?: LogoutReason, + ): Promise { + // logout can result in an account switch to the next up user + const accountSwitchFinishPromise = + this.accountSwitcherService.listenForSwitchAccountFinish(null); + + // send the logout message + this.messagingService.send("logout", { userId, logoutReason }); + + // wait for the account switch to finish + const result = await accountSwitchFinishPromise; + if (result) { + return { userId: result.userId, authenticationStatus: result.authenticationStatus }; + } + // if there is no account switch, return undefined + return undefined; + } +} diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index c353cdb4f93..f24d769b912 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -1539,6 +1539,7 @@ export default class MainBackground { } } + // TODO: PM-21212 - consolidate the logic of this method into the new ExtensionLogoutService async logout(logoutReason: LogoutReason, userId?: UserId) { const activeUserId = await firstValueFrom( this.accountService.activeAccount$.pipe( diff --git a/apps/browser/src/popup/app-routing.module.ts b/apps/browser/src/popup/app-routing.module.ts index 2963b3daec5..b530a868b61 100644 --- a/apps/browser/src/popup/app-routing.module.ts +++ b/apps/browser/src/popup/app-routing.module.ts @@ -496,7 +496,8 @@ const routes: Routes = [ canActivate: [tdeDecryptionRequiredGuard()], data: { pageIcon: DevicesIcon, - }, + showAcctSwitcher: true, + } satisfies ExtensionAnonLayoutWrapperData, children: [{ path: "", component: LoginDecryptionOptionsComponent }], }, { diff --git a/apps/browser/src/popup/app.component.ts b/apps/browser/src/popup/app.component.ts index f009ad064c4..b6d3615af94 100644 --- a/apps/browser/src/popup/app.component.ts +++ b/apps/browser/src/popup/app.component.ts @@ -11,16 +11,17 @@ import { } from "@angular/core"; import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; import { NavigationEnd, Router, RouterOutlet } from "@angular/router"; -import { Subject, takeUntil, firstValueFrom, concatMap, filter, tap } from "rxjs"; +import { Subject, takeUntil, firstValueFrom, concatMap, filter, tap, map } from "rxjs"; import { DeviceTrustToastService } from "@bitwarden/angular/auth/services/device-trust-toast.service.abstraction"; import { DocumentLangSetter } from "@bitwarden/angular/platform/i18n"; -import { LogoutReason } from "@bitwarden/auth/common"; +import { LogoutReason, UserDecryptionOptionsServiceAbstraction } from "@bitwarden/auth/common"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { AnimationControlService } from "@bitwarden/common/platform/abstractions/animation-control.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { MessageListener } from "@bitwarden/common/platform/messaging"; @@ -32,7 +33,7 @@ import { ToastOptions, ToastService, } from "@bitwarden/components"; -import { BiometricsService, BiometricStateService } from "@bitwarden/key-management"; +import { BiometricsService, BiometricStateService, KeyService } from "@bitwarden/key-management"; import { PopupCompactModeService } from "../platform/popup/layout/popup-compact-mode.service"; import { PopupSizeService } from "../platform/popup/layout/popup-size.service"; @@ -82,9 +83,12 @@ export class AppComponent implements OnInit, OnDestroy { private biometricStateService: BiometricStateService, private biometricsService: BiometricsService, private deviceTrustToastService: DeviceTrustToastService, + private userDecryptionOptionsService: UserDecryptionOptionsServiceAbstraction, + private keyService: KeyService, private readonly destoryRef: DestroyRef, private readonly documentLangSetter: DocumentLangSetter, private popupSizeService: PopupSizeService, + private logService: LogService, ) { this.deviceTrustToastService.setupListeners$.pipe(takeUntilDestroyed()).subscribe(); @@ -137,14 +141,38 @@ export class AppComponent implements OnInit, OnDestroy { this.changeDetectorRef.detectChanges(); } else if (msg.command === "authBlocked" || msg.command === "goHome") { await this.router.navigate(["login"]); - } else if ( - msg.command === "locked" && - (msg.userId == null || msg.userId == this.activeUserId) - ) { + } else if (msg.command === "locked") { + if (msg.userId == null) { + this.logService.error("'locked' message received without userId."); + return; + } + + if (msg.userId !== this.activeUserId) { + this.logService.error( + `'locked' message received with userId ${msg.userId} but active userId is ${this.activeUserId}.`, + ); + return; + } + await this.biometricsService.setShouldAutopromptNow(false); - // 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.router.navigate(["lock"]); + + // When user is locked, normally we can just send them the lock screen. + // However, for account switching scenarios, we need to consider the TDE lock state. + const tdeEnabled = await firstValueFrom( + this.userDecryptionOptionsService + .userDecryptionOptionsById$(msg.userId) + .pipe(map((decryptionOptions) => decryptionOptions?.trustedDeviceOption != null)), + ); + + const everHadUserKey = await firstValueFrom( + this.keyService.everHadUserKey$(msg.userId), + ); + if (tdeEnabled && !everHadUserKey) { + await this.router.navigate(["login-initiated"]); + return; + } + + await this.router.navigate(["lock"]); } else if (msg.command === "showDialog") { // 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 diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index 6ede88dfc13..00e493fc035 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -1,7 +1,6 @@ // FIXME: Update this file to be type safe and remove this and next line // @ts-strict-ignore import { APP_INITIALIZER, NgModule, NgZone } from "@angular/core"; -import { Router } from "@angular/router"; import { merge, of, Subject } from "rxjs"; import { CollectionService } from "@bitwarden/admin-console/common"; @@ -25,7 +24,6 @@ import { JslibServicesModule } from "@bitwarden/angular/services/jslib-services. import { AnonLayoutWrapperDataService, LoginComponentService, - LoginDecryptionOptionsService, TwoFactorAuthComponentService, TwoFactorAuthEmailComponentService, TwoFactorAuthDuoComponentService, @@ -37,6 +35,7 @@ import { LoginEmailService, PinServiceAbstraction, SsoUrlService, + LogoutService, } from "@bitwarden/auth/common"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; import { EventCollectionService as EventCollectionServiceAbstraction } from "@bitwarden/common/abstractions/event/event-collection.service"; @@ -137,11 +136,12 @@ import { SshImportPromptService, } from "@bitwarden/vault"; +import { AccountSwitcherService } from "../../auth/popup/account-switching/services/account-switcher.service"; import { ForegroundLockService } from "../../auth/popup/accounts/foreground-lock.service"; import { ExtensionAnonLayoutWrapperDataService } from "../../auth/popup/extension-anon-layout-wrapper/extension-anon-layout-wrapper-data.service"; import { ExtensionLoginComponentService } from "../../auth/popup/login/extension-login-component.service"; import { ExtensionSsoComponentService } from "../../auth/popup/login/extension-sso-component.service"; -import { ExtensionLoginDecryptionOptionsService } from "../../auth/popup/login-decryption-options/extension-login-decryption-options.service"; +import { ExtensionLogoutService } from "../../auth/popup/logout/extension-logout.service"; import { ExtensionTwoFactorAuthComponentService } from "../../auth/services/extension-two-factor-auth-component.service"; import { ExtensionTwoFactorAuthDuoComponentService } from "../../auth/services/extension-two-factor-auth-duo-component.service"; import { ExtensionTwoFactorAuthEmailComponentService } from "../../auth/services/extension-two-factor-auth-email-component.service"; @@ -642,6 +642,11 @@ const safeProviders: SafeProvider[] = [ useClass: ExtensionAnonLayoutWrapperDataService, deps: [], }), + safeProvider({ + provide: LogoutService, + useClass: ExtensionLogoutService, + deps: [MessagingServiceAbstraction, AccountSwitcherService], + }), safeProvider({ provide: CompactModeService, useExisting: PopupCompactModeService, @@ -652,11 +657,6 @@ const safeProviders: SafeProvider[] = [ useClass: ExtensionSsoComponentService, deps: [SyncService, AuthService, EnvironmentService, I18nServiceAbstraction, LogService], }), - safeProvider({ - provide: LoginDecryptionOptionsService, - useClass: ExtensionLoginDecryptionOptionsService, - deps: [MessagingServiceAbstraction, Router], - }), safeProvider({ provide: SshImportPromptService, useClass: DefaultSshImportPromptService, diff --git a/apps/desktop/src/app/app.component.ts b/apps/desktop/src/app/app.component.ts index b578be6ad5b..dc1621210de 100644 --- a/apps/desktop/src/app/app.component.ts +++ b/apps/desktop/src/app/app.component.ts @@ -29,7 +29,11 @@ import { ModalRef } from "@bitwarden/angular/components/modal/modal.ref"; import { DocumentLangSetter } from "@bitwarden/angular/platform/i18n"; import { ModalService } from "@bitwarden/angular/services/modal.service"; import { FingerprintDialogComponent, LoginApprovalComponent } from "@bitwarden/auth/angular"; -import { DESKTOP_SSO_CALLBACK, LogoutReason } from "@bitwarden/auth/common"; +import { + DESKTOP_SSO_CALLBACK, + LogoutReason, + UserDecryptionOptionsServiceAbstraction, +} from "@bitwarden/auth/common"; import { EventUploadService } from "@bitwarden/common/abstractions/event/event-upload.service"; import { SearchService } from "@bitwarden/common/abstractions/search.service"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; @@ -165,6 +169,7 @@ export class AppComponent implements OnInit, OnDestroy { private accountService: AccountService, private organizationService: OrganizationService, private deviceTrustToastService: DeviceTrustToastService, + private userDecryptionOptionsService: UserDecryptionOptionsServiceAbstraction, private readonly destroyRef: DestroyRef, private readonly documentLangSetter: DocumentLangSetter, ) { @@ -416,15 +421,35 @@ export class AppComponent implements OnInit, OnDestroy { this.router.navigate(["/remove-password"]); break; case "switchAccount": { - if (message.userId != null) { - await this.accountService.switchAccount(message.userId); + if (message.userId == null) { + this.logService.error("'switchAccount' message received without userId."); + return; } + + await this.accountService.switchAccount(message.userId); + const locked = (await this.authService.getAuthStatus(message.userId)) === AuthenticationStatus.Locked; if (locked) { this.modalService.closeAll(); - await this.router.navigate(["lock"]); + + // We only have to handle TDE lock on "switchAccount" message scenarios but not normal + // lock scenarios since the user will have always decrypted the vault at least once in those cases. + const tdeEnabled = await firstValueFrom( + this.userDecryptionOptionsService + .userDecryptionOptionsById$(message.userId) + .pipe(map((decryptionOptions) => decryptionOptions?.trustedDeviceOption != null)), + ); + + const everHadUserKey = await firstValueFrom( + this.keyService.everHadUserKey$(message.userId), + ); + if (tdeEnabled && !everHadUserKey) { + await this.router.navigate(["login-initiated"]); + } else { + await this.router.navigate(["lock"]); + } } else { this.messagingService.send("unlocked"); this.loading = true; @@ -600,6 +625,8 @@ export class AppComponent implements OnInit, OnDestroy { } } + // TODO: PM-21212 - consolidate the logic of this method into the new LogoutService + // (requires creating a desktop specific implementation of the LogoutService) // Even though the userId parameter is no longer optional doesn't mean a message couldn't be // passing null-ish values to us. private async logOut(logoutReason: LogoutReason, userId: UserId) { diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 15fd6b0fbaf..e1f806c4d3e 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -42,6 +42,7 @@ import { AuthRequestServiceAbstraction, DefaultAuthRequestApiService, DefaultLoginSuccessHandlerService, + DefaultLogoutService, InternalUserDecryptionOptionsServiceAbstraction, LoginApprovalComponentServiceAbstraction, LoginEmailService, @@ -50,6 +51,7 @@ import { LoginStrategyServiceAbstraction, LoginSuccessHandlerService, LogoutReason, + LogoutService, PinService, PinServiceAbstraction, UserDecryptionOptionsService, @@ -405,6 +407,7 @@ const safeProviders: SafeProvider[] = [ provide: STATE_FACTORY, useValue: new StateFactory(GlobalState, Account), }), + // TODO: PM-21212 - Deprecate LogoutCallback in favor of LogoutService safeProvider({ provide: LOGOUT_CALLBACK, useFactory: @@ -1540,6 +1543,11 @@ const safeProviders: SafeProvider[] = [ useClass: MasterPasswordApiService, deps: [ApiServiceAbstraction, LogService], }), + safeProvider({ + provide: LogoutService, + useClass: DefaultLogoutService, + deps: [MessagingServiceAbstraction], + }), safeProvider({ provide: DocumentLangSetter, useClass: DocumentLangSetter, diff --git a/libs/auth/src/angular/login-decryption-options/login-decryption-options.component.ts b/libs/auth/src/angular/login-decryption-options/login-decryption-options.component.ts index 0de51d83ac8..172823f23da 100644 --- a/libs/auth/src/angular/login-decryption-options/login-decryption-options.component.ts +++ b/libs/auth/src/angular/login-decryption-options/login-decryption-options.component.ts @@ -10,6 +10,7 @@ import { catchError, defer, firstValueFrom, from, map, of, switchMap, throwError import { JslibModule } from "@bitwarden/angular/jslib.module"; import { LoginEmailServiceAbstraction, + LogoutService, UserDecryptionOptions, UserDecryptionOptionsServiceAbstraction, } from "@bitwarden/auth/common"; @@ -109,6 +110,7 @@ export class LoginDecryptionOptionsComponent implements OnInit { private toastService: ToastService, private userDecryptionOptionsService: UserDecryptionOptionsServiceAbstraction, private validationService: ValidationService, + private logoutService: LogoutService, ) { this.clientType = this.platformUtilsService.getClientType(); } @@ -156,19 +158,17 @@ export class LoginDecryptionOptionsComponent implements OnInit { } private async handleMissingEmail() { + // TODO: PM-15174 - the solution for this bug will allow us to show the toast on app re-init after + // the user has been logged out and the process reload has occurred. this.toastService.showToast({ variant: "error", title: null, message: this.i18nService.t("activeUserEmailNotFoundLoggingYouOut"), }); - setTimeout(async () => { - // We can't simply redirect to `/login` because the user is authed and the unauthGuard - // will prevent navigation. We must logout the user first via messagingService, which - // redirects to `/`, which will be handled by the redirectGuard to navigate the user to `/login`. - // The timeout just gives the user a chance to see the error toast before process reload runs on logout. - await this.loginDecryptionOptionsService.logOut(); - }, 5000); + await this.logoutService.logout(this.activeAccountId); + // navigate to root so redirect guard can properly route next active user or null user to correct page + await this.router.navigate(["/"]); } private observeAndPersistRememberDeviceValueChanges() { @@ -312,7 +312,9 @@ export class LoginDecryptionOptionsComponent implements OnInit { const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id; if (confirmed) { - this.messagingService.send("logout", { userId: userId }); + await this.logoutService.logout(userId); + // navigate to root so redirect guard can properly route next active user or null user to correct page + await this.router.navigate(["/"]); } } } diff --git a/libs/auth/src/angular/login-decryption-options/login-decryption-options.service.ts b/libs/auth/src/angular/login-decryption-options/login-decryption-options.service.ts index d81d56d6393..3eb96470ed3 100644 --- a/libs/auth/src/angular/login-decryption-options/login-decryption-options.service.ts +++ b/libs/auth/src/angular/login-decryption-options/login-decryption-options.service.ts @@ -3,8 +3,4 @@ export abstract class LoginDecryptionOptionsService { * Handles client-specific logic that runs after a user was successfully created */ abstract handleCreateUserSuccess(): Promise; - /** - * Logs the user out - */ - abstract logOut(): Promise; } diff --git a/libs/auth/src/common/abstractions/index.ts b/libs/auth/src/common/abstractions/index.ts index c0dc500ddb9..937b79e5ba0 100644 --- a/libs/auth/src/common/abstractions/index.ts +++ b/libs/auth/src/common/abstractions/index.ts @@ -6,3 +6,4 @@ export * from "./user-decryption-options.service.abstraction"; export * from "./auth-request.service.abstraction"; export * from "./login-approval-component.service.abstraction"; export * from "./login-success-handler.service"; +export * from "./logout.service"; diff --git a/libs/auth/src/common/abstractions/logout.service.ts b/libs/auth/src/common/abstractions/logout.service.ts new file mode 100644 index 00000000000..16ea3746df2 --- /dev/null +++ b/libs/auth/src/common/abstractions/logout.service.ts @@ -0,0 +1,19 @@ +import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; +import { UserId } from "@bitwarden/common/types/guid"; + +import { LogoutReason } from "../types"; + +export interface NewActiveUser { + userId: UserId; + authenticationStatus: AuthenticationStatus; +} + +export abstract class LogoutService { + /** + * Logs out the user. + * @param userId The user id. + * @param logoutReason The optional reason for logging out. + * @returns The new active user or undefined if there isn't a new active account. + */ + abstract logout(userId: UserId, logoutReason?: LogoutReason): Promise; +} diff --git a/libs/auth/src/common/services/index.ts b/libs/auth/src/common/services/index.ts index 73d31799b7e..f66a827a2c2 100644 --- a/libs/auth/src/common/services/index.ts +++ b/libs/auth/src/common/services/index.ts @@ -7,3 +7,4 @@ export * from "./auth-request/auth-request-api.service"; export * from "./accounts/lock.service"; export * from "./login-success-handler/default-login-success-handler.service"; export * from "./sso-redirect/sso-url.service"; +export * from "./logout/default-logout.service"; diff --git a/libs/auth/src/common/services/logout/default-logout.service.spec.ts b/libs/auth/src/common/services/logout/default-logout.service.spec.ts new file mode 100644 index 00000000000..3febd841695 --- /dev/null +++ b/libs/auth/src/common/services/logout/default-logout.service.spec.ts @@ -0,0 +1,40 @@ +import { MockProxy, mock } from "jest-mock-extended"; + +import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; +import { UserId } from "@bitwarden/common/types/guid"; + +import { LogoutService } from "../../abstractions"; +import { LogoutReason } from "../../types"; + +import { DefaultLogoutService } from "./default-logout.service"; + +describe("DefaultLogoutService", () => { + let logoutService: LogoutService; + let messagingService: MockProxy; + + beforeEach(() => { + messagingService = mock(); + logoutService = new DefaultLogoutService(messagingService); + }); + + it("instantiates", () => { + expect(logoutService).not.toBeFalsy(); + }); + + describe("logout", () => { + it("sends logout message without a logout reason when not provided", async () => { + const userId = "1" as UserId; + + await logoutService.logout(userId); + + expect(messagingService.send).toHaveBeenCalledWith("logout", { userId }); + }); + + it("sends logout message with a logout reason when provided", async () => { + const userId = "1" as UserId; + const logoutReason: LogoutReason = "vaultTimeout"; + await logoutService.logout(userId, logoutReason); + expect(messagingService.send).toHaveBeenCalledWith("logout", { userId, logoutReason }); + }); + }); +}); diff --git a/libs/auth/src/common/services/logout/default-logout.service.ts b/libs/auth/src/common/services/logout/default-logout.service.ts new file mode 100644 index 00000000000..4e96c7cfba1 --- /dev/null +++ b/libs/auth/src/common/services/logout/default-logout.service.ts @@ -0,0 +1,14 @@ +import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; +import { UserId } from "@bitwarden/common/types/guid"; + +import { LogoutService, NewActiveUser } from "../../abstractions/logout.service"; +import { LogoutReason } from "../../types"; + +export class DefaultLogoutService implements LogoutService { + constructor(protected messagingService: MessagingService) {} + async logout(userId: UserId, logoutReason?: LogoutReason): Promise { + this.messagingService.send("logout", { userId, logoutReason }); + + return undefined; + } +} diff --git a/libs/common/src/key-management/device-trust/services/device-trust.service.implementation.ts b/libs/common/src/key-management/device-trust/services/device-trust.service.implementation.ts index f3fb9547366..b02c8922ccb 100644 --- a/libs/common/src/key-management/device-trust/services/device-trust.service.implementation.ts +++ b/libs/common/src/key-management/device-trust/services/device-trust.service.implementation.ts @@ -89,9 +89,7 @@ export class DeviceTrustService implements DeviceTrustServiceAbstraction { ) { this.supportsDeviceTrust$ = this.userDecryptionOptionsService.userDecryptionOptions$.pipe( map((options) => { - // TODO: Eslint upgrade. Please resolve this since the ?? does nothing - // eslint-disable-next-line no-constant-binary-expression - return options?.trustedDeviceOption != null ?? false; + return options?.trustedDeviceOption != null; }), ); } @@ -99,9 +97,7 @@ export class DeviceTrustService implements DeviceTrustServiceAbstraction { supportsDeviceTrustByUserId$(userId: UserId): Observable { return this.userDecryptionOptionsService.userDecryptionOptionsById$(userId).pipe( map((options) => { - // TODO: Eslint upgrade. Please resolve this since the ?? does nothing - // eslint-disable-next-line no-constant-binary-expression - return options?.trustedDeviceOption != null ?? false; + return options?.trustedDeviceOption != null; }), ); } 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 043e2333a9e..89796148e23 100644 --- a/libs/key-management-ui/src/lock/components/lock.component.ts +++ b/libs/key-management-ui/src/lock/components/lock.component.ts @@ -17,7 +17,7 @@ import { import { JslibModule } from "@bitwarden/angular/jslib.module"; import { AnonLayoutWrapperDataService } from "@bitwarden/auth/angular"; -import { PinServiceAbstraction } from "@bitwarden/auth/common"; +import { LogoutService, PinServiceAbstraction } from "@bitwarden/auth/common"; import { InternalPolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { MasterPasswordPolicyOptions } from "@bitwarden/common/admin-console/models/domain/master-password-policy-options"; import { Account, AccountService } from "@bitwarden/common/auth/abstractions/account.service"; @@ -156,6 +156,7 @@ export class LockComponent implements OnInit, OnDestroy { private userAsymmetricKeysRegenerationService: UserAsymmetricKeysRegenerationService, private biometricService: BiometricsService, + private logoutService: LogoutService, private lockComponentService: LockComponentService, private anonLayoutWrapperDataService: AnonLayoutWrapperDataService, @@ -353,7 +354,9 @@ export class LockComponent implements OnInit, OnDestroy { }); if (confirmed && this.activeAccount != null) { - this.messagingService.send("logout", { userId: this.activeAccount.id }); + await this.logoutService.logout(this.activeAccount.id); + // navigate to root so redirect guard can properly route next active user or null user to correct page + await this.router.navigate(["/"]); } }