From 1fdc6629e3314ef33b6d647cf08b3cb47dfe89e4 Mon Sep 17 00:00:00 2001 From: Jared Snider <116684653+JaredSnider-Bitwarden@users.noreply.github.com> Date: Tue, 2 Jan 2024 17:22:17 -0500 Subject: [PATCH] Auth/PM-3859 - Desktop - Create lock mechanism to prevent app menu redraw on sync complete when user logging out (#6920) * PM-3859 - Desktop App Comp - Build lock mechanism for update app menu which doesn't try to update the menu for users who are being logged out which was causing errors (primary scenario was triggered by logging in on desktop with a TDE user w/out a MP, triggering a sync, then hitting the command to lock the vault right after which would trigger a log out while the sync was still in process. Then, while the log out was in process, the sync would try and trigger an update to the app menu but it would error as some of the user's state had already been cleaned up) * PM-3859 - App comp - remove use of promise.all to prevent any race conditions from causing intermittent logout errors with state being cleared and then values trying to be set on the cleared state (I observed setMasterKey get called after state account.keys was cleared - received error when attempting to set value on undefined). * PM-3859 - Desktop Vault Items Component - on log out, if you were on the vault screen, the loss of focus on the vault search text box would trigger a search 200 ms after log out had been triggered. This would eventually attempt to set an undefined master key (VaultItemsComponent.doSearch() --> cipherService.getAllDecrypted() --> cryptoService.getUserKeyWithLegacySupport() --> cryptoService.getMasterKey() --> cryptoService.setMasterKey()). However, at this point, the account had been cleared as part of the log out process and an error would be thrown in the state service for trying to set account.keys.masterKey to undefined when the account and account.keys were undefined. These changes prevent the search from firing until the value changes and also prevents setMasterKey from being called if it is undefined. --- apps/desktop/src/app/app.component.ts | 62 ++++++++++++++----- .../vault/app/vault/vault-items.component.ts | 3 +- .../src/platform/services/crypto.service.ts | 6 +- 3 files changed, 53 insertions(+), 18 deletions(-) diff --git a/apps/desktop/src/app/app.component.ts b/apps/desktop/src/app/app.component.ts index f5d3ec09bb..ad87230d39 100644 --- a/apps/desktop/src/app/app.component.ts +++ b/apps/desktop/src/app/app.component.ts @@ -113,6 +113,8 @@ export class AppComponent implements OnInit, OnDestroy { private destroy$ = new Subject(); + private accountCleanUpInProgress: { [userId: string]: boolean } = {}; + constructor( private broadcasterService: BroadcasterService, private folderService: InternalFolderService, @@ -495,7 +497,11 @@ export class AppComponent implements OnInit, OnDestroy { } else { const accounts: { [userId: string]: MenuAccount } = {}; for (const i in stateAccounts) { - if (i != null && stateAccounts[i]?.profile?.userId != null) { + if ( + i != null && + stateAccounts[i]?.profile?.userId != null && + !this.isAccountCleanUpInProgress(stateAccounts[i].profile.userId) // skip accounts that are being cleaned up + ) { const userId = stateAccounts[i].profile.userId; const availableTimeoutActions = await firstValueFrom( this.vaultTimeoutSettingsService.availableVaultTimeoutActions$(userId), @@ -525,22 +531,31 @@ export class AppComponent implements OnInit, OnDestroy { private async logOut(expired: boolean, userId?: string) { const userBeingLoggedOut = await this.stateService.getUserId({ userId: userId }); - await Promise.all([ - this.eventUploadService.uploadEvents(userBeingLoggedOut), - this.syncService.setLastSync(new Date(0), userBeingLoggedOut), - this.cryptoService.clearKeys(userBeingLoggedOut), - this.settingsService.clear(userBeingLoggedOut), - this.cipherService.clear(userBeingLoggedOut), - this.folderService.clear(userBeingLoggedOut), - this.collectionService.clear(userBeingLoggedOut), - this.passwordGenerationService.clear(userBeingLoggedOut), - this.vaultTimeoutSettingsService.clear(userBeingLoggedOut), - this.policyService.clear(userBeingLoggedOut), - this.keyConnectorService.clear(), - ]); - const preLogoutActiveUserId = this.activeUserId; - await this.stateService.clean({ userId: userBeingLoggedOut }); + // 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; + try { + await this.eventUploadService.uploadEvents(userBeingLoggedOut); + await this.syncService.setLastSync(new Date(0), userBeingLoggedOut); + await this.cryptoService.clearKeys(userBeingLoggedOut); + await this.settingsService.clear(userBeingLoggedOut); + await this.cipherService.clear(userBeingLoggedOut); + await this.folderService.clear(userBeingLoggedOut); + await this.collectionService.clear(userBeingLoggedOut); + await this.passwordGenerationService.clear(userBeingLoggedOut); + await this.vaultTimeoutSettingsService.clear(userBeingLoggedOut); + await this.policyService.clear(userBeingLoggedOut); + await this.keyConnectorService.clear(); + + preLogoutActiveUserId = this.activeUserId; + await this.stateService.clean({ userId: userBeingLoggedOut }); + } finally { + this.finishAccountCleanUp(userBeingLoggedOut); + } if (this.activeUserId == null) { this.router.navigate(["login"]); @@ -675,4 +690,19 @@ export class AppComponent implements OnInit, OnDestroy { const action = await this.stateService.getVaultTimeoutAction({ userId: userId }); return [timeout, action]; } + + // Mark an account's clean up as started + private startAccountCleanUp(userId: string): void { + this.accountCleanUpInProgress[userId] = true; + } + + // Mark an account's clean up as finished + private finishAccountCleanUp(userId: string): void { + this.accountCleanUpInProgress[userId] = false; + } + + // Check if an account's clean up is in progress + private isAccountCleanUpInProgress(userId: string): boolean { + return this.accountCleanUpInProgress[userId] === true; + } } diff --git a/apps/desktop/src/vault/app/vault/vault-items.component.ts b/apps/desktop/src/vault/app/vault/vault-items.component.ts index 10f07d132a..2c5d256e2f 100644 --- a/apps/desktop/src/vault/app/vault/vault-items.component.ts +++ b/apps/desktop/src/vault/app/vault/vault-items.component.ts @@ -1,4 +1,5 @@ import { Component } from "@angular/core"; +import { distinctUntilChanged } from "rxjs"; import { VaultItemsComponent as BaseVaultItemsComponent } from "@bitwarden/angular/vault/components/vault-items.component"; import { SearchService } from "@bitwarden/common/abstractions/search.service"; @@ -21,7 +22,7 @@ export class VaultItemsComponent extends BaseVaultItemsComponent { super(searchService, cipherService); // eslint-disable-next-line rxjs-angular/prefer-takeuntil - searchBarService.searchText$.subscribe((searchText) => { + searchBarService.searchText$.pipe(distinctUntilChanged()).subscribe((searchText) => { this.searchText = searchText; this.search(200); }); diff --git a/libs/common/src/platform/services/crypto.service.ts b/libs/common/src/platform/services/crypto.service.ts index 443e7cae87..f057053c2b 100644 --- a/libs/common/src/platform/services/crypto.service.ts +++ b/libs/common/src/platform/services/crypto.service.ts @@ -161,7 +161,11 @@ export class CryptoService implements CryptoServiceAbstraction { let masterKey = await this.stateService.getMasterKey({ userId: userId }); if (!masterKey) { masterKey = (await this.stateService.getCryptoMasterKey({ userId: userId })) as MasterKey; - await this.setMasterKey(masterKey, userId); + // if master key was null/undefined and getCryptoMasterKey also returned null/undefined, + // don't set master key as it is unnecessary + if (masterKey) { + await this.setMasterKey(masterKey, userId); + } } return masterKey; }