Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-14366] Deprecated active user state from billing state service #12273

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
79f0f64
Updated billing state provider to not rely on ActiveUserStateProvider
cturnbull-bitwarden Dec 4, 2024
056fbfe
Updated usages
cturnbull-bitwarden Dec 4, 2024
e943dc9
Merge branch 'main' into billing/PM-14366/deprectete-active-user-statโ€ฆ
cturnbull-bitwarden Dec 6, 2024
79e02e1
Resolved browser build
cturnbull-bitwarden Dec 6, 2024
5f5d38e
Resolved web build
cturnbull-bitwarden Dec 6, 2024
4643a64
Resolved CLI build
cturnbull-bitwarden Dec 6, 2024
e9d9ec9
resolved desktop build
cturnbull-bitwarden Dec 6, 2024
8232a72
Merge branch 'main' into billing/PM-14366/deprectete-active-user-statโ€ฆ
cturnbull-bitwarden Dec 6, 2024
55e6f24
Update apps/cli/src/tools/send/commands/create.command.ts
cturnbull-bitwarden Dec 10, 2024
39c8a0d
Merge branch 'main' into billing/PM-14366/deprectete-active-user-statโ€ฆ
cturnbull-bitwarden Dec 16, 2024
f9f4ceb
Move subscription visibility logic from component to service
cturnbull-bitwarden Dec 16, 2024
ad0c18e
Resolved unit test failures. Using existing userIds where present
cturnbull-bitwarden Dec 16, 2024
bc9dc37
Merge branch 'main' into billing/PM-14366/deprectete-active-user-statโ€ฆ
cturnbull-bitwarden Dec 16, 2024
62662e3
Simplified activeUserId access
cturnbull-bitwarden Dec 16, 2024
85af1e6
Merge branch 'main' into billing/PM-14366/deprectete-active-user-statโ€ฆ
cturnbull-bitwarden Dec 17, 2024
a9d072c
Resolved typescript strict errors
cturnbull-bitwarden Dec 17, 2024
5f03ba7
Merge branch 'main' into billing/PM-14366/deprectete-active-user-statโ€ฆ
cturnbull-bitwarden Dec 18, 2024
6a69690
Merge branch 'main' into billing/PM-14366/deprectete-active-user-statโ€ฆ
cturnbull-bitwarden Dec 18, 2024
d6c1494
Merge branch 'main' into billing/PM-14366/deprectete-active-user-statโ€ฆ
cturnbull-bitwarden Dec 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import { mock, MockProxy } from "jest-mock-extended";
import { of } from "rxjs";

