Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-14419] At-risk passwords change password service #13279

Merged
merged 6 commits into from
Feb 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
<ng-container *ngIf="launchingCipher() != cipher">
{{ "change" | i18n }}
</ng-container>
<i
*ngIf="launchingCipher() == cipher"
class="bwi bwi-spinner bwi-spin"
aria-hidden="true"
></i>
</button>
</bit-item-action>
</bit-item>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -70,6 +72,7 @@ describe("AtRiskPasswordsComponent", () => {
const setInlineMenuVisibility = jest.fn();
const mockToastService = mock<ToastService>();
const mockAtRiskPasswordPageService = mock<AtRiskPasswordPageService>();
const mockChangeLoginPasswordService = mock<ChangeLoginPasswordService>();

beforeEach(async () => {
mockTasks$ = new BehaviorSubject<SecurityTask[]>([
Expand Down Expand Up @@ -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 },
],
},
})
Expand Down
Original file line number Diff line number Diff line change
@@ -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";

Expand All @@ -16,14 +16,16 @@
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,
ToastService,
TypographyModule,
} from "@bitwarden/components";
import {
ChangeLoginPasswordService,
DefaultChangeLoginPasswordService,
filterOutNullish,
PasswordRepromptService,
SecurityTaskType,
Expand All @@ -37,21 +39,24 @@
import { AtRiskPasswordPageService } from "./at-risk-password-page.service";

@Component({
selector: "vault-at-risk-passwords",
standalone: true,
imports: [
PopupPageComponent,
PopupHeaderComponent,
PopOutComponent,
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 {
Expand All @@ -60,12 +65,20 @@
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<CipherView | null>(null);

private activeUserData$ = this.accountService.activeAccount$.pipe(
filterOutNullish(),
Expand Down Expand Up @@ -138,12 +151,6 @@
});
}

async launchChangePassword(cipher: CipherView) {
if (cipher.login?.uri) {
this.platformUtilsService.launchUri(cipher.login.uri);
}
}

async activateInlineAutofillMenuVisibility() {
await this.autofillSettingsService.setInlineMenuVisibility(
AutofillOverlayVisibility.OnButtonClick,
Expand All @@ -159,4 +166,19 @@
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);

Check warning on line 173 in apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.ts#L171-L173

Added lines #L171 - L173 were not covered by tests

if (url == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โ“ Would it be a better experience to hide the change button if the URL isn't available? At the moment if there is no URL the user doesn't have any feedback when clicking the button.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is a trade off, on page load there would be a two requests flying for each of the ciphers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this was the tradeoff to avoid spamming too many requests for large lists of ciphers. Though you do bring up a good point about there being a lack of feedback if there is no URL.

@sukhleenb What should the behavior be if we're unable to determine a change password URL and the item no longer as a URL associated? This should be pretty unlikely as items without URLs won't normally have tasks generated for them, but the cipher could technically have its URL removed after the task was created.

return;

Check warning on line 176 in apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.ts#L176

Added line #L176 was not covered by tests
}

this.platformUtilsService.launchUri(url);

Check warning on line 179 in apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.ts#L179

Added line #L179 was not covered by tests
} finally {
this.launchingCipher.set(null);

Check warning on line 181 in apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/components/at-risk-passwords/at-risk-passwords.component.ts#L181

Added line #L181 was not covered by tests
}
};
}
10 changes: 10 additions & 0 deletions libs/vault/src/abstractions/change-login-password.service.ts
Original file line number Diff line number Diff line change
@@ -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<string | null>;
}
3 changes: 3 additions & 0 deletions libs/vault/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
/**
* Jest needs to run in custom environment to mock Request/Response objects
* @jest-environment ../../libs/shared/test.environment.ts
*/
shane-melton marked this conversation as resolved.
Show resolved Hide resolved

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<ApiService>();

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");
});
});
Loading
Loading