diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 067fae1de8..a2778f6971 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -1,4 +1,4 @@ -import { Subject, firstValueFrom, map, merge, timeout } from "rxjs"; +import { Subject, filter, firstValueFrom, map, merge, timeout } from "rxjs"; import { PinCryptoServiceAbstraction, @@ -1200,31 +1200,46 @@ export default class MainBackground { } async logout(expired: boolean, userId?: UserId) { - userId ??= ( - await firstValueFrom( - this.accountService.activeAccount$.pipe( - timeout({ - first: 2000, - with: () => { - throw new Error("No active account found to logout"); - }, - }), - ), - ) - )?.id; + const activeUserId = await firstValueFrom( + this.accountService.activeAccount$.pipe( + map((a) => a?.id), + timeout({ + first: 2000, + with: () => { + throw new Error("No active account found to logout"); + }, + }), + ), + ); - await this.eventUploadService.uploadEvents(userId as UserId); + const userBeingLoggedOut = userId ?? activeUserId; + + await this.eventUploadService.uploadEvents(userBeingLoggedOut); + + // HACK: We shouldn't wait for the authentication status to change but instead subscribe to the + // authentication status to do various actions. + const logoutPromise = firstValueFrom( + this.authService.authStatusFor$(userBeingLoggedOut).pipe( + filter((authenticationStatus) => authenticationStatus === AuthenticationStatus.LoggedOut), + timeout({ + first: 5_000, + with: () => { + throw new Error("The logout process did not complete in a reasonable amount of time."); + }, + }), + ), + ); await Promise.all([ - this.syncService.setLastSync(new Date(0), userId), - this.cryptoService.clearKeys(userId), - this.cipherService.clear(userId), - this.folderService.clear(userId), - this.collectionService.clear(userId), - this.passwordGenerationService.clear(userId), - this.vaultTimeoutSettingsService.clear(userId), + this.syncService.setLastSync(new Date(0), userBeingLoggedOut), + this.cryptoService.clearKeys(userBeingLoggedOut), + this.cipherService.clear(userBeingLoggedOut), + this.folderService.clear(userBeingLoggedOut), + this.collectionService.clear(userBeingLoggedOut), + this.passwordGenerationService.clear(userBeingLoggedOut), + this.vaultTimeoutSettingsService.clear(userBeingLoggedOut), this.vaultFilterService.clear(), - this.biometricStateService.logout(userId), + this.biometricStateService.logout(userBeingLoggedOut), /* We intentionally do not clear: * - autofillSettingsService * - badgeSettingsService @@ -1235,20 +1250,28 @@ export default class MainBackground { //Needs to be checked before state is cleaned const needStorageReseed = await this.needsStorageReseed(); - const newActiveUser = await firstValueFrom( - this.accountService.nextUpAccount$.pipe(map((a) => a?.id)), - ); - await this.stateService.clean({ userId: userId }); - await this.accountService.clean(userId); + const newActiveUser = + userBeingLoggedOut === activeUserId + ? await firstValueFrom(this.accountService.nextUpAccount$.pipe(map((a) => a?.id))) + : null; - await this.stateEventRunnerService.handleEvent("logout", userId); + await this.stateService.clean({ userId: userBeingLoggedOut }); + await this.accountService.clean(userBeingLoggedOut); + + await this.stateEventRunnerService.handleEvent("logout", userBeingLoggedOut); + + // HACK: Wait for the user logging outs authentication status to transition to LoggedOut + await logoutPromise; if (newActiveUser != null) { // we have a new active user, do not continue tearing down application - await this.switchAccount(newActiveUser as UserId); + await this.switchAccount(newActiveUser); this.messagingService.send("switchAccountFinish"); } else { - this.messagingService.send("doneLoggingOut", { expired: expired, userId: userId }); + this.messagingService.send("doneLoggingOut", { + expired: expired, + userId: userBeingLoggedOut, + }); } if (needStorageReseed) { diff --git a/apps/desktop/src/app/app.component.ts b/apps/desktop/src/app/app.component.ts index 4e540efdc6..97122e6110 100644 --- a/apps/desktop/src/app/app.component.ts +++ b/apps/desktop/src/app/app.component.ts @@ -8,7 +8,7 @@ import { ViewContainerRef, } from "@angular/core"; import { Router } from "@angular/router"; -import { firstValueFrom, map, Subject, takeUntil } from "rxjs"; +import { filter, firstValueFrom, map, Subject, takeUntil, timeout } from "rxjs"; import { ModalRef } from "@bitwarden/angular/components/modal/modal.ref"; import { ModalService } from "@bitwarden/angular/services/modal.service"; @@ -564,19 +564,42 @@ export class AppComponent implements OnInit, OnDestroy { this.messagingService.send("updateAppMenu", { updateRequest: updateRequest }); } - private async logOut(expired: boolean, userId?: string) { - const userBeingLoggedOut = - (userId as UserId) ?? - (await firstValueFrom(this.accountService.activeAccount$.pipe(map((a) => a?.id)))); + // 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(expired: boolean, userId: UserId) { + const activeUserId = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((a) => a?.id)), + ); + + const userBeingLoggedOut = userId ?? activeUserId; // Mark account as being cleaned up so that the updateAppMenu logic (executed on syncCompleted) // doesn't attempt to update a user that is being logged out as we will manually // call updateAppMenu when the logout is complete. this.startAccountCleanUp(userBeingLoggedOut); - let preLogoutActiveUserId; - const nextUpAccount = await firstValueFrom(this.accountService.nextUpAccount$); + const nextUpAccount = + activeUserId === userBeingLoggedOut + ? await firstValueFrom(this.accountService.nextUpAccount$) // We'll need to switch accounts + : null; + try { + // HACK: We shouldn't wait for authentication status to change here but instead subscribe to the + // authentication status to do various actions. + const logoutPromise = firstValueFrom( + this.authService.authStatusFor$(userBeingLoggedOut).pipe( + filter((authenticationStatus) => authenticationStatus === AuthenticationStatus.LoggedOut), + timeout({ + first: 5_000, + with: () => { + throw new Error( + "The logout process did not complete in a reasonable amount of time.", + ); + }, + }), + ), + ); + // Provide the userId of the user to upload events for await this.eventUploadService.uploadEvents(userBeingLoggedOut); await this.syncService.setLastSync(new Date(0), userBeingLoggedOut); @@ -590,26 +613,32 @@ export class AppComponent implements OnInit, OnDestroy { await this.stateEventRunnerService.handleEvent("logout", userBeingLoggedOut); - preLogoutActiveUserId = this.activeUserId; await this.stateService.clean({ userId: userBeingLoggedOut }); await this.accountService.clean(userBeingLoggedOut); + + // HACK: Wait for the user logging outs authentication status to transition to LoggedOut + await logoutPromise; } finally { this.finishAccountCleanUp(userBeingLoggedOut); } - if (nextUpAccount == 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.router.navigate(["login"]); - } else if (preLogoutActiveUserId !== nextUpAccount.id) { - this.messagingService.send("switchAccount", { userId: nextUpAccount.id }); + // We only need to change the display at all if the account being looked at is the one + // being logged out. If it was a background account, no need to do anything. + if (userBeingLoggedOut === activeUserId) { + if (nextUpAccount != null) { + this.messagingService.send("switchAccount", { userId: nextUpAccount.id }); + } else { + // We don't have another user to switch to, bring them to the login page so they + // can sign into a user. + void this.router.navigate(["login"]); + } } await this.updateAppMenu(); // This must come last otherwise the logout will prematurely trigger // a process reload before all the state service user data can be cleaned up - if (userBeingLoggedOut === preLogoutActiveUserId) { + if (userBeingLoggedOut === activeUserId) { this.authService.logOut(async () => { if (expired) { this.platformUtilsService.showToast( @@ -700,7 +729,7 @@ export class AppComponent implements OnInit, OnDestroy { // 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 options[1] === "logOut" - ? this.logOut(false, userId) + ? this.logOut(false, userId as UserId) : await this.vaultTimeoutService.lock(userId); } } diff --git a/apps/web/src/app/app.component.ts b/apps/web/src/app/app.component.ts index 1939bb11f5..0810564f31 100644 --- a/apps/web/src/app/app.component.ts +++ b/apps/web/src/app/app.component.ts @@ -2,7 +2,7 @@ import { DOCUMENT } from "@angular/common"; import { Component, Inject, NgZone, OnDestroy, OnInit } from "@angular/core"; import { NavigationEnd, Router } from "@angular/router"; import * as jq from "jquery"; -import { Subject, firstValueFrom, map, switchMap, takeUntil, timer } from "rxjs"; +import { Subject, filter, firstValueFrom, map, switchMap, takeUntil, timeout, timer } from "rxjs"; import { EventUploadService } from "@bitwarden/common/abstractions/event/event-upload.service"; import { NotificationsService } from "@bitwarden/common/abstractions/notifications.service"; @@ -13,6 +13,7 @@ import { InternalPolicyService } from "@bitwarden/common/admin-console/abstracti import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service"; +import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { PaymentMethodWarningsServiceAbstraction as PaymentMethodWarningService } from "@bitwarden/common/billing/abstractions/payment-method-warnings-service.abstraction"; import { BroadcasterService } from "@bitwarden/common/platform/abstractions/broadcaster.service"; import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; @@ -136,9 +137,7 @@ export class AppComponent implements OnDestroy, OnInit { this.router.navigate(["/"]); break; case "logout": - // 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.logOut(!!message.expired, message.redirect); + await this.logOut(!!message.expired, message.redirect); break; case "lockVault": await this.vaultTimeoutService.lock(); @@ -266,7 +265,20 @@ export class AppComponent implements OnDestroy, OnInit { private async logOut(expired: boolean, redirect = true) { await this.eventUploadService.uploadEvents(); - const userId = await this.stateService.getUserId(); + const userId = (await this.stateService.getUserId()) as UserId; + + const logoutPromise = firstValueFrom( + this.authService.authStatusFor$(userId).pipe( + filter((authenticationStatus) => authenticationStatus === AuthenticationStatus.LoggedOut), + timeout({ + first: 5_000, + with: () => { + throw new Error("The logout process did not complete in a reasonable amount of time."); + }, + }), + ), + ); + await Promise.all([ this.syncService.setLastSync(new Date(0)), this.cryptoService.clearKeys(), @@ -274,11 +286,11 @@ export class AppComponent implements OnDestroy, OnInit { this.folderService.clear(userId), this.collectionService.clear(userId), this.passwordGenerationService.clear(), - this.biometricStateService.logout(userId as UserId), + this.biometricStateService.logout(userId), this.paymentMethodWarningService.clear(), ]); - await this.stateEventRunnerService.handleEvent("logout", userId as UserId); + await this.stateEventRunnerService.handleEvent("logout", userId); await this.searchService.clearIndex(); this.authService.logOut(async () => { @@ -291,6 +303,9 @@ export class AppComponent implements OnDestroy, OnInit { } await this.stateService.clean({ userId: userId }); + + await logoutPromise; + if (redirect) { // 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/libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts b/libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts index 42ffb5192b..14b26fa541 100644 --- a/libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts +++ b/libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts @@ -1,5 +1,5 @@ import { MockProxy, any, mock } from "jest-mock-extended"; -import { BehaviorSubject, of } from "rxjs"; +import { BehaviorSubject, from, of } from "rxjs"; import { FakeAccountService, mockAccountServiceWith } from "../../../spec/fake-account-service"; import { SearchService } from "../../abstractions/search.service"; @@ -106,6 +106,13 @@ describe("VaultTimeoutService", () => { // Both are available by default and the specific test can change this per test availableVaultTimeoutActionsSubject.next([VaultTimeoutAction.Lock, VaultTimeoutAction.LogOut]); + authService.authStatusFor$.mockImplementation((userId) => { + return from([ + accounts[userId]?.authStatus ?? AuthenticationStatus.LoggedOut, + AuthenticationStatus.Locked, + ]); + }); + authService.getAuthStatus.mockImplementation((userId) => { return Promise.resolve(accounts[userId]?.authStatus); }); @@ -387,18 +394,6 @@ describe("VaultTimeoutService", () => { expect(stateEventRunnerService.handleEvent).toHaveBeenCalledWith("lock", "user1"); }); - it("should call messaging service locked message if no user passed into lock", 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(messagingService.send).toHaveBeenCalledWith("locked", { userId: undefined }); - }); - it("should call locked callback if no user passed into lock", async () => { setupLock(); @@ -414,25 +409,31 @@ describe("VaultTimeoutService", () => { it("should call state event runner with user passed into lock", async () => { setupLock(); - await vaultTimeoutService.lock("user2"); + const user2 = "user2" as UserId; - expect(stateEventRunnerService.handleEvent).toHaveBeenCalledWith("lock", "user2"); + await vaultTimeoutService.lock(user2); + + expect(stateEventRunnerService.handleEvent).toHaveBeenCalledWith("lock", user2); }); it("should call messaging service locked message with user passed into lock", async () => { setupLock(); - await vaultTimeoutService.lock("user2"); + const user2 = "user2" as UserId; - expect(messagingService.send).toHaveBeenCalledWith("locked", { userId: "user2" }); + await vaultTimeoutService.lock(user2); + + expect(messagingService.send).toHaveBeenCalledWith("locked", { userId: user2 }); }); it("should call locked callback with user passed into lock", async () => { setupLock(); - await vaultTimeoutService.lock("user2"); + const user2 = "user2" as UserId; - expect(lockedCallback).toHaveBeenCalledWith("user2"); + await vaultTimeoutService.lock(user2); + + expect(lockedCallback).toHaveBeenCalledWith(user2); }); }); }); diff --git a/libs/common/src/services/vault-timeout/vault-timeout.service.ts b/libs/common/src/services/vault-timeout/vault-timeout.service.ts index 2f3a259562..a75fb6d4c4 100644 --- a/libs/common/src/services/vault-timeout/vault-timeout.service.ts +++ b/libs/common/src/services/vault-timeout/vault-timeout.service.ts @@ -1,4 +1,4 @@ -import { combineLatest, firstValueFrom, switchMap } from "rxjs"; +import { combineLatest, filter, firstValueFrom, map, switchMap, timeout } from "rxjs"; import { SearchService } from "../../abstractions/search.service"; import { VaultTimeoutSettingsService } from "../../abstractions/vault-timeout/vault-timeout-settings.service"; @@ -80,7 +80,7 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { ); } - async lock(userId?: string): Promise { + async lock(userId?: UserId): Promise { const authed = await this.stateService.getIsAuthenticated({ userId: userId }); if (!authed) { return; @@ -94,7 +94,27 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { await this.logOut(userId); } - const currentUserId = (await firstValueFrom(this.accountService.activeAccount$)).id; + const currentUserId = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((a) => a?.id)), + ); + + const lockingUserId = userId ?? currentUserId; + + // HACK: Start listening for the transition of the locking user from something to the locked state. + // This is very much a hack to ensure that the authentication status to retrievable right after + // it does its work. Particularly the `lockedCallback` and `"locked"` message. Instead + // lockedCallback should be deprecated and people should subscribe and react to `authStatusFor$` themselves. + const lockPromise = firstValueFrom( + this.authService.authStatusFor$(lockingUserId).pipe( + filter((authStatus) => authStatus === AuthenticationStatus.Locked), + timeout({ + first: 5_000, + with: () => { + throw new Error("The lock process did not complete in a reasonable amount of time."); + }, + }), + ), + ); if (userId == null || userId === currentUserId) { await this.searchService.clearIndex(); @@ -102,19 +122,21 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { await this.collectionService.clearActiveUserCache(); } - await this.masterPasswordService.clearMasterKey((userId ?? currentUserId) as UserId); + await this.masterPasswordService.clearMasterKey(lockingUserId); - await this.stateService.setUserKeyAutoUnlock(null, { userId: userId }); - await this.stateService.setCryptoMasterKeyAuto(null, { userId: userId }); + await this.stateService.setUserKeyAutoUnlock(null, { userId: lockingUserId }); + await this.stateService.setCryptoMasterKeyAuto(null, { userId: lockingUserId }); - await this.cipherService.clearCache(userId); + await this.cipherService.clearCache(lockingUserId); - await this.stateEventRunnerService.handleEvent("lock", (userId ?? currentUserId) as UserId); + await this.stateEventRunnerService.handleEvent("lock", lockingUserId); - // FIXME: We should send the userId of the user that was locked, in the case of this method being passed - // undefined then it should give back the currentUserId. Better yet, this method shouldn't take - // an undefined userId at all. All receivers need to be checked for how they handle getting undefined. - this.messagingService.send("locked", { userId: userId }); + // HACK: Sit here and wait for the the auth status to transition to `Locked` + // to ensure the message and lockedCallback will get the correct status + // if/when they call it. + await lockPromise; + + this.messagingService.send("locked", { userId: lockingUserId }); if (this.lockedCallback != null) { await this.lockedCallback(userId); @@ -162,7 +184,7 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { return diffSeconds >= vaultTimeoutSeconds; } - private async executeTimeoutAction(userId: string): Promise { + private async executeTimeoutAction(userId: UserId): Promise { const timeoutAction = await firstValueFrom( this.vaultTimeoutSettingsService.vaultTimeoutAction$(userId), );