From 1b22320dc51416436e0fb86864b5da88395dfcd2 Mon Sep 17 00:00:00 2001 From: Shane Melton Date: Tue, 13 Aug 2024 10:32:02 -0700 Subject: [PATCH] [AC-1041] My Vault filter on first login fix (#10301) * [AC-1041] Ensure organizationTree$ updates whenever the policy observables emit * [AC-1041] Ensure enforcePersonalOwnership updates whenever the policy observable emits * [AC-1041] Do not attempt to pre-select null filter values or read-only collections --- .../vault/vault-select.component.ts | 28 ++++++++++++++----- .../services/vault-filter.service.spec.ts | 24 ++++++++++------ .../services/vault-filter.service.ts | 22 ++++++++++----- .../vault/individual-vault/vault.component.ts | 18 ++++++++---- 4 files changed, 64 insertions(+), 28 deletions(-) diff --git a/apps/browser/src/vault/popup/components/vault/vault-select.component.ts b/apps/browser/src/vault/popup/components/vault/vault-select.component.ts index de6a33724d..6780cd5792 100644 --- a/apps/browser/src/vault/popup/components/vault/vault-select.component.ts +++ b/apps/browser/src/vault/popup/components/vault/vault-select.component.ts @@ -5,17 +5,28 @@ import { Component, ElementRef, EventEmitter, + HostListener, + OnDestroy, OnInit, Output, TemplateRef, ViewChild, ViewContainerRef, - HostListener, - OnDestroy, } from "@angular/core"; -import { BehaviorSubject, concatMap, map, merge, Observable, Subject, takeUntil } from "rxjs"; +import { + BehaviorSubject, + combineLatest, + concatMap, + map, + merge, + Observable, + Subject, + takeUntil, +} from "rxjs"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; +import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; +import { PolicyType } from "@bitwarden/common/admin-console/enums"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; @@ -88,6 +99,7 @@ export class VaultSelectComponent implements OnInit, OnDestroy { private viewContainerRef: ViewContainerRef, private platformUtilsService: PlatformUtilsService, private organizationService: OrganizationService, + private policyService: PolicyService, ) {} @HostListener("document:keydown.escape", ["$event"]) @@ -103,11 +115,13 @@ export class VaultSelectComponent implements OnInit, OnDestroy { .pipe(takeUntil(this._destroy)) .pipe(map((orgs) => orgs.sort(Utils.getSortFunction(this.i18nService, "name")))); - this.organizations$ + combineLatest([ + this.organizations$, + this.policyService.policyAppliesToActiveUser$(PolicyType.PersonalOwnership), + ]) .pipe( - concatMap(async (organizations) => { - this.enforcePersonalOwnership = - await this.vaultFilterService.checkForPersonalOwnershipPolicy(); + concatMap(async ([organizations, enforcePersonalOwnership]) => { + this.enforcePersonalOwnership = enforcePersonalOwnership; if (this.shouldShow(organizations)) { if (this.enforcePersonalOwnership && !this.vaultFilterService.vaultFilter.myVaultOnly) { 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 e5938b5197..63bf3d5e4c 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 @@ -36,6 +36,8 @@ describe("vault filter service", () => { let organizations: ReplaySubject; let folderViews: ReplaySubject; let collectionViews: ReplaySubject; + let personalOwnershipPolicy: ReplaySubject; + let singleOrgPolicy: ReplaySubject; let stateProvider: FakeStateProvider; const mockUserId = Utils.newGuid() as UserId; @@ -56,10 +58,18 @@ describe("vault filter service", () => { organizations = new ReplaySubject(1); folderViews = new ReplaySubject(1); collectionViews = new ReplaySubject(1); + personalOwnershipPolicy = new ReplaySubject(1); + singleOrgPolicy = new ReplaySubject(1); organizationService.memberOrganizations$ = organizations; folderService.folderViews$ = folderViews; collectionService.decryptedCollections$ = collectionViews; + policyService.policyAppliesToActiveUser$ + .calledWith(PolicyType.PersonalOwnership) + .mockReturnValue(personalOwnershipPolicy); + policyService.policyAppliesToActiveUser$ + .calledWith(PolicyType.SingleOrg) + .mockReturnValue(singleOrgPolicy); vaultFilterService = new VaultFilterService( organizationService, @@ -100,6 +110,8 @@ describe("vault filter service", () => { beforeEach(() => { const storedOrgs = [createOrganization("1", "org1"), createOrganization("2", "org2")]; organizations.next(storedOrgs); + personalOwnershipPolicy.next(false); + singleOrgPolicy.next(false); }); it("returns a nested tree", async () => { @@ -111,9 +123,7 @@ describe("vault filter service", () => { }); it("hides My Vault if personal ownership policy is enabled", async () => { - policyService.policyAppliesToUser - .calledWith(PolicyType.PersonalOwnership) - .mockResolvedValue(true); + personalOwnershipPolicy.next(true); const tree = await firstValueFrom(vaultFilterService.organizationTree$); @@ -122,7 +132,7 @@ describe("vault filter service", () => { }); it("returns 1 organization and My Vault if single organization policy is enabled", async () => { - policyService.policyAppliesToUser.calledWith(PolicyType.SingleOrg).mockResolvedValue(true); + singleOrgPolicy.next(true); const tree = await firstValueFrom(vaultFilterService.organizationTree$); @@ -132,10 +142,8 @@ describe("vault filter service", () => { }); it("returns 1 organization if both single organization and personal ownership policies are enabled", async () => { - policyService.policyAppliesToUser.calledWith(PolicyType.SingleOrg).mockResolvedValue(true); - policyService.policyAppliesToUser - .calledWith(PolicyType.PersonalOwnership) - .mockResolvedValue(true); + singleOrgPolicy.next(true); + personalOwnershipPolicy.next(true); const tree = await firstValueFrom(vaultFilterService.organizationTree$); 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 36cde762a0..ac20f86d0e 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,6 +1,7 @@ import { Injectable } from "@angular/core"; import { BehaviorSubject, + combineLatest, combineLatestWith, firstValueFrom, map, @@ -39,10 +40,15 @@ const NestingDelimiter = "/"; @Injectable() export class VaultFilterService implements VaultFilterServiceAbstraction { - organizationTree$: Observable> = - this.organizationService.memberOrganizations$.pipe( - switchMap((orgs) => this.buildOrganizationTree(orgs)), - ); + organizationTree$: Observable> = combineLatest([ + this.organizationService.memberOrganizations$, + this.policyService.policyAppliesToActiveUser$(PolicyType.SingleOrg), + this.policyService.policyAppliesToActiveUser$(PolicyType.PersonalOwnership), + ]).pipe( + switchMap(([orgs, singleOrgPolicy, personalOwnershipPolicy]) => + this.buildOrganizationTree(orgs, singleOrgPolicy, personalOwnershipPolicy), + ), + ); protected _organizationFilter = new BehaviorSubject(null); @@ -125,14 +131,16 @@ export class VaultFilterService implements VaultFilterServiceAbstraction { } protected async buildOrganizationTree( - orgs?: Organization[], + orgs: Organization[], + singleOrgPolicy: boolean, + personalOwnershipPolicy: boolean, ): Promise> { const headNode = this.getOrganizationFilterHead(); - if (!(await this.policyService.policyAppliesToUser(PolicyType.PersonalOwnership))) { + if (!personalOwnershipPolicy) { const myVaultNode = this.getOrganizationFilterMyVault(); headNode.children.push(myVaultNode); } - if (await this.policyService.policyAppliesToUser(PolicyType.SingleOrg)) { + if (singleOrgPolicy) { orgs = orgs.slice(0, 1); } if (orgs) { 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 469fad05a8..77fd63a65f 100644 --- a/apps/web/src/app/vault/individual-vault/vault.component.ts +++ b/apps/web/src/app/vault/individual-vault/vault.component.ts @@ -586,18 +586,24 @@ export class VaultComponent implements OnInit, OnDestroy { async addCipher(cipherType?: CipherType) { const component = await this.editCipher(null); component.type = cipherType || this.activeFilter.cipherType; - if (this.activeFilter.organizationId !== "MyVault") { + if ( + this.activeFilter.organizationId !== "MyVault" && + this.activeFilter.organizationId != null + ) { component.organizationId = this.activeFilter.organizationId; component.collections = ( await firstValueFrom(this.vaultFilterService.filteredCollections$) ).filter((c) => !c.readOnly && c.id != null); } const selectedColId = this.activeFilter.collectionId; - if (selectedColId !== "AllCollections") { - component.organizationId = component.collections.find( - (collection) => collection.id === selectedColId, - )?.organizationId; - component.collectionIds = [selectedColId]; + if (selectedColId !== "AllCollections" && selectedColId != null) { + const selectedCollection = ( + await firstValueFrom(this.vaultFilterService.filteredCollections$) + ).find((c) => c.id === selectedColId); + component.organizationId = selectedCollection?.organizationId; + if (!selectedCollection.readOnly) { + component.collectionIds = [selectedColId]; + } } component.folderId = this.activeFilter.folderId; }