PM-14927 Pr comments incorporated

bitwarden_license/bit-common/src/tools/reports/risk-insights/services/critical-apps-api.service.spec.ts

@@ -1,12 +1,12 @@
 import { randomUUID } from "crypto";
 
-import { TestBed } from "@angular/core/testing";
+import { fakeAsync, flush, TestBed } from "@angular/core/testing";
 import { mock } from "jest-mock-extended";
 
 import { ApiService } from "@bitwarden/common/abstractions/api.service";
 import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service";
 import { EncString } from "@bitwarden/common/platform/models/domain/enc-string";
-import { Guid } from "@bitwarden/common/types/guid";
+import { OrganizationId, PasswordHealthReportApplicationId } from "@bitwarden/common/types/guid";
 import { KeyService } from "@bitwarden/key-management";
 
 import {
@@ -75,9 +75,13 @@ describe("CriticalAppsApiService", () => {
   it("should exclude records that already exist", async () => {
     // arrange
     // one record already exists
-    service.criticalApps = [
-      { id: randomUUID() as Guid, organizationId: "org1" as Guid, uri: "https://example.com" },
-    ];
+    service.setAppsInListForOrg([
+      {
+        id: randomUUID() as PasswordHealthReportApplicationId,
+        organizationId: "org1" as OrganizationId,
+        uri: "https://example.com",
+      },
+    ]);
 
     // two records are selected - one already in the database
     const selectedUrls = ["https://example.com", "https://example.org"];
@@ -110,8 +114,8 @@ describe("CriticalAppsApiService", () => {
     );
   });
 
-  it("should get critical apps", async () => {
-    const orgId = "org1";
+  it("should get critical apps", fakeAsync(() => {
+    const orgId = "org1" as OrganizationId;
     const response = [
       { id: "id1", organizationId: "org1", uri: "https://example.com" },
       { id: "id2", organizationId: "org1", uri: "https://example.org" },
@@ -119,9 +123,13 @@ describe("CriticalAppsApiService", () => {
 
     encryptService.decryptToUtf8.mockResolvedValue("https://example.com");
     apiService.send.mockResolvedValue(response);
-    await service.getCriticalApps(orgId);
+    const spy = jest.spyOn(service, "retrieveCriticalApps");
 
-    expect(keyService.getOrgKey).toHaveBeenCalledWith(orgId);
+    service.setOrganizationId(orgId as OrganizationId);
+    flush();
+
+    expect(spy).toHaveBeenCalled();
+    expect(keyService.getOrgKey).toHaveBeenCalledWith(orgId.toString());
     expect(encryptService.decryptToUtf8).toHaveBeenCalledTimes(2);
     expect(apiService.send).toHaveBeenCalledWith(
       "GET",
@@ -130,5 +138,5 @@ describe("CriticalAppsApiService", () => {
       true,
       true,
     );
-  });
+  }));
 });

bitwarden_license/bit-common/src/tools/reports/risk-insights/services/critical-apps-api.service.ts

@@ -1,10 +1,11 @@
-import { Injectable } from "@angular/core";
-import { BehaviorSubject, firstValueFrom, Observable } from "rxjs";
+import { Injectable, OnDestroy } from "@angular/core";
+import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
+import { BehaviorSubject, firstValueFrom, map, Observable } from "rxjs";
 
 import { ApiService } from "@bitwarden/common/abstractions/api.service";
 import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service";
 import { EncString } from "@bitwarden/common/platform/models/domain/enc-string";
-import { Guid } from "@bitwarden/common/types/guid";
+import { OrganizationId, PasswordHealthReportApplicationId } from "@bitwarden/common/types/guid";
 import { OrgKey } from "@bitwarden/common/types/key";
 import { KeyService } from "@bitwarden/key-management";
 
@@ -14,23 +15,34 @@ import { KeyService } from "@bitwarden/key-management";
 /* Retrieves and decrypts critical apps for a given organization
  *  Encrypts and saves data for a given organization
  */
-export class CriticalAppsApiService {
+export class CriticalAppsApiService implements OnDestroy {
+  private orgId = new BehaviorSubject<OrganizationId | null>(null);
   private criticalAppsList = new BehaviorSubject<PasswordHealthReportApplicationsResponse[]>([]);
 
+  private fetchOrg$ = this.orgId
+    .pipe(takeUntilDestroyed())
+    .subscribe((orgId) => this.retrieveCriticalApps(orgId));
+
+  ngOnDestroy(): void {
+    this.fetchOrg$.unsubscribe();
+  }
+
   constructor(
     private apiService: ApiService,
     private keyService: KeyService,
     private encryptService: EncryptService,
   ) {}
 
-  // Get a list of critical apps
-  get criticalApps$(): Observable<PasswordHealthReportApplicationsResponse[]> {
-    return this.criticalAppsList.asObservable();
+  // Get a list of critical apps for a given organization
+  getAppsListForOrg(orgId: string): Observable<PasswordHealthReportApplicationsResponse[]> {
+    return this.criticalAppsList
+      .asObservable()
+      .pipe(map((apps) => apps.filter((app) => app.organizationId === orgId)));
   }
 
   // Reset the critical apps list
-  set criticalApps(value: PasswordHealthReportApplicationsResponse[]) {
-    this.criticalAppsList.next(value);
+  setAppsInListForOrg(apps: PasswordHealthReportApplicationsResponse[]) {
+    this.criticalAppsList.next(apps);
   }
 
   // Save the selected critical apps for a given organization
@@ -38,8 +50,12 @@ export class CriticalAppsApiService {
     const key = await this.keyService.getOrgKey(orgId);
 
     // only save records that are not already in the database
-    const newEntries = await this.filterNewEntries(orgId, selectedUrls);
-    const criticalAppsRequests = await this.encryptNewEntries(orgId, key, newEntries);
+    const newEntries = await this.filterNewEntries(orgId as OrganizationId, selectedUrls);
+    const criticalAppsRequests = await this.encryptNewEntries(
+      orgId as OrganizationId,
+      key,
+      newEntries,
+    );
 
     // save the new entries to the database
     const dbResponse = await this.apiService.send(
@@ -51,25 +67,35 @@ export class CriticalAppsApiService {
     );
 
     // add the new entries to the criticalAppsList
+    const updatedList = [...this.criticalAppsList.value];
     for (const responseItem of dbResponse) {
       const decryptedUrl = await this.encryptService.decryptToUtf8(
         new EncString(responseItem.uri),
         key,
       );
-      if (!this.criticalAppsList.value.some((f) => f.uri === decryptedUrl)) {
-        this.criticalAppsList.value.push({
+      if (!updatedList.some((f) => f.uri === decryptedUrl)) {
+        updatedList.push({
           id: responseItem.id,
           organizationId: responseItem.organizationId,
           uri: decryptedUrl,
         } as PasswordHealthReportApplicationsResponse);
       }
     }
+    this.criticalAppsList.next(updatedList);
   }
 
   // Get the critical apps for a given organization
-  async getCriticalApps(
-    orgId: string,
-  ): Promise<Observable<PasswordHealthReportApplicationsResponse[]>> {
+  setOrganizationId(orgId: OrganizationId) {
+    this.orgId.next(orgId);
+  }
+
+  async retrieveCriticalApps(
+    orgId: OrganizationId | null,
+  ): Promise<PasswordHealthReportApplicationsResponse[]> {
+    if (orgId === null) {
+      return [];
+    }
+
     const response = await this.apiService.send(
       "GET",
       `/reports/password-health-report-applications/${orgId.toString()}`,
@@ -78,24 +104,31 @@ export class CriticalAppsApiService {
       true,
     );
 
-    this.criticalAppsList.next([]);
     const key = await this.keyService.getOrgKey(orgId);
+    const updatedList: PasswordHealthReportApplicationsResponse[] = [];
 
     await Promise.all(
-      response.map(async (r: { id: Guid; organizationId: Guid; uri: string }) => {
-        const decryptedUrl = await this.encryptService.decryptToUtf8(new EncString(r.uri), key);
-        this.criticalAppsList.value.push({
-          id: r.id,
-          organizationId: r.organizationId,
-          uri: decryptedUrl,
-        } as PasswordHealthReportApplicationsResponse);
-      }),
+      response.map(
+        async (r: {
+          id: PasswordHealthReportApplicationId;
+          organizationId: OrganizationId;
+          uri: string;
+        }) => {
+          const decryptedUrl = await this.encryptService.decryptToUtf8(new EncString(r.uri), key);
+          updatedList.push({
+            id: r.id,
+            organizationId: r.organizationId,
+            uri: decryptedUrl,
+          } as PasswordHealthReportApplicationsResponse);
+        },
+      ),
     );
 
-    return this.criticalAppsList.asObservable();
+    this.criticalAppsList.next(updatedList);
+    return updatedList;
   }
 
-  private async filterNewEntries(orgId: string, selectedUrls: string[]): Promise<string[]> {
+  private async filterNewEntries(orgId: OrganizationId, selectedUrls: string[]): Promise<string[]> {
     return await firstValueFrom(this.criticalAppsList).then((criticalApps) => {
       const criticalAppsUri = criticalApps
         .filter((f) => f.organizationId === orgId)
@@ -105,7 +138,7 @@ export class CriticalAppsApiService {
   }
 
   private async encryptNewEntries(
-    orgId: string,
+    orgId: OrganizationId,
     key: OrgKey,
     newEntries: string[],
   ): Promise<PasswordHealthReportApplicationsRequest[]> {
@@ -122,12 +155,12 @@ export class CriticalAppsApiService {
 }
 
 export interface PasswordHealthReportApplicationsRequest {
-  organizationId: Guid;
+  organizationId: OrganizationId;
   url: string;
 }
 
 export interface PasswordHealthReportApplicationsResponse {
-  id: Guid;
-  organizationId: Guid;
+  id: PasswordHealthReportApplicationId;
+  organizationId: OrganizationId;
   uri: string;
 }

bitwarden_license/bit-web/src/app/tools/access-intelligence/all-applications.component.ts

@@ -28,6 +28,7 @@ import { SharedModule } from "@bitwarden/web-vault/app/shared";
 import { PipesModule } from "@bitwarden/web-vault/app/vault/individual-vault/pipes/pipes.module";
 
 import { applicationTableMockData } from "./application-table.mock";
+import { OrganizationId } from "@bitwarden/common/types/guid";
 
 @Component({
   standalone: true,
@@ -63,12 +64,12 @@ export class AllApplicationsComponent implements OnInit {
           return params;
           // TODO: use organizationId to fetch data
         }),
-        switchMap(async (params) => {
-          const organizationId = (await params).get("organizationId");
-          await this.criticalAppsService.getCriticalApps(organizationId);
-        }),
+        switchMap(async (params) => await params),
       )
-      .subscribe();
+      .subscribe((params) => {
+        const orgId = params.get("organizationId");
+        this.criticalAppsService.setOrganizationId(orgId as OrganizationId);
+      });
 
     this.isCritialAppsFeatureEnabled = await this.configService.getFeatureFlag(
       FeatureFlag.CriticalApps,

bitwarden_license/bit-web/src/app/tools/access-intelligence/risk-insights.component.html

@@ -22,10 +22,10 @@
   <bit-tab label="{{ 'allApplicationsWithCount' | i18n: apps.length }}">
     <tools-all-applications></tools-all-applications>
   </bit-tab>
-  <bit-tab *ngIf="isCritialAppsFeatureEnabled">
+  <bit-tab *ngIf="isCriticalAppsFeatureEnabled">
     <ng-template bitTabLabel>
       <i class="bwi bwi-star"></i>
-      {{ "criticalApplicationsWithCount" | i18n: criticalApps.length }}
+      {{ "criticalApplicationsWithCount" | i18n: (criticalApps$ | async)?.length ?? 0 }}
     </ng-template>
     <tools-critical-applications></tools-critical-applications>
   </bit-tab>

bitwarden_license/bit-web/src/app/tools/access-intelligence/risk-insights.component.ts

@@ -5,7 +5,10 @@ import { ActivatedRoute, Router } from "@angular/router";
 
 import { JslibModule } from "@bitwarden/angular/jslib.module";
 // eslint-disable-next-line no-restricted-imports -- used for dependency injection
-import { CriticalAppsApiService } from "@bitwarden/bit-common/tools/reports/risk-insights";
+import {
+  CriticalAppsApiService,
+  PasswordHealthReportApplicationsResponse,
+} from "@bitwarden/bit-common/tools/reports/risk-insights";
 import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
 import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
 import { AsyncActionsModule, ButtonModule, TabsModule } from "@bitwarden/components";
@@ -18,6 +21,7 @@ import { NotifiedMembersTableComponent } from "./notified-members-table.componen
 import { PasswordHealthMembersURIComponent } from "./password-health-members-uri.component";
 import { PasswordHealthMembersComponent } from "./password-health-members.component";
 import { PasswordHealthComponent } from "./password-health.component";
+import { Observable } from "rxjs";
 
 export enum RiskInsightsTabType {
   AllApps = 0,
@@ -46,10 +50,10 @@ export enum RiskInsightsTabType {
 export class RiskInsightsComponent implements OnInit {
   tabIndex: RiskInsightsTabType;
   dataLastUpdated = new Date();
-  isCritialAppsFeatureEnabled = false;
+  isCriticalAppsFeatureEnabled = false;
 
   apps: any[] = applicationTableMockData;
-  criticalApps: any[] = [];
+  criticalApps$: Observable<PasswordHealthReportApplicationsResponse[]> = new Observable();
   notifiedMembers: any[] = [];
 
   async refreshData() {
@@ -71,7 +75,7 @@ export class RiskInsightsComponent implements OnInit {
   };
 
   async ngOnInit() {
-    this.isCritialAppsFeatureEnabled = await this.configService.getFeatureFlag(
+    this.isCriticalAppsFeatureEnabled = await this.configService.getFeatureFlag(
       FeatureFlag.CriticalApps,
     );
   }
@@ -85,9 +89,7 @@ export class RiskInsightsComponent implements OnInit {
     route.queryParams.pipe(takeUntilDestroyed()).subscribe(({ tabIndex }) => {
       this.tabIndex = !isNaN(tabIndex) ? tabIndex : RiskInsightsTabType.AllApps;
     });
-
-    this.criticalAppsApiService.criticalApps$.pipe(takeUntilDestroyed()).subscribe((apps) => {
-      this.criticalApps = apps;
-    });
+    const orgId = this.route.snapshot.paramMap.get("organizationId");
+    this.criticalApps$ = this.criticalAppsApiService.getAppsListForOrg(orgId);
   }
 }

libs/common/src/types/guid.ts

@@ -10,3 +10,4 @@ export type PolicyId = Opaque<string, "PolicyId">;
 export type CipherId = Opaque<string, "CipherId">;
 export type SendId = Opaque<string, "SendId">;
 export type IndexedEntityId = Opaque<string, "IndexedEntityId">;
+export type PasswordHealthReportApplicationId = Opaque<string, "PasswordHealthReportApplicationId">;