From df1caadf1987793023f032ec55eaba69531208ec Mon Sep 17 00:00:00 2001 From: gbubemismith Date: Wed, 6 Nov 2024 10:46:20 -0500 Subject: [PATCH] Migrated folder service from using active user state to single user state Added extra test cases for encrypted folder and decrypted folders Updated derived state to use decrypt with key --- .../folder/folder.service.abstraction.ts | 23 ++- .../services/folder/folder.service.spec.ts | 148 ++++++++++-------- .../vault/services/folder/folder.service.ts | 124 ++++++++------- .../vault/services/key-state/folder.state.ts | 47 ++++-- 4 files changed, 192 insertions(+), 150 deletions(-) diff --git a/libs/common/src/vault/abstractions/folder/folder.service.abstraction.ts b/libs/common/src/vault/abstractions/folder/folder.service.abstraction.ts index df21b136f41..745ebb3b47d 100644 --- a/libs/common/src/vault/abstractions/folder/folder.service.abstraction.ts +++ b/libs/common/src/vault/abstractions/folder/folder.service.abstraction.ts @@ -11,23 +11,22 @@ import { FolderWithIdRequest } from "../../models/request/folder-with-id.request import { FolderView } from "../../models/view/folder.view"; export abstract class FolderService implements UserKeyRotationDataProvider { - folders$: Observable; - folderViews$: Observable; + folders$: (userId$: Observable) => Observable; + folderViews$: (userId$: Observable) => Observable; - clearCache: () => Promise; + clearDecryptedFolderState: (userId: UserId) => Promise; encrypt: (model: FolderView, key: SymmetricCryptoKey) => Promise; - get: (id: string) => Promise; - getDecrypted$: (id: string) => Observable; - getAllFromState: () => Promise; + get: (id: string, userId$: Observable) => Promise; + getDecrypted$: (id: string, userId$: Observable) => Observable; + getAllFromState: (userId$: Observable) => Promise; /** * @deprecated Only use in CLI! */ - getFromState: (id: string) => Promise; + getFromState: (id: string, userId$: Observable) => Promise; /** * @deprecated Only use in CLI! */ - getAllDecryptedFromState: () => Promise; - decryptFolders: (folders: Folder[]) => Promise; + getAllDecryptedFromState: (userId$: Observable) => Promise; /** * Returns user folders re-encrypted with the new user key. * @param originalUserKey the original user key @@ -44,8 +43,8 @@ export abstract class FolderService implements UserKeyRotationDataProvider Promise; + upsert: (folder: FolderData | FolderData[], userId: UserId) => Promise; replace: (folders: { [id: string]: FolderData }, userId: UserId) => Promise; - clear: (userId?: string) => Promise; - delete: (id: string | string[]) => Promise; + clear: (userId: UserId) => Promise; + delete: (id: string | string[], userId: UserId) => Promise; } diff --git a/libs/common/src/vault/services/folder/folder.service.spec.ts b/libs/common/src/vault/services/folder/folder.service.spec.ts index 193d0e85e61..0b382ce5692 100644 --- a/libs/common/src/vault/services/folder/folder.service.spec.ts +++ b/libs/common/src/vault/services/folder/folder.service.spec.ts @@ -1,10 +1,10 @@ import { mock, MockProxy } from "jest-mock-extended"; -import { firstValueFrom } from "rxjs"; +import { BehaviorSubject, firstValueFrom, of } from "rxjs"; import { KeyService } from "../../../../../key-management/src/abstractions/key.service"; -import { makeStaticByteArray } from "../../../../spec"; +import { makeEncString } from "../../../../spec"; import { FakeAccountService, mockAccountServiceWith } from "../../../../spec/fake-account-service"; -import { FakeActiveUserState } from "../../../../spec/fake-state"; +import { FakeSingleUserState } from "../../../../spec/fake-state"; import { FakeStateProvider } from "../../../../spec/fake-state-provider"; import { EncryptService } from "../../../platform/abstractions/encrypt.service"; import { I18nService } from "../../../platform/abstractions/i18n.service"; @@ -29,8 +29,9 @@ describe("Folder Service", () => { let stateProvider: FakeStateProvider; const mockUserId = Utils.newGuid() as UserId; + const mockUserId$ = of(mockUserId); let accountService: FakeAccountService; - let folderState: FakeActiveUserState>; + let folderState: FakeSingleUserState>; beforeEach(() => { keyService = mock(); @@ -42,11 +43,9 @@ describe("Folder Service", () => { stateProvider = new FakeStateProvider(accountService); i18nService.collator = new Intl.Collator("en"); + i18nService.t.mockReturnValue("No Folder"); - keyService.hasUserKey.mockResolvedValue(true); - keyService.getUserKeyWithLegacySupport.mockResolvedValue( - new SymmetricCryptoKey(makeStaticByteArray(32)) as UserKey, - ); + keyService.userKey$.mockReturnValue(new BehaviorSubject("mockOriginalUserKey" as any)); encryptService.decryptToUtf8.mockResolvedValue("DEC"); folderService = new FolderService( @@ -57,10 +56,53 @@ describe("Folder Service", () => { stateProvider, ); - folderState = stateProvider.activeUser.getFake(FOLDER_ENCRYPTED_FOLDERS); + folderState = stateProvider.singleUser.getFake(mockUserId, FOLDER_ENCRYPTED_FOLDERS); // Initial state - folderState.nextState({ "1": folderData("1", "test") }); + folderState.nextState({ "1": folderData("1") }); + }); + + describe("folders$", () => { + it("emits encrypted folders from state", async () => { + const folder1 = folderData("1"); + const folder2 = folderData("2"); + + await stateProvider.setUserState( + FOLDER_ENCRYPTED_FOLDERS, + Object.fromEntries([folder1, folder2].map((f) => [f.id, f])), + mockUserId, + ); + + const result = await firstValueFrom(folderService.folders$(mockUserId$)); + + expect(result.length).toBe(2); + expect(result).toIncludeAllPartialMembers([ + { id: "1", name: makeEncString("ENC_STRING_1") }, + { id: "2", name: makeEncString("ENC_STRING_2") }, + ]); + }); + }); + + describe("folderView$", () => { + it("emits decrypted folders from state", async () => { + const folder1 = folderData("1"); + const folder2 = folderData("2"); + + await stateProvider.setUserState( + FOLDER_ENCRYPTED_FOLDERS, + Object.fromEntries([folder1, folder2].map((f) => [f.id, f])), + mockUserId, + ); + + const result = await firstValueFrom(folderService.folderViews$(mockUserId$)); + + expect(result.length).toBe(3); + expect(result).toIncludeAllPartialMembers([ + { id: "1", name: "DEC" }, + { id: "2", name: "DEC" }, + { name: "No Folder" }, + ]); + }); }); it("encrypt", async () => { @@ -83,105 +125,77 @@ describe("Folder Service", () => { describe("get", () => { it("exists", async () => { - const result = await folderService.get("1"); + const result = await folderService.get("1", mockUserId$); expect(result).toEqual({ id: "1", - name: { - encryptedString: "test", - encryptionType: 0, - }, + name: makeEncString("ENC_STRING_" + 1), revisionDate: null, }); }); it("not exists", async () => { - const result = await folderService.get("2"); + const result = await folderService.get("2", mockUserId$); expect(result).toBe(undefined); }); }); it("upsert", async () => { - await folderService.upsert(folderData("2", "test 2")); + await folderService.upsert(folderData("2"), mockUserId); - expect(await firstValueFrom(folderService.folders$)).toEqual([ + expect(await firstValueFrom(folderService.folders$(mockUserId$))).toEqual([ { id: "1", - name: { - encryptedString: "test", - encryptionType: 0, - }, + name: makeEncString("ENC_STRING_" + 1), revisionDate: null, }, { id: "2", - name: { - encryptedString: "test 2", - encryptionType: 0, - }, + name: makeEncString("ENC_STRING_" + 2), revisionDate: null, }, ]); }); it("replace", async () => { - await folderService.replace({ "2": folderData("2", "test 2") }, mockUserId); + await folderService.replace({ "4": folderData("4") }, mockUserId); - expect(await firstValueFrom(folderService.folders$)).toEqual([ + expect(await firstValueFrom(folderService.folders$(mockUserId$))).toEqual([ { - id: "2", - name: { - encryptedString: "test 2", - encryptionType: 0, - }, + id: "4", + name: makeEncString("ENC_STRING_" + 4), revisionDate: null, }, ]); }); it("delete", async () => { - await folderService.delete("1"); + await folderService.delete("1", mockUserId); - expect((await firstValueFrom(folderService.folders$)).length).toBe(0); + expect((await firstValueFrom(folderService.folders$(mockUserId$))).length).toBe(0); }); - it("clearCache", async () => { - await folderService.clearCache(); - - expect((await firstValueFrom(folderService.folders$)).length).toBe(1); - expect((await firstValueFrom(folderService.folderViews$)).length).toBe(0); - }); - - describe("clear", () => { + describe("clearDecryptedFolderState", () => { it("null userId", async () => { - await folderService.clear(); - - expect((await firstValueFrom(folderService.folders$)).length).toBe(0); - expect((await firstValueFrom(folderService.folderViews$)).length).toBe(0); + await expect(folderService.clearDecryptedFolderState(null)).rejects.toThrow( + "User ID is required.", + ); }); - /** - * TODO: Fix this test to address the problem where the fakes for the active user state is not - * updated as expected - */ - // it("matching userId", async () => { - // stateService.getUserId.mockResolvedValue("1"); - // await folderService.clear("1" as UserId); + it("userId provided", async () => { + await folderService.clearDecryptedFolderState(mockUserId); - // expect((await firstValueFrom(folderService.folders$)).length).toBe(0); - // }); + expect((await firstValueFrom(folderService.folders$(mockUserId$))).length).toBe(1); + expect((await firstValueFrom(folderService.folderViews$(mockUserId$))).length).toBe(0); + }); + }); - /** - * TODO: Fix this test to address the problem where the fakes for the active user state is not - * updated as expected - */ - // it("mismatching userId", async () => { - // await folderService.clear("12" as UserId); + it("clear", async () => { + await folderService.clear(mockUserId); - // expect((await firstValueFrom(folderService.folders$)).length).toBe(1); - // expect((await firstValueFrom(folderService.folderViews$)).length).toBe(2); - // }); + expect((await firstValueFrom(folderService.folders$(mockUserId$))).length).toBe(0); + expect((await firstValueFrom(folderService.folderViews$(mockUserId$))).length).toBe(0); }); describe("getRotatedData", () => { @@ -207,10 +221,10 @@ describe("Folder Service", () => { }); }); - function folderData(id: string, name: string) { + function folderData(id: string) { const data = new FolderData({} as any); data.id = id; - data.name = name; + data.name = makeEncString("ENC_STRING_" + data.id).encryptedString; return data; } diff --git a/libs/common/src/vault/services/folder/folder.service.ts b/libs/common/src/vault/services/folder/folder.service.ts index 2a76e82f3b7..51a45920059 100644 --- a/libs/common/src/vault/services/folder/folder.service.ts +++ b/libs/common/src/vault/services/folder/folder.service.ts @@ -1,12 +1,11 @@ -import { Observable, firstValueFrom, map, shareReplay } from "rxjs"; +import { Observable, firstValueFrom, map, of, shareReplay, switchMap } from "rxjs"; import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service"; import { KeyService } from "../../../../../key-management/src/abstractions/key.service"; import { I18nService } from "../../../platform/abstractions/i18n.service"; -import { Utils } from "../../../platform/misc/utils"; import { SymmetricCryptoKey } from "../../../platform/models/domain/symmetric-crypto-key"; -import { ActiveUserState, DerivedState, StateProvider } from "../../../platform/state"; +import { DerivedState, StateProvider } from "../../../platform/state"; import { UserId } from "../../../types/guid"; import { UserKey } from "../../../types/key"; import { CipherService } from "../../../vault/abstractions/cipher.service"; @@ -19,35 +18,37 @@ import { FolderWithIdRequest } from "../../models/request/folder-with-id.request import { FOLDER_DECRYPTED_FOLDERS, FOLDER_ENCRYPTED_FOLDERS } from "../key-state/folder.state"; export class FolderService implements InternalFolderServiceAbstraction { - folders$: Observable; - folderViews$: Observable; - - private encryptedFoldersState: ActiveUserState>; - private decryptedFoldersState: DerivedState; - constructor( private keyService: KeyService, private encryptService: EncryptService, private i18nService: I18nService, private cipherService: CipherService, private stateProvider: StateProvider, - ) { - this.encryptedFoldersState = this.stateProvider.getActive(FOLDER_ENCRYPTED_FOLDERS); - this.decryptedFoldersState = this.stateProvider.getDerived( - this.encryptedFoldersState.state$, - FOLDER_DECRYPTED_FOLDERS, - { folderService: this, keyService: this.keyService }, - ); + ) {} - this.folders$ = this.encryptedFoldersState.state$.pipe( - map((folderData) => Object.values(folderData).map((f) => new Folder(f))), - ); + folders$(userId$: Observable): Observable { + return userId$.pipe( + switchMap((userId) => this.encryptedFoldersState(userId).state$), + map((folders) => { + if (folders == null) { + return []; + } - this.folderViews$ = this.decryptedFoldersState.state$; + return Object.values(folders).map((f) => new Folder(f)); + }), + ); } - async clearCache(): Promise { - await this.decryptedFoldersState.forceValue([]); + folderViews$(userId$: Observable) { + return userId$.pipe(switchMap((userId) => this.decryptedFoldersState(userId).state$)); + } + + async clearDecryptedFolderState(userId: UserId): Promise { + if (userId == null) { + throw new Error("User ID is required."); + } + + await this.decryptedFoldersState(userId).forceValue([]); } // TODO: This should be moved to EncryptService or something @@ -58,29 +59,29 @@ export class FolderService implements InternalFolderServiceAbstraction { return folder; } - async get(id: string): Promise { - const folders = await firstValueFrom(this.folders$); + async get(id: string, userId$: Observable): Promise { + const folders = await firstValueFrom(this.folders$(userId$)); return folders.find((folder) => folder.id === id); } - getDecrypted$(id: string): Observable { - return this.folderViews$.pipe( + getDecrypted$(id: string, userId$: Observable): Observable { + return this.folderViews$(userId$).pipe( map((folders) => folders.find((folder) => folder.id === id)), shareReplay({ refCount: true, bufferSize: 1 }), ); } - async getAllFromState(): Promise { - return await firstValueFrom(this.folders$); + async getAllFromState(userId$: Observable): Promise { + return await firstValueFrom(this.folders$(userId$)); } /** * @deprecated For the CLI only * @param id id of the folder */ - async getFromState(id: string): Promise { - const folder = await this.get(id); + async getFromState(id: string, userId$: Observable): Promise { + const folder = await this.get(id, userId$); if (!folder) { return null; } @@ -91,12 +92,12 @@ export class FolderService implements InternalFolderServiceAbstraction { /** * @deprecated Only use in CLI! */ - async getAllDecryptedFromState(): Promise { - return await firstValueFrom(this.folderViews$); + async getAllDecryptedFromState(userId$: Observable): Promise { + return await firstValueFrom(this.folderViews$(userId$)); } - async upsert(folderData: FolderData | FolderData[]): Promise { - await this.encryptedFoldersState.update((folders) => { + async upsert(folderData: FolderData | FolderData[], userId: UserId): Promise { + await this.encryptedFoldersState(userId).update((folders) => { if (folders == null) { folders = {}; } @@ -125,17 +126,13 @@ export class FolderService implements InternalFolderServiceAbstraction { }); } - async clear(userId?: UserId): Promise { - if (userId == null) { - await this.encryptedFoldersState.update(() => ({})); - await this.decryptedFoldersState.forceValue([]); - } else { - await this.stateProvider.getUser(userId, FOLDER_ENCRYPTED_FOLDERS).update(() => ({})); - } + async clear(userId: UserId): Promise { + await this.encryptedFoldersState(userId).update(() => ({})); + await this.decryptedFoldersState(userId).forceValue([]); } - async delete(id: string | string[]): Promise { - await this.encryptedFoldersState.update((folders) => { + async delete(id: string | string[], userId: UserId): Promise { + await this.encryptedFoldersState(userId).update((folders) => { if (folders == null) { return; } @@ -162,25 +159,11 @@ export class FolderService implements InternalFolderServiceAbstraction { } } if (updates.length > 0) { - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.cipherService.upsert(updates.map((c) => c.toCipherData())); + await this.cipherService.upsert(updates.map((c) => c.toCipherData())); } } } - async decryptFolders(folders: Folder[]) { - const decryptFolderPromises = folders.map((f) => f.decrypt()); - const decryptedFolders = await Promise.all(decryptFolderPromises); - - decryptedFolders.sort(Utils.getSortFunction(this.i18nService, "name")); - - const noneFolder = new FolderView(); - noneFolder.name = this.i18nService.t("noneFolder"); - decryptedFolders.push(noneFolder); - return decryptedFolders; - } - async getRotatedData( originalUserKey: UserKey, newUserKey: UserKey, @@ -191,7 +174,7 @@ export class FolderService implements InternalFolderServiceAbstraction { } let encryptedFolders: FolderWithIdRequest[] = []; - const folders = await firstValueFrom(this.folderViews$); + const folders = await firstValueFrom(this.folderViews$(of(userId))); if (!folders) { return encryptedFolders; } @@ -203,4 +186,27 @@ export class FolderService implements InternalFolderServiceAbstraction { ); return encryptedFolders; } + + /** + * @returns a SingleUserState for the encrypted folders. + */ + private encryptedFoldersState(userId: UserId) { + return this.stateProvider.getUser(userId, FOLDER_ENCRYPTED_FOLDERS); + } + + /** + * + * @returns a SingleUserState for the decrypted folders. + */ + private decryptedFoldersState(userId: UserId): DerivedState { + return this.stateProvider.getDerived( + this.encryptedFoldersState(userId).combinedState$, + FOLDER_DECRYPTED_FOLDERS, + { + encryptService: this.encryptService, + i18nService: this.i18nService, + keyService: this.keyService, + }, + ); + } } diff --git a/libs/common/src/vault/services/key-state/folder.state.ts b/libs/common/src/vault/services/key-state/folder.state.ts index 7262d72d58e..35f941274b5 100644 --- a/libs/common/src/vault/services/key-state/folder.state.ts +++ b/libs/common/src/vault/services/key-state/folder.state.ts @@ -1,8 +1,13 @@ +import { firstValueFrom } from "rxjs"; import { Jsonify } from "type-fest"; -import { KeyService } from "../../../../../key-management/src/abstractions/key.service"; +import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service"; +import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { Utils } from "@bitwarden/common/platform/misc/utils"; +import { UserId } from "@bitwarden/common/types/guid"; +import { KeyService } from "@bitwarden/key-management"; + import { DeriveDefinition, FOLDER_DISK, UserKeyDefinition } from "../../../platform/state"; -import { FolderService } from "../../abstractions/folder/folder.service.abstraction"; import { FolderData } from "../../models/data/folder.data"; import { Folder } from "../../models/domain/folder"; import { FolderView } from "../../models/view/folder.view"; @@ -16,19 +21,37 @@ export const FOLDER_ENCRYPTED_FOLDERS = UserKeyDefinition.record( }, ); -export const FOLDER_DECRYPTED_FOLDERS = DeriveDefinition.from< - Record, +export const FOLDER_DECRYPTED_FOLDERS = new DeriveDefinition< + [UserId, Record], FolderView[], - { folderService: FolderService; keyService: KeyService } ->(FOLDER_ENCRYPTED_FOLDERS, { + { + encryptService: EncryptService; + i18nService: I18nService; + keyService: KeyService; + } +>(FOLDER_DISK, "decryptedFolders", { deserializer: (obj) => obj.map((f) => FolderView.fromJSON(f)), - derive: async (from, { folderService, keyService }) => { - const folders = Object.values(from || {}).map((f) => new Folder(f)); - - if (await keyService.hasUserKey()) { - return await folderService.decryptFolders(folders); - } else { + derive: async ([userId, folderData], { encryptService, i18nService, keyService }) => { + if (!folderData) { return []; } + + const folders = Object.values(folderData).map((f) => new Folder(f)); + + const userKey = await firstValueFrom(keyService.userKey$(userId)); + if (!userKey) { + return []; + } + + const decryptFolderPromises = folders.map((f) => f.decryptWithKey(userKey, encryptService)); + const decryptedFolders = await Promise.all(decryptFolderPromises); + + decryptedFolders.sort(Utils.getSortFunction(i18nService, "name")); + + const noneFolder = new FolderView(); + noneFolder.name = i18nService.t("noneFolder"); + decryptedFolders.push(noneFolder); + + return decryptedFolders; }, });