From 9d163550fd84706a8627d03a50331c46aca448e9 Mon Sep 17 00:00:00 2001 From: Shane Melton Date: Thu, 10 Oct 2024 14:54:23 -0700 Subject: [PATCH] [PM-6991] Improve reactivity of cipherViews$ observable in cipher service (#11141) * [PM-6991] Remove self reference to cipherViews$ * [PM-6991] Update cipherViews$ observable to be reactive to encrypted ciphers and localData * [PM-6991] Use the cipherViews$ observable in the Web vault * [PM-6991] Update the vault filter service to use cipherViews$ observable * [PM-6991] Add deprecation notice to getAllDecrypted * [PM-6991] Ensure cipherViews$ emits an empty array whenever the decrypted cipher cache is cleared * [PM-6991] Fix cipher service test * [PM-6991] Use shareReplay instead of share * [PM-6991] Remove ciphersExpectingUpdate and replace with a null filter instead for cipherViews$ * [PM-6991] Skip refreshing on null cipherViews$ to avoid flashing an empty vault after modifying ciphers --- .../services/vault-filter.service.spec.ts | 6 +- .../services/vault-filter.service.ts | 9 +- .../vault/individual-vault/vault.component.ts | 2 +- .../src/vault/abstractions/cipher.service.ts | 2 +- .../src/vault/services/cipher.service.spec.ts | 4 +- .../src/vault/services/cipher.service.ts | 83 ++++++++++++------- 6 files changed, 65 insertions(+), 41 deletions(-) diff --git a/apps/web/src/app/vault/individual-vault/vault-filter/services/vault-filter.service.spec.ts b/apps/web/src/app/vault/individual-vault/vault-filter/services/vault-filter.service.spec.ts index c6a97a4310..0386a20adb 100644 --- a/apps/web/src/app/vault/individual-vault/vault-filter/services/vault-filter.service.spec.ts +++ b/apps/web/src/app/vault/individual-vault/vault-filter/services/vault-filter.service.spec.ts @@ -35,6 +35,7 @@ describe("vault filter service", () => { let organizations: ReplaySubject; let folderViews: ReplaySubject; let collectionViews: ReplaySubject; + let cipherViews: ReplaySubject; let personalOwnershipPolicy: ReplaySubject; let singleOrgPolicy: ReplaySubject; let stateProvider: FakeStateProvider; @@ -57,6 +58,7 @@ describe("vault filter service", () => { organizations = new ReplaySubject(1); folderViews = new ReplaySubject(1); collectionViews = new ReplaySubject(1); + cipherViews = new ReplaySubject(1); personalOwnershipPolicy = new ReplaySubject(1); singleOrgPolicy = new ReplaySubject(1); @@ -69,6 +71,7 @@ describe("vault filter service", () => { policyService.policyAppliesToActiveUser$ .calledWith(PolicyType.SingleOrg) .mockReturnValue(singleOrgPolicy); + cipherService.cipherViews$ = cipherViews; vaultFilterService = new VaultFilterService( organizationService, @@ -162,7 +165,7 @@ describe("vault filter service", () => { createCipherView("1", "org test id", "folder test id"), createCipherView("2", "non matching org id", "non matching folder id"), ]; - cipherService.getAllDecrypted.mockResolvedValue(storedCiphers); + cipherViews.next(storedCiphers); const storedFolders = [ createFolderView("folder test id", "test"), @@ -191,6 +194,7 @@ describe("vault filter service", () => { createFolderView("Folder 3 Id", "Folder 1/Folder 3"), ]; folderViews.next(storedFolders); + cipherViews.next([]); const result = await firstValueFrom(vaultFilterService.folderTree$); diff --git a/apps/web/src/app/vault/individual-vault/vault-filter/services/vault-filter.service.ts b/apps/web/src/app/vault/individual-vault/vault-filter/services/vault-filter.service.ts index 601c86f230..043ba2dcd2 100644 --- a/apps/web/src/app/vault/individual-vault/vault-filter/services/vault-filter.service.ts +++ b/apps/web/src/app/vault/individual-vault/vault-filter/services/vault-filter.service.ts @@ -25,6 +25,7 @@ import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.servi import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction"; import { CipherType } from "@bitwarden/common/vault/enums"; import { TreeNode } from "@bitwarden/common/vault/models/domain/tree-node"; +import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { FolderView } from "@bitwarden/common/vault/models/view/folder.view"; import { ServiceUtils } from "@bitwarden/common/vault/service-utils"; import { COLLAPSED_GROUPINGS } from "@bitwarden/common/vault/services/key-state/collapsed-groupings.state"; @@ -55,9 +56,9 @@ export class VaultFilterService implements VaultFilterServiceAbstraction { protected _organizationFilter = new BehaviorSubject(null); filteredFolders$: Observable = this.folderService.folderViews$.pipe( - combineLatestWith(this._organizationFilter), - switchMap(([folders, org]) => { - return this.filterFolders(folders, org); + combineLatestWith(this.cipherService.cipherViews$, this._organizationFilter), + switchMap(([folders, ciphers, org]) => { + return this.filterFolders(folders, ciphers, org); }), ); folderTree$: Observable> = this.filteredFolders$.pipe( @@ -231,6 +232,7 @@ export class VaultFilterService implements VaultFilterServiceAbstraction { protected async filterFolders( storedFolders: FolderView[], + ciphers: CipherView[], org?: Organization, ): Promise { // If no org or "My Vault" is selected, show all folders @@ -239,7 +241,6 @@ export class VaultFilterService implements VaultFilterServiceAbstraction { } // Otherwise, show only folders that have ciphers from the selected org and the "no folder" folder - const ciphers = await this.cipherService.getAllDecrypted(); const orgCiphers = ciphers.filter((c) => c.organizationId == org?.id); return storedFolders.filter( (f) => orgCiphers.some((oc) => oc.folderId == f.id) || f.id == null, diff --git a/apps/web/src/app/vault/individual-vault/vault.component.ts b/apps/web/src/app/vault/individual-vault/vault.component.ts index 3da86fcb41..e37a50465b 100644 --- a/apps/web/src/app/vault/individual-vault/vault.component.ts +++ b/apps/web/src/app/vault/individual-vault/vault.component.ts @@ -281,7 +281,7 @@ export class VaultComponent implements OnInit, OnDestroy { this.currentSearchText$ = this.route.queryParams.pipe(map((queryParams) => queryParams.search)); const ciphers$ = combineLatest([ - Utils.asyncToObservable(() => this.cipherService.getAllDecrypted()), + this.cipherService.cipherViews$.pipe(filter((c) => c !== null)), filter$, this.currentSearchText$, ]).pipe( diff --git a/libs/common/src/vault/abstractions/cipher.service.ts b/libs/common/src/vault/abstractions/cipher.service.ts index e82c07653c..f0e19a2134 100644 --- a/libs/common/src/vault/abstractions/cipher.service.ts +++ b/libs/common/src/vault/abstractions/cipher.service.ts @@ -17,7 +17,7 @@ import { FieldView } from "../models/view/field.view"; import { AddEditCipherInfo } from "../types/add-edit-cipher-info"; export abstract class CipherService implements UserKeyRotationDataProvider { - cipherViews$: Observable>; + cipherViews$: Observable; ciphers$: Observable>; localData$: Observable>; /** diff --git a/libs/common/src/vault/services/cipher.service.spec.ts b/libs/common/src/vault/services/cipher.service.spec.ts index 0873fa9d92..3e8ec843fd 100644 --- a/libs/common/src/vault/services/cipher.service.spec.ts +++ b/libs/common/src/vault/services/cipher.service.spec.ts @@ -1,5 +1,5 @@ import { mock } from "jest-mock-extended"; -import { BehaviorSubject, of } from "rxjs"; +import { BehaviorSubject, map, of } from "rxjs"; import { BulkEncryptService } from "@bitwarden/common/platform/abstractions/bulk-encrypt.service"; @@ -381,7 +381,7 @@ describe("Cipher Service", () => { Cipher1: cipher1, Cipher2: cipher2, }); - cipherService.cipherViews$ = decryptedCiphers; + cipherService.cipherViews$ = decryptedCiphers.pipe(map((ciphers) => Object.values(ciphers))); encryptService.decryptToBytes.mockResolvedValue(new Uint8Array(32)); encryptedKey = new EncString("Re-encrypted Cipher Key"); diff --git a/libs/common/src/vault/services/cipher.service.ts b/libs/common/src/vault/services/cipher.service.ts index 77e696b5cd..a7377a93ee 100644 --- a/libs/common/src/vault/services/cipher.service.ts +++ b/libs/common/src/vault/services/cipher.service.ts @@ -1,4 +1,14 @@ -import { firstValueFrom, map, Observable, skipWhile, switchMap } from "rxjs"; +import { + combineLatest, + filter, + firstValueFrom, + map, + merge, + Observable, + shareReplay, + Subject, + switchMap, +} from "rxjs"; import { SemVer } from "semver"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; @@ -24,13 +34,7 @@ import Domain from "../../platform/models/domain/domain-base"; import { EncArrayBuffer } from "../../platform/models/domain/enc-array-buffer"; import { EncString } from "../../platform/models/domain/enc-string"; import { SymmetricCryptoKey } from "../../platform/models/domain/symmetric-crypto-key"; -import { - ActiveUserState, - CIPHERS_MEMORY, - DeriveDefinition, - DerivedState, - StateProvider, -} from "../../platform/state"; +import { ActiveUserState, StateProvider } from "../../platform/state"; import { CipherId, CollectionId, OrganizationId, UserId } from "../../types/guid"; import { OrgKey, UserKey } from "../../types/key"; import { CipherService as CipherServiceAbstraction } from "../abstractions/cipher.service"; @@ -81,14 +85,25 @@ export class CipherService implements CipherServiceAbstraction { private sortedCiphersCache: SortedCiphersCache = new SortedCiphersCache( this.sortCiphersByLastUsed, ); - private ciphersExpectingUpdate: DerivedState; + /** + * Observable that forces the `cipherViews$` observable to re-emit with the provided value. + * Used to let subscribers of `cipherViews$` know that the decrypted ciphers have been cleared for the active user. + * @private + */ + private forceCipherViews$: Subject = new Subject(); localData$: Observable>; ciphers$: Observable>; - cipherViews$: Observable>; - viewFor$(id: CipherId) { - return this.cipherViews$.pipe(map((views) => views[id])); - } + + /** + * Observable that emits an array of decrypted ciphers for the active user. + * This observable will not emit until the encrypted ciphers have either been loaded from state or after sync. + * + * A `null` value indicates that the latest encrypted ciphers have not been decrypted yet and that + * decryption is in progress. The latest decrypted ciphers will be emitted once decryption is complete. + * + */ + cipherViews$: Observable; addEditCipherInfo$: Observable; private localDataState: ActiveUserState>; @@ -115,23 +130,16 @@ export class CipherService implements CipherServiceAbstraction { this.encryptedCiphersState = this.stateProvider.getActive(ENCRYPTED_CIPHERS); this.decryptedCiphersState = this.stateProvider.getActive(DECRYPTED_CIPHERS); this.addEditCipherInfoState = this.stateProvider.getActive(ADD_EDIT_CIPHER_INFO_KEY); - this.ciphersExpectingUpdate = this.stateProvider.getDerived( - this.encryptedCiphersState.state$, - new DeriveDefinition(CIPHERS_MEMORY, "ciphersExpectingUpdate", { - derive: (_: Record) => false, - deserializer: (value) => value, - }), - {}, - ); this.localData$ = this.localDataState.state$.pipe(map((data) => data ?? {})); - // First wait for ciphersExpectingUpdate to be false before emitting ciphers - this.ciphers$ = this.ciphersExpectingUpdate.state$.pipe( - skipWhile((expectingUpdate) => expectingUpdate), - switchMap(() => this.encryptedCiphersState.state$), - map((ciphers) => ciphers ?? {}), + this.ciphers$ = this.encryptedCiphersState.state$.pipe(map((ciphers) => ciphers ?? {})); + + // Decrypted ciphers depend on both ciphers and local data and need to be updated when either changes + this.cipherViews$ = combineLatest([this.encryptedCiphersState.state$, this.localData$]).pipe( + filter(([ciphers]) => ciphers != null), // Skip if ciphers haven't been loaded yor synced yet + switchMap(() => merge(this.forceCipherViews$, this.getAllDecrypted())), + shareReplay({ bufferSize: 1, refCount: true }), ); - this.cipherViews$ = this.decryptedCiphersState.state$.pipe(map((views) => views ?? {})); this.addEditCipherInfo$ = this.addEditCipherInfoState.state$; } @@ -160,8 +168,14 @@ export class CipherService implements CipherServiceAbstraction { } async clearCache(userId?: UserId): Promise { - userId ??= await firstValueFrom(this.stateProvider.activeUserId$); + const activeUserId = await firstValueFrom(this.stateProvider.activeUserId$); + userId ??= activeUserId; await this.clearDecryptedCiphersState(userId); + + // Force the cipherView$ observable (which always tracks the active user) to re-emit + if (userId == activeUserId) { + this.forceCipherViews$.next(null); + } } async encrypt( @@ -354,6 +368,11 @@ export class CipherService implements CipherServiceAbstraction { return response; } + /** + * Decrypts all ciphers for the active user and caches them in memory. If the ciphers have already been decrypted and + * cached, the cached ciphers are returned. + * @deprecated Use `cipherViews$` observable instead + */ @sequentialize(() => "getAllDecrypted") async getAllDecrypted(): Promise { let decCiphers = await this.getDecryptedCiphers(); @@ -375,7 +394,9 @@ export class CipherService implements CipherServiceAbstraction { } private async getDecryptedCiphers() { - return Object.values(await firstValueFrom(this.cipherViews$)); + return Object.values( + await firstValueFrom(this.decryptedCiphersState.state$.pipe(map((c) => c ?? {}))), + ); } private async decryptCiphers(ciphers: Cipher[], userId: UserId) { @@ -932,8 +953,6 @@ export class CipherService implements CipherServiceAbstraction { userId: UserId = null, ): Promise> { 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(userId); const updatedCiphers = await this.stateProvider .getUser(userId, ENCRYPTED_CIPHERS) @@ -1254,7 +1273,7 @@ export class CipherService implements CipherServiceAbstraction { let encryptedCiphers: CipherWithIdRequest[] = []; - const ciphers = await this.getAllDecrypted(); + const ciphers = await firstValueFrom(this.cipherViews$); if (!ciphers) { return encryptedCiphers; }