From 14c98a45598faf8d9d5a5bd24df991fb73a8f019 Mon Sep 17 00:00:00 2001 From: Brandon Date: Thu, 23 Jan 2025 10:53:53 -0500 Subject: [PATCH 1/4] refactor PolicyService.getAll$ to make userId not optional --- .../services/free-families-policy.service.ts | 19 ++++++++++++------- .../settings/sponsored-families.component.ts | 2 +- .../settings/sponsoring-org-row.component.ts | 7 ++++++- .../src/tools/send/add-edit.component.ts | 5 ++++- .../policy/policy.service.abstraction.ts | 2 +- .../services/policy/policy.service.ts | 2 +- .../options/send-options.component.ts | 15 +++++++++++---- 7 files changed, 36 insertions(+), 16 deletions(-) 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 c148bd007be..c845b9924a9 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,14 +111,18 @@ export class FreeFamiliesPolicyService { }); } - return this.policyService.getAll$(PolicyType.FreeFamiliesSponsorshipPolicy).pipe( - map((policies) => ({ - isFreeFamilyPolicyEnabled: policies.some( - (policy) => policy.organizationId === organizationId && policy.enabled, + return getUserId(this.accountService.activeAccount$).pipe( + switchMap((userId) => + this.policyService.getAll$(PolicyType.FreeFamiliesSponsorshipPolicy, userId).pipe( + map((policies) => ({ + isFreeFamilyPolicyEnabled: policies.some( + (policy) => policy.organizationId === organizationId && policy.enabled, + ), + belongToOneEnterpriseOrgs, + belongToMultipleEnterpriseOrgs, + })), ), - belongToOneEnterpriseOrgs, - belongToMultipleEnterpriseOrgs, - })), + ), ); } 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 b9f70dd3085..d4a93ba7cd8 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 b40902112c8..de07232b6d5 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 @@ -8,6 +8,8 @@ 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() { @@ -53,9 +56,11 @@ export class SponsoringOrgRowComponent implements OnInit { FeatureFlag.DisableFreeFamiliesSponsorship, ); + const userId = await firstValueFrom(getUserId(this.accountService.activeAccount$)); + if (this.isFreeFamilyFlagEnabled) { this.isFreeFamilyPolicyEnabled$ = this.policyService - .getAll$(PolicyType.FreeFamiliesSponsorshipPolicy) + .getAll$(PolicyType.FreeFamiliesSponsorshipPolicy, userId) .pipe( map( (policies) => diff --git a/libs/angular/src/tools/send/add-edit.component.ts b/libs/angular/src/tools/send/add-edit.component.ts index aeee1fa104c..3ae40781949 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,8 +165,10 @@ export class AddEditComponent implements OnInit, OnDestroy { } }); + const userId = await firstValueFrom(getUserId(this.accountService.activeAccount$)); + this.policyService - .getAll$(PolicyType.SendOptions) + .getAll$(PolicyType.SendOptions, userId) .pipe( 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 bed341115ee..4280756326c 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.ts b/libs/common/src/admin-console/services/policy/policy.service.ts index bf99b9ce721..3378d2021ef 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 636b7546af8..fa124bc60dd 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,13 +91,18 @@ 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) + + getUserId(this.accountService.activeAccount$) .pipe( - map((policies) => policies?.some((p) => p.data.disableHideEmail)), takeUntilDestroyed(), + switchMap((userId) => + this.policyService + .getAll$(PolicyType.SendOptions, userId) + .pipe(map((policies) => policies?.some((p) => p.data.disableHideEmail))), + ), ) .subscribe((disableHideEmail) => { this.disableHideEmail = disableHideEmail; From 0390f407c1ea99faf0139d4cd919bf8fdb2a2374 Mon Sep 17 00:00:00 2001 From: Brandon Date: Thu, 23 Jan 2025 11:10:15 -0500 Subject: [PATCH 2/4] add fix to browser --- apps/browser/src/services/families-policy.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/browser/src/services/families-policy.service.ts b/apps/browser/src/services/families-policy.service.ts index 887e8836953..755d3e84591 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) => From 0b3bb89996e3920fbcbec60fe38a2c0f56ffe6d4 Mon Sep 17 00:00:00 2001 From: Brandon Date: Thu, 23 Jan 2025 13:52:21 -0500 Subject: [PATCH 3/4] fix test to read from mock singleUserState --- .../services/policy/policy.service.spec.ts | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) 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 f0ebfddf66e..12b57f1b4f7 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([ From 906a280620ed1931b3393eed7f87b7fedacecd11 Mon Sep 17 00:00:00 2001 From: Brandon Date: Thu, 23 Jan 2025 21:22:51 -0500 Subject: [PATCH 4/4] remove nested pipes, cleanup --- .../services/free-families-policy.service.ts | 20 ++++++------- .../settings/sponsoring-org-row.component.ts | 28 +++++++++---------- .../src/tools/send/add-edit.component.ts | 7 ++--- .../options/send-options.component.ts | 10 +++---- 4 files changed, 31 insertions(+), 34 deletions(-) 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 c845b9924a9..b07ccefdbab 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 @@ -111,18 +111,18 @@ export class FreeFamiliesPolicyService { }); } - return getUserId(this.accountService.activeAccount$).pipe( + return this.accountService.activeAccount$.pipe( + getUserId, switchMap((userId) => - this.policyService.getAll$(PolicyType.FreeFamiliesSponsorshipPolicy, userId).pipe( - map((policies) => ({ - isFreeFamilyPolicyEnabled: policies.some( - (policy) => policy.organizationId === organizationId && policy.enabled, - ), - belongToOneEnterpriseOrgs, - belongToMultipleEnterpriseOrgs, - })), - ), + this.policyService.getAll$(PolicyType.FreeFamiliesSponsorshipPolicy, userId), ), + map((policies) => ({ + isFreeFamilyPolicyEnabled: policies.some( + (policy) => policy.organizationId === organizationId && policy.enabled, + ), + belongToOneEnterpriseOrgs, + belongToMultipleEnterpriseOrgs, + })), ); } 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 de07232b6d5..6e9e00b0ee1 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,7 +2,7 @@ // @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"; @@ -56,20 +56,20 @@ export class SponsoringOrgRowComponent implements OnInit { FeatureFlag.DisableFreeFamiliesSponsorship, ); - const userId = await firstValueFrom(getUserId(this.accountService.activeAccount$)); - if (this.isFreeFamilyFlagEnabled) { - this.isFreeFamilyPolicyEnabled$ = this.policyService - .getAll$(PolicyType.FreeFamiliesSponsorshipPolicy, userId) - .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 3ae40781949..4f7d4b6b600 100644 --- a/libs/angular/src/tools/send/add-edit.component.ts +++ b/libs/angular/src/tools/send/add-edit.component.ts @@ -165,11 +165,10 @@ export class AddEditComponent implements OnInit, OnDestroy { } }); - const userId = await firstValueFrom(getUserId(this.accountService.activeAccount$)); - - this.policyService - .getAll$(PolicyType.SendOptions, userId) + 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/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 fa124bc60dd..22f1deb2fac 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 @@ -95,14 +95,12 @@ export class SendOptionsComponent implements OnInit { ) { this.sendFormContainer.registerChildForm("sendOptionsForm", this.sendOptionsForm); - getUserId(this.accountService.activeAccount$) + this.accountService.activeAccount$ .pipe( + getUserId, + switchMap((userId) => this.policyService.getAll$(PolicyType.SendOptions, userId)), + map((policies) => policies?.some((p) => p.data.disableHideEmail)), takeUntilDestroyed(), - switchMap((userId) => - this.policyService - .getAll$(PolicyType.SendOptions, userId) - .pipe(map((policies) => policies?.some((p) => p.data.disableHideEmail))), - ), ) .subscribe((disableHideEmail) => { this.disableHideEmail = disableHideEmail;