From ce2606b406882435448b07024f06cef246d980f5 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Fri, 8 Jul 2022 10:18:07 +0200 Subject: [PATCH] [EC-267] Unassigned collection has disappeared in web vault (#3000) * feat: add unassigned collection to decrypted collections * feat: add support for unassigned in individual vault * fix: dont activate collection when not selected * fix: remove collection selection completely when pruning * feat: prune collection selection if selecting my vault * fix: filter and only show organization ciphers when unassigned collection is selected * fix: only show unassigned for admins * feat: add unassigned logic to organizational vault buildFilter * refactor: move buildFilter to VaultFilterModel * chore: add buildFilter tests * fix: bugs in filtering logic * refactor: use VaultFilter.buildFilter on desktop * chore: group and reword tests for better readability * feat: add additional test * fix: connect unassigned collection to organization * fix: test by adding missing * chore: tweak test group naming * fix: change undefined to null to better reflect real values --- apps/desktop/src/app/vault/vault.component.ts | 43 +--- .../collection-filter.component.html | 2 +- .../vault-filter/vault-filter.service.ts | 7 + .../individual-vault.component.ts | 39 +-- .../organization-vault.component.ts | 39 +-- .../src/app/modules/vault/vault.service.ts | 2 +- .../components/collection-filter.component.ts | 1 + .../models/vault-filter.model.spec.ts | 237 ++++++++++++++++++ .../vault-filter/models/vault-filter.model.ts | 42 ++++ .../vault-filter/vault-filter.component.ts | 7 +- .../common/src/services/collection.service.ts | 1 + 11 files changed, 309 insertions(+), 111 deletions(-) create mode 100644 libs/angular/src/modules/vault-filter/models/vault-filter.model.spec.ts diff --git a/apps/desktop/src/app/vault/vault.component.ts b/apps/desktop/src/app/vault/vault.component.ts index 70d19b2b6e..ea454c1cc2 100644 --- a/apps/desktop/src/app/vault/vault.component.ts +++ b/apps/desktop/src/app/vault/vault.component.ts @@ -128,7 +128,7 @@ export class VaultComponent implements OnInit, OnDestroy { await this.openGenerator(false); break; case "syncCompleted": - await this.ciphersComponent.reload(this.buildFilter()); + await this.ciphersComponent.reload(this.activeFilter.buildFilter()); await this.vaultFilterComponent.reloadCollectionsAndFolders(this.activeFilter); await this.vaultFilterComponent.reloadOrganizations(); break; @@ -241,7 +241,7 @@ export class VaultComponent implements OnInit, OnDestroy { selectedOrganizationId: params.selectedOrganizationId, myVaultOnly: params.myVaultOnly ?? false, }); - await this.ciphersComponent.reload(this.buildFilter()); + await this.ciphersComponent.reload(this.activeFilter.buildFilter()); }); } @@ -540,7 +540,10 @@ export class VaultComponent implements OnInit, OnDestroy { this.i18nService.t(this.calculateSearchBarLocalizationString(vaultFilter)) ); this.activeFilter = vaultFilter; - await this.ciphersComponent.reload(this.buildFilter(), vaultFilter.status === "trash"); + await this.ciphersComponent.reload( + this.activeFilter.buildFilter(), + vaultFilter.status === "trash" + ); this.go(); } @@ -570,40 +573,6 @@ export class VaultComponent implements OnInit, OnDestroy { return "searchVault"; } - private buildFilter(): (cipher: CipherView) => boolean { - return (cipher) => { - let cipherPassesFilter = true; - if (this.activeFilter.status === "favorites" && cipherPassesFilter) { - cipherPassesFilter = cipher.favorite; - } - if (this.activeFilter.status === "trash" && cipherPassesFilter) { - cipherPassesFilter = cipher.isDeleted; - } - if (this.activeFilter.cipherType != null && cipherPassesFilter) { - cipherPassesFilter = cipher.type === this.activeFilter.cipherType; - } - if ( - this.activeFilter.selectedFolder && - this.activeFilter.selectedFolderId != "none" && - cipherPassesFilter - ) { - cipherPassesFilter = cipher.folderId === this.activeFilter.selectedFolderId; - } - if (this.activeFilter.selectedCollectionId != null && cipherPassesFilter) { - cipherPassesFilter = - cipher.collectionIds != null && - cipher.collectionIds.indexOf(this.activeFilter.selectedCollectionId) > -1; - } - if (this.activeFilter.selectedOrganizationId != null && cipherPassesFilter) { - cipherPassesFilter = cipher.organizationId === this.activeFilter.selectedOrganizationId; - } - if (this.activeFilter.myVaultOnly && cipherPassesFilter) { - cipherPassesFilter = cipher.organizationId === null; - } - return cipherPassesFilter; - }; - } - async openGenerator(comingFromAddEdit: boolean, passwordType = true) { if (this.modal != null) { this.modal.close(); diff --git a/apps/web/src/app/modules/vault-filter/components/collection-filter.component.html b/apps/web/src/app/modules/vault-filter/components/collection-filter.component.html index 24463dca19..417d635bad 100644 --- a/apps/web/src/app/modules/vault-filter/components/collection-filter.component.html +++ b/apps/web/src/app/modules/vault-filter/components/collection-filter.component.html @@ -23,7 +23,7 @@
  • diff --git a/apps/web/src/app/modules/vault-filter/vault-filter.service.ts b/apps/web/src/app/modules/vault-filter/vault-filter.service.ts index 6e5b13a846..d85ef3df16 100644 --- a/apps/web/src/app/modules/vault-filter/vault-filter.service.ts +++ b/apps/web/src/app/modules/vault-filter/vault-filter.service.ts @@ -7,6 +7,7 @@ import { ApiService } from "@bitwarden/common/abstractions/api.service"; import { CipherService } from "@bitwarden/common/abstractions/cipher.service"; import { CollectionService } from "@bitwarden/common/abstractions/collection.service"; import { FolderService } from "@bitwarden/common/abstractions/folder.service"; +import { I18nService } from "@bitwarden/common/abstractions/i18n.service"; import { OrganizationService } from "@bitwarden/common/abstractions/organization.service"; import { PolicyService } from "@bitwarden/common/abstractions/policy.service"; import { StateService } from "@bitwarden/common/abstractions/state.service"; @@ -27,6 +28,7 @@ export class VaultFilterService extends BaseVaultFilterService { cipherService: CipherService, collectionService: CollectionService, policyService: PolicyService, + private i18nService: I18nService, protected apiService: ApiService ) { super( @@ -69,6 +71,11 @@ export class VaultFilterService extends BaseVaultFilterService { result = await this.collectionService.decryptMany(collectionDomains); } + const noneCollection = new CollectionView(); + noneCollection.name = this.i18nService.t("unassigned"); + noneCollection.organizationId = organizationId; + result.push(noneCollection); + const nestedCollections = await this.collectionService.getAllNested(result); return new DynamicTreeNode({ fullList: result, diff --git a/apps/web/src/app/modules/vault/modules/individual-vault/individual-vault.component.ts b/apps/web/src/app/modules/vault/modules/individual-vault/individual-vault.component.ts index 7178578da8..567e390c08 100644 --- a/apps/web/src/app/modules/vault/modules/individual-vault/individual-vault.component.ts +++ b/apps/web/src/app/modules/vault/modules/individual-vault/individual-vault.component.ts @@ -173,7 +173,10 @@ export class IndividualVaultComponent implements OnInit, OnDestroy { async applyVaultFilter(vaultFilter: VaultFilter) { this.ciphersComponent.showAddNew = vaultFilter.status !== "trash"; this.activeFilter = vaultFilter; - await this.ciphersComponent.reload(this.buildFilter(), vaultFilter.status === "trash"); + await this.ciphersComponent.reload( + this.activeFilter.buildFilter(), + vaultFilter.status === "trash" + ); this.filterComponent.searchPlaceholder = this.vaultService.calculateSearchBarLocalizationString( this.activeFilter ); @@ -196,40 +199,6 @@ export class IndividualVaultComponent implements OnInit, OnDestroy { this.ciphersComponent.search(200); } - private buildFilter(): (cipher: CipherView) => boolean { - return (cipher) => { - let cipherPassesFilter = true; - if (this.activeFilter.status === "favorites" && cipherPassesFilter) { - cipherPassesFilter = cipher.favorite; - } - if (this.activeFilter.status === "trash" && cipherPassesFilter) { - cipherPassesFilter = cipher.isDeleted; - } - if (this.activeFilter.cipherType != null && cipherPassesFilter) { - cipherPassesFilter = cipher.type === this.activeFilter.cipherType; - } - if ( - this.activeFilter.selectedFolder && - this.activeFilter.selectedFolderId != "none" && - cipherPassesFilter - ) { - cipherPassesFilter = cipher.folderId === this.activeFilter.selectedFolderId; - } - if (this.activeFilter.selectedCollectionId != null && cipherPassesFilter) { - cipherPassesFilter = - cipher.collectionIds != null && - cipher.collectionIds.indexOf(this.activeFilter.selectedCollectionId) > -1; - } - if (this.activeFilter.selectedOrganizationId != null && cipherPassesFilter) { - cipherPassesFilter = cipher.organizationId === this.activeFilter.selectedOrganizationId; - } - if (this.activeFilter.myVaultOnly && cipherPassesFilter) { - cipherPassesFilter = cipher.organizationId === null; - } - return cipherPassesFilter; - }; - } - async editCipherAttachments(cipher: CipherView) { const canAccessPremium = await this.stateService.getCanAccessPremium(); if (cipher.organizationId == null && !canAccessPremium) { diff --git a/apps/web/src/app/modules/vault/modules/organization-vault/organization-vault.component.ts b/apps/web/src/app/modules/vault/modules/organization-vault/organization-vault.component.ts index 088c1370e7..aaf37e97e4 100644 --- a/apps/web/src/app/modules/vault/modules/organization-vault/organization-vault.component.ts +++ b/apps/web/src/app/modules/vault/modules/organization-vault/organization-vault.component.ts @@ -162,46 +162,15 @@ export class OrganizationVaultComponent implements OnInit, OnDestroy { async applyVaultFilter(vaultFilter: VaultFilter) { this.ciphersComponent.showAddNew = vaultFilter.status !== "trash"; this.activeFilter = vaultFilter; - await this.ciphersComponent.reload(this.buildFilter(), vaultFilter.status === "trash"); + await this.ciphersComponent.reload( + this.activeFilter.buildFilter(), + vaultFilter.status === "trash" + ); this.vaultFilterComponent.searchPlaceholder = this.vaultService.calculateSearchBarLocalizationString(this.activeFilter); this.go(); } - private buildFilter(): (cipher: CipherView) => boolean { - return (cipher) => { - let cipherPassesFilter = true; - if (this.activeFilter.status === "favorites" && cipherPassesFilter) { - cipherPassesFilter = cipher.favorite; - } - if (this.deleted && cipherPassesFilter) { - cipherPassesFilter = cipher.isDeleted; - } - if (this.activeFilter.cipherType != null && cipherPassesFilter) { - cipherPassesFilter = cipher.type === this.activeFilter.cipherType; - } - if ( - this.activeFilter.selectedFolder != null && - this.activeFilter.selectedFolderId != "none" && - cipherPassesFilter - ) { - cipherPassesFilter = cipher.folderId === this.activeFilter.selectedFolderId; - } - if (this.activeFilter.selectedCollectionId != null && cipherPassesFilter) { - cipherPassesFilter = - cipher.collectionIds != null && - cipher.collectionIds.indexOf(this.activeFilter.selectedCollectionId) > -1; - } - if (this.activeFilter.selectedOrganizationId != null && cipherPassesFilter) { - cipherPassesFilter = cipher.organizationId === this.activeFilter.selectedOrganizationId; - } - if (this.activeFilter.myVaultOnly && cipherPassesFilter) { - cipherPassesFilter = cipher.organizationId === null; - } - return cipherPassesFilter; - }; - } - filterSearchText(searchText: string) { this.ciphersComponent.searchText = searchText; this.ciphersComponent.search(200); diff --git a/apps/web/src/app/modules/vault/vault.service.ts b/apps/web/src/app/modules/vault/vault.service.ts index 4488173b4b..e792ac736f 100644 --- a/apps/web/src/app/modules/vault/vault.service.ts +++ b/apps/web/src/app/modules/vault/vault.service.ts @@ -14,7 +14,7 @@ export class VaultService { if (vaultFilter.selectedFolderId != null && vaultFilter.selectedFolderId != "none") { return "searchFolder"; } - if (vaultFilter.selectedCollectionId != null) { + if (vaultFilter.selectedCollection) { return "searchCollection"; } if (vaultFilter.selectedOrganizationId != null) { diff --git a/libs/angular/src/modules/vault-filter/components/collection-filter.component.ts b/libs/angular/src/modules/vault-filter/components/collection-filter.component.ts index ed35390dae..313db3f04c 100644 --- a/libs/angular/src/modules/vault-filter/components/collection-filter.component.ts +++ b/libs/angular/src/modules/vault-filter/components/collection-filter.component.ts @@ -41,6 +41,7 @@ export class CollectionFilterComponent { applyFilter(collection: CollectionView) { this.activeFilter.resetFilter(); + this.activeFilter.selectedCollection = true; this.activeFilter.selectedCollectionId = collection.id; this.onFilterChange.emit(this.activeFilter); } diff --git a/libs/angular/src/modules/vault-filter/models/vault-filter.model.spec.ts b/libs/angular/src/modules/vault-filter/models/vault-filter.model.spec.ts new file mode 100644 index 0000000000..1c63481ab7 --- /dev/null +++ b/libs/angular/src/modules/vault-filter/models/vault-filter.model.spec.ts @@ -0,0 +1,237 @@ +import { CipherType } from "@bitwarden/common/enums/cipherType"; +import { CipherView } from "@bitwarden/common/models/view/cipherView"; + +import { VaultFilter } from "./vault-filter.model"; + +describe("VaultFilter", () => { + describe("filterFunction", () => { + describe("generic cipher", () => { + it("should return true when not filtering for anything specific", () => { + const cipher = createCipher(); + const filterFunction = createFilterFunction({ status: "all" }); + + const result = filterFunction(cipher); + + expect(result).toBe(true); + }); + }); + + describe("given a favorite cipher", () => { + const cipher = createCipher({ favorite: true }); + + it("should return true when filtering for favorites", () => { + const filterFunction = createFilterFunction({ status: "favorites" }); + + const result = filterFunction(cipher); + + expect(result).toBe(true); + }); + + it("should return false when filtering for trash", () => { + const filterFunction = createFilterFunction({ status: "trash" }); + + const result = filterFunction(cipher); + + expect(result).toBe(false); + }); + }); + + describe("given a deleted cipher", () => { + const cipher = createCipher({ deletedDate: new Date() }); + + it("should return true when filtering for trash", () => { + const filterFunction = createFilterFunction({ status: "trash" }); + + const result = filterFunction(cipher); + + expect(result).toBe(true); + }); + + it("should return false when filtering for favorites", () => { + const filterFunction = createFilterFunction({ status: "favorites" }); + + const result = filterFunction(cipher); + + expect(result).toBe(false); + }); + }); + + describe("given a cipher with type", () => { + it("should return true when filter matches cipher type", () => { + const cipher = createCipher({ type: CipherType.Identity }); + const filterFunction = createFilterFunction({ cipherType: CipherType.Identity }); + + const result = filterFunction(cipher); + + expect(result).toBe(true); + }); + + it("should return false when filter does not match cipher type", () => { + const cipher = createCipher({ type: CipherType.Card }); + const filterFunction = createFilterFunction({ cipherType: CipherType.Identity }); + + const result = filterFunction(cipher); + + expect(result).toBe(false); + }); + }); + + describe("given a cipher with folder id", () => { + it("should return true when filter matches folder id", () => { + const cipher = createCipher({ folderId: "folderId" }); + const filterFunction = createFilterFunction({ + selectedFolder: true, + selectedFolderId: "folderId", + }); + + const result = filterFunction(cipher); + + expect(result).toBe(true); + }); + + it("should return false when filter does not match folder id", () => { + const cipher = createCipher({ folderId: "folderId" }); + const filterFunction = createFilterFunction({ + selectedFolder: true, + selectedFolderId: "anotherFolderId", + }); + + const result = filterFunction(cipher); + + expect(result).toBe(false); + }); + }); + + describe("given a cipher without folder", () => { + const cipher = createCipher({ folderId: null }); + + it("should return true when filtering on unassigned folder", () => { + const filterFunction = createFilterFunction({ + selectedFolder: true, + selectedFolderId: null, + }); + + const result = filterFunction(cipher); + + expect(result).toBe(true); + }); + }); + + describe("given an organizational cipher (with organization and collections)", () => { + const cipher = createCipher({ + organizationId: "organizationId", + collectionIds: ["collectionId", "anotherId"], + }); + + it("should return true when filter matches collection id", () => { + const filterFunction = createFilterFunction({ + selectedCollection: true, + selectedCollectionId: "collectionId", + }); + + const result = filterFunction(cipher); + + expect(result).toBe(true); + }); + + it("should return false when filter does not match collection id", () => { + const filterFunction = createFilterFunction({ + selectedCollection: true, + selectedCollectionId: "nonMatchingId", + }); + + const result = filterFunction(cipher); + + expect(result).toBe(false); + }); + + it("should return false when filter does not match organization id", () => { + const filterFunction = createFilterFunction({ + selectedOrganizationId: "anotherOrganizationId", + }); + + const result = filterFunction(cipher); + + expect(result).toBe(false); + }); + + it("should return false when filtering for my vault only", () => { + const filterFunction = createFilterFunction({ + myVaultOnly: true, + }); + + const result = filterFunction(cipher); + + expect(result).toBe(false); + }); + }); + + describe("given an unassigned organizational cipher (with organization, without collection)", () => { + const cipher = createCipher({ organizationId: "organizationId", collectionIds: [] }); + + it("should return true when filtering for unassigned collection", () => { + const filterFunction = createFilterFunction({ + selectedCollection: true, + selectedCollectionId: null, + }); + + const result = filterFunction(cipher); + + expect(result).toBe(true); + }); + + it("should return true when filter matches organization id", () => { + const filterFunction = createFilterFunction({ + selectedOrganizationId: "organizationId", + }); + + const result = filterFunction(cipher); + + expect(result).toBe(true); + }); + }); + + describe("given an individual cipher (without organization or collection)", () => { + const cipher = createCipher({ organizationId: null, collectionIds: [] }); + + it("should return false when filtering for unassigned collection", () => { + const filterFunction = createFilterFunction({ + selectedCollection: true, + selectedCollectionId: null, + }); + + const result = filterFunction(cipher); + + expect(result).toBe(false); + }); + + it("should return true when filtering for my vault only", () => { + const cipher = createCipher({ organizationId: null }); + const filterFunction = createFilterFunction({ + myVaultOnly: true, + }); + + const result = filterFunction(cipher); + + expect(result).toBe(true); + }); + }); + }); +}); + +function createFilterFunction(options: Partial = {}) { + return new VaultFilter(options).buildFilter(); +} + +function createCipher(options: Partial = {}) { + const cipher = new CipherView(); + + cipher.favorite = options.favorite ?? false; + cipher.deletedDate = options.deletedDate; + cipher.type = options.type; + cipher.folderId = options.folderId; + cipher.collectionIds = options.collectionIds; + cipher.organizationId = options.organizationId; + + return cipher; +} diff --git a/libs/angular/src/modules/vault-filter/models/vault-filter.model.ts b/libs/angular/src/modules/vault-filter/models/vault-filter.model.ts index f28cd20336..942704f128 100644 --- a/libs/angular/src/modules/vault-filter/models/vault-filter.model.ts +++ b/libs/angular/src/modules/vault-filter/models/vault-filter.model.ts @@ -1,9 +1,13 @@ import { CipherType } from "@bitwarden/common/enums/cipherType"; +import { CipherView } from "@bitwarden/common/models/view/cipherView"; import { CipherStatus } from "./cipher-status.model"; +export type VaultFilterFunction = (cipher: CipherView) => boolean; + export class VaultFilter { cipherType?: CipherType; + selectedCollection = false; // This is needed because of how the "Unassigned" collection works. It has a null id. selectedCollectionId?: string; status?: CipherStatus; selectedFolder = false; // This is needed because of how the "No Folder" folder works. It has a null id. @@ -19,6 +23,7 @@ export class VaultFilter { resetFilter() { this.cipherType = null; this.status = null; + this.selectedCollection = false; this.selectedCollectionId = null; this.selectedFolder = false; this.selectedFolderId = null; @@ -29,4 +34,41 @@ export class VaultFilter { this.selectedOrganizationId = null; this.resetFilter(); } + + buildFilter(): VaultFilterFunction { + return (cipher) => { + let cipherPassesFilter = true; + if (this.status === "favorites" && cipherPassesFilter) { + cipherPassesFilter = cipher.favorite; + } + if (this.status === "trash" && cipherPassesFilter) { + cipherPassesFilter = cipher.isDeleted; + } + if (this.cipherType != null && cipherPassesFilter) { + cipherPassesFilter = cipher.type === this.cipherType; + } + if (this.selectedFolder && this.selectedFolderId == null && cipherPassesFilter) { + cipherPassesFilter = cipher.folderId == null; + } + if (this.selectedFolder && this.selectedFolderId != null && cipherPassesFilter) { + cipherPassesFilter = cipher.folderId === this.selectedFolderId; + } + if (this.selectedCollection && this.selectedCollectionId == null && cipherPassesFilter) { + cipherPassesFilter = + cipher.organizationId != null && + (cipher.collectionIds == null || cipher.collectionIds.length === 0); + } + if (this.selectedCollection && this.selectedCollectionId != null && cipherPassesFilter) { + cipherPassesFilter = + cipher.collectionIds != null && cipher.collectionIds.includes(this.selectedCollectionId); + } + if (this.selectedOrganizationId != null && cipherPassesFilter) { + cipherPassesFilter = cipher.organizationId === this.selectedOrganizationId; + } + if (this.myVaultOnly && cipherPassesFilter) { + cipherPassesFilter = cipher.organizationId === null; + } + return cipherPassesFilter; + }; + } } diff --git a/libs/angular/src/modules/vault-filter/vault-filter.component.ts b/libs/angular/src/modules/vault-filter/vault-filter.component.ts index b620ed82e2..2e77e2fd87 100644 --- a/libs/angular/src/modules/vault-filter/vault-filter.component.ts +++ b/libs/angular/src/modules/vault-filter/vault-filter.component.ts @@ -111,9 +111,12 @@ export class VaultFilterComponent implements OnInit { protected pruneInvalidCollectionSelection(filter: VaultFilter): VaultFilter { if ( - filter.selectedCollectionId != null && - !this.collections?.hasId(filter.selectedCollectionId) + filter.myVaultOnly || + (filter.selectedCollection && + filter.selectedCollectionId != null && + !this.collections?.hasId(filter.selectedCollectionId)) ) { + filter.selectedCollection = false; filter.selectedCollectionId = null; } return filter; diff --git a/libs/common/src/services/collection.service.ts b/libs/common/src/services/collection.service.ts index 4d3563d7c5..12054916e0 100644 --- a/libs/common/src/services/collection.service.ts +++ b/libs/common/src/services/collection.service.ts @@ -86,6 +86,7 @@ export class CollectionService implements CollectionServiceAbstraction { const collections = await this.getAll(); decryptedCollections = await this.decryptMany(collections); + await this.stateService.setDecryptedCollections(decryptedCollections); return decryptedCollections; }