From 6dc6aec88e3bcd607197cfaf517704b0b82df34d Mon Sep 17 00:00:00 2001 From: SmithThe4th Date: Thu, 21 Mar 2024 16:42:34 -0400 Subject: [PATCH] [PM-6199] VaultFilterService keeps decrypted collections in memory after logout (#8120) * updated vault filter service to make use of collection service * removed fix me * reverted to use reload collections on org vault as collection admin service does not support state management yet * fixed statement --- .../abstractions/vault-filter.service.ts | 4 ++- .../services/vault-filter.service.spec.ts | 27 +++++++++---------- .../services/vault-filter.service.ts | 27 +++++++------------ .../vault/individual-vault/vault.component.ts | 4 --- .../vault-filter/vault-filter.service.ts | 5 ++-- .../vault/abstractions/collection.service.ts | 2 ++ 6 files changed, 30 insertions(+), 39 deletions(-) diff --git a/apps/web/src/app/vault/individual-vault/vault-filter/services/abstractions/vault-filter.service.ts b/apps/web/src/app/vault/individual-vault/vault-filter/services/abstractions/vault-filter.service.ts index 72be67f7a0..3f8a0fb99d 100644 --- a/apps/web/src/app/vault/individual-vault/vault-filter/services/abstractions/vault-filter.service.ts +++ b/apps/web/src/app/vault/individual-vault/vault-filter/services/abstractions/vault-filter.service.ts @@ -5,6 +5,7 @@ import { CollectionView } from "@bitwarden/common/src/vault/models/view/collecti import { FolderView } from "@bitwarden/common/src/vault/models/view/folder.view"; import { TreeNode } from "@bitwarden/common/vault/models/domain/tree-node"; +import { CollectionAdminView } from "../../../../core/views/collection-admin.view"; import { CipherTypeFilter, CollectionFilter, @@ -20,7 +21,6 @@ export abstract class VaultFilterService { folderTree$: Observable>; collectionTree$: Observable>; cipherTypeTree$: Observable>; - reloadCollections: (collections: CollectionView[]) => void; getCollectionNodeFromTree: (id: string) => Promise>; setCollapsedFilterNodes: (collapsedFilterNodes: Set) => Promise; expandOrgFilter: () => Promise; @@ -30,4 +30,6 @@ export abstract class VaultFilterService { head: CipherTypeFilter, array: CipherTypeFilter[], ) => Observable>; + // TODO: Remove this from org vault when collection admin service adopts state management + reloadCollections?: (collections: CollectionAdminView[]) => void; } 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 511da100b5..e5938b5197 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 @@ -15,6 +15,7 @@ import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.servic import { Utils } from "@bitwarden/common/platform/misc/utils"; import { UserId } from "@bitwarden/common/types/guid"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; +import { CollectionService } from "@bitwarden/common/vault/abstractions/collection.service"; import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { CollectionView } from "@bitwarden/common/vault/models/view/collection.view"; @@ -31,8 +32,10 @@ describe("vault filter service", () => { let cipherService: MockProxy; let policyService: MockProxy; let i18nService: MockProxy; + let collectionService: MockProxy; let organizations: ReplaySubject; let folderViews: ReplaySubject; + let collectionViews: ReplaySubject; let stateProvider: FakeStateProvider; const mockUserId = Utils.newGuid() as UserId; @@ -48,12 +51,15 @@ describe("vault filter service", () => { accountService = mockAccountServiceWith(mockUserId); stateProvider = new FakeStateProvider(accountService); i18nService.collator = new Intl.Collator("en-US"); + collectionService = mock(); organizations = new ReplaySubject(1); folderViews = new ReplaySubject(1); + collectionViews = new ReplaySubject(1); organizationService.memberOrganizations$ = organizations; folderService.folderViews$ = folderViews; + collectionService.decryptedCollections$ = collectionViews; vaultFilterService = new VaultFilterService( organizationService, @@ -62,6 +68,7 @@ describe("vault filter service", () => { policyService, i18nService, stateProvider, + collectionService, ); collapsedGroupingsState = stateProvider.activeUser.getFake(COLLAPSED_GROUPINGS); }); @@ -196,9 +203,7 @@ describe("vault filter service", () => { createCollectionView("1", "collection 1", "org test id"), createCollectionView("2", "collection 2", "non matching org id"), ]; - // 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 - vaultFilterService.reloadCollections(storedCollections); + collectionViews.next(storedCollections); await expect(firstValueFrom(vaultFilterService.filteredCollections$)).resolves.toEqual([ createCollectionView("1", "collection 1", "org test id"), @@ -213,9 +218,7 @@ describe("vault filter service", () => { createCollectionView("id-2", "Collection 1/Collection 2", "org test id"), createCollectionView("id-3", "Collection 1/Collection 3", "org test id"), ]; - // 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 - vaultFilterService.reloadCollections(storedCollections); + collectionViews.next(storedCollections); const result = await firstValueFrom(vaultFilterService.collectionTree$); @@ -228,9 +231,7 @@ describe("vault filter service", () => { createCollectionView("id-1", "Collection 1", "org test id"), createCollectionView("id-3", "Collection 1/Collection 2/Collection 3", "org test id"), ]; - // 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 - vaultFilterService.reloadCollections(storedCollections); + collectionViews.next(storedCollections); const result = await firstValueFrom(vaultFilterService.collectionTree$); @@ -246,9 +247,7 @@ describe("vault filter service", () => { createCollectionView("id-3", "Collection 1/Collection 2/Collection 3", "org test id"), createCollectionView("id-4", "Collection 1/Collection 4", "org test id"), ]; - // 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 - vaultFilterService.reloadCollections(storedCollections); + collectionViews.next(storedCollections); const result = await firstValueFrom(vaultFilterService.collectionTree$); @@ -266,9 +265,7 @@ describe("vault filter service", () => { createCollectionView("id-1", "Collection 1", "org test id"), createCollectionView("id-3", "Collection 1/Collection 2/Collection 3", "org test id"), ]; - // 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 - vaultFilterService.reloadCollections(storedCollections); + collectionViews.next(storedCollections); const result = await firstValueFrom(vaultFilterService.collectionTree$); 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 9dfc07b5db..6bedac5bb6 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 @@ -1,13 +1,11 @@ import { Injectable } from "@angular/core"; import { BehaviorSubject, - combineLatest, combineLatestWith, firstValueFrom, map, Observable, of, - ReplaySubject, switchMap, } from "rxjs"; @@ -18,6 +16,7 @@ import { Organization } from "@bitwarden/common/admin-console/models/domain/orga import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { ActiveUserState, StateProvider } from "@bitwarden/common/platform/state"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; +import { CollectionService } from "@bitwarden/common/vault/abstractions/collection.service"; 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"; @@ -57,17 +56,14 @@ export class VaultFilterService implements VaultFilterServiceAbstraction { map((folders) => this.buildFolderTree(folders)), ); - // TODO: Remove once collections is refactored with observables - // replace with collection service observable - private collectionViews$ = new ReplaySubject(1); - filteredCollections$: Observable = combineLatest([ - this.collectionViews$, - this._organizationFilter, - ]).pipe( - switchMap(([collections, org]) => { - return this.filterCollections(collections, org); - }), - ); + filteredCollections$: Observable = + this.collectionService.decryptedCollections$.pipe( + combineLatestWith(this._organizationFilter), + switchMap(([collections, org]) => { + return this.filterCollections(collections, org); + }), + ); + collectionTree$: Observable> = this.filteredCollections$.pipe( map((collections) => this.buildCollectionTree(collections)), ); @@ -87,12 +83,9 @@ export class VaultFilterService implements VaultFilterServiceAbstraction { protected policyService: PolicyService, protected i18nService: I18nService, protected stateProvider: StateProvider, + protected collectionService: CollectionService, ) {} - async reloadCollections(collections: CollectionView[]) { - this.collectionViews$.next(collections); - } - async getCollectionNodeFromTree(id: string) { const collections = await firstValueFrom(this.collectionTree$); return ServiceUtils.getTreeNodeObject(collections, id) as TreeNode; 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 d7d9ab8074..6fee59d65b 100644 --- a/apps/web/src/app/vault/individual-vault/vault.component.ts +++ b/apps/web/src/app/vault/individual-vault/vault.component.ts @@ -406,10 +406,6 @@ export class VaultComponent implements OnInit, OnDestroy { (filter.organizationId === undefined || filter.organizationId === Unassigned); this.isEmpty = collections?.length === 0 && ciphers?.length === 0; - // This is a temporary fix to avoid double fetching collections. - // TODO: Remove when implementing new VVR menu - this.vaultFilterService.reloadCollections(allCollections); - this.performingInitialLoad = false; this.refreshing = false; }, diff --git a/apps/web/src/app/vault/org-vault/vault-filter/vault-filter.service.ts b/apps/web/src/app/vault/org-vault/vault-filter/vault-filter.service.ts index f82ba945e4..c6d4ee590b 100644 --- a/apps/web/src/app/vault/org-vault/vault-filter/vault-filter.service.ts +++ b/apps/web/src/app/vault/org-vault/vault-filter/vault-filter.service.ts @@ -6,11 +6,11 @@ import { PolicyService } from "@bitwarden/common/admin-console/abstractions/poli import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { StateProvider } from "@bitwarden/common/platform/state"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; +import { CollectionService } from "@bitwarden/common/vault/abstractions/collection.service"; import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction"; import { TreeNode } from "@bitwarden/common/vault/models/domain/tree-node"; import { CollectionAdminView } from "../../../vault/core/views/collection-admin.view"; -import { CollectionAdminService } from "../../core/collection-admin.service"; import { VaultFilterService as BaseVaultFilterService } from "../../individual-vault/vault-filter/services/vault-filter.service"; import { CollectionFilter } from "../../individual-vault/vault-filter/shared/models/vault-filter.type"; @@ -32,7 +32,7 @@ export class VaultFilterService extends BaseVaultFilterService implements OnDest policyService: PolicyService, i18nService: I18nService, stateProvider: StateProvider, - protected collectionAdminService: CollectionAdminService, + collectionService: CollectionService, ) { super( organizationService, @@ -41,6 +41,7 @@ export class VaultFilterService extends BaseVaultFilterService implements OnDest policyService, i18nService, stateProvider, + collectionService, ); } diff --git a/libs/common/src/vault/abstractions/collection.service.ts b/libs/common/src/vault/abstractions/collection.service.ts index ab60615329..87ce8edf43 100644 --- a/libs/common/src/vault/abstractions/collection.service.ts +++ b/libs/common/src/vault/abstractions/collection.service.ts @@ -7,6 +7,8 @@ import { TreeNode } from "../models/domain/tree-node"; import { CollectionView } from "../models/view/collection.view"; export abstract class CollectionService { + decryptedCollections$: Observable; + clearActiveUserCache: () => Promise; encrypt: (model: CollectionView) => Promise; decryptedCollectionViews$: (ids: CollectionId[]) => Observable;