From c67e6df8394c4e0c41fcd497662f89fc91533458 Mon Sep 17 00:00:00 2001 From: Shane Melton Date: Thu, 13 Feb 2025 10:58:44 -0800 Subject: [PATCH] [PM-14419] At-risk passwords change password service (#13279) * [PM-14419] Introduce the change-login-password service and its default implementation * [PM-14419] Use the change login password service on the at-risk passwords page * [PM-14419] Add unit tests * [PM-14419] Use existing fixed test environment * [PM-14419] Add mock implementation for ChangeLoginPasswordService in at-risk passwords tests * [PM-14419] Linter --- .../at-risk-passwords.component.html | 11 +- .../at-risk-passwords.component.spec.ts | 9 +- .../at-risk-passwords.component.ts | 48 ++++-- .../change-login-password.service.ts | 10 ++ libs/vault/src/index.ts | 3 + ...ault-change-login-password.service.spec.ts | 157 ++++++++++++++++++ .../default-change-login-password.service.ts | 96 +++++++++++ 7 files changed, 319 insertions(+), 15 deletions(-) create mode 100644 libs/vault/src/abstractions/change-login-password.service.ts create mode 100644 libs/vault/src/services/default-change-login-password.service.spec.ts create mode 100644 libs/vault/src/services/default-change-login-password.service.ts diff --git a/apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.html b/apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.html index ece00af3df2..044848eec8c 100644 --- a/apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.html +++ b/apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.html @@ -59,11 +59,20 @@ type="button" bitBadge variant="primary" + appStopProp (click)="launchChangePassword(cipher)" [title]="'changeButtonTitle' | i18n: cipher.name" [attr.aria-label]="'changeButtonTitle' | i18n: cipher.name" + [disabled]="launchingCipher() == cipher" > - {{ "change" | i18n }} + + {{ "change" | i18n }} + + diff --git a/apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.spec.ts b/apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.spec.ts index c71c9fa56c0..39b9650538a 100644 --- a/apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.spec.ts +++ b/apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.spec.ts @@ -18,6 +18,8 @@ import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.servi import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { ToastService } from "@bitwarden/components"; import { + ChangeLoginPasswordService, + DefaultChangeLoginPasswordService, PasswordRepromptService, SecurityTask, SecurityTaskType, @@ -70,6 +72,7 @@ describe("AtRiskPasswordsComponent", () => { const setInlineMenuVisibility = jest.fn(); const mockToastService = mock(); const mockAtRiskPasswordPageService = mock(); + const mockChangeLoginPasswordService = mock(); beforeEach(async () => { mockTasks$ = new BehaviorSubject([ @@ -156,12 +159,16 @@ describe("AtRiskPasswordsComponent", () => { .overrideComponent(AtRiskPasswordsComponent, { remove: { imports: [PopupHeaderComponent, PopupPageComponent], - providers: [AtRiskPasswordPageService], + providers: [ + AtRiskPasswordPageService, + { provide: ChangeLoginPasswordService, useClass: DefaultChangeLoginPasswordService }, + ], }, add: { imports: [MockPopupHeaderComponent, MockPopupPageComponent], providers: [ { provide: AtRiskPasswordPageService, useValue: mockAtRiskPasswordPageService }, + { provide: ChangeLoginPasswordService, useValue: mockChangeLoginPasswordService }, ], }, }) diff --git a/apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.ts b/apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.ts index f075335102f..4753bc77ecb 100644 --- a/apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.ts +++ b/apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.ts @@ -1,5 +1,5 @@ import { CommonModule } from "@angular/common"; -import { Component, inject } from "@angular/core"; +import { Component, inject, signal } from "@angular/core"; import { Router } from "@angular/router"; import { combineLatest, firstValueFrom, map, of, shareReplay, startWith, switchMap } from "rxjs"; @@ -16,7 +16,7 @@ import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/pl import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { - BadgeComponent, + BadgeModule, ButtonModule, CalloutModule, ItemModule, @@ -24,6 +24,8 @@ import { TypographyModule, } from "@bitwarden/components"; import { + ChangeLoginPasswordService, + DefaultChangeLoginPasswordService, filterOutNullish, PasswordRepromptService, SecurityTaskType, @@ -37,8 +39,6 @@ import { PopupPageComponent } from "../../../../platform/popup/layout/popup-page import { AtRiskPasswordPageService } from "./at-risk-password-page.service"; @Component({ - selector: "vault-at-risk-passwords", - standalone: true, imports: [ PopupPageComponent, PopupHeaderComponent, @@ -46,12 +46,17 @@ import { AtRiskPasswordPageService } from "./at-risk-password-page.service"; ItemModule, CommonModule, JslibModule, - BadgeComponent, TypographyModule, CalloutModule, ButtonModule, + BadgeModule, ], - providers: [AtRiskPasswordPageService], + providers: [ + AtRiskPasswordPageService, + { provide: ChangeLoginPasswordService, useClass: DefaultChangeLoginPasswordService }, + ], + selector: "vault-at-risk-passwords", + standalone: true, templateUrl: "./at-risk-passwords.component.html", }) export class AtRiskPasswordsComponent { @@ -60,12 +65,20 @@ export class AtRiskPasswordsComponent { private cipherService = inject(CipherService); private i18nService = inject(I18nService); private accountService = inject(AccountService); - private platformUtilsService = inject(PlatformUtilsService); private passwordRepromptService = inject(PasswordRepromptService); private router = inject(Router); private autofillSettingsService = inject(AutofillSettingsServiceAbstraction); private toastService = inject(ToastService); private atRiskPasswordPageService = inject(AtRiskPasswordPageService); + private changeLoginPasswordService = inject(ChangeLoginPasswordService); + private platformUtilsService = inject(PlatformUtilsService); + + /** + * The cipher that is currently being launched. Used to show a loading spinner on the badge button. + * The UI utilize a bitBadge which does not support async actions (like bitButton does). + * @protected + */ + protected launchingCipher = signal(null); private activeUserData$ = this.accountService.activeAccount$.pipe( filterOutNullish(), @@ -138,12 +151,6 @@ export class AtRiskPasswordsComponent { }); } - async launchChangePassword(cipher: CipherView) { - if (cipher.login?.uri) { - this.platformUtilsService.launchUri(cipher.login.uri); - } - } - async activateInlineAutofillMenuVisibility() { await this.autofillSettingsService.setInlineMenuVisibility( AutofillOverlayVisibility.OnButtonClick, @@ -159,4 +166,19 @@ export class AtRiskPasswordsComponent { const { userId } = await firstValueFrom(this.activeUserData$); await this.atRiskPasswordPageService.dismissCallout(userId); } + + launchChangePassword = async (cipher: CipherView) => { + try { + this.launchingCipher.set(cipher); + const url = await this.changeLoginPasswordService.getChangePasswordUrl(cipher); + + if (url == null) { + return; + } + + this.platformUtilsService.launchUri(url); + } finally { + this.launchingCipher.set(null); + } + }; } diff --git a/libs/vault/src/abstractions/change-login-password.service.ts b/libs/vault/src/abstractions/change-login-password.service.ts new file mode 100644 index 00000000000..c89162d42be --- /dev/null +++ b/libs/vault/src/abstractions/change-login-password.service.ts @@ -0,0 +1,10 @@ +import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; + +export abstract class ChangeLoginPasswordService { + /** + * Attempts to find a well-known change password URL for the given cipher. Only works for Login ciphers with at + * least one http/https URL. If no well-known change password URL is found, the first URL is returned. + * Non-Login ciphers and Logins with no valid http/https URLs return null. + */ + abstract getChangePasswordUrl(cipher: CipherView): Promise; +} diff --git a/libs/vault/src/index.ts b/libs/vault/src/index.ts index d0823580506..ac905c1f5ef 100644 --- a/libs/vault/src/index.ts +++ b/libs/vault/src/index.ts @@ -25,3 +25,6 @@ export * from "./components/add-edit-folder-dialog/add-edit-folder-dialog.compon export * as VaultIcons from "./icons"; export * from "./tasks"; + +export * from "./abstractions/change-login-password.service"; +export * from "./services/default-change-login-password.service"; diff --git a/libs/vault/src/services/default-change-login-password.service.spec.ts b/libs/vault/src/services/default-change-login-password.service.spec.ts new file mode 100644 index 00000000000..4805f298797 --- /dev/null +++ b/libs/vault/src/services/default-change-login-password.service.spec.ts @@ -0,0 +1,157 @@ +/** + * Jest needs to run in custom environment to mock Request/Response objects + * @jest-environment ../../libs/shared/test.environment.ts + */ + +import { mock } from "jest-mock-extended"; + +import { ApiService } from "@bitwarden/common/abstractions/api.service"; +import { CipherType } from "@bitwarden/common/vault/enums"; +import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; +import { LoginUriView } from "@bitwarden/common/vault/models/view/login-uri.view"; +import { LoginView } from "@bitwarden/common/vault/models/view/login.view"; + +import { DefaultChangeLoginPasswordService } from "./default-change-login-password.service"; + +describe("DefaultChangeLoginPasswordService", () => { + let service: DefaultChangeLoginPasswordService; + + let mockShouldNotExistResponse: Response; + let mockWellKnownResponse: Response; + + const mockApiService = mock(); + + beforeEach(() => { + mockApiService.nativeFetch.mockClear(); + + // Default responses to success state + mockShouldNotExistResponse = new Response("Not Found", { status: 404 }); + mockWellKnownResponse = new Response("OK", { status: 200 }); + + mockApiService.nativeFetch.mockImplementation((request) => { + if ( + request.url.endsWith("resource-that-should-not-exist-whose-status-code-should-not-be-200") + ) { + return Promise.resolve(mockShouldNotExistResponse); + } + + if (request.url.endsWith(".well-known/change-password")) { + return Promise.resolve(mockWellKnownResponse); + } + + throw new Error("Unexpected request"); + }); + service = new DefaultChangeLoginPasswordService(mockApiService); + }); + + it("should return null for non-login ciphers", async () => { + const cipher = { + type: CipherType.Card, + } as CipherView; + + const url = await service.getChangePasswordUrl(cipher); + + expect(url).toBeNull(); + }); + + it("should return null for logins with no URIs", async () => { + const cipher = { + type: CipherType.Login, + login: Object.assign(new LoginView(), { uris: [] as LoginUriView[] }), + } as CipherView; + + const url = await service.getChangePasswordUrl(cipher); + + expect(url).toBeNull(); + }); + + it("should return null for logins with no valid HTTP/HTTPS URIs", async () => { + const cipher = { + type: CipherType.Login, + login: Object.assign(new LoginView(), { + uris: [{ uri: "ftp://example.com" }], + }), + } as CipherView; + + const url = await service.getChangePasswordUrl(cipher); + + expect(url).toBeNull(); + }); + + it("should check the origin for a reliable status code", async () => { + const cipher = { + type: CipherType.Login, + login: Object.assign(new LoginView(), { + uris: [{ uri: "https://example.com" }], + }), + } as CipherView; + + await service.getChangePasswordUrl(cipher); + + expect(mockApiService.nativeFetch).toHaveBeenCalledWith( + expect.objectContaining({ + url: "https://example.com/.well-known/resource-that-should-not-exist-whose-status-code-should-not-be-200", + }), + ); + }); + + it("should attempt to fetch the well-known change password URL", async () => { + const cipher = { + type: CipherType.Login, + login: Object.assign(new LoginView(), { + uris: [{ uri: "https://example.com" }], + }), + } as CipherView; + + await service.getChangePasswordUrl(cipher); + + expect(mockApiService.nativeFetch).toHaveBeenCalledWith( + expect.objectContaining({ + url: "https://example.com/.well-known/change-password", + }), + ); + }); + + it("should return the well-known change password URL when successful at verifying the response", async () => { + const cipher = { + type: CipherType.Login, + login: Object.assign(new LoginView(), { + uris: [{ uri: "https://example.com" }], + }), + } as CipherView; + + const url = await service.getChangePasswordUrl(cipher); + + expect(url).toBe("https://example.com/.well-known/change-password"); + }); + + it("should return the original URI when unable to verify the response", async () => { + mockShouldNotExistResponse = new Response("Ok", { status: 200 }); + + const cipher = { + type: CipherType.Login, + login: Object.assign(new LoginView(), { + uris: [{ uri: "https://example.com" }], + }), + } as CipherView; + + const url = await service.getChangePasswordUrl(cipher); + + expect(url).toBe("https://example.com"); + }); + + it("should return the original URI when the well-known URL is not found", async () => { + mockWellKnownResponse = new Response("Not Found", { status: 404 }); + + const cipher = { + type: CipherType.Login, + login: Object.assign(new LoginView(), { + uris: [{ uri: "https://example.com" }], + }), + } as CipherView; + + const url = await service.getChangePasswordUrl(cipher); + + expect(url).toBe("https://example.com"); + }); +}); diff --git a/libs/vault/src/services/default-change-login-password.service.ts b/libs/vault/src/services/default-change-login-password.service.ts new file mode 100644 index 00000000000..25648318c14 --- /dev/null +++ b/libs/vault/src/services/default-change-login-password.service.ts @@ -0,0 +1,96 @@ +import { Injectable } from "@angular/core"; + +import { ApiService } from "@bitwarden/common/abstractions/api.service"; +import { Utils } from "@bitwarden/common/platform/misc/utils"; +import { CipherType } from "@bitwarden/common/vault/enums"; +import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; + +import { ChangeLoginPasswordService } from "../abstractions/change-login-password.service"; + +@Injectable() +export class DefaultChangeLoginPasswordService implements ChangeLoginPasswordService { + constructor(private apiService: ApiService) {} + + /** + * @inheritDoc + */ + async getChangePasswordUrl(cipher: CipherView): Promise { + // Ensure we have a cipher with at least one URI + if (cipher.type !== CipherType.Login || cipher.login == null || !cipher.login.hasUris) { + return null; + } + + // Find the first valid URL that is an HTTP or HTTPS URL + const url = cipher.login.uris + .map((m) => Utils.getUrl(m.uri)) + .find((m) => m != null && (m.protocol === "http:" || m.protocol === "https:")); + + if (url == null) { + return null; + } + + const [reliable, wellKnownChangeUrl] = await Promise.all([ + this.hasReliableHttpStatusCode(url.origin), + this.getWellKnownChangePasswordUrl(url.origin), + ]); + + if (!reliable || wellKnownChangeUrl == null) { + return cipher.login.uri; + } + + return wellKnownChangeUrl; + } + + /** + * Checks if the server returns a non-200 status code for a resource that should not exist. + * See https://w3c.github.io/webappsec-change-password-url/response-code-reliability.html#semantics + * @param urlOrigin The origin of the URL to check + */ + private async hasReliableHttpStatusCode(urlOrigin: string): Promise { + try { + const url = new URL( + "./.well-known/resource-that-should-not-exist-whose-status-code-should-not-be-200", + urlOrigin, + ); + + const request = new Request(url, { + method: "GET", + mode: "same-origin", + credentials: "omit", + cache: "no-store", + redirect: "follow", + }); + + const response = await this.apiService.nativeFetch(request); + return !response.ok; + } catch { + return false; + } + } + + /** + * Builds a well-known change password URL for the given origin. Attempts to fetch the URL to ensure a valid response + * is returned. Returns null if the request throws or the response is not 200 OK. + * See https://w3c.github.io/webappsec-change-password-url/ + * @param urlOrigin The origin of the URL to check + */ + private async getWellKnownChangePasswordUrl(urlOrigin: string): Promise { + try { + const url = new URL("./.well-known/change-password", urlOrigin); + + const request = new Request(url, { + method: "GET", + mode: "same-origin", + credentials: "omit", + cache: "no-store", + redirect: "follow", + }); + + const response = await this.apiService.nativeFetch(request); + + return response.ok ? url.toString() : null; + } catch { + return null; + } + } +}