From 7dbe1dabfb6cea2ff274db4fca1591bba0b575fd Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Mon, 12 Jan 2026 11:00:39 +0100 Subject: [PATCH] [PM-30285] Fix incorrect number of ciphers and folders being reported in diagnostics tool (#18236) * Fix incorrect number of ciphers and folders being reported in diagnostics tool * Cleanup tests * Cleanup tests * Cleanup --- .../data-recovery/steps/cipher-step.spec.ts | 35 ++ .../data-recovery/steps/cipher-step.ts | 1 + .../data-recovery/steps/folder-step.spec.ts | 404 ++++++++++++++++++ .../data-recovery/steps/folder-step.ts | 2 + 4 files changed, 442 insertions(+) create mode 100644 apps/web/src/app/key-management/data-recovery/steps/folder-step.spec.ts diff --git a/apps/web/src/app/key-management/data-recovery/steps/cipher-step.spec.ts b/apps/web/src/app/key-management/data-recovery/steps/cipher-step.spec.ts index 9ae0600fb2a..7d6ad41cb00 100644 --- a/apps/web/src/app/key-management/data-recovery/steps/cipher-step.spec.ts +++ b/apps/web/src/app/key-management/data-recovery/steps/cipher-step.spec.ts @@ -122,6 +122,41 @@ describe("CipherStep", () => { expect(logger.record).toHaveBeenCalledWith("Cipher ID cipher-3 was undecryptable"); expect(logger.record).toHaveBeenCalledWith("Found 2 undecryptable ciphers"); }); + + it("returns correct results when running diagnostics multiple times", async () => { + const userId = "user-id" as UserId; + const cipher1 = { id: "cipher-1", organizationId: null } as Cipher; + const cipher2 = { id: "cipher-2", organizationId: null } as Cipher; + + const workingData: RecoveryWorkingData = { + userId, + userKey: null, + encryptedPrivateKey: null, + isPrivateKeyCorrupt: false, + ciphers: [cipher1, cipher2], + folders: [], + }; + + // First run: cipher1 succeeds, cipher2 fails + cipherEncryptionService.decrypt + .mockResolvedValueOnce({} as any) + .mockRejectedValueOnce(new Error("Decryption failed")); + + const result1 = await cipherStep.runDiagnostics(workingData, logger); + + expect(result1).toBe(false); + expect(cipherStep.canRecover(workingData)).toBe(true); + + // Second run: all ciphers succeed + cipherEncryptionService.decrypt.mockResolvedValue({} as any); + + const result2 = await cipherStep.runDiagnostics(workingData, logger); + + expect(result2).toBe(true); + expect(cipherStep.canRecover(workingData)).toBe(false); + expect(cipherStep["undecryptableCipherIds"]).toHaveLength(0); + expect(cipherStep["decryptableCipherIds"]).toHaveLength(2); + }); }); describe("canRecover", () => { diff --git a/apps/web/src/app/key-management/data-recovery/steps/cipher-step.ts b/apps/web/src/app/key-management/data-recovery/steps/cipher-step.ts index 01c2d9bc2a1..47000f8880b 100644 --- a/apps/web/src/app/key-management/data-recovery/steps/cipher-step.ts +++ b/apps/web/src/app/key-management/data-recovery/steps/cipher-step.ts @@ -25,6 +25,7 @@ export class CipherStep implements RecoveryStep { } this.undecryptableCipherIds = []; + this.decryptableCipherIds = []; // The tool is currently only implemented to handle ciphers that are corrupt for a user. For an organization, the case of // local user not having access to the organization key is not properly handled here, and should be implemented separately. // For now, this just filters out and does not consider corrupt organization ciphers. diff --git a/apps/web/src/app/key-management/data-recovery/steps/folder-step.spec.ts b/apps/web/src/app/key-management/data-recovery/steps/folder-step.spec.ts new file mode 100644 index 00000000000..8e59007732e --- /dev/null +++ b/apps/web/src/app/key-management/data-recovery/steps/folder-step.spec.ts @@ -0,0 +1,404 @@ +import { mock, MockProxy } from "jest-mock-extended"; + +import { UserKey } from "@bitwarden/common/types/key"; +import { FolderApiServiceAbstraction } from "@bitwarden/common/vault/abstractions/folder/folder-api.service.abstraction"; +import { Folder } from "@bitwarden/common/vault/models/domain/folder"; +import { DialogService } from "@bitwarden/components"; +import { PureCrypto } from "@bitwarden/sdk-internal"; +import { UserId } from "@bitwarden/user-core"; + +import { LogRecorder } from "../log-recorder"; + +import { FolderStep } from "./folder-step"; +import { RecoveryWorkingData } from "./recovery-step"; + +// Mock SdkLoadService +jest.mock("@bitwarden/common/platform/abstractions/sdk/sdk-load.service", () => ({ + SdkLoadService: { + Ready: Promise.resolve(), + }, +})); + +jest.mock("@bitwarden/sdk-internal", () => ({ + PureCrypto: { + symmetric_decrypt_string: jest.fn(), + }, +})); + +describe("FolderStep", () => { + let folderStep: FolderStep; + let folderService: MockProxy; + let dialogService: MockProxy; + let logger: MockProxy; + + const mockUserKey = { + toEncoded: jest.fn().mockReturnValue("encoded-user-key"), + } as unknown as UserKey; + + beforeEach(() => { + folderService = mock(); + dialogService = mock(); + logger = mock(); + + folderStep = new FolderStep(folderService, dialogService); + + jest.clearAllMocks(); + }); + + describe("runDiagnostics", () => { + it("returns false and logs error when userKey is missing", async () => { + const workingData: RecoveryWorkingData = { + userId: "user-id" as UserId, + userKey: null, + encryptedPrivateKey: null, + isPrivateKeyCorrupt: false, + ciphers: [], + folders: [], + }; + + const result = await folderStep.runDiagnostics(workingData, logger); + + expect(result).toBe(false); + expect(logger.record).toHaveBeenCalledWith("Missing user key"); + }); + + it("returns true when all folders are decryptable", async () => { + const folder1 = { id: "folder-1", name: { encryptedString: "encrypted-name-1" } }; + const folder2 = { id: "folder-2", name: { encryptedString: "encrypted-name-2" } }; + + const workingData: RecoveryWorkingData = { + userId: "user-id" as UserId, + userKey: mockUserKey, + encryptedPrivateKey: null, + isPrivateKeyCorrupt: false, + ciphers: [], + folders: [folder1, folder2] as Folder[], + }; + + (PureCrypto.symmetric_decrypt_string as jest.Mock).mockReturnValue("decrypted-name"); + + const result = await folderStep.runDiagnostics(workingData, logger); + + expect(result).toBe(true); + expect(PureCrypto.symmetric_decrypt_string).toHaveBeenCalledWith( + "encrypted-name-1", + "encoded-user-key", + ); + expect(PureCrypto.symmetric_decrypt_string).toHaveBeenCalledWith( + "encrypted-name-2", + "encoded-user-key", + ); + expect(logger.record).toHaveBeenCalledWith("Found 0 undecryptable folders"); + expect(logger.record).toHaveBeenCalledWith("Found 2 decryptable folders"); + }); + + it("returns false and records folders with no name", async () => { + const folder1 = { id: "folder-1", name: { encryptedString: "encrypted-name-1" } }; + const folder2 = { id: "folder-2", name: null as null }; + const folder3 = { id: "folder-3", name: { encryptedString: null as null } }; + + const workingData: RecoveryWorkingData = { + userId: "user-id" as UserId, + userKey: mockUserKey, + encryptedPrivateKey: null, + isPrivateKeyCorrupt: false, + ciphers: [], + folders: [folder1, folder2, folder3] as Folder[], + }; + + (PureCrypto.symmetric_decrypt_string as jest.Mock).mockReturnValue("decrypted-name"); + + const result = await folderStep.runDiagnostics(workingData, logger); + + expect(result).toBe(false); + expect(logger.record).toHaveBeenCalledWith("Folder ID folder-2 has no name"); + expect(logger.record).toHaveBeenCalledWith("Folder ID folder-3 has no name"); + expect(logger.record).toHaveBeenCalledWith("Found 2 undecryptable folders"); + expect(logger.record).toHaveBeenCalledWith("Found 1 decryptable folders"); + }); + + it("returns false and records undecryptable folders", async () => { + const folder1 = { id: "folder-1", name: { encryptedString: "encrypted-name-1" } }; + const folder2 = { id: "folder-2", name: { encryptedString: "encrypted-name-2" } }; + const folder3 = { id: "folder-3", name: { encryptedString: "encrypted-name-3" } }; + + const workingData: RecoveryWorkingData = { + userId: "user-id" as UserId, + userKey: mockUserKey, + encryptedPrivateKey: null, + isPrivateKeyCorrupt: false, + ciphers: [], + folders: [folder1, folder2, folder3] as Folder[], + }; + + (PureCrypto.symmetric_decrypt_string as jest.Mock) + .mockReturnValueOnce("decrypted-name") // folder1 succeeds + .mockImplementationOnce(() => { + throw new Error("Decryption failed"); + }) // folder2 fails + .mockImplementationOnce(() => { + throw new Error("Decryption failed"); + }); // folder3 fails + + const result = await folderStep.runDiagnostics(workingData, logger); + + expect(result).toBe(false); + expect(logger.record).toHaveBeenCalledWith( + "Folder name for folder ID folder-2 was undecryptable", + ); + expect(logger.record).toHaveBeenCalledWith( + "Folder name for folder ID folder-3 was undecryptable", + ); + expect(logger.record).toHaveBeenCalledWith("Found 2 undecryptable folders"); + expect(logger.record).toHaveBeenCalledWith("Found 1 decryptable folders"); + }); + + it("returns correct results when running diagnostics multiple times", async () => { + const folder1 = { id: "folder-1", name: { encryptedString: "encrypted-name-1" } }; + const folder2 = { id: "folder-2", name: { encryptedString: "encrypted-name-2" } }; + + const workingData: RecoveryWorkingData = { + userId: "user-id" as UserId, + userKey: mockUserKey, + encryptedPrivateKey: null, + isPrivateKeyCorrupt: false, + ciphers: [], + folders: [folder1, folder2] as Folder[], + }; + + // First run: folder1 succeeds, folder2 fails + (PureCrypto.symmetric_decrypt_string as jest.Mock) + .mockReturnValueOnce("decrypted-name") + .mockImplementationOnce(() => { + throw new Error("Decryption failed"); + }); + + const result1 = await folderStep.runDiagnostics(workingData, logger); + + expect(result1).toBe(false); + expect(folderStep.canRecover(workingData)).toBe(true); + + // Second run: all folders succeed + (PureCrypto.symmetric_decrypt_string as jest.Mock).mockReturnValue("decrypted-name"); + + const result2 = await folderStep.runDiagnostics(workingData, logger); + + expect(result2).toBe(true); + expect(folderStep.canRecover(workingData)).toBe(false); + expect(folderStep["undecryptableFolderIds"]).toEqual([]); + expect(folderStep["decryptableFolderIds"]).toEqual(["folder-1", "folder-2"]); + }); + }); + + describe("canRecover", () => { + it("returns false when there are no undecryptable folders", async () => { + const workingData: RecoveryWorkingData = { + userId: "user-id" as UserId, + userKey: mockUserKey, + encryptedPrivateKey: null, + isPrivateKeyCorrupt: false, + ciphers: [], + folders: [ + { id: "folder-1", name: { encryptedString: "encrypted-name-1" } }, + { id: "folder-2", name: { encryptedString: "encrypted-name-2" } }, + ] as Folder[], + }; + + (PureCrypto.symmetric_decrypt_string as jest.Mock).mockReturnValue("decrypted-name"); + + await folderStep.runDiagnostics(workingData, logger); + const result = folderStep.canRecover(workingData); + + expect(result).toBe(false); + }); + + it("returns true when there are undecryptable folders but at least one decryptable folder", async () => { + const workingData: RecoveryWorkingData = { + userId: "user-id" as UserId, + userKey: mockUserKey, + encryptedPrivateKey: null, + isPrivateKeyCorrupt: false, + ciphers: [], + folders: [ + { id: "folder-1", name: { encryptedString: "encrypted-name-1" } }, + { id: "folder-2", name: { encryptedString: "encrypted-name-2" } }, + ] as Folder[], + }; + + (PureCrypto.symmetric_decrypt_string as jest.Mock) + .mockReturnValueOnce("decrypted-name") + .mockImplementationOnce(() => { + throw new Error("Decryption failed"); + }); + + await folderStep.runDiagnostics(workingData, logger); + const result = folderStep.canRecover(workingData); + + expect(result).toBe(true); + }); + + it("returns false when all folders are undecryptable", async () => { + const workingData: RecoveryWorkingData = { + userId: "user-id" as UserId, + userKey: mockUserKey, + encryptedPrivateKey: null, + isPrivateKeyCorrupt: false, + ciphers: [], + folders: [ + { id: "folder-1", name: { encryptedString: "encrypted-name-1" } }, + { id: "folder-2", name: { encryptedString: "encrypted-name-2" } }, + ] as Folder[], + }; + + (PureCrypto.symmetric_decrypt_string as jest.Mock).mockImplementation(() => { + throw new Error("Decryption failed"); + }); + + await folderStep.runDiagnostics(workingData, logger); + const result = folderStep.canRecover(workingData); + + expect(result).toBe(false); + }); + }); + + describe("runRecovery", () => { + it("logs and returns early when there are no undecryptable folders", async () => { + const workingData: RecoveryWorkingData = { + userId: "user-id" as UserId, + userKey: mockUserKey, + encryptedPrivateKey: null, + isPrivateKeyCorrupt: false, + ciphers: [], + folders: [], + }; + + await folderStep.runRecovery(workingData, logger); + + expect(logger.record).toHaveBeenCalledWith("No undecryptable folders to recover"); + expect(dialogService.openSimpleDialog).not.toHaveBeenCalled(); + expect(folderService.delete).not.toHaveBeenCalled(); + }); + + it("throws error when userId is missing", async () => { + const folder1 = { id: "folder-1", name: { encryptedString: "encrypted-name-1" } }; + + const workingData: RecoveryWorkingData = { + userId: "user-id" as UserId, + userKey: mockUserKey, + encryptedPrivateKey: null, + isPrivateKeyCorrupt: false, + ciphers: [], + folders: [folder1] as Folder[], + }; + + (PureCrypto.symmetric_decrypt_string as jest.Mock).mockImplementation(() => { + throw new Error("Decryption failed"); + }); + await folderStep.runDiagnostics(workingData, logger); + + // Now set userId to null for recovery + workingData.userId = null; + + await expect(folderStep.runRecovery(workingData, logger)).rejects.toThrow("Missing user ID"); + expect(logger.record).toHaveBeenCalledWith("Missing user ID"); + }); + + it("throws error when user cancels deletion", async () => { + const userId = "user-id" as UserId; + const folder1 = { id: "folder-1", name: { encryptedString: "encrypted-name-1" } }; + + const workingData: RecoveryWorkingData = { + userId, + userKey: mockUserKey, + encryptedPrivateKey: null, + isPrivateKeyCorrupt: false, + ciphers: [], + folders: [folder1] as Folder[], + }; + + (PureCrypto.symmetric_decrypt_string as jest.Mock).mockImplementation(() => { + throw new Error("Decryption failed"); + }); + await folderStep.runDiagnostics(workingData, logger); + + dialogService.openSimpleDialog.mockResolvedValue(false); + + await expect(folderStep.runRecovery(workingData, logger)).rejects.toThrow( + "Folder recovery cancelled by user", + ); + + expect(logger.record).toHaveBeenCalledWith("Showing confirmation dialog for 1 folders"); + expect(logger.record).toHaveBeenCalledWith("User cancelled folder deletion"); + expect(folderService.delete).not.toHaveBeenCalled(); + }); + + it("deletes undecryptable folders when user confirms", async () => { + const userId = "user-id" as UserId; + const folder1 = { id: "folder-1", name: { encryptedString: "encrypted-name-1" } }; + const folder2 = { id: "folder-2", name: { encryptedString: "encrypted-name-2" } }; + + const workingData: RecoveryWorkingData = { + userId, + userKey: mockUserKey, + encryptedPrivateKey: null, + isPrivateKeyCorrupt: false, + ciphers: [], + folders: [folder1, folder2] as Folder[], + }; + + (PureCrypto.symmetric_decrypt_string as jest.Mock).mockImplementation(() => { + throw new Error("Decryption failed"); + }); + await folderStep.runDiagnostics(workingData, logger); + + dialogService.openSimpleDialog.mockResolvedValue(true); + folderService.delete.mockResolvedValue(undefined); + + await folderStep.runRecovery(workingData, logger); + + expect(logger.record).toHaveBeenCalledWith("Showing confirmation dialog for 2 folders"); + expect(logger.record).toHaveBeenCalledWith("Deleting 2 folders"); + expect(folderService.delete).toHaveBeenCalledWith("folder-1", userId); + expect(folderService.delete).toHaveBeenCalledWith("folder-2", userId); + expect(logger.record).toHaveBeenCalledWith("Deleted folder folder-1"); + expect(logger.record).toHaveBeenCalledWith("Deleted folder folder-2"); + expect(logger.record).toHaveBeenCalledWith("Successfully deleted 2 folders"); + }); + + it("continues deleting folders even if some deletions fail", async () => { + const userId = "user-id" as UserId; + const folder1 = { id: "folder-1", name: { encryptedString: "encrypted-name-1" } }; + const folder2 = { id: "folder-2", name: { encryptedString: "encrypted-name-2" } }; + const folder3 = { id: "folder-3", name: { encryptedString: "encrypted-name-3" } }; + + const workingData: RecoveryWorkingData = { + userId, + userKey: mockUserKey, + encryptedPrivateKey: null, + isPrivateKeyCorrupt: false, + ciphers: [], + folders: [folder1, folder2, folder3] as Folder[], + }; + + (PureCrypto.symmetric_decrypt_string as jest.Mock).mockImplementation(() => { + throw new Error("Decryption failed"); + }); + await folderStep.runDiagnostics(workingData, logger); + + dialogService.openSimpleDialog.mockResolvedValue(true); + folderService.delete + .mockResolvedValueOnce(undefined) // folder1 succeeds + .mockRejectedValueOnce(new Error("Network error")) // folder2 fails + .mockResolvedValueOnce(undefined); // folder3 succeeds + + await folderStep.runRecovery(workingData, logger); + + expect(folderService.delete).toHaveBeenCalledTimes(3); + expect(logger.record).toHaveBeenCalledWith("Deleted folder folder-1"); + expect(logger.record).toHaveBeenCalledWith( + "Failed to delete folder folder-2: Error: Network error", + ); + expect(logger.record).toHaveBeenCalledWith("Deleted folder folder-3"); + }); + }); +}); diff --git a/apps/web/src/app/key-management/data-recovery/steps/folder-step.ts b/apps/web/src/app/key-management/data-recovery/steps/folder-step.ts index 90e252ce6c3..2087c360ddc 100644 --- a/apps/web/src/app/key-management/data-recovery/steps/folder-step.ts +++ b/apps/web/src/app/key-management/data-recovery/steps/folder-step.ts @@ -25,6 +25,8 @@ export class FolderStep implements RecoveryStep { } this.undecryptableFolderIds = []; + this.decryptableFolderIds = []; + for (const folder of workingData.folders) { if (!folder.name?.encryptedString) { logger.record(`Folder ID ${folder.id} has no name`);