From 973aba1bf44249d405127b60ef92c3000d942520 Mon Sep 17 00:00:00 2001 From: Shane Melton Date: Tue, 30 Jul 2024 08:14:17 -0700 Subject: [PATCH] [PM-8379] Fix Vault Popup Items service loading (#10324) * [PM-8379] Avoid fetching decrypted ciphers if no sync has completed * [PM-8379] Do not emit ciphers loading when applying filters --- .../vault-popup-items.service.spec.ts | 28 +++++++++---------- .../services/vault-popup-items.service.ts | 6 +++- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/apps/browser/src/vault/popup/services/vault-popup-items.service.spec.ts b/apps/browser/src/vault/popup/services/vault-popup-items.service.spec.ts index f6457b8db5..2ead93e512 100644 --- a/apps/browser/src/vault/popup/services/vault-popup-items.service.spec.ts +++ b/apps/browser/src/vault/popup/services/vault-popup-items.service.spec.ts @@ -1,11 +1,12 @@ import { TestBed } from "@angular/core/testing"; import { mock } from "jest-mock-extended"; -import { BehaviorSubject } from "rxjs"; +import { BehaviorSubject, firstValueFrom, timeout } from "rxjs"; import { SearchService } from "@bitwarden/common/abstractions/search.service"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { ProductTierType } from "@bitwarden/common/billing/enums"; +import { SyncService } from "@bitwarden/common/platform/sync"; import { ObservableTracker } from "@bitwarden/common/spec"; import { CipherId } from "@bitwarden/common/types/guid"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; @@ -37,6 +38,7 @@ describe("VaultPopupItemsService", () => { const searchService = mock(); const collectionService = mock(); const vaultAutofillServiceMock = mock(); + const syncServiceMock = mock(); beforeEach(() => { allCiphers = cipherFactory(10); @@ -90,6 +92,8 @@ describe("VaultPopupItemsService", () => { organizationServiceMock.organizations$ = new BehaviorSubject([mockOrg]); collectionService.decryptedCollections$ = new BehaviorSubject(mockCollections); + syncServiceMock.getLastSync.mockResolvedValue(new Date()); + testBed = TestBed.configureTestingModule({ providers: [ { provide: CipherService, useValue: cipherServiceMock }, @@ -99,6 +103,7 @@ describe("VaultPopupItemsService", () => { { provide: VaultPopupListFiltersService, useValue: vaultPopupListFiltersServiceMock }, { provide: CollectionService, useValue: collectionService }, { provide: VaultPopupAutofillService, useValue: vaultAutofillServiceMock }, + { provide: SyncService, useValue: syncServiceMock }, ], }); @@ -155,6 +160,14 @@ describe("VaultPopupItemsService", () => { await expect(tracker.pauseUntilReceived(3)).rejects.toThrow("Timeout exceeded"); }); + it("should not emit cipher list if syncService.getLastSync returns null", async () => { + syncServiceMock.getLastSync.mockResolvedValue(null); + + const obs$ = service.autoFillCiphers$.pipe(timeout(50)); + + await expect(firstValueFrom(obs$)).rejects.toThrow("Timeout has occurred"); + }); + describe("autoFillCiphers$", () => { it("should return empty array if there is no current tab", (done) => { (vaultAutofillServiceMock.currentAutofillTab$ as BehaviorSubject).next(null); @@ -394,19 +407,6 @@ describe("VaultPopupItemsService", () => { expect(tracked.emissions[1]).toBe(true); expect(tracked.emissions[2]).toBe(false); }); - - it("should cycle when filters are applied", async () => { - // Restart tracking - tracked = new ObservableTracker(service.loading$); - service.applyFilter("test"); - - await trackedCiphers.pauseUntilReceived(2); - - expect(tracked.emissions.length).toBe(3); - expect(tracked.emissions[0]).toBe(false); - expect(tracked.emissions[1]).toBe(true); - expect(tracked.emissions[2]).toBe(false); - }); }); describe("applyFilter", () => { diff --git a/apps/browser/src/vault/popup/services/vault-popup-items.service.ts b/apps/browser/src/vault/popup/services/vault-popup-items.service.ts index eb1af2cf71..e78e289c75 100644 --- a/apps/browser/src/vault/popup/services/vault-popup-items.service.ts +++ b/apps/browser/src/vault/popup/services/vault-popup-items.service.ts @@ -5,6 +5,7 @@ import { concatMap, distinctUntilChanged, distinctUntilKeyChanged, + filter, from, map, merge, @@ -21,6 +22,7 @@ import { import { SearchService } from "@bitwarden/common/abstractions/search.service"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { Utils } from "@bitwarden/common/platform/misc/utils"; +import { SyncService } from "@bitwarden/common/platform/sync"; import { CollectionId, OrganizationId } from "@bitwarden/common/types/guid"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { CollectionService } from "@bitwarden/common/vault/abstractions/collection.service"; @@ -78,6 +80,8 @@ export class VaultPopupItemsService { ).pipe( runInsideAngular(inject(NgZone)), // Workaround to ensure cipher$ state provider emissions are run inside Angular tap(() => this._ciphersLoading$.next()), + switchMap(() => Utils.asyncToObservable(() => this.syncService.getLastSync())), + filter((lastSync) => lastSync !== null), // Only attempt to load ciphers if we performed a sync switchMap(() => Utils.asyncToObservable(() => this.cipherService.getAllDecrypted())), switchMap((ciphers) => combineLatest([ @@ -108,7 +112,6 @@ export class VaultPopupItemsService { this._searchText$, this.vaultPopupListFiltersService.filterFunction$, ]).pipe( - tap(() => this._ciphersLoading$.next()), map(([ciphers, searchText, filterFunction]): [CipherView[], string] => [ filterFunction(ciphers), searchText, @@ -236,6 +239,7 @@ export class VaultPopupItemsService { private searchService: SearchService, private collectionService: CollectionService, private vaultPopupAutofillService: VaultPopupAutofillService, + private syncService: SyncService, ) {} applyFilter(newSearchText: string) {