1
0
mirror of https://github.com/bitwarden/browser synced 2026-02-15 07:54:55 +00:00

[PM-25685][PM-31077] - Migrate all Folder models (#17077)

* enforce strict types on folders

* fix folder api service

* fix tests

* fix test

* fix type issue

* fix test

* add extra checks for folders. add specs

* fix folder.id checks

* fix id logic

* remove unecessary check

* name name and id optional in folder model

* fix tests

* Update folder and folderview

* fix folder with id export

* fix tests

* fix tests

* more defensive typing

* fix tests

* no need to check for presence

* check for empty name in folder toDomain

* fixes to folder

* initialize id in folder constructor. fix failing tests

* remove optional param to folder constructor

* fix folder

* fix test

* remove remaining checks for null folder id

* fix logic

* pass null for empty folder ids

* make id more explicit

* fix failing test

* fix failing test

* fix "No Folder" filter
This commit is contained in:
Jordan Aasen
2026-02-12 13:52:29 -08:00
committed by GitHub
parent 1be55763a3
commit 2a72d2e74d
27 changed files with 157 additions and 113 deletions

View File

@@ -438,7 +438,7 @@ describe("VaultPopupListFiltersService", () => {
describe("folders$", () => {
it('returns no folders when "No Folder" is the only option', (done) => {
folderViews$.next([{ id: null, name: "No Folder" }]);
folderViews$.next([{ id: "", name: "No Folder" }]);
service.folders$.subscribe((folders) => {
expect(folders).toEqual([]);
@@ -448,7 +448,7 @@ describe("VaultPopupListFiltersService", () => {
it('moves "No Folder" to the end of the list', (done) => {
folderViews$.next([
{ id: null, name: "No Folder" },
{ id: "", name: "No Folder" },
{ id: "2345", name: "Folder 2" },
{ id: "1234", name: "Folder 1" },
]);

View File

@@ -387,7 +387,7 @@ export class VaultPopupListFiltersService {
FolderView[],
PopupCipherViewLike[],
] => {
if (folders.length === 1 && folders[0].id === null) {
if (folders.length === 1 && !folders[0].id) {
// Do not display folder selections when only the "no folder" option is available.
return [filters as PopupListFilter, [], cipherViews];
}
@@ -396,7 +396,7 @@ export class VaultPopupListFiltersService {
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);
if (noFolder) {
// Update `name` of the "no folder" option to "Items with no folder"
@@ -406,7 +406,7 @@ export class VaultPopupListFiltersService {
};
// Move the "no folder" option to the end of the list
arrangedFolders = [...folders.filter((f) => f.id !== null), updatedNoFolder];
arrangedFolders = [...folders.filter((f) => f.id), updatedNoFolder];
}
return [filters as PopupListFilter, arrangedFolders, cipherViews];
},
@@ -545,11 +545,7 @@ export class VaultPopupListFiltersService {
// When the organization filter changes and a folder is already selected,
// reset the folder filter if the folder does not belong to the new organization filter
if (
currentFilters.folder &&
currentFilters.folder.id !== null &&
organization.id !== MY_VAULT_ID
) {
if (currentFilters.folder && currentFilters.folder.id && organization.id !== MY_VAULT_ID) {
// Get all ciphers that belong to the new organization
const orgCiphers = this.cipherViews.filter((c) => c.organizationId === organization.id);

View File

@@ -143,6 +143,17 @@ describe("VaultFilter", () => {
expect(result).toBe(true);
});
it("should return true when filtering on unassigned folder via empty string id", () => {
const filterFunction = createFilterFunction({
selectedFolder: true,
selectedFolderId: "",
});
const result = filterFunction(cipher);
expect(result).toBe(true);
});
});
describe("given an organizational cipher (with organization and collections)", () => {

View File

@@ -63,10 +63,19 @@ export class VaultFilter {
if (this.cipherType != null && cipherPassesFilter) {
cipherPassesFilter = CipherViewLikeUtils.getType(cipher) === this.cipherType;
}
if (this.selectedFolder && this.selectedFolderId == null && cipherPassesFilter) {
cipherPassesFilter = cipher.folderId == null;
if (
this.selectedFolder &&
(this.selectedFolderId == null || this.selectedFolderId === "") &&
cipherPassesFilter
) {
cipherPassesFilter = cipher.folderId == null || cipher.folderId === "";
}
if (this.selectedFolder && this.selectedFolderId != null && cipherPassesFilter) {
if (
this.selectedFolder &&
this.selectedFolderId != null &&
this.selectedFolderId !== "" &&
cipherPassesFilter
) {
cipherPassesFilter = cipher.folderId === this.selectedFolderId;
}
if (this.selectedCollection && this.selectedCollectionId == null && cipherPassesFilter) {

View File

@@ -83,7 +83,7 @@ export class VaultFilterService implements DeprecatedVaultFilterServiceAbstracti
const ciphers = await this.cipherService.getAllDecrypted(userId);
const orgCiphers = ciphers.filter((c) => c.organizationId == organizationId);
folders = storedFolders.filter(
(f) => orgCiphers.some((oc) => oc.folderId == f.id) || f.id == null,
(f) => orgCiphers.some((oc) => oc.folderId == f.id) || !f.id,
);
}

View File

@@ -1,5 +1,3 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { EncString } from "../../key-management/crypto/models/enc-string";
import { Folder as FolderDomain } from "../../vault/models/domain/folder";
import { FolderView } from "../../vault/models/view/folder.view";
@@ -7,6 +5,8 @@ import { FolderView } from "../../vault/models/view/folder.view";
import { safeGetString } from "./utils";
export class FolderExport {
name: string = "";
static template(): FolderExport {
const req = new FolderExport();
req.name = "Folder name";
@@ -19,14 +19,12 @@ export class FolderExport {
}
static toDomain(req: FolderExport, domain = new FolderDomain()) {
domain.name = req.name != null ? new EncString(req.name) : null;
domain.name = new EncString(req.name);
return domain;
}
name: string;
// Use build method instead of ctor so that we can control order of JSON stringify for pretty print
build(o: FolderView | FolderDomain) {
this.name = safeGetString(o.name);
this.name = safeGetString(o.name ?? "") ?? "";
}
}

View File

@@ -1,5 +1,3 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { Jsonify } from "type-fest";
import { FolderResponse } from "../response/folder.response";
@@ -10,12 +8,19 @@ export class FolderData {
revisionDate: string;
constructor(response: Partial<FolderResponse>) {
this.name = response?.name;
this.id = response?.id;
this.revisionDate = response?.revisionDate;
this.name = response.name ?? "";
this.id = response.id ?? "";
this.revisionDate = response.revisionDate ?? new Date().toISOString();
}
static fromJSON(obj: Jsonify<FolderData>) {
return Object.assign(new FolderData({}), obj);
static fromJSON(obj: Jsonify<FolderData | null>) {
if (obj == null) {
return null;
}
return new FolderData({
id: obj.id,
name: obj.name,
revisionDate: obj.revisionDate,
});
}
}

View File

@@ -8,7 +8,7 @@ import {
mockFromJson,
} from "../../../../spec";
import { EncryptService } from "../../../key-management/crypto/abstractions/encrypt.service";
import { EncryptedString, EncString } from "../../../key-management/crypto/models/enc-string";
import { EncString } from "../../../key-management/crypto/models/enc-string";
import { FolderData } from "../../models/data/folder.data";
import { Folder } from "../../models/domain/folder";
@@ -49,6 +49,30 @@ describe("Folder", () => {
});
});
describe("constructor", () => {
it("initializes properties from FolderData", () => {
const revisionDate = new Date("2022-08-04T01:06:40.441Z");
const folder = new Folder({
id: "id",
name: "name",
revisionDate: revisionDate.toISOString(),
});
expect(folder.id).toBe("id");
expect(folder.revisionDate).toEqual(revisionDate);
expect(folder.name).toBeInstanceOf(EncString);
expect((folder.name as EncString).encryptedString).toBe("name");
});
it("initializes empty properties when no FolderData is provided", () => {
const folder = new Folder();
expect(folder.id).toBe("");
expect(folder.name).toBeInstanceOf(EncString);
expect(folder.revisionDate).toBeInstanceOf(Date);
});
});
describe("fromJSON", () => {
jest.mock("../../../key-management/crypto/models/enc-string");
jest.spyOn(EncString, "fromJSON").mockImplementation(mockFromJson);
@@ -57,17 +81,13 @@ describe("Folder", () => {
const revisionDate = new Date("2022-08-04T01:06:40.441Z");
const actual = Folder.fromJSON({
revisionDate: revisionDate.toISOString(),
name: "name" as EncryptedString,
name: "name",
id: "id",
});
const expected = {
revisionDate: revisionDate,
name: "name_fromJSON",
id: "id",
};
expect(actual).toMatchObject(expected);
expect(actual?.id).toBe("id");
expect(actual?.revisionDate).toEqual(revisionDate);
expect(actual?.name).toBe("name_fromJSON");
});
});
@@ -89,9 +109,7 @@ describe("Folder", () => {
const view = await folder.decryptWithKey(key, encryptService);
expect(view).toEqual({
name: "encName",
});
expect(view.name).toBe("encName");
});
it("assigns the folder id and revision date", async () => {

View File

@@ -1,5 +1,3 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { Jsonify } from "type-fest";
import { EncryptService } from "../../../key-management/crypto/abstractions/encrypt.service";
@@ -9,16 +7,10 @@ import { SymmetricCryptoKey } from "../../../platform/models/domain/symmetric-cr
import { FolderData } from "../data/folder.data";
import { FolderView } from "../view/folder.view";
export class Test extends Domain {
id: string;
name: EncString;
revisionDate: Date;
}
export class Folder extends Domain {
id: string;
name: EncString;
revisionDate: Date;
id: string = "";
name: EncString = new EncString("");
revisionDate: Date = new Date();
constructor(obj?: FolderData) {
super();
@@ -26,17 +18,9 @@ export class Folder extends Domain {
return;
}
this.buildDomainModel(
this,
obj,
{
id: null,
name: null,
},
["id"],
);
this.revisionDate = obj.revisionDate != null ? new Date(obj.revisionDate) : null;
this.id = obj.id;
this.name = new EncString(obj.name);
this.revisionDate = new Date(obj.revisionDate);
}
decrypt(key: SymmetricCryptoKey): Promise<FolderView> {
@@ -62,7 +46,14 @@ export class Folder extends Domain {
}
static fromJSON(obj: Jsonify<Folder>) {
const revisionDate = obj.revisionDate == null ? null : new Date(obj.revisionDate);
return Object.assign(new Folder(), obj, { name: EncString.fromJSON(obj.name), revisionDate });
if (obj == null) {
return null;
}
const folder = new Folder();
folder.id = obj.id;
folder.name = EncString.fromJSON(obj.name);
folder.revisionDate = new Date(obj.revisionDate);
return folder;
}
}

View File

@@ -7,6 +7,6 @@ export class FolderWithIdRequest extends FolderRequest {
constructor(folder: Folder) {
super(folder);
this.id = folder.id;
this.id = folder.id ?? "";
}
}

View File

@@ -1,19 +1,17 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { Jsonify } from "type-fest";
import { View } from "../../../models/view/view";
import { DecryptedObject } from "../../../platform/models/domain/domain-base";
import { Folder } from "../domain/folder";
import { ITreeNodeObject } from "../domain/tree-node";
export class FolderView implements View, ITreeNodeObject {
id: string = null;
name: string = null;
revisionDate: Date = null;
id: string = "";
name: string = "";
revisionDate: Date;
constructor(f?: Folder | DecryptedObject<Folder, "name">) {
constructor(f?: Folder) {
if (!f) {
this.revisionDate = new Date();
return;
}
@@ -22,7 +20,12 @@ export class FolderView implements View, ITreeNodeObject {
}
static fromJSON(obj: Jsonify<FolderView>) {
const revisionDate = obj.revisionDate == null ? null : new Date(obj.revisionDate);
return Object.assign(new FolderView(), obj, { revisionDate });
const folderView = new FolderView();
folderView.id = obj.id ?? "";
folderView.name = obj.name ?? "";
if (obj.revisionDate != null) {
folderView.revisionDate = new Date(obj.revisionDate);
}
return folderView;
}
}

View File

@@ -17,11 +17,11 @@ export class FolderApiService implements FolderApiServiceAbstraction {
const request = new FolderRequest(folder);
let response: FolderResponse;
if (folder.id == null) {
if (folder.id) {
response = await this.putFolder(folder.id, request);
} else {
response = await this.postFolder(request);
folder.id = response.id;
} else {
response = await this.putFolder(folder.id, request);
}
const data = new FolderData(response);

View File

@@ -122,6 +122,7 @@ describe("Folder Service", () => {
encryptedString: "ENC",
encryptionType: 0,
},
revisionDate: expect.any(Date),
});
});
@@ -132,7 +133,7 @@ describe("Folder Service", () => {
expect(result).toEqual({
id: "1",
name: makeEncString("ENC_STRING_" + 1),
revisionDate: null,
revisionDate: expect.any(Date),
});
});
@@ -150,12 +151,12 @@ describe("Folder Service", () => {
{
id: "1",
name: makeEncString("ENC_STRING_" + 1),
revisionDate: null,
revisionDate: expect.any(Date),
},
{
id: "2",
name: makeEncString("ENC_STRING_" + 2),
revisionDate: null,
revisionDate: expect.any(Date),
},
]);
});
@@ -167,7 +168,7 @@ describe("Folder Service", () => {
{
id: "4",
name: makeEncString("ENC_STRING_" + 4),
revisionDate: null,
revisionDate: expect.any(Date),
},
]);
});
@@ -203,7 +204,7 @@ describe("Folder Service", () => {
const folderViews = await firstValueFrom(folderService.folderViews$(mockUserId));
expect(folderViews.length).toBe(1);
expect(folderViews[0].id).toBeNull(); // Should be the "No Folder" folder
expect(folderViews[0].id).toEqual(""); // Should be the "No Folder" folder
});
describe("getRotatedData", () => {

View File

@@ -354,7 +354,7 @@ export class ImportComponent implements OnInit, OnDestroy, AfterViewInit {
switchMap((userId) => {
return this.folderService.folderViews$(userId);
}),
map((folders) => folders.filter((f) => f.id != null)),
map((folders) => folders.filter((f) => !!f.id)),
);
this.formGroup.controls.targetSelector.disable();

View File

@@ -2,12 +2,12 @@
// @ts-strict-ignore
import * as papa from "papaparse";
import { CollectionView, Collection } from "@bitwarden/common/admin-console/models/collections";
import { CollectionView } from "@bitwarden/common/admin-console/models/collections";
import { normalizeExpiryYearFormat } from "@bitwarden/common/autofill/utils";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { Utils } from "@bitwarden/common/platform/misc/utils";
import { ConsoleLogService } from "@bitwarden/common/platform/services/console-log.service";
import { OrganizationId } from "@bitwarden/common/types/guid";
import { CollectionId, OrganizationId } from "@bitwarden/common/types/guid";
import { FieldType, SecureNoteType, CipherType } from "@bitwarden/common/vault/enums";
import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view";
import { FieldView } from "@bitwarden/common/vault/models/view/field.view";
@@ -277,8 +277,7 @@ export abstract class BaseImporter {
const collection = new CollectionView({
name: f.name,
organizationId: this.organizationId,
// FIXME: Folder.id may be null, this should be changed when refactoring Folders to be ts-strict
id: Collection.isCollectionId(f.id) ? f.id : null,
id: f.id && f.id !== "" ? (f.id as CollectionId) : null,
});
return collection;
});

View File

@@ -23,7 +23,7 @@ describe("KeePass2 Xml Importer", () => {
const actual = [folder];
const result = await importer.parse(TestData);
expect(result.folders).toEqual(actual);
expect(result.folders[0].name).toEqual(actual[0].name);
});
it("parse XML should contains login details", async () => {

View File

@@ -57,10 +57,15 @@ describe("Password Depot 17 Xml Importer", () => {
const importer = new PasswordDepot17XmlImporter();
const folder = new FolderView();
folder.name = "tempDB";
const actual = [folder];
const result = await importer.parse(PasswordTestData);
expect(result.folders).toEqual(actual);
expect(result.folders).toEqual([
expect.objectContaining({
id: "",
name: "tempDB",
revisionDate: expect.any(Date),
}),
]);
});
it("should parse password type into logins", async () => {

View File

@@ -138,7 +138,7 @@ function expectEqualCiphers(ciphers: CipherView[] | Cipher[], jsonResult: string
}
function expectEqualFolderViews(folderViews: FolderView[] | Folder[], jsonResult: string) {
const actual = JSON.stringify(JSON.parse(jsonResult).folders);
const actual = JSON.parse(jsonResult).folders;
const folders: FolderResponse[] = [];
folderViews.forEach((c) => {
const folder = new FolderResponse();
@@ -148,21 +148,19 @@ function expectEqualFolderViews(folderViews: FolderView[] | Folder[], jsonResult
});
expect(actual.length).toBeGreaterThan(0);
expect(actual).toEqual(JSON.stringify(folders));
expect(actual).toEqual(folders);
}
function expectEqualFolders(folders: Folder[], jsonResult: string) {
const actual = JSON.stringify(JSON.parse(jsonResult).folders);
const items: Folder[] = [];
folders.forEach((c) => {
const item = new Folder();
item.id = c.id;
item.name = c.name;
items.push(item);
});
const actual = JSON.parse(jsonResult).folders;
const expected = folders.map((c) => ({
id: c.id,
name: c.name?.encryptedString,
}));
expect(actual.length).toBeGreaterThan(0);
expect(actual).toEqual(JSON.stringify(items));
expect(actual).toEqual(expected);
}
describe("VaultExportService", () => {

View File

@@ -240,7 +240,7 @@ export class IndividualVaultExportService
};
folders.forEach((f) => {
if (f.id == null) {
if (!f.id) {
return;
}
const folder = new FolderWithIdExport();
@@ -268,7 +268,7 @@ export class IndividualVaultExportService
private buildCsvExport(decFolders: FolderView[], decCiphers: CipherView[]): string {
const foldersMap = new Map<string, FolderView>();
decFolders.forEach((f) => {
if (f.id != null) {
if (f.id) {
foldersMap.set(f.id, f);
}
});
@@ -302,7 +302,7 @@ export class IndividualVaultExportService
};
decFolders.forEach((f) => {
if (f.id == null) {
if (!f.id) {
return;
}
const folder = new FolderWithIdExport();

View File

@@ -101,7 +101,7 @@ describe("AddEditFolderDialogComponent", () => {
const newFolder = new FolderView();
newFolder.name = "New Folder";
expect(encrypt).toHaveBeenCalledWith(newFolder, "");
expect(encrypt).toHaveBeenCalledWith(expect.objectContaining({ name: "New Folder" }), "");
expect(save).toHaveBeenCalled();
});

View File

@@ -116,6 +116,14 @@ describe("createFilter", () => {
expect(result).toBe(true);
});
it("should return true when filtering on empty-string folder id", () => {
const filterFunction = createFilterFunction({ folderId: "" });
const result = filterFunction(cipher);
expect(result).toBe(true);
});
});
describe("given an organizational cipher (with organization and collections)", () => {

View File

@@ -55,8 +55,11 @@ export function createFilterFunction(
return false;
}
}
const isNoFolderFilter = filter.folderId === Unassigned || filter.folderId === "";
const cipherHasFolder = cipher.folderId != null && cipher.folderId !== "";
// No folder
if (filter.folderId === Unassigned && cipher.folderId != null) {
if (isNoFolderFilter && cipherHasFolder) {
return false;
}
// Folder
@@ -64,6 +67,7 @@ export function createFilterFunction(
filter.folderId !== undefined &&
filter.folderId !== All &&
filter.folderId !== Unassigned &&
filter.folderId !== "" &&
cipher.folderId !== filter.folderId
) {
return false;

View File

@@ -87,7 +87,7 @@ export class RoutedVaultFilterBridge implements VaultFilter {
return this.legacyFilter.selectedFolderNode;
}
set selectedFolderNode(value: TreeNode<FolderFilter>) {
const folderId = value != null && value.node.id === null ? Unassigned : value?.node.id;
const folderId = value?.node.id ? value.node.id : Unassigned;
this.bridgeService.navigate({
...this.routedFilter,
folderId,

View File

@@ -134,7 +134,7 @@ export class VaultFilter {
if (this.selectedFolderNode) {
// No folder
if (this.folderId === null && cipherPassesFilter) {
cipherPassesFilter = cipher.folderId === null;
cipherPassesFilter = cipher.folderId == null || cipher.folderId === "";
}
// Folder
if (this.folderId !== null && cipherPassesFilter) {

View File

@@ -145,7 +145,7 @@ function createLegacyFilterForEndUser(
);
}
if (filter.folderId !== undefined && filter.folderId === Unassigned) {
if (filter.folderId !== undefined && (filter.folderId === Unassigned || filter.folderId === "")) {
legacyFilter.selectedFolderNode = ServiceUtils.getTreeNodeObject(folderTree, null);
} else if (filter.folderId !== undefined && filter.folderId !== Unassigned) {
legacyFilter.selectedFolderNode = ServiceUtils.getTreeNodeObject(folderTree, filter.folderId);

View File

@@ -195,8 +195,8 @@ describe("vault filter service", () => {
];
folderViews.next(storedFolders);
await expect(firstValueFrom(vaultFilterService.filteredFolders$)).resolves.toEqual([
createFolderView("folder test id", "test"),
await expect(firstValueFrom(vaultFilterService.filteredFolders$)).resolves.toMatchObject([
{ id: "folder test id", name: "test" },
]);
});

View File

@@ -290,9 +290,7 @@ export class VaultFilterService implements VaultFilterServiceAbstraction {
// Otherwise, show only folders that have ciphers from the selected org and the "no folder" folder
const orgCiphers = ciphers.filter((c) => c.organizationId == org?.id);
return storedFolders.filter(
(f) => orgCiphers.some((oc) => oc.folderId == f.id) || f.id == null,
);
return storedFolders.filter((f) => orgCiphers.some((oc) => oc.folderId == f.id) || !f.id);
}
protected buildFolderTree(folders?: FolderView[]): TreeNode<FolderFilter> {