From b0c81f461e45fdfe05cb3d53cd9e19533006f49c Mon Sep 17 00:00:00 2001 From: Leslie Tilton <23057410+Banrion@users.noreply.github.com> Date: Tue, 14 Oct 2025 12:21:03 -0500 Subject: [PATCH] Updated migration of critical applications and merged old saved data to new critical applications on report object --- .../risk-insights/models/api-models.types.ts | 19 +- .../models/drawer-models.types.ts | 2 +- .../api/risk-insights-api.service.spec.ts | 4 +- .../services/api/risk-insights-api.service.ts | 18 +- .../domain/critical-apps.service.spec.ts | 4 +- .../services/domain/critical-apps.service.ts | 48 ++- .../risk-insights-encryption.service.spec.ts | 3 + .../risk-insights-encryption.service.ts | 131 ++++++-- ...risk-insights-orchestrator.service.spec.ts | 48 ++- .../risk-insights-orchestrator.service.ts | 293 +++++++++--------- .../domain/risk-insights-report.service.ts | 7 +- .../reports/risk-insights/services/index.ts | 14 +- .../view/risk-insights-data.service.ts | 7 +- .../access-intelligence.module.ts | 7 +- .../risk-insights.component.html | 2 +- .../risk-insights.component.ts | 8 +- 16 files changed, 381 insertions(+), 234 deletions(-) diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/models/api-models.types.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/models/api-models.types.ts index bf963dad8f8..529789ebb8d 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/models/api-models.types.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/models/api-models.types.ts @@ -1,6 +1,6 @@ import { EncString } from "@bitwarden/common/key-management/crypto/models/enc-string"; import { BaseResponse } from "@bitwarden/common/models/response/base.response"; -import { OrganizationId } from "@bitwarden/common/types/guid"; +import { OrganizationId, OrganizationReportId } from "@bitwarden/common/types/guid"; import { createNewSummaryData } from "../helpers"; @@ -46,11 +46,11 @@ export interface SaveRiskInsightsReportRequest { } export class SaveRiskInsightsReportResponse extends BaseResponse { - id: string; + id: OrganizationReportId; constructor(response: any) { super(response); - this.id = this.getResponseProperty("organizationId"); + this.id = this.getResponseProperty("id"); } } export function isSaveRiskInsightsReportResponse(obj: any): obj is SaveRiskInsightsReportResponse { @@ -69,7 +69,7 @@ export class GetRiskInsightsReportResponse extends BaseResponse { constructor(response: any) { super(response); - this.id = this.getResponseProperty("organizationId"); + this.id = this.getResponseProperty("id"); this.organizationId = this.getResponseProperty("organizationId"); this.creationDate = new Date(this.getResponseProperty("creationDate")); this.reportData = new EncString(this.getResponseProperty("reportData")); @@ -130,3 +130,14 @@ export class MemberCipherDetailsResponse extends BaseResponse { this.cipherIds = this.getResponseProperty("CipherIds"); } } + +export interface UpdateRiskInsightsApplicationDataRequest { + data: { + applicationData: string; + }; +} +export class UpdateRiskInsightsApplicationDataResponse extends BaseResponse { + constructor(response: any) { + super(response); + } +} diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/models/drawer-models.types.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/models/drawer-models.types.ts index 5824dede1f9..dffb22af3ee 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/models/drawer-models.types.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/models/drawer-models.types.ts @@ -18,6 +18,7 @@ export type DrawerDetails = { appAtRiskMembers?: AppAtRiskMembersDialogParams | null; atRiskAppDetails?: AtRiskApplicationDetail[] | null; }; + export type AppAtRiskMembersDialogParams = { members: MemberDetails[]; applicationName: string; @@ -28,7 +29,6 @@ export type AppAtRiskMembersDialogParams = { * At risk member detail that contains the email * and the count of at risk ciphers */ - export type AtRiskMemberDetail = { email: string; atRiskPasswordCount: number; diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/api/risk-insights-api.service.spec.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/api/risk-insights-api.service.spec.ts index d6e615ee58a..28520dc4619 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/api/risk-insights-api.service.spec.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/api/risk-insights-api.service.spec.ts @@ -233,7 +233,9 @@ describe("RiskInsightsApiService", () => { mockApiService.send.mockResolvedValueOnce(undefined); const result = await firstValueFrom( - service.updateRiskInsightsApplicationData$(mockApplication.encryptedString, orgId, reportId), + service.updateRiskInsightsApplicationData$(reportId, orgId, { + data: { applicationData: mockApplication.encryptedString! }, + }), ); expect(mockApiService.send).toHaveBeenCalledWith( "PATCH", diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/api/risk-insights-api.service.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/api/risk-insights-api.service.ts index d71f84cc5b2..d1896f487b2 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/api/risk-insights-api.service.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/api/risk-insights-api.service.ts @@ -4,7 +4,11 @@ import { ApiService } from "@bitwarden/common/abstractions/api.service"; import { ErrorResponse } from "@bitwarden/common/models/response/error.response"; import { OrganizationId, OrganizationReportId } from "@bitwarden/common/types/guid"; -import { EncryptedDataWithKey } from "../../models"; +import { + EncryptedDataWithKey, + UpdateRiskInsightsApplicationDataRequest, + UpdateRiskInsightsApplicationDataResponse, +} from "../../models"; import { GetRiskInsightsApplicationDataResponse, GetRiskInsightsReportResponse, @@ -102,18 +106,20 @@ export class RiskInsightsApiService { } updateRiskInsightsApplicationData$( - applicationData: string, - orgId: OrganizationId, reportId: OrganizationReportId, - ): Observable { + orgId: OrganizationId, + request: UpdateRiskInsightsApplicationDataRequest, + ): Observable { const dbResponse = this.apiService.send( "PATCH", `/reports/organizations/${orgId.toString()}/data/application/${reportId.toString()}`, - applicationData, + { ...request.data, id: reportId, organizationId: orgId }, true, true, ); - return from(dbResponse as Promise); + return from(dbResponse).pipe( + map((response) => new UpdateRiskInsightsApplicationDataResponse(response)), + ); } } diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/critical-apps.service.spec.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/critical-apps.service.spec.ts index a0281c1e1a9..d69814572c7 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/critical-apps.service.spec.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/critical-apps.service.spec.ts @@ -181,7 +181,7 @@ describe("CriticalAppsService", () => { privateCriticalAppsSubject.next(initialList); // act - await service.dropCriticalApp(SomeOrganization, selectedUrl); + await service.dropCriticalAppByUrl(SomeOrganization, selectedUrl); // expectations expect(criticalAppsApiService.dropCriticalApp).toHaveBeenCalledWith({ @@ -213,7 +213,7 @@ describe("CriticalAppsService", () => { privateCriticalAppsSubject.next(initialList); // act - await service.dropCriticalApp(SomeOrganization, selectedUrl); + await service.dropCriticalAppByUrl(SomeOrganization, selectedUrl); // expectations expect(criticalAppsApiService.dropCriticalApp).not.toHaveBeenCalled(); diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/critical-apps.service.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/critical-apps.service.ts index 4186880236c..d305de57603 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/critical-apps.service.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/critical-apps.service.ts @@ -1,13 +1,17 @@ import { BehaviorSubject, + catchError, filter, first, firstValueFrom, forkJoin, + from, map, Observable, of, switchMap, + tap, + throwError, zip, } from "rxjs"; @@ -124,9 +128,14 @@ export class CriticalAppsService { this.criticalAppsListSubject$.next(updatedList); } - // Drop a critical app for a given organization - // Only one app may be dropped at a time - async dropCriticalApp(orgId: OrganizationId, selectedUrl: string) { + /** + * Drop a critical application by url + * + * @param orgId + * @param selectedUrl + * @returns + */ + async dropCriticalAppByUrl(orgId: OrganizationId, selectedUrl: string) { if (orgId != this.organizationId.value) { throw new Error("Organization ID mismatch"); } @@ -140,16 +149,41 @@ export class CriticalAppsService { } // TODO Uncomment when done testing that the migration is working - // await this.criticalAppsApiService.dropCriticalApp({ - // organizationId: app.organizationId, - // passwordHealthReportApplicationIds: [app.id], - // }); + await this.criticalAppsApiService.dropCriticalApp({ + organizationId: app.organizationId, + passwordHealthReportApplicationIds: [app.id], + }); this.criticalAppsListSubject$.next( this.criticalAppsListSubject$.value.filter((f) => f.uri !== selectedUrl), ); } + /** + * Drop multiple critical applications by id + * + * @param orgId + * @param ids + * @returns + */ + dropCriticalAppsById(orgId: OrganizationId, ids: string[]) { + return from( + this.criticalAppsApiService.dropCriticalApp({ + organizationId: orgId, + passwordHealthReportApplicationIds: ids, + }), + ).pipe( + tap((response) => { + this.criticalAppsListSubject$.next( + this.criticalAppsListSubject$.value.filter((f) => ids.some((id) => id === f.id)), + ); + }), + catchError((error: unknown) => { + return throwError(() => error); + }), + ); + } + private retrieveCriticalApps( orgId: OrganizationId | null, ): Observable { diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-encryption.service.spec.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-encryption.service.spec.ts index 42de26a2022..2efd97b3c30 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-encryption.service.spec.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-encryption.service.spec.ts @@ -9,6 +9,7 @@ import { makeSymmetricCryptoKey } from "@bitwarden/common/spec"; import { OrganizationId, UserId } from "@bitwarden/common/types/guid"; import { OrgKey } from "@bitwarden/common/types/key"; import { KeyService } from "@bitwarden/key-management"; +import { LogService } from "@bitwarden/logging"; import { EncryptedReportData, DecryptedReportData } from "../../models"; import { mockApplicationData, mockReportData, mockSummaryData } from "../../models/mocks/mock-data"; @@ -20,6 +21,7 @@ describe("RiskInsightsEncryptionService", () => { const mockKeyService = mock(); const mockEncryptService = mock(); const mockKeyGenerationService = mock(); + const mockLogService = mock(); const ENCRYPTED_TEXT = "This data has been encrypted"; const ENCRYPTED_KEY = "Re-encrypted Cipher Key"; @@ -43,6 +45,7 @@ describe("RiskInsightsEncryptionService", () => { mockKeyService, mockEncryptService, mockKeyGenerationService, + mockLogService, ); jest.clearAllMocks(); diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-encryption.service.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-encryption.service.ts index 6cb5acf4ec6..5206cd1ecff 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-encryption.service.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-encryption.service.ts @@ -6,14 +6,24 @@ import { EncString } from "@bitwarden/common/key-management/crypto/models/enc-st import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; import { OrganizationId, UserId } from "@bitwarden/common/types/guid"; import { KeyService } from "@bitwarden/key-management"; +import { LogService } from "@bitwarden/logging"; -import { DecryptedReportData, EncryptedReportData, EncryptedDataWithKey } from "../../models"; +import { createNewSummaryData } from "../../helpers"; +import { + DecryptedReportData, + EncryptedReportData, + EncryptedDataWithKey, + ApplicationHealthReportDetail, + OrganizationReportSummary, + OrganizationReportApplication, +} from "../../models"; export class RiskInsightsEncryptionService { constructor( private keyService: KeyService, private encryptService: EncryptService, private keyGeneratorService: KeyGenerationService, + private logService: LogService, ) {} async encryptRiskInsightsReport( @@ -24,6 +34,7 @@ export class RiskInsightsEncryptionService { data: DecryptedReportData, wrappedKey?: EncString, ): Promise { + this.logService.info("[RiskInsightsEncryptionService] Encrypting risk insights report"); const { userId, organizationId } = context; const orgKey = await firstValueFrom( this.keyService @@ -36,16 +47,24 @@ export class RiskInsightsEncryptionService { ); if (!orgKey) { + this.logService.warning( + "[RiskInsightsEncryptionService] Attempted to encrypt report data without org id", + ); throw new Error("Organization key not found"); } let contentEncryptionKey: SymmetricCryptoKey; - if (!wrappedKey) { - // Generate a new key - contentEncryptionKey = await this.keyGeneratorService.createKey(512); - } else { - // Unwrap the existing key - contentEncryptionKey = await this.encryptService.unwrapSymmetricKey(wrappedKey, orgKey); + try { + if (!wrappedKey) { + // Generate a new key + contentEncryptionKey = await this.keyGeneratorService.createKey(512); + } else { + // Unwrap the existing key + contentEncryptionKey = await this.encryptService.unwrapSymmetricKey(wrappedKey, orgKey); + } + } catch (error: unknown) { + this.logService.error("[RiskInsightsEncryptionService] Failed to get encryption key", error); + throw new Error("Failed to get encryption key"); } const { reportData, summaryData, applicationData } = data; @@ -75,6 +94,9 @@ export class RiskInsightsEncryptionService { !encryptedApplicationData.encryptedString || !wrappedEncryptionKey.encryptedString ) { + this.logService.error( + "[RiskInsightsEncryptionService] Encryption failed, encrypted strings are null", + ); throw new Error("Encryption failed, encrypted strings are null"); } @@ -97,6 +119,8 @@ export class RiskInsightsEncryptionService { encryptedData: EncryptedReportData, wrappedKey: EncString, ): Promise { + this.logService.info("[RiskInsightsEncryptionService] Decrypting risk insights report"); + const { userId, organizationId } = context; const orgKey = await firstValueFrom( this.keyService @@ -109,47 +133,106 @@ export class RiskInsightsEncryptionService { ); if (!orgKey) { + this.logService.warning( + "[RiskInsightsEncryptionService] Attempted to decrypt report data without org id", + ); throw new Error("Organization key not found"); } const unwrappedEncryptionKey = await this.encryptService.unwrapSymmetricKey(wrappedKey, orgKey); if (!unwrappedEncryptionKey) { + this.logService.error("[RiskInsightsEncryptionService] Encryption key not found"); throw Error("Encryption key not found"); } const { encryptedReportData, encryptedSummaryData, encryptedApplicationData } = encryptedData; - if (!encryptedReportData || !encryptedSummaryData || !encryptedApplicationData) { - throw new Error("Missing data"); - } // Decrypt the data - const decryptedReportData = await this.encryptService.decryptString( + const decryptedReportData = await this._handleDecryptReport( encryptedReportData, unwrappedEncryptionKey, ); - const decryptedSummaryData = await this.encryptService.decryptString( + const decryptedSummaryData = await this._handleDecryptSummary( encryptedSummaryData, unwrappedEncryptionKey, ); - const decryptedApplicationData = await this.encryptService.decryptString( + const decryptedApplicationData = await this._handleDecryptApplication( encryptedApplicationData, unwrappedEncryptionKey, ); - if (!decryptedReportData || !decryptedSummaryData || !decryptedApplicationData) { - throw new Error("Decryption failed, decrypted strings are null"); - } - - const decryptedReportDataJson = JSON.parse(decryptedReportData); - const decryptedSummaryDataJson = JSON.parse(decryptedSummaryData); - const decryptedApplicationDataJson = JSON.parse(decryptedApplicationData); - const decryptedFullReport = { - reportData: decryptedReportDataJson, - summaryData: decryptedSummaryDataJson, - applicationData: decryptedApplicationDataJson, + reportData: decryptedReportData, + summaryData: decryptedSummaryData, + applicationData: decryptedApplicationData, }; return decryptedFullReport; } + + private async _handleDecryptReport( + encryptedData: EncString | null, + key: SymmetricCryptoKey, + ): Promise { + if (encryptedData == null) { + return []; + } + + try { + const decryptedData = await this.encryptService.decryptString(encryptedData, key); + const parsedData = JSON.parse(decryptedData); + + // TODO Add type guard to check that parsed data is actual type + return parsedData as ApplicationHealthReportDetail[]; + } catch (error: unknown) { + this.logService.error("[RiskInsightsEncryptionService] Failed to decrypt report", error); + return []; + } + } + + private async _handleDecryptSummary( + encryptedData: EncString | null, + key: SymmetricCryptoKey, + ): Promise { + if (encryptedData == null) { + return createNewSummaryData(); + } + + try { + const decryptedData = await this.encryptService.decryptString(encryptedData, key); + const parsedData = JSON.parse(decryptedData); + + // TODO Add type guard to check that parsed data is actual type + return parsedData as OrganizationReportSummary; + } catch (error: unknown) { + this.logService.error( + "[RiskInsightsEncryptionService] Failed to decrypt report summary", + error, + ); + return createNewSummaryData(); + } + } + + private async _handleDecryptApplication( + encryptedData: EncString | null, + key: SymmetricCryptoKey, + ): Promise { + if (encryptedData == null) { + return []; + } + + try { + const decryptedData = await this.encryptService.decryptString(encryptedData, key); + const parsedData = JSON.parse(decryptedData); + + // TODO Add type guard to check that parsed data is actual type + return parsedData as OrganizationReportApplication[]; + } catch (error: unknown) { + this.logService.error( + "[RiskInsightsEncryptionService] Failed to decrypt report applications", + error, + ); + return []; + } + } } diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.spec.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.spec.ts index 1ae37ea447b..1e6fad1b021 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.spec.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.spec.ts @@ -8,7 +8,7 @@ import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.servi import { LogService } from "@bitwarden/logging"; import { createNewSummaryData } from "../../helpers"; -import { ReportState } from "../../models"; +import { RiskInsightsData, SaveRiskInsightsReportResponse } from "../../models"; import { mockApplicationData, mockEnrichedReportData, @@ -68,18 +68,14 @@ describe("RiskInsightsOrchestratorService", () => { const privateOrganizationDetailsSubject = service["_organizationDetailsSubject"]; const privateUserIdSubject = service["_userIdSubject"]; // Arrange - const reportState: ReportState = { - loading: false, - error: null, - data: { - id: mockReportId, - reportData: [], - summaryData: createNewSummaryData(), - applicationData: [], - creationDate: new Date(), - }, + const reportState: RiskInsightsData = { + id: mockReportId, + reportData: [], + summaryData: createNewSummaryData(), + applicationData: [], + creationDate: new Date(), }; - mockReportService.getRiskInsightsReport$.mockReturnValueOnce(of(reportState.data)); + mockReportService.getRiskInsightsReport$.mockReturnValueOnce(of(reportState)); // Set up organization and user context privateOrganizationDetailsSubject.next({ organizationId: mockOrgId, @@ -97,7 +93,7 @@ describe("RiskInsightsOrchestratorService", () => { mockOrgId, mockUserId, ); - expect(state.data).toEqual(reportState.data); + expect(state.data).toEqual(reportState); done(); } }); @@ -134,7 +130,9 @@ describe("RiskInsightsOrchestratorService", () => { mockReportService.generateApplicationsReport.mockReturnValueOnce(mockEnrichedReportData); mockReportService.getApplicationsSummary.mockReturnValueOnce(mockSummaryData); mockReportService.getOrganizationApplications.mockReturnValueOnce(mockApplicationData); - mockReportService.saveRiskInsightsReport$.mockReturnValueOnce(of(null)); + mockReportService.saveRiskInsightsReport$.mockReturnValueOnce( + of({ id: mockReportId } as SaveRiskInsightsReportResponse), + ); privateOrganizationDetailsSubject.next({ organizationId: mockOrgId, organizationName: mockOrgName, @@ -146,7 +144,7 @@ describe("RiskInsightsOrchestratorService", () => { // Assert service.rawReportData$.subscribe((state) => { - if (!state.loading) { + if (!state.loading && state.data) { expect(mockReportService.generateApplicationsReport).toHaveBeenCalledWith(mockOrgId); expect(mockReportService.saveRiskInsightsReport$).toHaveBeenCalledWith( mockEnrichedReportData, @@ -189,22 +187,20 @@ describe("RiskInsightsOrchestratorService", () => { }); describe("destroy", () => { - it("should complete destroy$ subject and unsubscribe reportStateSubscription", (done) => { - const privateDestroy = service["_destroy$"]; - const privateReportStateSubscription = service["_reportStateSubscription"]; + it("should complete destroy$ subject and unsubscribe reportStateSubscription", () => { + const privateDestroy = (service as any)._destroy$; + const privateReportStateSubscription = (service as any)._reportStateSubscription; + + // Spy on the methods you expect to be called. + const destroyCompleteSpy = jest.spyOn(privateDestroy, "complete"); const unsubscribeSpy = jest.spyOn(privateReportStateSubscription, "unsubscribe"); + // Execute the destroy method. service.destroy(); + // Assert that the methods were called as expected. + expect(destroyCompleteSpy).toHaveBeenCalled(); expect(unsubscribeSpy).toHaveBeenCalled(); - privateDestroy.subscribe({ - error: (err: unknown) => { - done.fail("Should not error: " + err); - }, - complete: () => { - done(); - }, - }); }); }); }); diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.ts index fa245d329e8..5c45c3dfd39 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.ts @@ -8,6 +8,7 @@ import { of, Subject, Subscription, + throwError, } from "rxjs"; import { catchError, @@ -36,8 +37,16 @@ import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.servi import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { LogService } from "@bitwarden/logging"; -import { buildPasswordUseMap, flattenMemberDetails, getTrimmedCipherUris } from "../../helpers"; -import { ApplicationHealthReportDetailEnriched } from "../../models"; +import { + buildPasswordUseMap, + createNewSummaryData, + flattenMemberDetails, + getTrimmedCipherUris, +} from "../../helpers"; +import { + ApplicationHealthReportDetailEnriched, + PasswordHealthReportApplicationsResponse, +} from "../../models"; import { RiskInsightsEnrichedData } from "../../models/report-data-service.types"; import { CipherHealthReport, @@ -92,7 +101,9 @@ export class RiskInsightsOrchestratorService { // --------------------------- Trigger subjects --------------------- private _initializeOrganizationTriggerSubject = new Subject(); private _fetchReportTriggerSubject = new Subject(); - private _reportStateSubscription: Subscription; + + private _reportStateSubscription: Subscription | null = null; + private _migrationSubscription: Subscription | null = null; constructor( private accountService: AccountService, @@ -121,6 +132,9 @@ export class RiskInsightsOrchestratorService { if (this._reportStateSubscription) { this._reportStateSubscription.unsubscribe(); } + if (this._migrationSubscription) { + this._migrationSubscription.unsubscribe(); + } this._destroy$.next(); this._destroy$.complete(); } @@ -151,50 +165,33 @@ export class RiskInsightsOrchestratorService { this._initializeOrganizationTriggerSubject.next(organizationId); } - setCriticalApplications$(criticalApplications: string[]): Observable { + saveCriticalApplications$(criticalApplications: string[]): Observable { + this.logService.debug( + "[RiskInsightsOrchestratorService] Saving critical applications to report", + ); return this.rawReportData$.pipe( take(1), - withLatestFrom(this.organizationDetails$, this._userId$), + filter((data) => !data.loading && data.data != null), + withLatestFrom( + this.organizationDetails$.pipe(filter((org) => !!org && !!org.organizationId)), + this._userId$.pipe(filter((userId) => !!userId)), + ), map(([reportState, organizationDetails, userId]) => { - if (!organizationDetails) { - this.logService.warning( - "[RiskInsightsOrchestratorService] No organization details available when setting critical applications.", - ); - return { - reportState, - organizationDetails: null, - updatedState: reportState, - }; // Return current state if no org details - } - - // Handle the case where there is no report data - if (!reportState?.data) { - this.logService.warning( - "[RiskInsightsOrchestratorService] Attempted to set critical applications with no report data.", - ); - return { - reportState, - organizationDetails, - updatedState: reportState, - }; - } - // Create a set for quick lookup of the new critical apps const newCriticalAppNamesSet = new Set(criticalApplications); - - const existingApplicationData = reportState.data.applicationData || []; + const existingApplicationData = reportState?.data?.applicationData || []; const updatedApplicationData = this._mergeApplicationData( existingApplicationData, newCriticalAppNamesSet, ); - const updatedState: ReportState = { + const updatedState = { ...reportState, data: { ...reportState.data, applicationData: updatedApplicationData, }, - }; + } as ReportState; this.logService.debug( "[RiskInsightsOrchestratorService] Updated applications data", @@ -207,12 +204,12 @@ export class RiskInsightsOrchestratorService { this.riskInsightsEncryptionService.encryptRiskInsightsReport( { organizationId: organizationDetails!.organizationId, - userId, + userId: userId!, }, { - reportData: reportState.data.reportData, - summaryData: reportState.data.summaryData, - applicationData: updatedState.data.applicationData, + reportData: reportState?.data?.reportData ?? [], + summaryData: reportState?.data?.summaryData ?? createNewSummaryData(), + applicationData: updatedState?.data?.applicationData ?? [], }, ), ).pipe( @@ -225,19 +222,25 @@ export class RiskInsightsOrchestratorService { ); }), switchMap(({ reportState, organizationDetails, updatedState, encryptedData }) => { - // Chain the save operation using switchMap + this.logService.debug( + `[RiskInsightsOrchestratorService] Saving updated applicationData with report id: ${reportState?.data?.id} and org id: ${organizationDetails?.organizationId}`, + ); + if (!reportState?.data?.id || !organizationDetails?.organizationId) { + return of({ ...reportState }); + } return this.reportApiService .updateRiskInsightsApplicationData$( - encryptedData.encryptedApplicationData.encryptedString, - organizationDetails.organizationId, reportState.data.id, + organizationDetails.organizationId, + { + data: { + applicationData: encryptedData.encryptedApplicationData.toSdk(), + }, + }, ) .pipe( - // Map the result of the save operation to the updated state map(() => updatedState), - // Use tap to push the updated state to the subject tap((finalState) => this._rawReportDataSubject.next(finalState)), - // Handle errors from the save operation catchError((error: unknown) => { this.logService.error("Failed to save updated applicationData", error); return of({ ...reportState, error: "Failed to save application data" }); @@ -263,6 +266,9 @@ export class RiskInsightsOrchestratorService { }, }), ), + tap((fetchedReport) => + this.logService.debug("[RiskInsightsOrchestratorService] _fetchReport$", fetchedReport), + ), catchError(() => of({ loading: false, error: "Failed to fetch report", data: null })), startWith({ loading: true, error: null, data: null }), ); @@ -279,18 +285,17 @@ export class RiskInsightsOrchestratorService { return forkJoin([this._ciphers$, memberCiphers$]).pipe( tap(() => this.logService.debug("[RiskInsightsOrchestratorService] Generating new report")), - switchMap(([ciphers, memberCiphers]) => this._getCipherHealth(ciphers, memberCiphers)), + switchMap(([ciphers, memberCiphers]) => this._getCipherHealth(ciphers ?? [], memberCiphers)), map((cipherHealthReports) => this.reportService.generateApplicationsReport(cipherHealthReports), ), withLatestFrom(this.rawReportData$), - map(([report, previousReport]) => ({ report: report, summary: this.reportService.getApplicationsSummary(report), applications: this.reportService.getOrganizationApplications( report, - previousReport.data.applicationData, + previousReport?.data?.applicationData ?? [], ), })), switchMap(({ report, summary, applications }) => @@ -354,7 +359,6 @@ export class RiskInsightsOrchestratorService { const applications = getTrimmedCipherUris(cipher); const weakPasswordDetail = this.passwordHealthService.findWeakPasswordDetails(cipher); const reusedPasswordCount = passwordUseMap.get(cipher.login.password!) ?? 0; - return { cipher, cipherMembers, @@ -364,68 +368,79 @@ export class RiskInsightsOrchestratorService { reusedPasswordCount, exposedPasswordDetail, }, - }; + } as CipherHealthReport; }); }), ); } - private _runMigrationAndCleanup$(): Observable { - // Start with rawReportData$ to ensure it has a value - return this.rawReportData$.pipe( - // Ensure rawReportData has a data payload - filter((reportState) => !!reportState.data), - take(1), // Use the first valid report state - // Now switch to the migration logic - switchMap((rawReportData) => - this.criticalAppsService.criticalAppsList$.pipe( - take(1), - withLatestFrom(this.organizationDetails$), - switchMap(([savedCriticalApps, organizationDetails]) => { - // Check if there are any critical apps to migrate. - if (!savedCriticalApps || savedCriticalApps.length === 0) { - this.logService.debug( - "[RiskInsightsOrchestratorService] No critical apps to migrate.", + private _mergeApplicationData( + existingApplications: OrganizationReportApplication[], + criticalApplications: Set, + ): OrganizationReportApplication[] { + const setToMerge = new Set(criticalApplications); + // First, iterate through the existing apps and update their isCritical flag + const updatedApps = existingApplications.map((app) => { + const foundCritical = setToMerge.has(app.applicationName); + + if (foundCritical) { + setToMerge.delete(app.applicationName); + } + + return { + ...app, + isCritical: foundCritical || app.isCritical, + }; + }); + + setToMerge.forEach((applicationName) => { + updatedApps.push({ + applicationName, + isCritical: true, + reviewedDate: null, + }); + }); + + return updatedApps; + } + + private _runMigrationAndCleanup$(criticalApps: PasswordHealthReportApplicationsResponse[]) { + return of(criticalApps).pipe( + withLatestFrom(this.organizationDetails$), + switchMap(([savedCriticalApps, organizationDetails]) => { + // No saved critical apps for migration + if (!savedCriticalApps || savedCriticalApps.length === 0) { + this.logService.debug("[RiskInsightsOrchestratorService] No critical apps to migrate."); + return of([]); + } + + const criticalAppsNames = savedCriticalApps.map((app) => app.uri); + const criticalAppsIds = savedCriticalApps.map((app) => app.id); + + // Use the setCriticalApplications$ function to update and save the report + return this.saveCriticalApplications$(criticalAppsNames).pipe( + // After setCriticalApplications$ completes, trigger the deletion. + switchMap(() => { + return this.criticalAppsService + .dropCriticalAppsById(organizationDetails!.organizationId, criticalAppsIds) + .pipe( + // After all deletes complete, map to the migrated apps. + tap(() => { + this.logService.debug( + "[RiskInsightsOrchestratorService] Migrated and deleted critical applications.", + ); + }), ); - return of([]); - } - - // Map the saved critical apps to the new format - const migratedApps = savedCriticalApps.map( - (app): OrganizationReportApplication => ({ - applicationName: app.uri, - isCritical: true, - reviewedDate: null, - }), - ); - - // Use the setCriticalApplications$ function to update and save the report - return this.setCriticalApplications$( - migratedApps.map((app) => app.applicationName), - ).pipe( - // After setCriticalApplications$ completes, trigger the deletion. - switchMap(() => { - const deleteObservables = savedCriticalApps.map( - (app) => of(null), - // this.criticalAppsService.dropCriticalApp( - // organizationDetails!.organizationId, - // app.id, - // ), - ); - return forkJoin(deleteObservables).pipe( - // After all deletes complete, map to the migrated apps. - map(() => { - this.logService.debug( - "[RiskInsightsOrchestratorService] Migrated and deleted critical applications.", - ); - return migratedApps; - }), - ); - }), - ); }), - ), - ), + catchError((error: unknown) => { + this.logService.error( + "[RiskInsightsOrchestratorService] Failed to save migrated critical applications", + error, + ); + return throwError(() => error); + }), + ); + }), ); } @@ -481,14 +496,15 @@ export class RiskInsightsOrchestratorService { private _setupEnrichedReportData() { // Setup the enriched report data pipeline const enrichmentSubscription = combineLatest([ - this.rawReportData$.pipe(filter((data) => !!data)), + this.rawReportData$.pipe(filter((data) => !!data && !!data?.data)), this._ciphers$.pipe(filter((data) => !!data)), ]).pipe( switchMap(([rawReportData, ciphers]) => { this.logService.debug( "[RiskInsightsOrchestratorService] Enriching report data with ciphers and critical app status", ); - const criticalApps = rawReportData?.data?.applicationData.filter((app) => app.isCritical); + const criticalApps = + rawReportData?.data?.applicationData.filter((app) => app.isCritical) ?? []; const criticalApplicationNames = new Set(criticalApps.map((ca) => ca.applicationName)); const rawReports = rawReportData.data?.reportData || []; const cipherMap = this.reportService.getApplicationCipherMap(ciphers, rawReports); @@ -499,10 +515,10 @@ export class RiskInsightsOrchestratorService { isMarkedAsCritical: criticalApplicationNames.has(app.applicationName), })); - const enrichedData: RiskInsightsEnrichedData = { + const enrichedData = { ...rawReportData.data, reportData: enrichedReports, - }; + } as RiskInsightsEnrichedData; return of(enrichedData); }), @@ -523,7 +539,7 @@ export class RiskInsightsOrchestratorService { exhaustMap(([orgId, userId]) => this.organizationService.organizations$(userId!).pipe( getOrganizationById(orgId), - map((org) => ({ organizationId: orgId, organizationName: org.name })), + map((org) => ({ organizationId: orgId!, organizationName: org?.name ?? "" })), ), ), tap(async (orgDetails) => { @@ -539,24 +555,39 @@ export class RiskInsightsOrchestratorService { } private _setupMigrationAndCleanup() { - this.criticalAppsService.criticalAppsList$ + const criticalApps$ = this.criticalAppsService.criticalAppsList$.pipe( + tap((criticalApps) => { + this.logService.debug( + "[RiskInsightsOrchestratorService] criticalAppsService.criticalAppsList", + criticalApps, + ); + }), + filter((criticalApps) => criticalApps.length > 0), + take(1), + ); + + const rawReportData$ = this.rawReportData$.pipe( + tap((reportState) => { + this.logService.debug( + `[RiskInsightsOrchestratorService] Report state on _setupMigrationAndCleanup`, + !!reportState.data, + reportState, + ); + }), + filter((reportState) => !!reportState.data), + take(1), + ); + + this._migrationSubscription = forkJoin([criticalApps$, rawReportData$]) .pipe( - filter((criticalApps) => criticalApps.length > 0), - tap(() => { + tap(([criticalApps]) => { this.logService.debug( - "[RiskInsightsOrchestratorService] Detected legacy critical apps, running migration and cleanup.", + `[RiskInsightsOrchestratorService] Detected ${criticalApps.length} legacy critical apps, running migration and cleanup`, + criticalApps, ); }), - switchMap(() => - this._runMigrationAndCleanup$().pipe( - tap((migratedApps) => { - if (migratedApps.length > 0) { - this.logService.debug( - "[RiskInsightsOrchestratorService] Migration and cleanup completed.", - migratedApps, - ); - } - }), + switchMap(([criticalApps, _reportState]) => + this._runMigrationAndCleanup$(criticalApps).pipe( catchError((error: unknown) => { this.logService.error( "[RiskInsightsOrchestratorService] Migration and cleanup failed.", @@ -566,6 +597,7 @@ export class RiskInsightsOrchestratorService { }), ), ), + take(1), ) .subscribe(); } @@ -578,9 +610,9 @@ export class RiskInsightsOrchestratorService { this._userId$.pipe(filter((user) => !!user)), ]).pipe(shareReplay({ bufferSize: 1, refCount: true })); - // A stream for the initial report fetch (triggered by critical apps loading) + // A stream for the initial report fetch const initialReportLoad$ = reportDependencies$.pipe( - take(1), // Fetch only once on initial data load + take(1), exhaustMap(([orgDetails, userId]) => this._fetchReport$(orgDetails!.organizationId, userId!)), ); @@ -598,7 +630,7 @@ export class RiskInsightsOrchestratorService { filter((isRunning) => isRunning), withLatestFrom(reportDependencies$), exhaustMap(([_, [orgDetails, userId]]) => - this._generateNewApplicationsReport$(orgDetails!.organizationId, userId), + this._generateNewApplicationsReport$(orgDetails!.organizationId, userId!), ), ); @@ -633,19 +665,4 @@ export class RiskInsightsOrchestratorService { this._userIdSubject.next(userId); }); } - - private _mergeApplicationData( - existingApps: OrganizationReportApplication[], - newCriticalAppNamesSet: Set, - ): OrganizationReportApplication[] { - // First, iterate through the existing apps and update their isCritical flag - const updatedApps = existingApps.map((app) => { - return { - ...app, - isCritical: newCriticalAppNamesSet.has(app.applicationName) ?? app.isCritical, - }; - }); - - return updatedApps; - } } diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-report.service.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-report.service.ts index 27b3c1b496c..00591b5efe4 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-report.service.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-report.service.ts @@ -171,15 +171,12 @@ export class RiskInsightsReportService { creationDate: response.creationDate, })), catchError((error: unknown) => { - // TODO Handle errors appropriately - // console.error("An error occurred when decrypting report", error); - return EMPTY; + return throwError(() => error); }), ); }), catchError((error: unknown) => { - // console.error("An error occurred when fetching the last report", error); - return EMPTY; + return throwError(() => error); }), ); } diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/index.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/index.ts index c34b0ff6b25..1e14c09d089 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/index.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/index.ts @@ -1,9 +1,11 @@ -export * from "./api/member-cipher-details-api.service"; -export * from "./domain/password-health.service"; -export * from "./domain/critical-apps.service"; export * from "./api/critical-apps-api.service"; +export * from "./api/member-cipher-details-api.service"; export * from "./api/risk-insights-api.service"; -export * from "./domain/risk-insights-report.service"; -export * from "./view/risk-insights-data.service"; -export * from "./view/all-activities.service"; export * from "./api/security-tasks-api.service"; +export * from "./domain/critical-apps.service"; +export * from "./domain/password-health.service"; +export * from "./domain/risk-insights-encryption.service"; +export * from "./domain/risk-insights-orchestrator.service"; +export * from "./domain/risk-insights-report.service"; +export * from "./view/all-activities.service"; +export * from "./view/risk-insights-data.service"; diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/view/risk-insights-data.service.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/view/risk-insights-data.service.ts index 814062ba83f..9eba719bd21 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/view/risk-insights-data.service.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/view/risk-insights-data.service.ts @@ -183,7 +183,10 @@ export class RiskInsightsDataService { // ------------------------------ Critical application methods -------------- saveCriticalApplications(selectedUrls: string[]) { - this.orchestrator.setCriticalApplications$(selectedUrls); + // Saving critical applications to the report + this.orchestrator.saveCriticalApplications$(selectedUrls); + + // Legacy support: also save to the CriticalAppsService for backward compatibility return this.organizationDetails$.pipe( exhaustMap((organizationDetails) => { if (!organizationDetails?.organizationId) { @@ -207,7 +210,7 @@ export class RiskInsightsDataService { if (!organizationDetails?.organizationId) { return EMPTY; } - return this.criticalAppsService.dropCriticalApp( + return this.criticalAppsService.dropCriticalAppByUrl( organizationDetails?.organizationId, hostname, ); diff --git a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/access-intelligence.module.ts b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/access-intelligence.module.ts index 1913f5e8001..326a221d4c7 100644 --- a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/access-intelligence.module.ts +++ b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/access-intelligence.module.ts @@ -70,11 +70,10 @@ import { RiskInsightsComponent } from "./risk-insights.component"; provide: RiskInsightsDataService, deps: [CriticalAppsService, RiskInsightsReportService, RiskInsightsOrchestratorService], }), - { + safeProvider({ provide: RiskInsightsEncryptionService, - useClass: RiskInsightsEncryptionService, - deps: [KeyService, EncryptService, KeyGenerationService], - }, + deps: [KeyService, EncryptService, KeyGenerationService, LogService], + }), safeProvider({ provide: CriticalAppsService, useClass: CriticalAppsService, diff --git a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/risk-insights.component.html b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/risk-insights.component.html index f3488298726..18df046b82c 100644 --- a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/risk-insights.component.html +++ b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/risk-insights.component.html @@ -26,7 +26,7 @@ buttonType="secondary" class="tw-border-none !tw-font-normal tw-cursor-pointer !tw-py-0" tabindex="0" - [bitAction]="refreshData.bind(this)" + [bitAction]="generateReport.bind(this)" > {{ "riskInsightsRunReport" | i18n }} diff --git a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/risk-insights.component.ts b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/risk-insights.component.ts index 849a195a395..e1264b009b8 100644 --- a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/risk-insights.component.ts +++ b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/risk-insights.component.ts @@ -59,7 +59,6 @@ export class RiskInsightsComponent implements OnInit, OnDestroy { private organizationId: OrganizationId = "" as OrganizationId; dataLastUpdated: Date | null = null; - refetching: boolean = false; constructor( private route: ActivatedRoute, @@ -114,19 +113,14 @@ export class RiskInsightsComponent implements OnInit, OnDestroy { } ngOnDestroy(): void { - // The component tells the facade when to stop this.dataService.destroy(); } - runReport = () => { - this.dataService.triggerReport(); - }; - /** * Refreshes the data by re-fetching the applications report. * This will automatically notify child components subscribed to the RiskInsightsDataService observables. */ - refreshData(): void { + generateReport(): void { if (this.organizationId) { this.dataService.triggerReport(); }