import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { NOOP_COMMAND_SUFFIX } from "@bitwarden/common/autofill/constants";
import { AutofillSettingsServiceAbstraction } from "@bitwarden/common/autofill/services/autofill-settings.service";
import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
import { UserId } from "@bitwarden/common/types/guid";
import { CipherType } from "@bitwarden/common/vault/enums";
import { Cipher } from "@bitwarden/common/vault/models/domain/cipher";
import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view";
Expand All @@ -19,6 +21,7 @@ describe("context-menu", () => {
let i18nService: MockProxy<I18nService>;
let logService: MockProxy<LogService>;
let billingAccountProfileStateService: MockProxy<BillingAccountProfileStateService>;
let accountService: MockProxy<AccountService>;

let removeAllSpy: jest.SpyInstance<void, [callback?: () => void]>;
let createSpy: jest.SpyInstance<
Expand All @@ -34,6 +37,7 @@ describe("context-menu", () => {
i18nService = mock();
logService = mock();
billingAccountProfileStateService = mock();
accountService = mock();

removeAllSpy = jest
.spyOn(chrome.contextMenus, "removeAll")
Expand All @@ -53,8 +57,15 @@ describe("context-menu", () => {
i18nService,
logService,
billingAccountProfileStateService,
accountService,
);
autofillSettingsService.enableContextMenu$ = of(true);
accountService.activeAccount$ = of({
id: "userId" as UserId,
email: "",
emailVerified: false,
name: undefined,
});
});

afterEach(() => jest.resetAllMocks());
Expand All @@ -69,15 +80,15 @@ describe("context-menu", () => {
});

it("has menu enabled, but does not have premium", async () => {
billingAccountProfileStateService.hasPremiumFromAnySource$ = of(false);
billingAccountProfileStateService.hasPremiumFromAnySource$.mockReturnValue(of(false));

const createdMenu = await sut.init();
expect(createdMenu).toBeTruthy();
expect(createSpy).toHaveBeenCalledTimes(10);
});

it("has menu enabled and has premium", async () => {
billingAccountProfileStateService.hasPremiumFromAnySource$ = of(true);
billingAccountProfileStateService.hasPremiumFromAnySource$.mockReturnValue(of(true));

const createdMenu = await sut.init();
expect(createdMenu).toBeTruthy();
Expand Down Expand Up @@ -131,16 +142,15 @@ describe("context-menu", () => {
});

it("create entry for each cipher piece", async () => {
billingAccountProfileStateService.hasPremiumFromAnySource$ = of(true);
billingAccountProfileStateService.hasPremiumFromAnySource$.mockReturnValue(of(true));

await sut.loadOptions("TEST_TITLE", "1", createCipher());

// One for autofill, copy username, copy password, and copy totp code
expect(createSpy).toHaveBeenCalledTimes(4);
});

it("creates a login/unlock item for each context menu action option when user is not authenticated", async () => {
billingAccountProfileStateService.hasPremiumFromAnySource$ = of(true);
billingAccountProfileStateService.hasPremiumFromAnySource$.mockReturnValue(of(true));

await sut.loadOptions("TEST_TITLE", "NOOP");

Expand Down
15 changes: 10 additions & 5 deletions apps/browser/src/autofill/browser/main-context-menu-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// @ts-strict-ignore
import { firstValueFrom } from "rxjs";

import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import {
AUTOFILL_CARD_ID,
AUTOFILL_ID,
Expand Down Expand Up @@ -149,6 +150,7 @@ export class MainContextMenuHandler {
private i18nService: I18nService,
private logService: LogService,
private billingAccountProfileStateService: BillingAccountProfileStateService,
private accountService: AccountService,
) {}

/**
Expand All @@ -168,11 +170,13 @@ export class MainContextMenuHandler {
this.initRunning = true;

try {
const account = await firstValueFrom(this.accountService.activeAccount$);
const hasPremium = await firstValueFrom(
this.billingAccountProfileStateService.hasPremiumFromAnySource$(account.id),
);
Comment on lines +174 to +176
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oo, moving this out of the loop was a good idea; thanks ๐Ÿ‘


for (const options of this.initContextMenuItems) {
if (
options.checkPremiumAccess &&
!(await firstValueFrom(this.billingAccountProfileStateService.hasPremiumFromAnySource$))
) {
if (options.checkPremiumAccess && !hasPremium) {
continue;
}

Expand Down Expand Up @@ -267,8 +271,9 @@ export class MainContextMenuHandler {
await createChildItem(COPY_USERNAME_ID);
}

const account = await firstValueFrom(this.accountService.activeAccount$);
const canAccessPremium = await firstValueFrom(
this.billingAccountProfileStateService.hasPremiumFromAnySource$,
this.billingAccountProfileStateService.hasPremiumFromAnySource$(account.id),
);
if (canAccessPremium && (!cipher || !Utils.isNullOrEmpty(cipher.login?.totp))) {
await createChildItem(COPY_VERIFICATION_CODE_ID);
Expand Down
22 changes: 16 additions & 6 deletions apps/browser/src/autofill/services/autofill.service.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { mock, mockReset, MockProxy } from "jest-mock-extended";
import { mock, MockProxy, mockReset } from "jest-mock-extended";
import { BehaviorSubject, of, Subject } from "rxjs";

import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
Expand Down Expand Up @@ -723,7 +723,9 @@ describe("AutofillService", () => {

it("throws an error if an autofill did not occur for any of the passed pages", async () => {
autofillOptions.tab.url = "https://a-different-url.com";
billingAccountProfileStateService.hasPremiumFromAnySource$ = of(true);
jest
.spyOn(billingAccountProfileStateService, "hasPremiumFromAnySource$")
.mockImplementation(() => of(true));

try {
await autofillService.doAutoFill(autofillOptions);
Expand Down Expand Up @@ -905,7 +907,9 @@ describe("AutofillService", () => {
it("returns a TOTP value", async () => {
const totpCode = "123456";
autofillOptions.cipher.login.totp = "totp";
billingAccountProfileStateService.hasPremiumFromAnySource$ = of(true);
jest
.spyOn(billingAccountProfileStateService, "hasPremiumFromAnySource$")
.mockImplementation(() => of(true));
jest.spyOn(autofillService, "getShouldAutoCopyTotp").mockResolvedValue(true);
jest.spyOn(totpService, "getCode").mockResolvedValue(totpCode);

Expand All @@ -918,7 +922,9 @@ describe("AutofillService", () => {

it("does not return a TOTP value if the user does not have premium features", async () => {
autofillOptions.cipher.login.totp = "totp";
billingAccountProfileStateService.hasPremiumFromAnySource$ = of(false);
jest
.spyOn(billingAccountProfileStateService, "hasPremiumFromAnySource$")
.mockImplementation(() => of(false));
jest.spyOn(autofillService, "getShouldAutoCopyTotp").mockResolvedValue(true);

const autofillResult = await autofillService.doAutoFill(autofillOptions);
Expand Down Expand Up @@ -952,7 +958,9 @@ describe("AutofillService", () => {
it("returns a null value if the user cannot access premium and the organization does not use TOTP", async () => {
autofillOptions.cipher.login.totp = "totp";
autofillOptions.cipher.organizationUseTotp = false;
billingAccountProfileStateService.hasPremiumFromAnySource$ = of(false);
jest
.spyOn(billingAccountProfileStateService, "hasPremiumFromAnySource$")
.mockImplementation(() => of(false));

const autofillResult = await autofillService.doAutoFill(autofillOptions);

Expand All @@ -962,7 +970,9 @@ describe("AutofillService", () => {
it("returns a null value if the user has disabled `auto TOTP copy`", async () => {
autofillOptions.cipher.login.totp = "totp";
autofillOptions.cipher.organizationUseTotp = true;
billingAccountProfileStateService.hasPremiumFromAnySource$ = of(true);
jest
.spyOn(billingAccountProfileStateService, "hasPremiumFromAnySource$")
.mockImplementation(() => of(true));
jest.spyOn(autofillService, "getShouldAutoCopyTotp").mockResolvedValue(false);
jest.spyOn(totpService, "getCode");

Expand Down
3 changes: 2 additions & 1 deletion apps/browser/src/autofill/services/autofill.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -416,8 +416,9 @@ export default class AutofillService implements AutofillServiceInterface {

let totp: string | null = null;

const activeAccount = await firstValueFrom(this.accountService.activeAccount$);
const canAccessPremium = await firstValueFrom(
this.billingAccountProfileStateService.hasPremiumFromAnySource$,
this.billingAccountProfileStateService.hasPremiumFromAnySource$(activeAccount.id),
);
const defaultUriMatch = await this.getDefaultUriMatchStrategy();

Expand Down
3 changes: 3 additions & 0 deletions apps/browser/src/background/main.background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -791,6 +791,8 @@ export default class MainBackground {

this.billingAccountProfileStateService = new DefaultBillingAccountProfileStateService(
this.stateProvider,
this.platformUtilsService,
this.apiService,
);

this.ssoLoginService = new SsoLoginService(this.stateProvider);
Expand Down Expand Up @@ -1221,6 +1223,7 @@ export default class MainBackground {
this.i18nService,
this.logService,
this.billingAccountProfileStateService,
this.accountService,
);

this.cipherContextMenuHandler = new CipherContextMenuHandler(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import { JslibModule } from "@bitwarden/angular/jslib.module";
import { PremiumComponent as BasePremiumComponent } from "@bitwarden/angular/vault/components/premium.component";
import { ApiService } from "@bitwarden/common/abstractions/api.service";
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";

Check warning on line 10 in apps/browser/src/billing/popup/settings/premium-v2.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/billing/popup/settings/premium-v2.component.ts#L10

Added line #L10 was not covered by tests
import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service";
Expand Down Expand Up @@ -58,6 +59,7 @@
dialogService: DialogService,
environmentService: EnvironmentService,
billingAccountProfileStateService: BillingAccountProfileStateService,
accountService: AccountService,
) {
super(
i18nService,
Expand All @@ -68,6 +70,7 @@
dialogService,
environmentService,
billingAccountProfileStateService,
accountService,
);

// Support old price string. Can be removed in future once all translations are properly updated.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import { PremiumComponent as BasePremiumComponent } from "@bitwarden/angular/vault/components/premium.component";
import { ApiService } from "@bitwarden/common/abstractions/api.service";
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";

Check warning on line 8 in apps/browser/src/billing/popup/settings/premium.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/billing/popup/settings/premium.component.ts#L8

Added line #L8 was not covered by tests
import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service";
Expand All @@ -31,6 +32,7 @@
dialogService: DialogService,
environmentService: EnvironmentService,
billingAccountProfileStateService: BillingAccountProfileStateService,
accountService: AccountService,
) {
super(
i18nService,
Expand All @@ -41,6 +43,7 @@
dialogService,
environmentService,
billingAccountProfileStateService,
accountService,
);

// Support old price string. Can be removed in future once all translations are properly updated.
Expand Down
12 changes: 11 additions & 1 deletion apps/browser/src/tools/popup/send-v2/send-v2.component.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,17 @@ describe("SendV2Component", () => {
CurrentAccountComponent,
],
providers: [
{ provide: AccountService, useValue: mock<AccountService>() },
{
provide: AccountService,
useValue: {
activeAccount$: of({
id: "123",
email: "[email protected]",
emailVerified: true,
name: "Test User",
}),
},
},
{ provide: AuthService, useValue: mock<AuthService>() },
{ provide: AvatarService, useValue: mock<AvatarService>() },
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { CommonModule } from "@angular/common";
import { Component } from "@angular/core";
import { RouterModule } from "@angular/router";
import { Observable, firstValueFrom } from "rxjs";
import { Observable, firstValueFrom, of, switchMap } from "rxjs";

Check warning on line 4 in apps/browser/src/tools/popup/settings/about-page/more-from-bitwarden-page-v2.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/tools/popup/settings/about-page/more-from-bitwarden-page-v2.component.ts#L4

Added line #L4 was not covered by tests

import { JslibModule } from "@bitwarden/angular/jslib.module";
import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";

Check warning on line 8 in apps/browser/src/tools/popup/settings/about-page/more-from-bitwarden-page-v2.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/tools/popup/settings/about-page/more-from-bitwarden-page-v2.component.ts#L8

Added line #L8 was not covered by tests
import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service";
import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service";
import { DialogService, ItemModule } from "@bitwarden/components";
Expand Down Expand Up @@ -36,12 +37,19 @@

constructor(
private dialogService: DialogService,
billingAccountProfileStateService: BillingAccountProfileStateService,
private billingAccountProfileStateService: BillingAccountProfileStateService,
private environmentService: EnvironmentService,
private organizationService: OrganizationService,
private familiesPolicyService: FamiliesPolicyService,
private accountService: AccountService,

Check warning on line 44 in apps/browser/src/tools/popup/settings/about-page/more-from-bitwarden-page-v2.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/tools/popup/settings/about-page/more-from-bitwarden-page-v2.component.ts#L44

Added line #L44 was not covered by tests
) {
this.canAccessPremium$ = billingAccountProfileStateService.hasPremiumFromAnySource$;
this.canAccessPremium$ = this.accountService.activeAccount$.pipe(

Check warning on line 46 in apps/browser/src/tools/popup/settings/about-page/more-from-bitwarden-page-v2.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/tools/popup/settings/about-page/more-from-bitwarden-page-v2.component.ts#L46

Added line #L46 was not covered by tests
switchMap((account) =>
account
? this.billingAccountProfileStateService.hasPremiumFromAnySource$(account.id)
: of(false),
),
);
this.familySponsorshipAvailable$ = this.organizationService.familySponsorshipAvailable$;
this.hasSingleEnterpriseOrg$ = this.familiesPolicyService.hasSingleEnterpriseOrg$();
this.isFreeFamilyPolicyEnabled$ = this.familiesPolicyService.isFreeFamilyPolicyEnabled$();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { Component, EventEmitter, Input, OnDestroy, OnInit, Output } from "@angular/core";
import { Subject, takeUntil } from "rxjs";
import { Subject, takeUntil, switchMap } from "rxjs";

Check warning on line 4 in apps/browser/src/vault/popup/components/action-buttons.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/components/action-buttons.component.ts#L4

Added line #L4 was not covered by tests

import { EventCollectionService } from "@bitwarden/common/abstractions/event/event-collection.service";
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";

Check warning on line 7 in apps/browser/src/vault/popup/components/action-buttons.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/components/action-buttons.component.ts#L7

Added line #L7 was not covered by tests
import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service";
import { EventType } from "@bitwarden/common/enums";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
Expand Down Expand Up @@ -36,11 +37,17 @@
private totpService: TotpServiceAbstraction,
private passwordRepromptService: PasswordRepromptService,
private billingAccountProfileStateService: BillingAccountProfileStateService,
private accountService: AccountService,

Check warning on line 40 in apps/browser/src/vault/popup/components/action-buttons.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/components/action-buttons.component.ts#L40

Added line #L40 was not covered by tests
) {}

ngOnInit() {
this.billingAccountProfileStateService.hasPremiumFromAnySource$
.pipe(takeUntil(this.componentIsDestroyed$))
this.accountService.activeAccount$

Check warning on line 44 in apps/browser/src/vault/popup/components/action-buttons.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/components/action-buttons.component.ts#L44

Added line #L44 was not covered by tests
.pipe(
switchMap((account) =>
this.billingAccountProfileStateService.hasPremiumFromAnySource$(account.id),

Check warning on line 47 in apps/browser/src/vault/popup/components/action-buttons.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/components/action-buttons.component.ts#L47

Added line #L47 was not covered by tests
),
takeUntil(this.componentIsDestroyed$),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non blocking โ›๏ธ this pattern for unsubscribing is outdated and we should favor using takeUntilDestroyed()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion! I created a tech debt task here for us to handle this in a follow-up PR

)
.subscribe((canAccessPremium: boolean) => {
this.userHasPremiumAccess = canAccessPremium;
});
Expand Down
Loading
Loading