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 @@ export default class NotificationBackground {
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 @@ export default class NotificationBackground {
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 @@ export default class NotificationBackground {
return false;
}

const folders = await firstValueFrom(this.folderService.folderViews$);
const folders = await firstValueFrom(this.folderService.folderViews$(this.activeUserId$));
return folders.some((x) => x.id === folderId);
}

Expand Down Expand Up @@ -695,7 +696,7 @@ export default class NotificationBackground {
* 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 @@ export default class MainBackground {
this.cipherService,
this.stateProvider,
);
this.folderApiService = new FolderApiService(this.folderService, this.apiService);
this.folderApiService = new FolderApiService(
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 @@ -242,7 +246,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 { Component } from "@angular/core";
import { Router } from "@angular/router";
import { map, Observable } from "rxjs";

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";

Expand All @@ -12,11 +13,14 @@ import { FolderView } from "@bitwarden/common/vault/models/view/folder.view";
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,
) {
this.folders$ = this.folderService.folderViews$.pipe(
this.folders$ = this.folderService.folderViews$(this.activeUserId$).pipe(
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 { CipherResponse } from "../vault/models/cipher.response";
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 @@ export class EditCommand {

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 @@ export class EditCommand {
}

private async editFolder(id: string, req: FolderExport) {
const folder = await this.folderService.getFromState(id);
const folder = await this.folderService.getFromState(id, this.activeUserId$);
if (folder == null) {
return Response.notFound();
}
Expand All @@ -147,7 +149,7 @@ export class EditCommand {
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$);
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 { FolderResponse } from "../vault/models/folder.response";
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 @@ export class GetCommand extends DownloadCommand {
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 @@ export class GetCommand extends DownloadCommand {
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$);
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$);
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 @@ export class GetCommand extends DownloadCommand {
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);
} else if (Utils.isGuid(id)) {
try {
const response = await this.apiService.getUserPublicKey(id);
Expand Down
Loading
Loading