From 85311090810e3eed4e0309a2276544ec754bef55 Mon Sep 17 00:00:00 2001 From: Alex <55413326+AlexRubik@users.noreply.github.com> Date: Mon, 22 Sep 2025 10:06:58 -0400 Subject: [PATCH] [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 | 1 + .../browser/src/background/main.background.ts | 9 ++++- .../service-container/service-container.ts | 9 ++++- .../pages/breach-report.component.spec.ts | 2 +- .../reports/pages/breach-report.component.ts | 2 +- .../src/services/jslib-services.module.ts | 8 +++- libs/common/src/abstractions/api.service.ts | 3 -- libs/common/src/abstractions/audit.service.ts | 2 +- libs/common/src/dirt/index.ts | 2 + libs/common/src/dirt/models/index.ts | 1 + .../response/breach-account.response.ts | 2 +- libs/common/src/dirt/models/response/index.ts | 1 + .../dirt/services/dirt-api.service.spec.ts | 20 ++++++++++ .../src/dirt/services/dirt-api.service.ts | 8 ++++ .../dirt/services/hibp-api.service.spec.ts | 39 +++++++++++++++++++ .../src/dirt/services/hibp-api.service.ts | 18 +++++++++ libs/common/src/dirt/services/index.ts | 2 + libs/common/src/services/api.service.spec.ts | 23 ----------- libs/common/src/services/api.service.ts | 9 ----- .../common/src/services/audit.service.spec.ts | 13 +++++-- libs/common/src/services/audit.service.ts | 6 ++- 21 files changed, 132 insertions(+), 48 deletions(-) create mode 100644 libs/common/src/dirt/index.ts create mode 100644 libs/common/src/dirt/models/index.ts rename libs/common/src/{ => dirt}/models/response/breach-account.response.ts (93%) create mode 100644 libs/common/src/dirt/models/response/index.ts create mode 100644 libs/common/src/dirt/services/dirt-api.service.spec.ts create mode 100644 libs/common/src/dirt/services/dirt-api.service.ts create mode 100644 libs/common/src/dirt/services/hibp-api.service.spec.ts create mode 100644 libs/common/src/dirt/services/hibp-api.service.ts create mode 100644 libs/common/src/dirt/services/index.ts diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index c88fde064fc..c1fb0b4794d 100644 --- a/.github/CODEOWNERS +++ b/.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 diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 792b8a8b418..00a6d7fe604 100644 --- a/apps/browser/src/background/main.background.ts +++ b/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); diff --git a/apps/cli/src/service-container/service-container.ts b/apps/cli/src/service-container/service-container.ts index bcfb53dc01d..7b148b2a3d5 100644 --- a/apps/cli/src/service-container/service-container.ts +++ b/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, diff --git a/apps/web/src/app/dirt/reports/pages/breach-report.component.spec.ts b/apps/web/src/app/dirt/reports/pages/breach-report.component.spec.ts index 9af15749a91..143dce3915e 100644 --- a/apps/web/src/app/dirt/reports/pages/breach-report.component.spec.ts +++ b/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"; diff --git a/apps/web/src/app/dirt/reports/pages/breach-report.component.ts b/apps/web/src/app/dirt/reports/pages/breach-report.component.ts index 47bf0844468..b197c7dcae8 100644 --- a/apps/web/src/app/dirt/reports/pages/breach-report.component.ts +++ b/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", diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 213a0c26184..732609293ae 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/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, diff --git a/libs/common/src/abstractions/api.service.ts b/libs/common/src/abstractions/api.service.ts index 3372dda0d7d..d746342d728 100644 --- a/libs/common/src/abstractions/api.service.ts +++ b/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; - abstract getHibpBreach(username: string): Promise; - abstract postBitPayInvoice(request: BitPayInvoiceRequest): Promise; abstract postSetupPayment(): Promise; diff --git a/libs/common/src/abstractions/audit.service.ts b/libs/common/src/abstractions/audit.service.ts index a871df506d3..a00b2bf038a 100644 --- a/libs/common/src/abstractions/audit.service.ts +++ b/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 { /** diff --git a/libs/common/src/dirt/index.ts b/libs/common/src/dirt/index.ts new file mode 100644 index 00000000000..3c3b6b6476a --- /dev/null +++ b/libs/common/src/dirt/index.ts @@ -0,0 +1,2 @@ +export * from "./models"; +export * from "./services"; diff --git a/libs/common/src/dirt/models/index.ts b/libs/common/src/dirt/models/index.ts new file mode 100644 index 00000000000..9d2cfb1f79c --- /dev/null +++ b/libs/common/src/dirt/models/index.ts @@ -0,0 +1 @@ +export * from "./response"; diff --git a/libs/common/src/models/response/breach-account.response.ts b/libs/common/src/dirt/models/response/breach-account.response.ts similarity index 93% rename from libs/common/src/models/response/breach-account.response.ts rename to libs/common/src/dirt/models/response/breach-account.response.ts index 5c3c1f762eb..b5885f58931 100644 --- a/libs/common/src/models/response/breach-account.response.ts +++ b/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; diff --git a/libs/common/src/dirt/models/response/index.ts b/libs/common/src/dirt/models/response/index.ts new file mode 100644 index 00000000000..46b91d7d0dd --- /dev/null +++ b/libs/common/src/dirt/models/response/index.ts @@ -0,0 +1 @@ +export * from "./breach-account.response"; diff --git a/libs/common/src/dirt/services/dirt-api.service.spec.ts b/libs/common/src/dirt/services/dirt-api.service.spec.ts new file mode 100644 index 00000000000..de87cb1888a --- /dev/null +++ b/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; + + beforeEach(() => { + apiService = { + send: jest.fn(), + } as any; + + sut = new DirtApiService(apiService); + }); + + it("should be created", () => { + expect(sut).toBeTruthy(); + }); +}); diff --git a/libs/common/src/dirt/services/dirt-api.service.ts b/libs/common/src/dirt/services/dirt-api.service.ts new file mode 100644 index 00000000000..de7e85d521a --- /dev/null +++ b/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 +} diff --git a/libs/common/src/dirt/services/hibp-api.service.spec.ts b/libs/common/src/dirt/services/hibp-api.service.spec.ts new file mode 100644 index 00000000000..fd2a54bdd10 --- /dev/null +++ b/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; + + 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); + }); + }); +}); diff --git a/libs/common/src/dirt/services/hibp-api.service.ts b/libs/common/src/dirt/services/hibp-api.service.ts new file mode 100644 index 00000000000..79ec7351f9d --- /dev/null +++ b/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 { + 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)); + } +} diff --git a/libs/common/src/dirt/services/index.ts b/libs/common/src/dirt/services/index.ts new file mode 100644 index 00000000000..2f2a64f1aaf --- /dev/null +++ b/libs/common/src/dirt/services/index.ts @@ -0,0 +1,2 @@ +export * from "./dirt-api.service"; +export * from "./hibp-api.service"; diff --git a/libs/common/src/services/api.service.spec.ts b/libs/common/src/services/api.service.spec.ts index daef4164029..6d6e96de9e3 100644 --- a/libs/common/src/services/api.service.spec.ts +++ b/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); - }); - }); }); diff --git a/libs/common/src/services/api.service.ts b/libs/common/src/services/api.service.ts index 8efe78b3354..70ba76fe797 100644 --- a/libs/common/src/services/api.service.ts +++ b/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 { - 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 { diff --git a/libs/common/src/services/audit.service.spec.ts b/libs/common/src/services/audit.service.spec.ts index ce594823a7b..b0e96eb5c5c 100644 --- a/libs/common/src/services/audit.service.spec.ts +++ b/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; let mockApi: jest.Mocked; + let mockHibpApi: jest.Mocked; 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; - auditService = new AuditService(mockCrypto, mockApi, 2); + mockHibpApi = { + getHibpBreach: jest.fn(), + } as unknown as jest.Mocked; + + 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(); }); }); diff --git a/libs/common/src/services/audit.service.ts b/libs/common/src/services/audit.service.ts index 4d06b50d6b5..8fc9f13476d 100644 --- a/libs/common/src/services/audit.service.ts +++ b/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 { try { - return await this.apiService.getHibpBreach(username); + return await this.hibpApiService.getHibpBreach(username); } catch (e) { const error = e as ErrorResponse; if (error.statusCode === 404) {