From 94b57687f5debfa161374276fe6a28eb59c2521d Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Tue, 14 May 2024 16:27:43 -0400 Subject: [PATCH] [PM-7926] Handle complex user logout events (#9115) * Update activity when switching users * Clear data of designated user * Do not switchMap to null, always to Promise or Observable * handle uninitialized popup services * Switch to new account immediately and log out as inactive. Split up done logging out and navigation so we can always display expire warning. * Do not navigate in account switcher, main.background takes care of it * Ignore storage updates from reseed events * Remove loading on cancelled logout * Catch missed account switch errors * Avoid usage of active user state in sync service Send service does not currently support specified user data manipulation, so we ensure that the notification was sent to the active user prior to processing the notification. * Clear sequentialize caches on account switch These caches are used to ensure that rapid calls to an async method are not repeated. However, the cached promises are valid only within a given userId context and must be cleared when that context changes. * Revert `void` promise for notification reconnect * Update libs/angular/src/services/jslib-services.module.ts Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com> * Handle switch account routing through messaging background -> app * Use account switch status to handle unlocked navigation case. * Revert "Handle switch account routing through messaging background -> app" This reverts commit 8f35078ecba4aa64abe8af1a982ec8cd6bac4b52. --------- Co-authored-by: Cesar Gonzalez Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com> --- .../account-switcher.component.ts | 16 ++--- .../account-switching/account.component.ts | 20 ++++--- .../services/account-switcher.service.spec.ts | 31 ++++++++++ .../services/account-switcher.service.ts | 59 ++++++++++++++----- .../browser/src/background/main.background.ts | 51 ++++++++-------- .../abstract-chrome-storage-api.service.ts | 7 ++- apps/browser/src/popup/app.component.ts | 9 +-- apps/cli/src/bw.ts | 1 + apps/desktop/src/app/app.component.ts | 3 + .../src/services/jslib-services.module.ts | 1 + .../src/auth/services/account.service.ts | 6 ++ .../src/platform/misc/sequentialize.spec.ts | 14 ++++- .../common/src/platform/misc/sequentialize.ts | 27 +++++---- .../src/vault/services/cipher.service.ts | 39 +++++++----- .../src/vault/services/sync/sync.service.ts | 24 ++++++-- 15 files changed, 214 insertions(+), 94 deletions(-) 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 e56a2d5c38..fbb9075156 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 @@ -1,7 +1,7 @@ import { Location } from "@angular/common"; import { Component, OnDestroy, OnInit } from "@angular/core"; import { Router } from "@angular/router"; -import { Subject, firstValueFrom, map, switchMap, takeUntil } from "rxjs"; +import { Subject, firstValueFrom, map, of, switchMap, takeUntil } from "rxjs"; import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout-settings.service"; import { VaultTimeoutService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout.service"; @@ -49,7 +49,7 @@ export class AccountSwitcherComponent implements OnInit, OnDestroy { readonly currentAccount$ = this.accountService.activeAccount$.pipe( switchMap((a) => a == null - ? null + ? of(null) : this.authService.activeAccountStatus$.pipe(map((s) => ({ ...a, status: s }))), ), ); @@ -106,12 +106,14 @@ export class AccountSwitcherComponent implements OnInit, OnDestroy { }); if (confirmed) { - this.messagingService.send("logout", { userId }); + 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(); + } } - - // 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(["home"]); + this.loading = false; } ngOnDestroy() { 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 3fe83f2414..3f152d61b9 100644 --- a/apps/browser/src/auth/popup/account-switching/account.component.ts +++ b/apps/browser/src/auth/popup/account-switching/account.component.ts @@ -1,10 +1,10 @@ import { CommonModule, Location } 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"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { AvatarModule } from "@bitwarden/components"; import { AccountSwitcherService, AvailableAccount } from "./services/account-switcher.service"; @@ -21,9 +21,9 @@ export class AccountComponent { constructor( private accountSwitcherService: AccountSwitcherService, - private router: Router, private location: Location, private i18nService: I18nService, + private logService: LogService, ) {} get specialAccountAddId() { @@ -32,15 +32,19 @@ export class AccountComponent { async selectAccount(id: string) { this.loading.emit(true); - await this.accountSwitcherService.selectAccount(id); + let result; + try { + result = await this.accountSwitcherService.selectAccount(id); + } catch (e) { + this.logService.error("Error selecting account", e); + } - if (id === this.specialAccountAddId) { - // 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(["home"]); - } else { + // 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(); } + this.loading.emit(false); } get status() { 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 d27410a5d0..be12d24944 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 @@ -186,4 +186,35 @@ 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 e5a3b8f8f5..2650c2db4e 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 @@ -41,7 +41,7 @@ export class AccountSwitcherService { SPECIAL_ADD_ACCOUNT_ID = "addAccount"; availableAccounts$: Observable; - switchAccountFinished$: Observable; + switchAccountFinished$: Observable<{ userId: UserId; status: AuthenticationStatus }>; constructor( private accountService: AccountService, @@ -111,11 +111,11 @@ export class AccountSwitcherService { ); // Create a reusable observable that listens to the switchAccountFinish message and returns the userId from the message - this.switchAccountFinished$ = fromChromeEvent<[message: { command: string; userId: string }]>( - chrome.runtime.onMessage, - ).pipe( + this.switchAccountFinished$ = fromChromeEvent< + [message: { command: string; userId: UserId; status: AuthenticationStatus }] + >(chrome.runtime.onMessage).pipe( filter(([message]) => message.command === "switchAccountFinish"), - map(([message]) => message.userId), + map(([message]) => ({ userId: message.userId, status: message.status })), ); } @@ -127,12 +127,46 @@ export class AccountSwitcherService { if (id === this.SPECIAL_ADD_ACCOUNT_ID) { id = null; } + const userId = id as UserId; // Creates a subscription to the switchAccountFinished observable but further // filters it to only care about the current userId. - const switchAccountFinishedPromise = firstValueFrom( + const switchAccountFinishedPromise = this.listenForSwitchAccountFinish(userId); + + // Initiate the actions required to make account switching happen + await this.accountService.switchAccount(userId); + this.messagingService.send("switchAccount", { userId }); // This message should cause switchAccountFinish to be sent + + // Wait until we receive the switchAccountFinished message + 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> { + return firstValueFrom( this.switchAccountFinished$.pipe( - filter((userId) => userId === id), + filter(({ userId }) => (expectedUserId ? userId === expectedUserId : true)), timeout({ // Much longer than account switching is expected to take for normal accounts // but the account switching process includes a possible full sync so we need to account @@ -143,20 +177,13 @@ export class AccountSwitcherService { throwError(() => new Error(AccountSwitcherService.incompleteAccountSwitchError)), }), ), - ); - - // Initiate the actions required to make account switching happen - await this.accountService.switchAccount(id as UserId); - this.messagingService.send("switchAccount", { userId: id }); // This message should cause switchAccountFinish to be sent - - // Wait until we recieve the switchAccountFinished message - await switchAccountFinishedPromise.catch((err) => { + ).catch((err) => { if ( err instanceof Error && err.message === AccountSwitcherService.incompleteAccountSwitchError ) { this.logService.warning("message 'switchAccount' never responded."); - return; + return null; } throw err; }); diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 970b12de50..c3722f2a48 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -100,6 +100,7 @@ import { Message, MessageListener, MessageSender } from "@bitwarden/common/platf // eslint-disable-next-line no-restricted-imports -- Used for dependency creation import { SubjectMessageSender } from "@bitwarden/common/platform/messaging/internal"; import { Lazy } from "@bitwarden/common/platform/misc/lazy"; +import { clearCaches } from "@bitwarden/common/platform/misc/sequentialize"; import { GlobalState } from "@bitwarden/common/platform/models/domain/global-state"; import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; import { AppIdService } from "@bitwarden/common/platform/services/app-id.service"; @@ -815,6 +816,7 @@ export default class MainBackground { logoutCallback, this.billingAccountProfileStateService, this.tokenService, + this.authService, ); this.eventUploadService = new EventUploadService( this.apiService, @@ -1180,6 +1182,7 @@ export default class MainBackground { * Switch accounts to indicated userId -- null is no active user */ async switchAccount(userId: UserId) { + let nextAccountStatus: AuthenticationStatus; try { const currentlyActiveAccount = await firstValueFrom( this.accountService.activeAccount$.pipe(map((account) => account?.id)), @@ -1187,6 +1190,8 @@ export default class MainBackground { // can be removed once password generation history is migrated to state providers await this.stateService.clearDecryptedData(currentlyActiveAccount); await this.accountService.switchAccount(userId); + // Clear sequentialized caches + clearCaches(); if (userId == null) { this.loginEmailService.setRememberEmail(false); @@ -1194,11 +1199,12 @@ export default class MainBackground { await this.refreshBadge(); await this.refreshMenu(); - await this.overlayBackground.updateOverlayCiphers(); + await this.overlayBackground?.updateOverlayCiphers(); // null in popup only contexts + this.messagingService.send("goHome"); return; } - const status = await this.authService.getAuthStatus(userId); + nextAccountStatus = await this.authService.getAuthStatus(userId); const forcePasswordReset = (await firstValueFrom(this.masterPasswordService.forceSetPasswordReason$(userId))) != ForceSetPasswordReason.None; @@ -1206,7 +1212,9 @@ export default class MainBackground { await this.systemService.clearPendingClipboard(); await this.notificationsService.updateConnection(false); - if (status === AuthenticationStatus.Locked) { + if (nextAccountStatus === AuthenticationStatus.LoggedOut) { + this.messagingService.send("goHome"); + } else if (nextAccountStatus === AuthenticationStatus.Locked) { this.messagingService.send("locked", { userId: userId }); } else if (forcePasswordReset) { this.messagingService.send("update-temp-password", { userId: userId }); @@ -1214,11 +1222,14 @@ export default class MainBackground { this.messagingService.send("unlocked", { userId: userId }); await this.refreshBadge(); await this.refreshMenu(); - await this.overlayBackground.updateOverlayCiphers(); + await this.overlayBackground?.updateOverlayCiphers(); // null in popup only contexts await this.syncService.fullSync(false); } } finally { - this.messagingService.send("switchAccountFinish", { userId: userId }); + this.messagingService.send("switchAccountFinish", { + userId: userId, + status: nextAccountStatus, + }); } } @@ -1239,6 +1250,13 @@ export default class MainBackground { await this.eventUploadService.uploadEvents(userBeingLoggedOut); + const newActiveUser = + userBeingLoggedOut === activeUserId + ? await firstValueFrom(this.accountService.nextUpAccount$.pipe(map((a) => a?.id))) + : null; + + await this.switchAccount(newActiveUser); + // 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( @@ -1273,11 +1291,6 @@ export default class MainBackground { //Needs to be checked before state is cleaned const needStorageReseed = await this.needsStorageReseed(userId); - const newActiveUser = - userBeingLoggedOut === activeUserId - ? await firstValueFrom(this.accountService.nextUpAccount$.pipe(map((a) => a?.id))) - : null; - await this.stateService.clean({ userId: userBeingLoggedOut }); await this.accountService.clean(userBeingLoggedOut); @@ -1286,16 +1299,10 @@ export default class MainBackground { // HACK: Wait for the user logging outs authentication status to transition to LoggedOut await logoutPromise; - await this.switchAccount(newActiveUser); - if (newActiveUser != null) { - // we have a new active user, do not continue tearing down application - this.messagingService.send("switchAccountFinish"); - } else { - this.messagingService.send("doneLoggingOut", { - expired: expired, - userId: userBeingLoggedOut, - }); - } + this.messagingService.send("doneLoggingOut", { + expired: expired, + userId: userBeingLoggedOut, + }); if (needStorageReseed) { await this.reseedStorage(); @@ -1308,9 +1315,7 @@ export default class MainBackground { } await this.refreshBadge(); await this.mainContextMenuHandler?.noAccess(); - // 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.notificationsService.updateConnection(false); + await this.notificationsService.updateConnection(false); await this.systemService.clearPendingClipboard(); await this.systemService.startProcessReload(this.authService); } diff --git a/apps/browser/src/platform/services/abstractions/abstract-chrome-storage-api.service.ts b/apps/browser/src/platform/services/abstractions/abstract-chrome-storage-api.service.ts index 259d6f154a..2bdc2fd0d4 100644 --- a/apps/browser/src/platform/services/abstractions/abstract-chrome-storage-api.service.ts +++ b/apps/browser/src/platform/services/abstractions/abstract-chrome-storage-api.service.ts @@ -1,4 +1,4 @@ -import { mergeMap } from "rxjs"; +import { filter, mergeMap } from "rxjs"; import { AbstractStorageService, @@ -34,6 +34,11 @@ export default abstract class AbstractChromeStorageService constructor(protected chromeStorageApi: chrome.storage.StorageArea) { this.updates$ = fromChromeEvent(this.chromeStorageApi.onChanged).pipe( + filter(([changes]) => { + // Our storage services support changing only one key at a time. If more are changed, it's due to + // reseeding storage and we should ignore the changes. + return Object.keys(changes).length === 1; + }), mergeMap(([changes]) => { return Object.entries(changes).map(([key, change]) => { // The `newValue` property isn't on the StorageChange object diff --git a/apps/browser/src/popup/app.component.ts b/apps/browser/src/popup/app.component.ts index 25fac44450..7e94e84ef5 100644 --- a/apps/browser/src/popup/app.component.ts +++ b/apps/browser/src/popup/app.component.ts @@ -91,13 +91,9 @@ export class AppComponent implements OnInit, OnDestroy { message: this.i18nService.t("loginExpired"), }); } - - // 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(["home"]); }); this.changeDetectorRef.detectChanges(); - } else if (msg.command === "authBlocked") { + } else if (msg.command === "authBlocked" || msg.command === "goHome") { // 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(["home"]); @@ -137,9 +133,6 @@ 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 this.router.navigate(["/remove-password"]); - } else if (msg.command === "switchAccountFinish") { - // TODO: unset loading? - // this.loading = false; } else if (msg.command == "update-temp-password") { // 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/cli/src/bw.ts b/apps/cli/src/bw.ts index cab38965f8..31de198194 100644 --- a/apps/cli/src/bw.ts +++ b/apps/cli/src/bw.ts @@ -664,6 +664,7 @@ export class Main { async (expired: boolean) => await this.logout(), this.billingAccountProfileStateService, this.tokenService, + this.authService, ); this.totpService = new TotpService(this.cryptoFunctionService, this.logService); diff --git a/apps/desktop/src/app/app.component.ts b/apps/desktop/src/app/app.component.ts index 806fa6de1b..e77ef8d3f0 100644 --- a/apps/desktop/src/app/app.component.ts +++ b/apps/desktop/src/app/app.component.ts @@ -38,6 +38,7 @@ import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/pl import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { SystemService } from "@bitwarden/common/platform/abstractions/system.service"; import { BiometricStateService } from "@bitwarden/common/platform/biometrics/biometric-state.service"; +import { clearCaches } from "@bitwarden/common/platform/misc/sequentialize"; import { StateEventRunnerService } from "@bitwarden/common/platform/state"; import { PasswordGenerationServiceAbstraction } from "@bitwarden/common/tools/generator/password"; import { UserId } from "@bitwarden/common/types/guid"; @@ -396,6 +397,8 @@ export class AppComponent implements OnInit, OnDestroy { this.router.navigate(["/remove-password"]); break; case "switchAccount": { + // Clear sequentialized caches + clearCaches(); if (message.userId != null) { await this.stateService.clearDecryptedData(message.userId); await this.accountService.switchAccount(message.userId); diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index ee14d97e68..c812f5fbec 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -631,6 +631,7 @@ const safeProviders: SafeProvider[] = [ LOGOUT_CALLBACK, BillingAccountProfileStateService, TokenServiceAbstraction, + AuthServiceAbstraction, ], }), safeProvider({ diff --git a/libs/common/src/auth/services/account.service.ts b/libs/common/src/auth/services/account.service.ts index 6740387ded..8af06b89e7 100644 --- a/libs/common/src/auth/services/account.service.ts +++ b/libs/common/src/auth/services/account.service.ts @@ -119,6 +119,7 @@ export class AccountServiceImplementation implements InternalAccountService { } async switchAccount(userId: UserId): Promise { + let updateActivity = false; await this.activeAccountIdState.update( (_, accounts) => { if (userId == null) { @@ -129,6 +130,7 @@ export class AccountServiceImplementation implements InternalAccountService { if (accounts?.[userId] == null) { throw new Error("Account does not exist"); } + updateActivity = true; return userId; }, { @@ -139,6 +141,10 @@ export class AccountServiceImplementation implements InternalAccountService { }, }, ); + + if (updateActivity) { + await this.setAccountActivity(userId, new Date()); + } } async setAccountActivity(userId: UserId, lastActivity: Date): Promise { diff --git a/libs/common/src/platform/misc/sequentialize.spec.ts b/libs/common/src/platform/misc/sequentialize.spec.ts index 1dc5f2b2c2..0b79cde387 100644 --- a/libs/common/src/platform/misc/sequentialize.spec.ts +++ b/libs/common/src/platform/misc/sequentialize.spec.ts @@ -1,4 +1,4 @@ -import { sequentialize } from "./sequentialize"; +import { clearCaches, sequentialize } from "./sequentialize"; describe("sequentialize decorator", () => { it("should call the function once", async () => { @@ -100,6 +100,18 @@ describe("sequentialize decorator", () => { allRes.sort(); expect(allRes).toEqual([3, 3, 6, 6, 9, 9]); }); + + describe("clearCaches", () => { + it("should clear all caches", async () => { + const foo = new Foo(); + const promise = Promise.all([foo.bar(1), foo.bar(1)]); + clearCaches(); + await foo.bar(1); + await promise; + // one call for the first two, one for the third after the cache was cleared + expect(foo.calls).toBe(2); + }); + }); }); class Foo { diff --git a/libs/common/src/platform/misc/sequentialize.ts b/libs/common/src/platform/misc/sequentialize.ts index c242114764..852c32db56 100644 --- a/libs/common/src/platform/misc/sequentialize.ts +++ b/libs/common/src/platform/misc/sequentialize.ts @@ -1,3 +1,19 @@ +const caches = new Map>>(); + +const getCache = (obj: any) => { + let cache = caches.get(obj); + if (cache != null) { + return cache; + } + cache = new Map>(); + caches.set(obj, cache); + return cache; +}; + +export function clearCaches() { + caches.clear(); +} + /** * Use as a Decorator on async functions, it will prevent multiple 'active' calls as the same time * @@ -11,17 +27,6 @@ export function sequentialize(cacheKey: (args: any[]) => string) { return (target: any, propertyKey: string | symbol, descriptor: PropertyDescriptor) => { const originalMethod: () => Promise = descriptor.value; - const caches = new Map>>(); - - const getCache = (obj: any) => { - let cache = caches.get(obj); - if (cache != null) { - return cache; - } - cache = new Map>(); - caches.set(obj, cache); - return cache; - }; return { value: function (...args: any[]) { diff --git a/libs/common/src/vault/services/cipher.service.ts b/libs/common/src/vault/services/cipher.service.ts index 174da701bd..537245459e 100644 --- a/libs/common/src/vault/services/cipher.service.ts +++ b/libs/common/src/vault/services/cipher.service.ts @@ -28,7 +28,7 @@ import { DerivedState, StateProvider, } from "../../platform/state"; -import { CipherId, CollectionId, OrganizationId } from "../../types/guid"; +import { CipherId, CollectionId, OrganizationId, UserId } from "../../types/guid"; import { UserKey, OrgKey } from "../../types/key"; import { CipherService as CipherServiceAbstraction } from "../abstractions/cipher.service"; import { CipherFileUploadService } from "../abstractions/file-upload/cipher-file-upload.service"; @@ -136,11 +136,12 @@ export class CipherService implements CipherServiceAbstraction { } async setDecryptedCipherCache(value: CipherView[]) { + const userId = await firstValueFrom(this.stateProvider.activeUserId$); // Sometimes we might prematurely decrypt the vault and that will result in no ciphers - // if we cache it then we may accidentially return it when it's not right, we'd rather try decryption again. + // if we cache it then we may accidentally return it when it's not right, we'd rather try decryption again. // We still want to set null though, that is the indicator that the cache isn't valid and we should do decryption. if (value == null || value.length !== 0) { - await this.setDecryptedCiphers(value); + await this.setDecryptedCiphers(value, userId); } if (this.searchService != null) { if (value == null) { @@ -151,15 +152,16 @@ export class CipherService implements CipherServiceAbstraction { } } - private async setDecryptedCiphers(value: CipherView[]) { + private async setDecryptedCiphers(value: CipherView[], userId: UserId) { const cipherViews: { [id: string]: CipherView } = {}; value?.forEach((c) => { cipherViews[c.id] = c; }); - await this.decryptedCiphersState.update(() => cipherViews); + await this.stateProvider.setUserState(DECRYPTED_CIPHERS, cipherViews, userId); } - async clearCache(userId?: string): Promise { + async clearCache(userId?: UserId): Promise { + userId ??= await firstValueFrom(this.stateProvider.activeUserId$); await this.clearDecryptedCiphersState(userId); } @@ -524,6 +526,7 @@ export class CipherService implements CipherServiceAbstraction { } async updateLastUsedDate(id: string): Promise { + const userId = await firstValueFrom(this.stateProvider.activeUserId$); let ciphersLocalData = await firstValueFrom(this.localData$); if (!ciphersLocalData) { @@ -553,10 +556,11 @@ export class CipherService implements CipherServiceAbstraction { break; } } - await this.setDecryptedCiphers(decryptedCipherCache); + await this.setDecryptedCiphers(decryptedCipherCache, userId); } async updateLastLaunchedDate(id: string): Promise { + const userId = await firstValueFrom(this.stateProvider.activeUserId$); let ciphersLocalData = await firstValueFrom(this.localData$); if (!ciphersLocalData) { @@ -586,7 +590,7 @@ export class CipherService implements CipherServiceAbstraction { break; } } - await this.setDecryptedCiphers(decryptedCipherCache); + await this.setDecryptedCiphers(decryptedCipherCache, userId); } async saveNeverDomain(domain: string): Promise { @@ -833,12 +837,18 @@ export class CipherService implements CipherServiceAbstraction { await this.updateEncryptedCipherState(() => ciphers); } + /** + * Updates ciphers for the currently active user. Inactive users can only clear all ciphers, for now. + * @param update update callback for encrypted cipher data + * @returns + */ private async updateEncryptedCipherState( update: (current: Record) => Record, ): Promise> { + const userId = await firstValueFrom(this.stateProvider.activeUserId$); // Store that we should wait for an update to return any ciphers await this.ciphersExpectingUpdate.forceValue(true); - await this.clearDecryptedCiphersState(); + await this.clearDecryptedCiphersState(userId); const [, updatedCiphers] = await this.encryptedCiphersState.update((current) => { const result = update(current ?? {}); return result; @@ -846,7 +856,8 @@ export class CipherService implements CipherServiceAbstraction { return updatedCiphers; } - async clear(userId?: string): Promise { + async clear(userId?: UserId): Promise { + userId ??= await firstValueFrom(this.stateProvider.activeUserId$); await this.clearEncryptedCiphersState(userId); await this.clearCache(userId); } @@ -1464,12 +1475,12 @@ export class CipherService implements CipherServiceAbstraction { } } - private async clearEncryptedCiphersState(userId?: string) { - await this.encryptedCiphersState.update(() => ({})); + private async clearEncryptedCiphersState(userId: UserId) { + await this.stateProvider.setUserState(ENCRYPTED_CIPHERS, {}, userId); } - private async clearDecryptedCiphersState(userId?: string) { - await this.setDecryptedCiphers(null); + private async clearDecryptedCiphersState(userId: UserId) { + await this.setDecryptedCiphers(null, userId); this.clearSortedCiphers(); } diff --git a/libs/common/src/vault/services/sync/sync.service.ts b/libs/common/src/vault/services/sync/sync.service.ts index 793bcf2437..b591a61e86 100644 --- a/libs/common/src/vault/services/sync/sync.service.ts +++ b/libs/common/src/vault/services/sync/sync.service.ts @@ -1,4 +1,4 @@ -import { firstValueFrom } from "rxjs"; +import { firstValueFrom, map, of, switchMap } from "rxjs"; import { UserDecryptionOptionsServiceAbstraction } from "@bitwarden/auth/common"; @@ -12,10 +12,12 @@ import { PolicyData } from "../../../admin-console/models/data/policy.data"; import { ProviderData } from "../../../admin-console/models/data/provider.data"; import { PolicyResponse } from "../../../admin-console/models/response/policy.response"; import { AccountService } from "../../../auth/abstractions/account.service"; +import { AuthService } from "../../../auth/abstractions/auth.service"; import { AvatarService } from "../../../auth/abstractions/avatar.service"; import { KeyConnectorService } from "../../../auth/abstractions/key-connector.service"; import { InternalMasterPasswordServiceAbstraction } from "../../../auth/abstractions/master-password.service.abstraction"; import { TokenService } from "../../../auth/abstractions/token.service"; +import { AuthenticationStatus } from "../../../auth/enums/authentication-status"; import { ForceSetPasswordReason } from "../../../auth/models/domain/force-set-password-reason"; import { DomainSettingsService } from "../../../autofill/services/domain-settings.service"; import { BillingAccountProfileStateService } from "../../../billing/abstractions/account/billing-account-profile-state.service"; @@ -74,6 +76,7 @@ export class SyncService implements SyncServiceAbstraction { private logoutCallback: (expired: boolean) => Promise, private billingAccountProfileStateService: BillingAccountProfileStateService, private tokenService: TokenService, + private authService: AuthService, ) {} async getLastSync(): Promise { @@ -247,7 +250,19 @@ export class SyncService implements SyncServiceAbstraction { async syncUpsertSend(notification: SyncSendNotification, isEdit: boolean): Promise { this.syncStarted(); - if (await this.stateService.getIsAuthenticated()) { + const [activeUserId, status] = await firstValueFrom( + this.accountService.activeAccount$.pipe( + switchMap((a) => { + if (a == null) { + of([null, AuthenticationStatus.LoggedOut]); + } + return this.authService.authStatusFor$(a.id).pipe(map((s) => [a.id, s])); + }), + ), + ); + // Process only notifications for currently active user when user is not logged out + // TODO: once send service allows data manipulation of non-active users, this should process any received notification + if (activeUserId === notification.userId && status !== AuthenticationStatus.LoggedOut) { try { const localSend = await firstValueFrom(this.sendService.get$(notification.id)); if ( @@ -361,15 +376,14 @@ export class SyncService implements SyncServiceAbstraction { private async setForceSetPasswordReasonIfNeeded(profileResponse: ProfileResponse) { // The `forcePasswordReset` flag indicates an admin has reset the user's password and must be updated if (profileResponse.forcePasswordReset) { - const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id; await this.masterPasswordService.setForceSetPasswordReason( ForceSetPasswordReason.AdminForcePasswordReset, - userId, + profileResponse.id, ); } const userDecryptionOptions = await firstValueFrom( - this.userDecryptionOptionsService.userDecryptionOptions$, + this.userDecryptionOptionsService.userDecryptionOptionsById$(profileResponse.id), ); if (userDecryptionOptions === null || userDecryptionOptions === undefined) {