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-12049] Remove usage of ActiveUserState from folder service #11880

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { mock, MockProxy } from "jest-mock-extended";
import { BehaviorSubject, firstValueFrom } from "rxjs";

import { PolicyService } from "@bitwarden/common/admin-console/services/policy/policy.service";
import { AccountInfo, AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { AccountInfo } from "@bitwarden/common/auth/abstractions/account.service";
import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
import { AuthService } from "@bitwarden/common/auth/services/auth.service";
import { ExtensionCommand } from "@bitwarden/common/autofill/constants";
Expand All @@ -11,8 +11,10 @@ import { UserNotificationSettingsService } from "@bitwarden/common/autofill/serv
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { Utils } from "@bitwarden/common/platform/misc/utils";
import { SelfHostedEnvironment } from "@bitwarden/common/platform/services/default-environment.service";
import { ThemeStateService } from "@bitwarden/common/platform/theming/theme-state.service";
import { FakeAccountService, mockAccountServiceWith } from "@bitwarden/common/spec";
import { UserId } from "@bitwarden/common/types/guid";
import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view";
import { FolderView } from "@bitwarden/common/vault/models/view/folder.view";
Expand Down Expand Up @@ -58,9 +60,12 @@ describe("NotificationBackground", () => {
const logService = mock<LogService>();
const themeStateService = mock<ThemeStateService>();
const configService = mock<ConfigService>();
const accountService = mock<AccountService>();
let accountService: FakeAccountService;

const userId = Utils.newGuid() as UserId;

beforeEach(() => {
accountService = mockAccountServiceWith(userId);
activeAccountStatusMock$ = new BehaviorSubject(AuthenticationStatus.Locked);
authService = mock<AuthService>();
authService.activeAccountStatus$ = activeAccountStatusMock$;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@
getWebVaultUrlForNotification: () => this.getWebVaultUrl(),
};

private activeUserId$ = this.accountService.activeAccount$.pipe(map((a) => a?.id));

constructor(
private autofillService: AutofillService,
private cipherService: CipherService,
Expand Down Expand Up @@ -609,11 +611,10 @@
return;
}

const activeUserId = await firstValueFrom(
this.accountService.activeAccount$.pipe(map((a) => a?.id)),
const cipher = await this.cipherService.encrypt(
cipherView,
await firstValueFrom(this.activeUserId$),
);

const cipher = await this.cipherService.encrypt(cipherView, activeUserId);
try {
// We've only updated the password, no need to broadcast editedCipher message
await this.cipherService.updateWithServer(cipher);
Expand Down Expand Up @@ -646,7 +647,7 @@
return false;
}

const folders = await firstValueFrom(this.folderService.folderViews$);
const folders = await firstValueFrom(this.folderService.folderViews$(this.activeUserId$));

Check warning on line 650 in apps/browser/src/autofill/background/notification.background.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/autofill/background/notification.background.ts#L650

Added line #L650 was not covered by tests
return folders.some((x) => x.id === folderId);
}

Expand Down Expand Up @@ -695,7 +696,7 @@
* Returns the first value found from the folder service's folderViews$ observable.
*/
private async getFolderData() {
return await firstValueFrom(this.folderService.folderViews$);
return await firstValueFrom(this.folderService.folderViews$(this.activeUserId$));
}

private async getWebVaultUrl(): Promise<string> {
Expand Down
6 changes: 5 additions & 1 deletion apps/browser/src/background/main.background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -833,7 +833,11 @@
this.cipherService,
this.stateProvider,
);
this.folderApiService = new FolderApiService(this.folderService, this.apiService);
this.folderApiService = new FolderApiService(

Check warning on line 836 in apps/browser/src/background/main.background.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/background/main.background.ts#L836

Added line #L836 was not covered by tests
this.folderService,
this.apiService,
this.accountService,
);

this.userVerificationService = new UserVerificationService(
this.keyService,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { Subject } from "rxjs";

import { CollectionService } from "@bitwarden/admin-console/common";
import { ApiService } from "@bitwarden/common/abstractions/api.service";
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
Expand All @@ -30,12 +29,12 @@ describe("ForegroundSyncService", () => {
const cipherService = mock<CipherService>();
const collectionService = mock<CollectionService>();
const apiService = mock<ApiService>();
const accountService = mock<AccountService>();
const accountService = mockAccountServiceWith(userId);
const authService = mock<AuthService>();
const sendService = mock<InternalSendService>();
const sendApiService = mock<SendApiService>();
const messageListener = mock<MessageListener>();
const stateProvider = new FakeStateProvider(mockAccountServiceWith(userId));
const stateProvider = new FakeStateProvider(accountService);

const sut = new ForegroundSyncService(
stateService,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,11 @@ import { OrganizationService } from "@bitwarden/common/admin-console/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 { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { ProductTierType } from "@bitwarden/common/billing/enums";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { mockAccountServiceWith } from "@bitwarden/common/spec";
import { UserId } from "@bitwarden/common/types/guid";
import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction";
import { CipherType } from "@bitwarden/common/vault/enums";
Expand All @@ -31,7 +34,7 @@ describe("VaultPopupListFiltersService", () => {
} as unknown as CollectionService;

const folderService = {
folderViews$,
folderViews$: () => folderViews$,
} as unknown as FolderService;

const cipherService = {
Expand All @@ -56,6 +59,8 @@ describe("VaultPopupListFiltersService", () => {
policyAppliesToActiveUser$.next(false);
policyService.policyAppliesToActiveUser$.mockClear();

const accountService = mockAccountServiceWith("userId" as UserId);

collectionService.getAllNested = () => Promise.resolve([]);
TestBed.configureTestingModule({
providers: [
Expand Down Expand Up @@ -84,6 +89,10 @@ describe("VaultPopupListFiltersService", () => {
useValue: policyService,
},
{ provide: FormBuilder, useClass: FormBuilder },
{
provide: AccountService,
useValue: accountService,
},
],
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { OrganizationService } from "@bitwarden/common/admin-console/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 { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { ProductTierType } from "@bitwarden/common/billing/enums";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { Utils } from "@bitwarden/common/platform/misc/utils";
Expand Down Expand Up @@ -81,6 +82,8 @@ export class VaultPopupListFiltersService {
map((ciphers) => Object.values(ciphers)),
);

private activeUserId$ = this.accountService.activeAccount$.pipe(map((a) => a?.id));

constructor(
private folderService: FolderService,
private cipherService: CipherService,
Expand All @@ -89,6 +92,7 @@ export class VaultPopupListFiltersService {
private collectionService: CollectionService,
private formBuilder: FormBuilder,
private policyService: PolicyService,
private accountService: AccountService,
) {
this.filterForm.controls.organization.valueChanges
.pipe(takeUntilDestroyed())
Expand Down Expand Up @@ -247,7 +251,7 @@ export class VaultPopupListFiltersService {
previousFilter.organization?.id === currentFilter.organization?.id,
),
),
this.folderService.folderViews$,
this.folderService.folderViews$(this.activeUserId$),
this.cipherViews$,
]).pipe(
map(([filters, folders, cipherViews]): [PopupListFilter, FolderView[], CipherView[]] => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@ import { By } from "@angular/platform-browser";
import { mock } from "jest-mock-extended";
import { BehaviorSubject } from "rxjs";

import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { mockAccountServiceWith } from "@bitwarden/common/spec";
import { UserId } from "@bitwarden/common/types/guid";
import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction";
import { FolderView } from "@bitwarden/common/vault/models/view/folder.view";
import { DialogService } from "@bitwarden/components";
Expand Down Expand Up @@ -52,8 +55,9 @@ describe("FoldersV2Component", () => {
{ provide: PlatformUtilsService, useValue: mock<PlatformUtilsService>() },
{ provide: ConfigService, useValue: mock<ConfigService>() },
{ provide: LogService, useValue: mock<LogService>() },
{ provide: FolderService, useValue: { folderViews$ } },
{ provide: FolderService, useValue: { folderViews$: () => folderViews$ } },
{ provide: I18nService, useValue: { t: (key: string) => key } },
{ provide: AccountService, useValue: mockAccountServiceWith("UserId" as UserId) },
],
})
.overrideComponent(FoldersV2Component, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Component } from "@angular/core";
import { map, Observable } from "rxjs";

import { JslibModule } from "@bitwarden/angular/jslib.module";
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction";
import { FolderView } from "@bitwarden/common/vault/models/view/folder.view";
import {
Expand Down Expand Up @@ -45,12 +46,14 @@ export class FoldersV2Component {
folders$: Observable<FolderView[]>;

NoFoldersIcon = VaultIcons.NoFolders;
private activeUserId$ = this.accountService.activeAccount$.pipe(map((a) => a?.id));

constructor(
private folderService: FolderService,
private dialogService: DialogService,
private accountService: AccountService,
) {
this.folders$ = this.folderService.folderViews$.pipe(
this.folders$ = this.folderService.folderViews$(this.activeUserId$).pipe(
Copy link
Member

Choose a reason for hiding this comment

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

๐Ÿ’ญ I'm not sure what we gain by making the folderViews$ reactive to / depend on the activeUserId$.

If the acitveUserId$ changes, we're almost certainly navigating away to another page (lock/logout) and the view will be destroyed very soon after. To avoid needing to await a firstValueFrom(this.activeUserId$) we could pipe/switchMap to the folderService.folderViews(activeUserId) instead.

map((folders) => {
// Remove the last folder, which is the "no folder" option folder
if (folders.length > 0) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import { Router } from "@angular/router";
import { map, Observable } from "rxjs";

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

Check warning on line 5 in apps/browser/src/vault/popup/settings/folders.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/settings/folders.component.ts#L5

Added line #L5 was not covered by tests
import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction";
import { FolderView } from "@bitwarden/common/vault/models/view/folder.view";

Expand All @@ -12,11 +13,14 @@
export class FoldersComponent {
folders$: Observable<FolderView[]>;

private activeUserId$ = this.accountService.activeAccount$.pipe(map((a) => a?.id));

constructor(
private folderService: FolderService,
private router: Router,
private accountService: AccountService,

Check warning on line 21 in apps/browser/src/vault/popup/settings/folders.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/settings/folders.component.ts#L21

Added line #L21 was not covered by tests
) {
this.folders$ = this.folderService.folderViews$.pipe(
this.folders$ = this.folderService.folderViews$(this.activeUserId$).pipe(

Check warning on line 23 in apps/browser/src/vault/popup/settings/folders.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/settings/folders.component.ts#L23

Added line #L23 was not covered by tests
map((folders) => {
if (folders.length > 0) {
folders = folders.slice(0, folders.length - 1);
Expand Down
3 changes: 2 additions & 1 deletion apps/browser/src/vault/services/vault-filter.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export class VaultFilterService extends BaseVaultFilterService {
collectionService: CollectionService,
policyService: PolicyService,
stateProvider: StateProvider,
private accountService: AccountService,
accountService: AccountService,
) {
super(
organizationService,
Expand All @@ -31,6 +31,7 @@ export class VaultFilterService extends BaseVaultFilterService {
collectionService,
policyService,
stateProvider,
accountService,
);
this.vaultFilter.myVaultOnly = false;
this.vaultFilter.selectedOrganizationId = null;
Expand Down
14 changes: 8 additions & 6 deletions apps/cli/src/commands/edit.command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import { FolderResponse } from "../vault/models/folder.response";

export class EditCommand {
private activeUserId$ = this.accountService.activeAccount$.pipe(map((a) => a?.id));

constructor(
private cipherService: CipherService,
private folderService: FolderService,
Expand Down Expand Up @@ -119,12 +121,12 @@

cipher.collectionIds = req;
try {
const activeUserId = await firstValueFrom(
this.accountService.activeAccount$.pipe(map((a) => a?.id)),
);
const updatedCipher = await this.cipherService.saveCollectionsWithServer(cipher);
const decCipher = await updatedCipher.decrypt(
await this.cipherService.getKeyForCipherKeyDecryption(updatedCipher, activeUserId),
await this.cipherService.getKeyForCipherKeyDecryption(
updatedCipher,
await firstValueFrom(this.activeUserId$),
),
);
const res = new CipherResponse(decCipher);
return Response.success(res);
Expand All @@ -134,7 +136,7 @@
}

private async editFolder(id: string, req: FolderExport) {
const folder = await this.folderService.getFromState(id);
const folder = await this.folderService.getFromState(id, this.activeUserId$);

Check warning on line 139 in apps/cli/src/commands/edit.command.ts

View check run for this annotation

Codecov / codecov/patch

apps/cli/src/commands/edit.command.ts#L139

Added line #L139 was not covered by tests
if (folder == null) {
return Response.notFound();
}
Expand All @@ -147,7 +149,7 @@
const encFolder = await this.folderService.encrypt(folderView, userKey);
try {
await this.folderApiService.save(encFolder);
const updatedFolder = await this.folderService.get(folder.id);
const updatedFolder = await this.folderService.get(folder.id, this.activeUserId$);

Check warning on line 152 in apps/cli/src/commands/edit.command.ts

View check run for this annotation

Codecov / codecov/patch

apps/cli/src/commands/edit.command.ts#L152

Added line #L152 was not covered by tests
const decFolder = await updatedFolder.decrypt();
const res = new FolderResponse(decFolder);
return Response.success(res);
Expand Down
22 changes: 11 additions & 11 deletions apps/cli/src/commands/get.command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@
import { DownloadCommand } from "./download.command";

export class GetCommand extends DownloadCommand {
private activeUserId$ = this.accountService.activeAccount$.pipe(map((a) => a?.id));

constructor(
private cipherService: CipherService,
private folderService: FolderService,
Expand Down Expand Up @@ -113,12 +115,12 @@
let decCipher: CipherView = null;
if (Utils.isGuid(id)) {
const cipher = await this.cipherService.get(id);
const activeUserId = await firstValueFrom(
this.accountService.activeAccount$.pipe(map((a) => a?.id)),
);
if (cipher != null) {
decCipher = await cipher.decrypt(
await this.cipherService.getKeyForCipherKeyDecryption(cipher, activeUserId),
await this.cipherService.getKeyForCipherKeyDecryption(
cipher,
await firstValueFrom(this.activeUserId$),
),
);
}
} else if (id.trim() !== "") {
Expand Down Expand Up @@ -384,12 +386,12 @@
private async getFolder(id: string) {
let decFolder: FolderView = null;
if (Utils.isGuid(id)) {
const folder = await this.folderService.getFromState(id);
const folder = await this.folderService.getFromState(id, this.activeUserId$);

Check warning on line 389 in apps/cli/src/commands/get.command.ts

View check run for this annotation

Codecov / codecov/patch

apps/cli/src/commands/get.command.ts#L389

Added line #L389 was not covered by tests
if (folder != null) {
decFolder = await folder.decrypt();
}
} else if (id.trim() !== "") {
let folders = await this.folderService.getAllDecryptedFromState();
let folders = await this.folderService.getAllDecryptedFromState(this.activeUserId$);

Check warning on line 394 in apps/cli/src/commands/get.command.ts

View check run for this annotation

Codecov / codecov/patch

apps/cli/src/commands/get.command.ts#L394

Added line #L394 was not covered by tests
folders = CliUtils.searchFolders(folders, id);
if (folders.length > 1) {
return Response.multipleResults(folders.map((f) => f.id));
Expand Down Expand Up @@ -551,11 +553,9 @@
private async getFingerprint(id: string) {
let fingerprint: string[] = null;
if (id === "me") {
const activeUserId = await firstValueFrom(
this.accountService.activeAccount$.pipe(map((a) => a?.id)),
);
const publicKey = await firstValueFrom(this.keyService.userPublicKey$(activeUserId));
fingerprint = await this.keyService.getFingerprint(activeUserId, publicKey);
const userId = await firstValueFrom(this.activeUserId$);
const publicKey = await firstValueFrom(this.keyService.userPublicKey$(userId));
fingerprint = await this.keyService.getFingerprint(userId, publicKey);

Check warning on line 558 in apps/cli/src/commands/get.command.ts

View check run for this annotation

Codecov / codecov/patch

apps/cli/src/commands/get.command.ts#L556-L558

Added lines #L556 - L558 were not covered by tests
} else if (Utils.isGuid(id)) {
try {
const response = await this.apiService.getUserPublicKey(id);
Expand Down
Loading
Loading