From 032fedf308ec251f17632d7d08c4daf6f41a4b1d Mon Sep 17 00:00:00 2001 From: Vijay Oommen Date: Wed, 4 Jun 2025 09:04:33 -0500 Subject: [PATCH] [PM-21040] Update Ciphers after editing in Reports (#14590) --- .../pages/cipher-report.component.spec.ts | 122 ++++++++++++++++++ .../reports/pages/cipher-report.component.ts | 64 ++++++++- .../exposed-passwords-report.component.ts | 25 +++- .../inactive-two-factor-report.component.ts | 82 ++++++++---- .../reused-passwords-report.component.ts | 50 ++++++- .../unsecured-websites-report.component.ts | 17 +++ .../pages/weak-passwords-report.component.ts | 57 +++----- 7 files changed, 343 insertions(+), 74 deletions(-) create mode 100644 apps/web/src/app/dirt/reports/pages/cipher-report.component.spec.ts diff --git a/apps/web/src/app/dirt/reports/pages/cipher-report.component.spec.ts b/apps/web/src/app/dirt/reports/pages/cipher-report.component.spec.ts new file mode 100644 index 00000000000..e29ebcd1cfe --- /dev/null +++ b/apps/web/src/app/dirt/reports/pages/cipher-report.component.spec.ts @@ -0,0 +1,122 @@ +import { mock, MockProxy } from "jest-mock-extended"; +import { of } from "rxjs"; + +import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; +import { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction"; +import { CipherType } from "@bitwarden/common/vault/enums/cipher-type"; +import { Cipher } from "@bitwarden/common/vault/models/domain/cipher"; +import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; +import { DialogService } from "@bitwarden/components"; +import { CipherFormConfigService, PasswordRepromptService } from "@bitwarden/vault"; + +import { VaultItemDialogResult } from "../../../vault/components/vault-item-dialog/vault-item-dialog.component"; +import { AdminConsoleCipherFormConfigService } from "../../../vault/org-vault/services/admin-console-cipher-form-config.service"; + +import { CipherReportComponent } from "./cipher-report.component"; + +describe("CipherReportComponent", () => { + let component: CipherReportComponent; + let mockAccountService: MockProxy; + let mockAdminConsoleCipherFormConfigService: MockProxy; + const mockCipher = { + id: "122-333-444", + type: CipherType.Login, + orgId: "222-444-555", + login: { + username: "test-username", + password: "test-password", + totp: "123", + }, + decrypt: jest.fn().mockResolvedValue({ id: "cipher1", name: "Updated" }), + } as unknown as Cipher; + const mockCipherService = mock(); + mockCipherService.get.mockResolvedValue(mockCipher as unknown as Cipher); + mockCipherService.getKeyForCipherKeyDecryption.mockResolvedValue({}); + mockCipherService.deleteWithServer.mockResolvedValue(undefined); + mockCipherService.softDeleteWithServer.mockResolvedValue(undefined); + + beforeEach(() => { + mockAccountService = mock(); + mockAccountService.activeAccount$ = of({ id: "user1" } as any); + mockAdminConsoleCipherFormConfigService = mock(); + + component = new CipherReportComponent( + mockCipherService, + mock(), + mock(), + mock(), + mockAccountService, + mock(), + mock(), + mock(), + mockAdminConsoleCipherFormConfigService, + ); + component.ciphers = []; + component.allCiphers = []; + }); + + it("should remove the cipher from the report if it was deleted", async () => { + const cipherToDelete = { id: "cipher1" } as any; + component.ciphers = [cipherToDelete, { id: "cipher2" } as any]; + + jest.spyOn(component, "determinedUpdatedCipherReportStatus").mockResolvedValue(null); + + await component.refresh(VaultItemDialogResult.Deleted, cipherToDelete); + + expect(component.ciphers).toEqual([{ id: "cipher2" }]); + expect(component.determinedUpdatedCipherReportStatus).toHaveBeenCalledWith( + VaultItemDialogResult.Deleted, + cipherToDelete, + ); + }); + + it("should update the cipher in the report if it was saved", async () => { + const cipherViewToUpdate = { ...mockCipher } as unknown as CipherView; + const updatedCipher = { ...mockCipher, name: "Updated" } as unknown as Cipher; + const updatedCipherView = { ...updatedCipher } as unknown as CipherView; + + component.ciphers = [cipherViewToUpdate]; + mockCipherService.get.mockResolvedValue(updatedCipher); + mockCipherService.getKeyForCipherKeyDecryption.mockResolvedValue("key"); + + jest.spyOn(updatedCipher, "decrypt").mockResolvedValue(updatedCipherView); + + jest + .spyOn(component, "determinedUpdatedCipherReportStatus") + .mockResolvedValue(updatedCipherView); + + await component.refresh(VaultItemDialogResult.Saved, updatedCipherView); + + expect(component.ciphers).toEqual([updatedCipherView]); + expect(component.determinedUpdatedCipherReportStatus).toHaveBeenCalledWith( + VaultItemDialogResult.Saved, + updatedCipherView, + ); + }); + + it("should remove the cipher from the report if it no longer meets the criteria after saving", async () => { + const cipherViewToUpdate = { ...mockCipher } as unknown as CipherView; + const updatedCipher = { ...mockCipher, name: "Updated" } as unknown as Cipher; + const updatedCipherView = { ...updatedCipher } as unknown as CipherView; + + component.ciphers = [cipherViewToUpdate]; + + mockCipherService.get.mockResolvedValue(updatedCipher); + mockCipherService.getKeyForCipherKeyDecryption.mockResolvedValue("key"); + + jest.spyOn(updatedCipher, "decrypt").mockResolvedValue(updatedCipherView); + + jest.spyOn(component, "determinedUpdatedCipherReportStatus").mockResolvedValue(null); + + await component.refresh(VaultItemDialogResult.Saved, updatedCipherView); + + expect(component.ciphers).toEqual([]); + expect(component.determinedUpdatedCipherReportStatus).toHaveBeenCalledWith( + VaultItemDialogResult.Saved, + updatedCipherView, + ); + }); +}); diff --git a/apps/web/src/app/dirt/reports/pages/cipher-report.component.ts b/apps/web/src/app/dirt/reports/pages/cipher-report.component.ts index d6c96ff232e..69dd360ad31 100644 --- a/apps/web/src/app/dirt/reports/pages/cipher-report.component.ts +++ b/apps/web/src/app/dirt/reports/pages/cipher-report.component.ts @@ -213,8 +213,68 @@ export class CipherReportComponent implements OnDestroy { this.allCiphers = []; } - protected async refresh(result: VaultItemDialogResult, cipher: CipherView) { - await this.load(); + async refresh(result: VaultItemDialogResult, cipher: CipherView) { + if (result === VaultItemDialogResult.Deleted) { + // update downstream report status if the cipher was deleted + await this.determinedUpdatedCipherReportStatus(result, cipher); + + // the cipher was deleted, filter it out from the report. + this.ciphers = this.ciphers.filter((ciph) => ciph.id !== cipher.id); + this.filterCiphersByOrg(this.ciphers); + return; + } + + if (result == VaultItemDialogResult.Saved) { + // Ensure we have the latest cipher data after saving. + const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)); + let updatedCipher = await this.cipherService.get(cipher.id, activeUserId); + + if (this.isAdminConsoleActive) { + updatedCipher = await this.adminConsoleCipherFormConfigService.getCipher( + cipher.id as CipherId, + this.organization, + ); + } + + // convert cipher to cipher view model + const updatedCipherView = await updatedCipher.decrypt( + await this.cipherService.getKeyForCipherKeyDecryption(updatedCipher, activeUserId), + ); + + // request downstream report status if the cipher was updated + // this will return a null if the updated cipher does not meet the criteria for the report + const updatedReportResult = await this.determinedUpdatedCipherReportStatus( + result, + updatedCipherView, + ); + + // determine the index of the updated cipher in the report + const index = this.ciphers.findIndex((c) => c.id === updatedCipherView.id); + + // the updated cipher does not meet the criteria for the report, it returns a null + if (updatedReportResult === null) { + this.ciphers.splice(index, 1); + } + + // the cipher is already in the report, update it. + if (updatedReportResult !== null && index > -1) { + this.ciphers[index] = updatedReportResult; + } + + // apply filters and set the data source + this.filterCiphersByOrg(this.ciphers); + } + } + + async determinedUpdatedCipherReportStatus( + result: VaultItemDialogResult, + updatedCipherView: CipherView, + ): Promise { + // Implement the logic to determine if the updated cipher is still in the report. + // This could be checking if the password is still weak or exposed, etc. + // For now, we will return the updated cipher view as is. + // Replace this with your actual logic in the child classes. + return updatedCipherView; } protected async repromptCipher(c: CipherView) { diff --git a/apps/web/src/app/dirt/reports/pages/exposed-passwords-report.component.ts b/apps/web/src/app/dirt/reports/pages/exposed-passwords-report.component.ts index 5710ea1176e..1a4141c4d68 100644 --- a/apps/web/src/app/dirt/reports/pages/exposed-passwords-report.component.ts +++ b/apps/web/src/app/dirt/reports/pages/exposed-passwords-report.component.ts @@ -10,6 +10,7 @@ import { CipherType } from "@bitwarden/common/vault/enums"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { DialogService } from "@bitwarden/components"; import { CipherFormConfigService, PasswordRepromptService } from "@bitwarden/vault"; +import { VaultItemDialogResult } from "@bitwarden/web-vault/app/vault/components/vault-item-dialog/vault-item-dialog.component"; import { AdminConsoleCipherFormConfigService } from "../../../vault/org-vault/services/admin-console-cipher-form-config.service"; @@ -73,10 +74,9 @@ export class ExposedPasswordsReportComponent extends CipherReportComponent imple return; } - const promise = this.auditService.passwordLeaked(login.password).then((exposedCount) => { - if (exposedCount > 0) { - const row = { ...ciph, exposedXTimes: exposedCount } as ReportResult; - exposedPasswordCiphers.push(row); + const promise = this.isPasswordExposed(ciph).then((result) => { + if (result) { + exposedPasswordCiphers.push(result); } }); promises.push(promise); @@ -87,8 +87,25 @@ export class ExposedPasswordsReportComponent extends CipherReportComponent imple this.dataSource.sort = { column: "exposedXTimes", direction: "desc" }; } + private async isPasswordExposed(cv: CipherView): Promise { + const { login } = cv; + return await this.auditService.passwordLeaked(login.password).then((exposedCount) => { + if (exposedCount > 0) { + return { ...cv, exposedXTimes: exposedCount } as ReportResult; + } + return null; + }); + } + protected canManageCipher(c: CipherView): boolean { // this will only ever be false from the org view; return true; } + + async determinedUpdatedCipherReportStatus( + result: VaultItemDialogResult, + updatedCipherView: CipherView, + ): Promise { + return await this.isPasswordExposed(updatedCipherView); + } } diff --git a/apps/web/src/app/dirt/reports/pages/inactive-two-factor-report.component.ts b/apps/web/src/app/dirt/reports/pages/inactive-two-factor-report.component.ts index 95810625dac..0024af35109 100644 --- a/apps/web/src/app/dirt/reports/pages/inactive-two-factor-report.component.ts +++ b/apps/web/src/app/dirt/reports/pages/inactive-two-factor-report.component.ts @@ -13,6 +13,7 @@ import { CipherType } from "@bitwarden/common/vault/enums"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { DialogService } from "@bitwarden/components"; import { CipherFormConfigService, PasswordRepromptService } from "@bitwarden/vault"; +import { VaultItemDialogResult } from "@bitwarden/web-vault/app/vault/components/vault-item-dialog/vault-item-dialog.component"; import { AdminConsoleCipherFormConfigService } from "../../../vault/org-vault/services/admin-console-cipher-form-config.service"; @@ -71,32 +72,12 @@ export class InactiveTwoFactorReportComponent extends CipherReportComponent impl this.filterStatus = [0]; allCiphers.forEach((ciph) => { - const { type, login, isDeleted, edit, id, viewPassword } = ciph; - if ( - type !== CipherType.Login || - (login.totp != null && login.totp !== "") || - !login.hasUris || - isDeleted || - (!this.organization && !edit) || - !viewPassword - ) { - return; - } + const [docFor2fa, isInactive2faCipher] = this.isInactive2faCipher(ciph); - for (let i = 0; i < login.uris.length; i++) { - const u = login.uris[i]; - if (u.uri != null && u.uri !== "") { - const uri = u.uri.replace("www.", ""); - const domain = Utils.getDomain(uri); - if (domain != null && this.services.has(domain)) { - if (this.services.get(domain) != null) { - docs.set(id, this.services.get(domain)); - } - // If the uri is in the 2fa list. Add the cipher to the inactive - // collection. No need to check any additional uris for the cipher. - inactive2faCiphers.push(ciph); - return; - } + if (isInactive2faCipher) { + inactive2faCiphers.push(ciph); + if (docFor2fa !== "") { + docs.set(ciph.id, docFor2fa); } } }); @@ -106,6 +87,39 @@ export class InactiveTwoFactorReportComponent extends CipherReportComponent impl } } + private isInactive2faCipher(cipher: CipherView): [string, boolean] { + let docFor2fa: string = ""; + let isInactive2faCipher: boolean = false; + + const { type, login, isDeleted, edit, viewPassword } = cipher; + if ( + type !== CipherType.Login || + (login.totp != null && login.totp !== "") || + !login.hasUris || + isDeleted || + (!this.organization && !edit) || + !viewPassword + ) { + return [docFor2fa, isInactive2faCipher]; + } + + for (let i = 0; i < login.uris.length; i++) { + const u = login.uris[i]; + if (u.uri != null && u.uri !== "") { + const uri = u.uri.replace("www.", ""); + const domain = Utils.getDomain(uri); + if (domain != null && this.services.has(domain)) { + if (this.services.get(domain) != null) { + docFor2fa = this.services.get(domain) || ""; + } + isInactive2faCipher = true; + break; + } + } + } + return [docFor2fa, isInactive2faCipher]; + } + private async load2fa() { if (this.services.size > 0) { return; @@ -142,4 +156,22 @@ export class InactiveTwoFactorReportComponent extends CipherReportComponent impl // this will only ever be false from the org view; return true; } + + async determinedUpdatedCipherReportStatus( + result: VaultItemDialogResult, + updatedCipherView: CipherView, + ): Promise { + if (result === VaultItemDialogResult.Deleted) { + return null; + } + + const [docFor2fa, isInactive2faCipher] = this.isInactive2faCipher(updatedCipherView); + + if (isInactive2faCipher) { + this.cipherDocs.set(updatedCipherView.id, docFor2fa); + return updatedCipherView; + } + + return null; + } } diff --git a/apps/web/src/app/dirt/reports/pages/reused-passwords-report.component.ts b/apps/web/src/app/dirt/reports/pages/reused-passwords-report.component.ts index 3e9abc779ba..8e1e4fcf0cc 100644 --- a/apps/web/src/app/dirt/reports/pages/reused-passwords-report.component.ts +++ b/apps/web/src/app/dirt/reports/pages/reused-passwords-report.component.ts @@ -11,6 +11,7 @@ import { CipherType } from "@bitwarden/common/vault/enums"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { DialogService } from "@bitwarden/components"; import { CipherFormConfigService, PasswordRepromptService } from "@bitwarden/vault"; +import { VaultItemDialogResult } from "@bitwarden/web-vault/app/vault/components/vault-item-dialog/vault-item-dialog.component"; import { AdminConsoleCipherFormConfigService } from "../../../vault/org-vault/services/admin-console-cipher-form-config.service"; @@ -22,6 +23,7 @@ import { CipherReportComponent } from "./cipher-report.component"; standalone: false, }) export class ReusedPasswordsReportComponent extends CipherReportComponent implements OnInit { + ciphersToCheckForReusedPasswords: CipherView[] = []; passwordUseMap: Map; disabled = true; @@ -54,12 +56,19 @@ export class ReusedPasswordsReportComponent extends CipherReportComponent implem } async setCiphers() { - const allCiphers = await this.getAllCiphers(); + this.ciphersToCheckForReusedPasswords = await this.getAllCiphers(); + const reusedPasswordCiphers = await this.checkCiphersForReusedPasswords( + this.ciphersToCheckForReusedPasswords, + ); + this.filterCiphersByOrg(reusedPasswordCiphers); + } + + protected async checkCiphersForReusedPasswords(ciphers: CipherView[]): Promise { const ciphersWithPasswords: CipherView[] = []; this.passwordUseMap = new Map(); this.filterStatus = [0]; - allCiphers.forEach((ciph) => { + ciphers.forEach((ciph) => { const { type, login, isDeleted, edit, viewPassword } = ciph; if ( type !== CipherType.Login || @@ -84,11 +93,46 @@ export class ReusedPasswordsReportComponent extends CipherReportComponent implem this.passwordUseMap.has(c.login.password) && this.passwordUseMap.get(c.login.password) > 1, ); - this.filterCiphersByOrg(reusedPasswordCiphers); + return reusedPasswordCiphers; } protected canManageCipher(c: CipherView): boolean { // this will only ever be false from an organization view return true; } + + async determinedUpdatedCipherReportStatus( + result: VaultItemDialogResult, + updatedCipherView: CipherView, + ): Promise { + if (result === VaultItemDialogResult.Deleted) { + this.ciphersToCheckForReusedPasswords = this.ciphersToCheckForReusedPasswords.filter( + (c) => c.id !== updatedCipherView.id, + ); + return null; + } + + // recalculate the reused passwords after an update + // if a password was changed, it could affect reused counts of other ciphers + + // find the cipher in our list and update it + const index = this.ciphersToCheckForReusedPasswords.findIndex( + (c) => c.id === updatedCipherView.id, + ); + + if (index !== -1) { + this.ciphersToCheckForReusedPasswords[index] = updatedCipherView; + } + + // Re-check the passwords for reused passwords for all ciphers + const reusedPasswordCiphers = await this.checkCiphersForReusedPasswords( + this.ciphersToCheckForReusedPasswords, + ); + + // set the updated ciphers list to the filtered reused passwords + this.filterCiphersByOrg(reusedPasswordCiphers); + + // return the updated cipher view + return updatedCipherView; + } } diff --git a/apps/web/src/app/dirt/reports/pages/unsecured-websites-report.component.ts b/apps/web/src/app/dirt/reports/pages/unsecured-websites-report.component.ts index d2cc792198e..4b9cc3fd789 100644 --- a/apps/web/src/app/dirt/reports/pages/unsecured-websites-report.component.ts +++ b/apps/web/src/app/dirt/reports/pages/unsecured-websites-report.component.ts @@ -10,6 +10,7 @@ import { CipherType } from "@bitwarden/common/vault/enums"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { DialogService } from "@bitwarden/components"; import { CipherFormConfigService, PasswordRepromptService } from "@bitwarden/vault"; +import { VaultItemDialogResult } from "@bitwarden/web-vault/app/vault/components/vault-item-dialog/vault-item-dialog.component"; import { AdminConsoleCipherFormConfigService } from "../../../vault/org-vault/services/admin-console-cipher-form-config.service"; @@ -93,4 +94,20 @@ export class UnsecuredWebsitesReportComponent extends CipherReportComponent impl // this will only ever be false from the org view; return true; } + + async determinedUpdatedCipherReportStatus( + result: VaultItemDialogResult, + updatedCipherView: CipherView, + ): Promise { + if (result === VaultItemDialogResult.Deleted) { + return null; + } + + // If the cipher still contains unsecured URIs, return it as is + if (this.cipherContainsUnsecured(updatedCipherView)) { + return updatedCipherView; + } + + return null; + } } diff --git a/apps/web/src/app/dirt/reports/pages/weak-passwords-report.component.ts b/apps/web/src/app/dirt/reports/pages/weak-passwords-report.component.ts index 1716a98190c..0472dbfaa6f 100644 --- a/apps/web/src/app/dirt/reports/pages/weak-passwords-report.component.ts +++ b/apps/web/src/app/dirt/reports/pages/weak-passwords-report.component.ts @@ -1,15 +1,12 @@ // FIXME: Update this file to be type safe and remove this and next line // @ts-strict-ignore import { Component, OnInit } from "@angular/core"; -import { firstValueFrom } from "rxjs"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; -import { getUserId } from "@bitwarden/common/auth/services/account.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { Utils } from "@bitwarden/common/platform/misc/utils"; import { PasswordStrengthServiceAbstraction } from "@bitwarden/common/tools/password-strength"; -import { CipherId } from "@bitwarden/common/types/guid"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction"; import { CipherType } from "@bitwarden/common/vault/enums"; @@ -71,46 +68,26 @@ export class WeakPasswordsReportComponent extends CipherReportComponent implemen this.findWeakPasswords(allCiphers); } - protected async refresh(result: VaultItemDialogResult, cipher: CipherView) { + async determinedUpdatedCipherReportStatus( + result: VaultItemDialogResult, + updatedCipherView: CipherView, + ): Promise { if (result === VaultItemDialogResult.Deleted) { - // remove the cipher from the list - this.weakPasswordCiphers = this.weakPasswordCiphers.filter((c) => c.id !== cipher.id); - this.filterCiphersByOrg(this.weakPasswordCiphers); - return; - } - - if (result == VaultItemDialogResult.Saved) { - const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)); - let updatedCipher = await this.cipherService.get(cipher.id, activeUserId); - - if (this.isAdminConsoleActive) { - updatedCipher = await this.adminConsoleCipherFormConfigService.getCipher( - cipher.id as CipherId, - this.organization, - ); - } - - const updatedCipherView = await updatedCipher.decrypt( - await this.cipherService.getKeyForCipherKeyDecryption(updatedCipher, activeUserId), + this.weakPasswordCiphers = this.weakPasswordCiphers.filter( + (c) => c.id !== updatedCipherView.id, ); - // update the cipher views - const updatedReportResult = this.determineWeakPasswordScore(updatedCipherView); - const index = this.weakPasswordCiphers.findIndex((c) => c.id === updatedCipherView.id); - - if (updatedReportResult == null) { - // the password is no longer weak - // remove the cipher from the list - this.weakPasswordCiphers.splice(index, 1); - this.filterCiphersByOrg(this.weakPasswordCiphers); - return; - } - - if (index > -1) { - // update the existing cipher - this.weakPasswordCiphers[index] = updatedReportResult; - this.filterCiphersByOrg(this.weakPasswordCiphers); - } + return null; } + + const updatedReportStatus = await this.determineWeakPasswordScore(updatedCipherView); + + const index = this.weakPasswordCiphers.findIndex((c) => c.id === updatedCipherView.id); + + if (index !== -1) { + this.weakPasswordCiphers[index] = updatedReportStatus; + } + + return updatedReportStatus; } protected findWeakPasswords(ciphers: CipherView[]): void {