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

Merged
merged 32 commits into from
Jan 2, 2025
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
df1caad
Migrated folder service from using active user state to single user sโ€ฆ
gbubemismith Nov 6, 2024
9011af9
Update callers in the web
gbubemismith Nov 6, 2024
d56dfd9
Update callers in the browser
gbubemismith Nov 6, 2024
2584759
Update callers in libs
gbubemismith Nov 6, 2024
c2cc509
Update callers in cli
gbubemismith Nov 7, 2024
07593b6
Fixed test
gbubemismith Nov 7, 2024
bb0d489
Fixed folder state test
gbubemismith Nov 7, 2024
3865ee6
Fixed test
gbubemismith Nov 7, 2024
735d009
removed duplicate activeUserId
gbubemismith Nov 7, 2024
c1a915c
Merge branch 'main' into vault/PM-12049
gbubemismith Nov 7, 2024
8f25b65
Added takewhile operator to only make calls when userId is present
gbubemismith Nov 8, 2024
faba3d0
Merge branch 'main' into vault/PM-12049
withinfocus Nov 11, 2024
47364bf
Simplified to accept a single user id instead of an observable
gbubemismith Nov 19, 2024
12a8501
Fixed merge conflict
gbubemismith Nov 19, 2024
6b7eaae
Required userid to be passed from notification service
gbubemismith Nov 26, 2024
134a6ca
Merge branch 'main' into vault/PM-12049
bnagawiecki Dec 6, 2024
a98dd28
fixed merge conflicts
gbubemismith Dec 10, 2024
8207a01
Merge branch 'main' into vault/PM-12049
gbubemismith Dec 11, 2024
45ec1e1
Merge branch 'main' into vault/PM-12049
gbubemismith Dec 17, 2024
bf73b16
[PM-15635] Folders not working on desktop (#12333)
gbubemismith Dec 17, 2024
87449d7
removed unnecessasry null encrypteed folder check
gbubemismith Dec 17, 2024
967b698
Handle null folderdata
gbubemismith Dec 19, 2024
275be11
[PM-16197] "Items with No Folder" shows as a folder to edit name and โ€ฆ
gbubemismith Dec 20, 2024
efd0fc8
Merge branch 'main' into vault/PM-12049
gbubemismith Dec 20, 2024
2f4c13f
Merge branch 'main' into vault/PM-12049
gbubemismith Dec 20, 2024
2a921e3
Fixed failing test
gbubemismith Dec 20, 2024
e69e997
switched to use memory-large-object
gbubemismith Dec 23, 2024
3739924
Merge branch 'main' into vault/PM-12049
gbubemismith Dec 24, 2024
f64fb81
Fixed ts sctrict issue
gbubemismith Dec 24, 2024
12559d9
fixed merge conflicts
gbubemismith Dec 24, 2024
caf6ae5
Merge branch 'main' into vault/PM-12049
gbubemismith Jan 2, 2025
4b0a42f
Merge branch 'main' into vault/PM-12049
gbubemismith Jan 2, 2025
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
Expand Up @@ -60,10 +60,18 @@ describe("NotificationBackground", () => {
const configService = mock<ConfigService>();
const accountService = mock<AccountService>();

const activeAccountSubject = new BehaviorSubject<{ id: UserId } & AccountInfo>({
id: "testId" as UserId,
email: "[email protected]",
emailVerified: true,
name: "Test User",
});

beforeEach(() => {
activeAccountStatusMock$ = new BehaviorSubject(AuthenticationStatus.Locked);
authService = mock<AuthService>();
authService.activeAccountStatus$ = activeAccountStatusMock$;
accountService.activeAccount$ = activeAccountSubject;
notificationBackground = new NotificationBackground(
autofillService,
cipherService,
Expand Down Expand Up @@ -683,13 +691,6 @@ describe("NotificationBackground", () => {
});

describe("saveOrUpdateCredentials", () => {
const activeAccountSubject = new BehaviorSubject<{ id: UserId } & AccountInfo>({
id: "testId" as UserId,
email: "[email protected]",
emailVerified: true,
name: "Test User",
});

let getDecryptedCipherByIdSpy: jest.SpyInstance;
let getAllDecryptedForUrlSpy: jest.SpyInstance;
let updatePasswordSpy: jest.SpyInstance;
Expand Down
22 changes: 9 additions & 13 deletions apps/browser/src/autofill/background/notification.background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,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 @@ -569,9 +571,7 @@
return;
}

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

const cipher = await this.cipherService.encrypt(newCipher, activeUserId);
try {
Expand Down Expand Up @@ -611,10 +611,7 @@
return;
}

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

const activeUserId = 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
Expand Down Expand Up @@ -647,17 +644,15 @@
if (Utils.isNullOrWhitespace(folderId) || folderId === "null") {
return false;
}

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

Check warning on line 648 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#L647-L648

Added lines #L647 - L648 were not covered by tests
return folders.some((x) => x.id === folderId);
}

private async getDecryptedCipherById(cipherId: string) {
const cipher = await this.cipherService.get(cipherId);
if (cipher != null && cipher.type === CipherType.Login) {
const activeUserId = await firstValueFrom(
this.accountService.activeAccount$.pipe(map((a) => a?.id)),
);
const activeUserId = await firstValueFrom(this.activeUserId$);

Check warning on line 655 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#L655

Added line #L655 was not covered by tests

return await cipher.decrypt(
await this.cipherService.getKeyForCipherKeyDecryption(cipher, activeUserId),
Expand Down Expand Up @@ -697,7 +692,8 @@
* Returns the first value found from the folder service's folderViews$ observable.
*/
private async getFolderData() {
return await firstValueFrom(this.folderService.folderViews$);
const activeUserId = await firstValueFrom(this.activeUserId$);
return await firstValueFrom(this.folderService.folderViews$(activeUserId));
}

private async getWebVaultUrl(): Promise<string> {
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 @@ -171,7 +171,7 @@ describe("AddEditFolderDialogComponent", () => {
it("deletes the folder", async () => {
await component.deleteFolder();

expect(deleteFolder).toHaveBeenCalledWith(folderView.id);
expect(deleteFolder).toHaveBeenCalledWith(folderView.id, "");
expect(showToast).toHaveBeenCalledWith({
variant: "success",
title: null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
} from "@angular/core";
import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
import { FormBuilder, ReactiveFormsModule, Validators } from "@angular/forms";
import { firstValueFrom } from "rxjs";
import { firstValueFrom, map } from "rxjs";

import { JslibModule } from "@bitwarden/angular/jslib.module";
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
Expand Down Expand Up @@ -67,6 +67,7 @@ export class AddEditFolderDialogComponent implements AfterViewInit, OnInit {
name: ["", Validators.required],
});

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

constructor(
Expand Down Expand Up @@ -114,10 +115,10 @@ export class AddEditFolderDialogComponent implements AfterViewInit, OnInit {
this.folder.name = this.folderForm.controls.name.value;

try {
const activeUserId = await firstValueFrom(this.accountService.activeAccount$);
const userKey = await this.keyService.getUserKeyWithLegacySupport(activeUserId.id);
const activeUserId = await firstValueFrom(this.activeUserId$);
const userKey = await this.keyService.getUserKeyWithLegacySupport(activeUserId);
const folder = await this.folderService.encrypt(this.folder, userKey);
await this.folderApiService.save(folder);
await this.folderApiService.save(folder, activeUserId);

this.toastService.showToast({
variant: "success",
Expand All @@ -144,7 +145,8 @@ export class AddEditFolderDialogComponent implements AfterViewInit, OnInit {
}

try {
await this.folderApiService.delete(this.folder.id);
const activeUserId = await firstValueFrom(this.activeUserId$);
await this.folderApiService.delete(this.folder.id, activeUserId);
this.toastService.showToast({
variant: "success",
title: null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@ 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 { StateProvider } from "@bitwarden/common/platform/state";
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 @@ -32,7 +35,7 @@ describe("VaultPopupListFiltersService", () => {
} as unknown as CollectionService;

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

const cipherService = {
Expand Down Expand Up @@ -60,6 +63,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 @@ -92,6 +97,10 @@ describe("VaultPopupListFiltersService", () => {
useValue: { getGlobal: () => ({ state$, update }) },
},
{ provide: FormBuilder, useClass: FormBuilder },
{
provide: AccountService,
useValue: accountService,
},
],
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,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 @@ -102,6 +103,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 @@ -111,6 +114,7 @@ export class VaultPopupListFiltersService {
private formBuilder: FormBuilder,
private policyService: PolicyService,
private stateProvider: StateProvider,
private accountService: AccountService,
) {
this.filterForm.controls.organization.valueChanges
.pipe(takeUntilDestroyed())
Expand Down Expand Up @@ -264,60 +268,67 @@ export class VaultPopupListFiltersService {
/**
* Folder array structured to be directly passed to `ChipSelectComponent`
*/
folders$: Observable<ChipSelectOption<FolderView>[]> = combineLatest([
this.filters$.pipe(
distinctUntilChanged(
(previousFilter, currentFilter) =>
// Only update the collections when the organizationId filter changes
previousFilter.organization?.id === currentFilter.organization?.id,
),
),
this.folderService.folderViews$,
this.cipherViews$,
]).pipe(
map(([filters, folders, cipherViews]): [PopupListFilter, FolderView[], CipherView[]] => {
if (folders.length === 1 && folders[0].id === null) {
// Do not display folder selections when only the "no folder" option is available.
return [filters, [], cipherViews];
}
folders$: Observable<ChipSelectOption<FolderView>[]> = this.activeUserId$.pipe(
switchMap((userId) =>
combineLatest([
this.filters$.pipe(
distinctUntilChanged(
(previousFilter, currentFilter) =>
// Only update the collections when the organizationId filter changes
previousFilter.organization?.id === currentFilter.organization?.id,
),
),
this.folderService.folderViews$(userId),
this.cipherViews$,
]).pipe(
map(([filters, folders, cipherViews]): [PopupListFilter, FolderView[], CipherView[]] => {
if (folders.length === 1 && folders[0].id === null) {
// Do not display folder selections when only the "no folder" option is available.
return [filters, [], cipherViews];
}

// Sort folders by alphabetic name
folders.sort(Utils.getSortFunction(this.i18nService, "name"));
let arrangedFolders = folders;
// Sort folders by alphabetic name
folders.sort(Utils.getSortFunction(this.i18nService, "name"));
let arrangedFolders = folders;

const noFolder = folders.find((f) => f.id === null);
const noFolder = folders.find((f) => f.id === null);

if (noFolder) {
// Update `name` of the "no folder" option to "Items with no folder"
noFolder.name = this.i18nService.t("itemsWithNoFolder");
if (noFolder) {
// Update `name` of the "no folder" option to "Items with no folder"
const updatedNoFolder = {
...noFolder,
name: this.i18nService.t("itemsWithNoFolder"),
};

// Move the "no folder" option to the end of the list
arrangedFolders = [...folders.filter((f) => f.id !== null), noFolder];
}
return [filters, arrangedFolders, cipherViews];
}),
map(([filters, folders, cipherViews]) => {
const organizationId = filters.organization?.id ?? null;
// Move the "no folder" option to the end of the list
arrangedFolders = [...folders.filter((f) => f.id !== null), updatedNoFolder];
}
return [filters, arrangedFolders, cipherViews];
}),
map(([filters, folders, cipherViews]) => {
const organizationId = filters.organization?.id ?? null;

// When no org or "My vault" is selected, return all folders
if (organizationId === null || organizationId === MY_VAULT_ID) {
return folders;
}
// When no org or "My vault" is selected, return all folders
if (organizationId === null || organizationId === MY_VAULT_ID) {
return folders;
}

const orgCiphers = cipherViews.filter((c) => c.organizationId === organizationId);
const orgCiphers = cipherViews.filter((c) => c.organizationId === organizationId);

// Return only the folders that have ciphers within the filtered organization
return folders.filter((f) => orgCiphers.some((oc) => oc.folderId === f.id));
}),
map((folders) => {
const nestedFolders = this.getAllFoldersNested(folders);
return new DynamicTreeNode<FolderView>({
fullList: folders,
nestedList: nestedFolders,
});
}),
map((folders) =>
folders.nestedList.map((f) => this.convertToChipSelectOption(f, "bwi-folder")),
// Return only the folders that have ciphers within the filtered organization
return folders.filter((f) => orgCiphers.some((oc) => oc.folderId === f.id));
}),
map((folders) => {
const nestedFolders = this.getAllFoldersNested(folders);
return new DynamicTreeNode<FolderView>({
fullList: folders,
nestedList: nestedFolders,
});
}),
map((folders) =>
folders.nestedList.map((f) => this.convertToChipSelectOption(f, "bwi-folder")),
),
),
),
);

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
Loading
Loading