From b23a41ac860b87eb346bf60d503d790b34b5cf1e Mon Sep 17 00:00:00 2001 From: Brandon Treston Date: Fri, 24 Jan 2025 09:58:38 -0500 Subject: [PATCH] [PM-17465] refactor PolicyService.getAll$ to make userId not optional (#13031) * refactor PolicyService.getAll$ to make userId not optional * add fix to browser * fix test to read from mock singleUserState * remove nested pipes, cleanup --- .../src/services/families-policy.service.ts | 2 +- .../services/free-families-policy.service.ts | 7 ++++- .../settings/sponsored-families.component.ts | 2 +- .../settings/sponsoring-org-row.component.ts | 29 +++++++++++-------- .../src/tools/send/add-edit.component.ts | 6 ++-- .../policy/policy.service.abstraction.ts | 2 +- .../services/policy/policy.service.spec.ts | 20 +++++++------ .../services/policy/policy.service.ts | 2 +- .../options/send-options.component.ts | 11 +++++-- 9 files changed, 50 insertions(+), 31 deletions(-) diff --git a/apps/browser/src/services/families-policy.service.ts b/apps/browser/src/services/families-policy.service.ts index 887e883695..755d3e8459 100644 --- a/apps/browser/src/services/families-policy.service.ts +++ b/apps/browser/src/services/families-policy.service.ts @@ -47,7 +47,7 @@ export class FamiliesPolicyService { map((organizations) => organizations.find((org) => org.canManageSponsorships)?.id), switchMap((enterpriseOrgId) => this.policyService - .getAll$(PolicyType.FreeFamiliesSponsorshipPolicy) + .getAll$(PolicyType.FreeFamiliesSponsorshipPolicy, userId) .pipe( map( (policies) => diff --git a/apps/web/src/app/billing/services/free-families-policy.service.ts b/apps/web/src/app/billing/services/free-families-policy.service.ts index c148bd007b..b07ccefdba 100644 --- a/apps/web/src/app/billing/services/free-families-policy.service.ts +++ b/apps/web/src/app/billing/services/free-families-policy.service.ts @@ -6,6 +6,7 @@ import { PolicyService } from "@bitwarden/common/admin-console/abstractions/poli import { PolicyType } from "@bitwarden/common/admin-console/enums"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { getUserId } from "@bitwarden/common/auth/services/account.service"; import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; @@ -110,7 +111,11 @@ export class FreeFamiliesPolicyService { }); } - return this.policyService.getAll$(PolicyType.FreeFamiliesSponsorshipPolicy).pipe( + return this.accountService.activeAccount$.pipe( + getUserId, + switchMap((userId) => + this.policyService.getAll$(PolicyType.FreeFamiliesSponsorshipPolicy, userId), + ), map((policies) => ({ isFreeFamilyPolicyEnabled: policies.some( (policy) => policy.organizationId === organizationId && policy.enabled, diff --git a/apps/web/src/app/billing/settings/sponsored-families.component.ts b/apps/web/src/app/billing/settings/sponsored-families.component.ts index b9f70dd308..d4a93ba7cd 100644 --- a/apps/web/src/app/billing/settings/sponsored-families.component.ts +++ b/apps/web/src/app/billing/settings/sponsored-families.component.ts @@ -98,7 +98,7 @@ export class SponsoredFamiliesComponent implements OnInit, OnDestroy { this.availableSponsorshipOrgs$ = combineLatest([ this.organizationService.organizations$(userId), - this.policyService.getAll$(PolicyType.FreeFamiliesSponsorshipPolicy), + this.policyService.getAll$(PolicyType.FreeFamiliesSponsorshipPolicy, userId), ]).pipe( map(([organizations, policies]) => organizations diff --git a/apps/web/src/app/billing/settings/sponsoring-org-row.component.ts b/apps/web/src/app/billing/settings/sponsoring-org-row.component.ts index b40902112c..6e9e00b0ee 100644 --- a/apps/web/src/app/billing/settings/sponsoring-org-row.component.ts +++ b/apps/web/src/app/billing/settings/sponsoring-org-row.component.ts @@ -2,12 +2,14 @@ // @ts-strict-ignore import { formatDate } from "@angular/common"; import { Component, EventEmitter, Input, Output, OnInit } from "@angular/core"; -import { firstValueFrom, map, Observable } from "rxjs"; +import { firstValueFrom, map, Observable, switchMap } from "rxjs"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; 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 { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { getUserId } from "@bitwarden/common/auth/services/account.service"; import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; @@ -38,6 +40,7 @@ export class SponsoringOrgRowComponent implements OnInit { private toastService: ToastService, private configService: ConfigService, private policyService: PolicyService, + private accountService: AccountService, ) {} async ngOnInit() { @@ -54,17 +57,19 @@ export class SponsoringOrgRowComponent implements OnInit { ); if (this.isFreeFamilyFlagEnabled) { - this.isFreeFamilyPolicyEnabled$ = this.policyService - .getAll$(PolicyType.FreeFamiliesSponsorshipPolicy) - .pipe( - map( - (policies) => - Array.isArray(policies) && - policies.some( - (policy) => policy.organizationId === this.sponsoringOrg.id && policy.enabled, - ), - ), - ); + this.isFreeFamilyPolicyEnabled$ = this.accountService.activeAccount$.pipe( + getUserId, + switchMap((userId) => + this.policyService.getAll$(PolicyType.FreeFamiliesSponsorshipPolicy, userId), + ), + map( + (policies) => + Array.isArray(policies) && + policies.some( + (policy) => policy.organizationId === this.sponsoringOrg.id && policy.enabled, + ), + ), + ); } } diff --git a/libs/angular/src/tools/send/add-edit.component.ts b/libs/angular/src/tools/send/add-edit.component.ts index aeee1fa104..4f7d4b6b60 100644 --- a/libs/angular/src/tools/send/add-edit.component.ts +++ b/libs/angular/src/tools/send/add-edit.component.ts @@ -16,6 +16,7 @@ import { import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { PolicyType } from "@bitwarden/common/admin-console/enums"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { getUserId } from "@bitwarden/common/auth/services/account.service"; import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service"; import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; @@ -164,9 +165,10 @@ export class AddEditComponent implements OnInit, OnDestroy { } }); - this.policyService - .getAll$(PolicyType.SendOptions) + this.accountService.activeAccount$ .pipe( + getUserId, + switchMap((userId) => this.policyService.getAll$(PolicyType.SendOptions, userId)), map((policies) => policies?.some((p) => p.data.disableHideEmail)), takeUntil(this.destroy$), ) diff --git a/libs/common/src/admin-console/abstractions/policy/policy.service.abstraction.ts b/libs/common/src/admin-console/abstractions/policy/policy.service.abstraction.ts index bed341115e..4280756326 100644 --- a/libs/common/src/admin-console/abstractions/policy/policy.service.abstraction.ts +++ b/libs/common/src/admin-console/abstractions/policy/policy.service.abstraction.ts @@ -30,7 +30,7 @@ export abstract class PolicyService { * A policy "applies" if it is enabled and the user is not exempt (e.g. because they are an Owner). * @param policyType the {@link PolicyType} to search for */ - getAll$: (policyType: PolicyType, userId?: UserId) => Observable; + getAll$: (policyType: PolicyType, userId: UserId) => Observable; /** * All {@link Policy} objects for the specified user (from sync data). diff --git a/libs/common/src/admin-console/services/policy/policy.service.spec.ts b/libs/common/src/admin-console/services/policy/policy.service.spec.ts index f0ebfddf66..12b57f1b4f 100644 --- a/libs/common/src/admin-console/services/policy/policy.service.spec.ts +++ b/libs/common/src/admin-console/services/policy/policy.service.spec.ts @@ -2,7 +2,7 @@ import { mock, MockProxy } from "jest-mock-extended"; import { firstValueFrom, of } from "rxjs"; import { FakeStateProvider, mockAccountServiceWith } from "../../../../spec"; -import { FakeActiveUserState } from "../../../../spec/fake-state"; +import { FakeActiveUserState, FakeSingleUserState } from "../../../../spec/fake-state"; import { OrganizationUserStatusType, OrganizationUserType, @@ -24,6 +24,7 @@ describe("PolicyService", () => { let stateProvider: FakeStateProvider; let organizationService: MockProxy; let activeUserState: FakeActiveUserState>; + let singleUserState: FakeSingleUserState>; let policyService: PolicyService; @@ -33,6 +34,7 @@ describe("PolicyService", () => { organizationService = mock(); activeUserState = stateProvider.activeUser.getFake(POLICIES); + singleUserState = stateProvider.singleUser.getFake(activeUserState.userId, POLICIES); const organizations$ = of([ // User @@ -295,7 +297,7 @@ describe("PolicyService", () => { describe("getAll$", () => { it("returns the specified PolicyTypes", async () => { - activeUserState.nextState( + singleUserState.nextState( arrayToRecord([ policyData("policy1", "org4", PolicyType.DisablePersonalVaultExport, true), policyData("policy2", "org1", PolicyType.ActivateAutofill, true), @@ -305,7 +307,7 @@ describe("PolicyService", () => { ); const result = await firstValueFrom( - policyService.getAll$(PolicyType.DisablePersonalVaultExport), + policyService.getAll$(PolicyType.DisablePersonalVaultExport, activeUserState.userId), ); expect(result).toEqual([ @@ -331,7 +333,7 @@ describe("PolicyService", () => { }); it("does not return disabled policies", async () => { - activeUserState.nextState( + singleUserState.nextState( arrayToRecord([ policyData("policy1", "org4", PolicyType.DisablePersonalVaultExport, true), policyData("policy2", "org1", PolicyType.ActivateAutofill, true), @@ -341,7 +343,7 @@ describe("PolicyService", () => { ); const result = await firstValueFrom( - policyService.getAll$(PolicyType.DisablePersonalVaultExport), + policyService.getAll$(PolicyType.DisablePersonalVaultExport, activeUserState.userId), ); expect(result).toEqual([ @@ -361,7 +363,7 @@ describe("PolicyService", () => { }); it("does not return policies that do not apply to the user because the user's role is exempt", async () => { - activeUserState.nextState( + singleUserState.nextState( arrayToRecord([ policyData("policy1", "org4", PolicyType.DisablePersonalVaultExport, true), policyData("policy2", "org1", PolicyType.ActivateAutofill, true), @@ -371,7 +373,7 @@ describe("PolicyService", () => { ); const result = await firstValueFrom( - policyService.getAll$(PolicyType.DisablePersonalVaultExport), + policyService.getAll$(PolicyType.DisablePersonalVaultExport, activeUserState.userId), ); expect(result).toEqual([ @@ -391,7 +393,7 @@ describe("PolicyService", () => { }); it("does not return policies for organizations that do not use policies", async () => { - activeUserState.nextState( + singleUserState.nextState( arrayToRecord([ policyData("policy1", "org4", PolicyType.DisablePersonalVaultExport, true), policyData("policy2", "org1", PolicyType.ActivateAutofill, true), @@ -401,7 +403,7 @@ describe("PolicyService", () => { ); const result = await firstValueFrom( - policyService.getAll$(PolicyType.DisablePersonalVaultExport), + policyService.getAll$(PolicyType.DisablePersonalVaultExport, activeUserState.userId), ); expect(result).toEqual([ diff --git a/libs/common/src/admin-console/services/policy/policy.service.ts b/libs/common/src/admin-console/services/policy/policy.service.ts index bf99b9ce72..3378d2021e 100644 --- a/libs/common/src/admin-console/services/policy/policy.service.ts +++ b/libs/common/src/admin-console/services/policy/policy.service.ts @@ -51,7 +51,7 @@ export class PolicyService implements InternalPolicyServiceAbstraction { ); } - getAll$(policyType: PolicyType, userId?: UserId) { + getAll$(policyType: PolicyType, userId: UserId) { const filteredPolicies$ = this.stateProvider.getUserState$(POLICIES, userId).pipe( map((policyData) => policyRecordToArray(policyData)), map((policies) => policies.filter((p) => p.type === policyType)), diff --git a/libs/tools/send/send-ui/src/send-form/components/options/send-options.component.ts b/libs/tools/send/send-ui/src/send-form/components/options/send-options.component.ts index 636b7546af..22f1deb2fa 100644 --- a/libs/tools/send/send-ui/src/send-form/components/options/send-options.component.ts +++ b/libs/tools/send/send-ui/src/send-form/components/options/send-options.component.ts @@ -4,11 +4,13 @@ import { CommonModule } from "@angular/common"; import { Component, Input, OnInit } from "@angular/core"; import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; import { FormBuilder, ReactiveFormsModule } from "@angular/forms"; -import { firstValueFrom, map } from "rxjs"; +import { firstValueFrom, map, switchMap } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { PolicyType } from "@bitwarden/common/admin-console/enums"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { getUserId } from "@bitwarden/common/auth/services/account.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { SendView } from "@bitwarden/common/tools/send/models/view/send.view"; import { SendApiService } from "@bitwarden/common/tools/send/services/send-api.service.abstraction"; @@ -89,11 +91,14 @@ export class SendOptionsComponent implements OnInit { private i18nService: I18nService, private toastService: ToastService, private generatorService: CredentialGeneratorService, + private accountService: AccountService, ) { this.sendFormContainer.registerChildForm("sendOptionsForm", this.sendOptionsForm); - this.policyService - .getAll$(PolicyType.SendOptions) + + this.accountService.activeAccount$ .pipe( + getUserId, + switchMap((userId) => this.policyService.getAll$(PolicyType.SendOptions, userId)), map((policies) => policies?.some((p) => p.data.disableHideEmail)), takeUntilDestroyed(), )