[PM-25417] DIRT API Service Refactor (ADR-0005) (#16353)

* encode username for uri and add spec

* verify response from getHibpBreach method

* test/validate for BreachAccountResponse type and length instead of mock response

* - extract dirt api method out of global api service
- create new directory structure
- change imports accordingly
- extract breach account response
- put extracted code into new dirt dir

* codeowners and dep injection for new hibp service

.github/CODEOWNERS

@@ -43,6 +43,7 @@ apps/web/src/app/dirt @bitwarden/team-data-insights-and-reporting-dev
 bitwarden_license/bit-common/src/dirt @bitwarden/team-data-insights-and-reporting-dev
 bitwarden_license/bit-web/src/app/dirt @bitwarden/team-data-insights-and-reporting-dev
 libs/dirt @bitwarden/team-data-insights-and-reporting-dev
+libs/common/src/dirt @bitwarden/team-data-insights-and-reporting-dev
 
 ## Localization/Crowdin (Platform and Tools team)
 apps/browser/src/_locales @bitwarden/team-tools-dev @bitwarden/team-platform-dev

apps/browser/src/background/main.background.ts

@@ -81,6 +81,7 @@ import {
 import { isUrlInList } from "@bitwarden/common/autofill/utils";
 import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service";
 import { DefaultBillingAccountProfileStateService } from "@bitwarden/common/billing/services/account/billing-account-profile-state.service";
+import { HibpApiService } from "@bitwarden/common/dirt/services/hibp-api.service";
 import { ClientType } from "@bitwarden/common/enums";
 import { ProcessReloadServiceAbstraction } from "@bitwarden/common/key-management/abstractions/process-reload.service";
 import {
@@ -347,6 +348,7 @@ export default class MainBackground {
   tokenService: TokenServiceAbstraction;
   appIdService: AppIdServiceAbstraction;
   apiService: ApiServiceAbstraction;
+  hibpApiService: HibpApiService;
   environmentService: BrowserEnvironmentService;
   cipherService: CipherServiceAbstraction;
   folderService: InternalFolderServiceAbstraction;
@@ -764,6 +766,7 @@ export default class MainBackground {
       { createRequest: (url, request) => new Request(url, request) },
     );
 
+    this.hibpApiService = new HibpApiService(this.apiService);
     this.fileUploadService = new FileUploadService(this.logService, this.apiService);
     this.cipherFileUploadService = new CipherFileUploadService(
       this.apiService,
@@ -1064,7 +1067,11 @@ export default class MainBackground {
       this.userNotificationSettingsService,
       messageListener,
     );
-    this.auditService = new AuditService(this.cryptoFunctionService, this.apiService);
+    this.auditService = new AuditService(
+      this.cryptoFunctionService,
+      this.apiService,
+      this.hibpApiService,
+    );
 
     this.importApiService = new ImportApiService(this.apiService);
 

apps/cli/src/service-container/service-container.ts

@@ -59,6 +59,7 @@ import {
 } from "@bitwarden/common/autofill/services/domain-settings.service";
 import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions/account/billing-account-profile-state.service";
 import { DefaultBillingAccountProfileStateService } from "@bitwarden/common/billing/services/account/billing-account-profile-state.service";
+import { HibpApiService } from "@bitwarden/common/dirt/services/hibp-api.service";
 import { ClientType } from "@bitwarden/common/enums";
 import {
   DefaultKeyGenerationService,
@@ -223,6 +224,7 @@ export class ServiceContainer {
   tokenService: TokenService;
   appIdService: AppIdService;
   apiService: NodeApiService;
+  hibpApiService: HibpApiService;
   environmentService: EnvironmentService;
   cipherService: CipherService;
   folderService: InternalFolderService;
@@ -867,7 +869,12 @@ export class ServiceContainer {
 
     this.userAutoUnlockKeyService = new UserAutoUnlockKeyService(this.keyService);
 
-    this.auditService = new AuditService(this.cryptoFunctionService, this.apiService);
+    this.hibpApiService = new HibpApiService(this.apiService);
+    this.auditService = new AuditService(
+      this.cryptoFunctionService,
+      this.apiService,
+      this.hibpApiService,
+    );
 
     this.eventUploadService = new EventUploadService(
       this.apiService,

apps/web/src/app/dirt/reports/pages/breach-report.component.spec.ts

@@ -8,7 +8,7 @@ import { BehaviorSubject } from "rxjs";
 import { I18nPipe } from "@bitwarden/angular/platform/pipes/i18n.pipe";
 import { AuditService } from "@bitwarden/common/abstractions/audit.service";
 import { AccountInfo, AccountService } from "@bitwarden/common/auth/abstractions/account.service";
-import { BreachAccountResponse } from "@bitwarden/common/models/response/breach-account.response";
+import { BreachAccountResponse } from "@bitwarden/common/dirt/models/response/breach-account.response";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
 import { UserId } from "@bitwarden/common/types/guid";
 

apps/web/src/app/dirt/reports/pages/breach-report.component.ts

@@ -6,7 +6,7 @@ import { firstValueFrom, map } from "rxjs";
 
 import { AuditService } from "@bitwarden/common/abstractions/audit.service";
 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
-import { BreachAccountResponse } from "@bitwarden/common/models/response/breach-account.response";
+import { BreachAccountResponse } from "@bitwarden/common/dirt/models/response/breach-account.response";
 
 @Component({
   selector: "app-breach-report",

libs/angular/src/services/jslib-services.module.ts

@@ -152,6 +152,7 @@ import { OrganizationBillingApiService } from "@bitwarden/common/billing/service
 import { OrganizationSponsorshipApiService } from "@bitwarden/common/billing/services/organization/organization-sponsorship-api.service";
 import { OrganizationBillingService } from "@bitwarden/common/billing/services/organization-billing.service";
 import { TaxService } from "@bitwarden/common/billing/services/tax.service";
+import { HibpApiService } from "@bitwarden/common/dirt/services/hibp-api.service";
 import {
   DefaultKeyGenerationService,
   KeyGenerationService,
@@ -463,7 +464,12 @@ const safeProviders: SafeProvider[] = [
   safeProvider({
     provide: AuditServiceAbstraction,
     useClass: AuditService,
-    deps: [CryptoFunctionServiceAbstraction, ApiServiceAbstraction],
+    deps: [CryptoFunctionServiceAbstraction, ApiServiceAbstraction, HibpApiService],
+  }),
+  safeProvider({
+    provide: HibpApiService,
+    useClass: HibpApiService,
+    deps: [ApiServiceAbstraction],
   }),
   safeProvider({
     provide: AuthServiceAbstraction,

libs/common/src/abstractions/api.service.ts

@@ -97,7 +97,6 @@ import { UpdateAvatarRequest } from "../models/request/update-avatar.request";
 import { UpdateDomainsRequest } from "../models/request/update-domains.request";
 import { VerifyDeleteRecoverRequest } from "../models/request/verify-delete-recover.request";
 import { VerifyEmailRequest } from "../models/request/verify-email.request";
-import { BreachAccountResponse } from "../models/response/breach-account.response";
 import { DomainsResponse } from "../models/response/domains.response";
 import { EventResponse } from "../models/response/event.response";
 import { ListResponse } from "../models/response/list.response";
@@ -516,8 +515,6 @@ export abstract class ApiService {
 
   abstract getUserPublicKey(id: string): Promise<UserKeyResponse>;
 
-  abstract getHibpBreach(username: string): Promise<BreachAccountResponse[]>;
-
   abstract postBitPayInvoice(request: BitPayInvoiceRequest): Promise<string>;
   abstract postSetupPayment(): Promise<string>;
 

libs/common/src/abstractions/audit.service.ts

@@ -1,4 +1,4 @@
-import { BreachAccountResponse } from "../models/response/breach-account.response";
+import { BreachAccountResponse } from "../dirt/models/response/breach-account.response";
 
 export abstract class AuditService {
   /**

libs/common/src/dirt/index.ts

@@ -0,0 +1,2 @@
+export * from "./models";
+export * from "./services";

libs/common/src/dirt/models/index.ts

@@ -0,0 +1 @@
+export * from "./response";

libs/common/src/dirt/models/response/breach-account.response.ts

@@ -1,4 +1,4 @@
-import { BaseResponse } from "./base.response";
+import { BaseResponse } from "../../../models/response/base.response";
 
 export class BreachAccountResponse extends BaseResponse {
   addedDate: string;

libs/common/src/dirt/models/response/index.ts

@@ -0,0 +1 @@
+export * from "./breach-account.response";

libs/common/src/dirt/services/dirt-api.service.spec.ts

@@ -0,0 +1,20 @@
+import { ApiService } from "../../abstractions/api.service";
+
+import { DirtApiService } from "./dirt-api.service";
+
+describe("DirtApiService", () => {
+  let sut: DirtApiService;
+  let apiService: jest.Mocked<ApiService>;
+
+  beforeEach(() => {
+    apiService = {
+      send: jest.fn(),
+    } as any;
+
+    sut = new DirtApiService(apiService);
+  });
+
+  it("should be created", () => {
+    expect(sut).toBeTruthy();
+  });
+});

libs/common/src/dirt/services/dirt-api.service.ts

@@ -0,0 +1,8 @@
+import { ApiService } from "../../abstractions/api.service";
+
+export class DirtApiService {
+  constructor(private apiService: ApiService) {}
+
+  // This service can be used for general DIRT-related API methods
+  // For specific domains like HIBP, use dedicated services like HibpApiService
+}

libs/common/src/dirt/services/hibp-api.service.spec.ts

@@ -0,0 +1,39 @@
+import { ApiService } from "../../abstractions/api.service";
+import { BreachAccountResponse } from "../models";
+
+import { HibpApiService } from "./hibp-api.service";
+
+describe("HibpApiService", () => {
+  let sut: HibpApiService;
+  let apiService: jest.Mocked<ApiService>;
+
+  beforeEach(() => {
+    apiService = {
+      send: jest.fn(),
+    } as any;
+
+    sut = new HibpApiService(apiService);
+  });
+
+  describe("getHibpBreach", () => {
+    it("should properly URL encode username with special characters", async () => {
+      const mockResponse = [{ name: "test" }];
+      const username = "connect#bwpm@simplelogin.co";
+
+      apiService.send.mockResolvedValue(mockResponse);
+
+      const result = await sut.getHibpBreach(username);
+
+      expect(apiService.send).toHaveBeenCalledWith(
+        "GET",
+        "/hibp/breach?username=" + encodeURIComponent(username),
+        null,
+        true,
+        true,
+      );
+      expect(result).toBeInstanceOf(Array);
+      expect(result).toHaveLength(1);
+      expect(result[0]).toBeInstanceOf(BreachAccountResponse);
+    });
+  });
+});

libs/common/src/dirt/services/hibp-api.service.ts

@@ -0,0 +1,18 @@
+import { ApiService } from "../../abstractions/api.service";
+import { BreachAccountResponse } from "../models";
+
+export class HibpApiService {
+  constructor(private apiService: ApiService) {}
+
+  async getHibpBreach(username: string): Promise<BreachAccountResponse[]> {
+    const encodedUsername = encodeURIComponent(username);
+    const r = await this.apiService.send(
+      "GET",
+      "/hibp/breach?username=" + encodedUsername,
+      null,
+      true,
+      true,
+    );
+    return r.map((a: any) => new BreachAccountResponse(a));
+  }
+}

libs/common/src/dirt/services/index.ts

@@ -0,0 +1,2 @@
+export * from "./dirt-api.service";
+export * from "./hibp-api.service";

libs/common/src/services/api.service.spec.ts

@@ -14,7 +14,6 @@ import {
   VaultTimeoutSettingsService,
   VaultTimeoutStringType,
 } from "../key-management/vault-timeout";
-import { BreachAccountResponse } from "../models/response/breach-account.response";
 import { ErrorResponse } from "../models/response/error.response";
 import { AppIdService } from "../platform/abstractions/app-id.service";
 import { Environment, EnvironmentService } from "../platform/abstractions/environment.service";
@@ -412,26 +411,4 @@ describe("ApiService", () => {
       ).rejects.toMatchObject(error);
     },
   );
-
-  describe("getHibpBreach", () => {
-    it("should properly URL encode username with special characters", async () => {
-      const mockResponse = [{ name: "test" }];
-      const username = "connect#bwpm@simplelogin.co";
-
-      jest.spyOn(sut, "send").mockResolvedValue(mockResponse);
-
-      const result = await sut.getHibpBreach(username);
-
-      expect(sut.send).toHaveBeenCalledWith(
-        "GET",
-        "/hibp/breach?username=" + encodeURIComponent(username),
-        null,
-        true,
-        true,
-      );
-      expect(result).toBeInstanceOf(Array);
-      expect(result).toHaveLength(1);
-      expect(result[0]).toBeInstanceOf(BreachAccountResponse);
-    });
-  });
 });

libs/common/src/services/api.service.ts

@@ -113,7 +113,6 @@ import { UpdateAvatarRequest } from "../models/request/update-avatar.request";
 import { UpdateDomainsRequest } from "../models/request/update-domains.request";
 import { VerifyDeleteRecoverRequest } from "../models/request/verify-delete-recover.request";
 import { VerifyEmailRequest } from "../models/request/verify-email.request";
-import { BreachAccountResponse } from "../models/response/breach-account.response";
 import { DomainsResponse } from "../models/response/domains.response";
 import { ErrorResponse } from "../models/response/error.response";
 import { EventResponse } from "../models/response/event.response";
@@ -1430,14 +1429,6 @@ export class ApiService implements ApiServiceAbstraction {
     return new UserKeyResponse(r);
   }
 
-  // HIBP APIs
-
-  async getHibpBreach(username: string): Promise<BreachAccountResponse[]> {
-    const encodedUsername = encodeURIComponent(username);
-    const r = await this.send("GET", "/hibp/breach?username=" + encodedUsername, null, true, true);
-    return r.map((a: any) => new BreachAccountResponse(a));
-  }
-
   // Misc
 
   async postBitPayInvoice(request: BitPayInvoiceRequest): Promise<string> {

libs/common/src/services/audit.service.spec.ts

@@ -1,4 +1,5 @@
 import { ApiService } from "../abstractions/api.service";
+import { HibpApiService } from "../dirt/services/hibp-api.service";
 import { CryptoFunctionService } from "../key-management/crypto/abstractions/crypto-function.service";
 import { ErrorResponse } from "../models/response/error.response";
 
@@ -17,6 +18,7 @@ describe("AuditService", () => {
   let auditService: AuditService;
   let mockCrypto: jest.Mocked<CryptoFunctionService>;
   let mockApi: jest.Mocked<ApiService>;
+  let mockHibpApi: jest.Mocked<HibpApiService>;
 
   beforeEach(() => {
     mockCrypto = {
@@ -27,10 +29,13 @@ describe("AuditService", () => {
       nativeFetch: jest.fn().mockResolvedValue({
         text: jest.fn().mockResolvedValue(`CDDEEFF:4\nDDEEFF:2\n123456:1`),
       }),
-      getHibpBreach: jest.fn(),
     } as unknown as jest.Mocked<ApiService>;
 
-    auditService = new AuditService(mockCrypto, mockApi, 2);
+    mockHibpApi = {
+      getHibpBreach: jest.fn(),
+    } as unknown as jest.Mocked<HibpApiService>;
+
+    auditService = new AuditService(mockCrypto, mockApi, mockHibpApi, 2);
   });
 
   it("should not exceed max concurrent passwordLeaked requests", async () => {
@@ -69,13 +74,13 @@ describe("AuditService", () => {
   });
 
   it("should return empty array for breachedAccounts on 404", async () => {
-    mockApi.getHibpBreach.mockRejectedValueOnce({ statusCode: 404 } as ErrorResponse);
+    mockHibpApi.getHibpBreach.mockRejectedValueOnce({ statusCode: 404 } as ErrorResponse);
     const result = await auditService.breachedAccounts("user@example.com");
     expect(result).toEqual([]);
   });
 
   it("should throw error for breachedAccounts on non-404 error", async () => {
-    mockApi.getHibpBreach.mockRejectedValueOnce({ statusCode: 500 } as ErrorResponse);
+    mockHibpApi.getHibpBreach.mockRejectedValueOnce({ statusCode: 500 } as ErrorResponse);
     await expect(auditService.breachedAccounts("user@example.com")).rejects.toThrow();
   });
 });

libs/common/src/services/audit.service.ts

@@ -3,8 +3,9 @@ import { mergeMap } from "rxjs/operators";
 
 import { ApiService } from "../abstractions/api.service";
 import { AuditService as AuditServiceAbstraction } from "../abstractions/audit.service";
+import { BreachAccountResponse } from "../dirt/models/response/breach-account.response";
+import { HibpApiService } from "../dirt/services/hibp-api.service";
 import { CryptoFunctionService } from "../key-management/crypto/abstractions/crypto-function.service";
-import { BreachAccountResponse } from "../models/response/breach-account.response";
 import { ErrorResponse } from "../models/response/error.response";
 import { Utils } from "../platform/misc/utils";
 
@@ -20,6 +21,7 @@ export class AuditService implements AuditServiceAbstraction {
   constructor(
     private cryptoFunctionService: CryptoFunctionService,
     private apiService: ApiService,
+    private hibpApiService: HibpApiService,
     private readonly maxConcurrent: number = 100, // default to 100, can be overridden
   ) {
     this.maxConcurrent = maxConcurrent;
@@ -69,7 +71,7 @@ export class AuditService implements AuditServiceAbstraction {
 
   async breachedAccounts(username: string): Promise<BreachAccountResponse[]> {
     try {
-      return await this.apiService.getHibpBreach(username);
+      return await this.hibpApiService.getHibpBreach(username);
     } catch (e) {
       const error = e as ErrorResponse;
       if (error.statusCode === 404) {