1
0
mirror of https://github.com/bitwarden/browser synced 2025-12-17 16:53:34 +00:00

[PM-24469] Implement Risk Insights for Premium in Cipher view component (#17012)

* [PM-24469] Refactor CipherViewComponent to use Angular signals and computed properties for improved reactivity

* [PM-24469] Refactor CipherViewComponent to utilize Angular signals for organization data retrieval

* [PM-24469] Refactor CipherViewComponent to utilize Angular signals for folder data retrieval

* [PM-24469] Cleanup organization signal

* [PM-24469] Refactor CipherViewComponent to replace signal for card expiration with computed property

* [PM-24469] Improve collections loading in CipherViewComponent

* [PM-24469] Remove redundant loadCipherData method

* [PM-24469] Refactor CipherViewComponent to replace signal with computed property for pending change password tasks

* [PM-24469] Refactor LoginCredentialsViewComponent to rename hadPendingChangePasswordTask to showChangePasswordLink for clarity

* [PM-24469] Introduce showChangePasswordLink computed property for improved readability

* [PM-24469] Initial RI for premium logic

* [PM-24469] Refactor checkPassword risk checking logic

* [PM-24469] Cleanup premium check

* [PM-24469] Cleanup UI visuals

* [PM-24469] Fix missing typography import

* [PM-24469] Cleanup docs

* [PM-24469] Add feature flag

* [PM-24469] Ensure password risk check is only performed when the feature is enabled, and the cipher is editable by the user, and it has a password

* [PM-24469] Refactor password risk evaluation logic and add unit tests for risk assessment

* [PM-24469] Fix mismatched CipherId type

* [PM-24469] Fix test dependencies

* [PM-24469] Fix config service mock in emergency view dialog spec

* [PM-24469] Wait for decrypted vault before calculating cipher risk

* [PM-24469] startWith(false) for passwordIsAtRisk signal to avoid showing stale values when cipher changes

* [PM-24469] Exclude organization owned ciphers from JIT risk analysis

* [PM-24469] Add initial cipher-view component test boilerplate

* [PM-24469] Add passwordIsAtRisk signal tests

* [PM-24469] Ignore soft deleted items for RI for premium feature

* [PM-24469] Fix tests
This commit is contained in:
Shane Melton
2025-11-04 12:15:53 -08:00
committed by GitHub
parent d364dfdda0
commit 7e5f02f90c
15 changed files with 732 additions and 180 deletions

View File

@@ -58,6 +58,7 @@ export enum FeatureFlag {
PM22136_SdkCipherEncryption = "pm-22136-sdk-cipher-encryption",
CipherKeyEncryption = "cipher-key-encryption",
AutofillConfirmation = "pm-25083-autofill-confirm-from-search",
RiskInsightsForPremium = "pm-23904-risk-insights-for-premium",
/* Platform */
IpcChannelFramework = "ipc-channel-framework",
@@ -106,6 +107,7 @@ export const DefaultFeatureFlagValue = {
[FeatureFlag.PM22134SdkCipherListView]: FALSE,
[FeatureFlag.PM22136_SdkCipherEncryption]: FALSE,
[FeatureFlag.AutofillConfirmation]: FALSE,
[FeatureFlag.RiskInsightsForPremium]: FALSE,
/* Auth */
[FeatureFlag.PM22110_DisableAlternateLoginMethods]: FALSE,

View File

@@ -0,0 +1,88 @@
import type { CipherRiskResult, CipherId } from "@bitwarden/sdk-internal";
import { isPasswordAtRisk } from "./cipher-risk.service";
describe("isPasswordAtRisk", () => {
const mockId = "00000000-0000-0000-0000-000000000000" as unknown as CipherId;
const createRisk = (overrides: Partial<CipherRiskResult> = {}): CipherRiskResult => ({
id: mockId,
password_strength: 4,
exposed_result: { type: "NotChecked" },
reuse_count: 1,
...overrides,
});
describe("exposed password risk", () => {
it.each([
{ value: 5, expected: true, desc: "found with value > 0" },
{ value: 0, expected: false, desc: "found but value is 0" },
])("should return $expected when password is $desc", ({ value, expected }) => {
const risk = createRisk({ exposed_result: { type: "Found", value } });
expect(isPasswordAtRisk(risk)).toBe(expected);
});
it("should return false when password is not checked", () => {
expect(isPasswordAtRisk(createRisk())).toBe(false);
});
});
describe("password reuse risk", () => {
it.each([
{ count: 2, expected: true, desc: "reused (reuse_count > 1)" },
{ count: 1, expected: false, desc: "not reused" },
{ count: undefined, expected: false, desc: "undefined" },
])("should return $expected when reuse_count is $desc", ({ count, expected }) => {
const risk = createRisk({ reuse_count: count });
expect(isPasswordAtRisk(risk)).toBe(expected);
});
});
describe("password strength risk", () => {
it.each([
{ strength: 0, expected: true },
{ strength: 1, expected: true },
{ strength: 2, expected: true },
{ strength: 3, expected: false },
{ strength: 4, expected: false },
])("should return $expected when password strength is $strength", ({ strength, expected }) => {
const risk = createRisk({ password_strength: strength });
expect(isPasswordAtRisk(risk)).toBe(expected);
});
});
describe("multiple risk factors", () => {
it.each<{ desc: string; overrides: Partial<CipherRiskResult>; expected: boolean }>([
{
desc: "exposed and reused",
overrides: {
exposed_result: { type: "Found" as const, value: 3 },
reuse_count: 2,
},
expected: true,
},
{
desc: "reused and weak strength",
overrides: { password_strength: 2, reuse_count: 2 },
expected: true,
},
{
desc: "all three risk factors",
overrides: {
password_strength: 1,
exposed_result: { type: "Found" as const, value: 10 },
reuse_count: 3,
},
expected: true,
},
{
desc: "no risk factors",
overrides: { reuse_count: undefined },
expected: false,
},
])("should return $expected when $desc present", ({ overrides, expected }) => {
const risk = createRisk(overrides);
expect(isPasswordAtRisk(risk)).toBe(expected);
});
});
});

View File

@@ -1,12 +1,10 @@
import type {
CipherRiskResult,
CipherRiskOptions,
ExposedPasswordResult,
PasswordReuseMap,
CipherId,
} from "@bitwarden/sdk-internal";
import { UserId } from "../../types/guid";
import { UserId, CipherId } from "../../types/guid";
import { CipherView } from "../models/view/cipher.view";
export abstract class CipherRiskService {
@@ -51,5 +49,21 @@ export abstract class CipherRiskService {
abstract buildPasswordReuseMap(ciphers: CipherView[], userId: UserId): Promise<PasswordReuseMap>;
}
// Re-export SDK types for convenience
export type { CipherRiskResult, CipherRiskOptions, ExposedPasswordResult, PasswordReuseMap };
/**
* Evaluates if a password represented by a CipherRiskResult is considered at risk.
*
* A password is considered at risk if any of the following conditions are true:
* - The password has been exposed in data breaches
* - The password is reused across multiple ciphers
* - The password has weak strength (password_strength < 3)
*
* @param risk - The CipherRiskResult to evaluate
* @returns true if the password is at risk, false otherwise
*/
export function isPasswordAtRisk(risk: CipherRiskResult): boolean {
return (
(risk.exposed_result.type === "Found" && risk.exposed_result.value > 0) ||
(risk.reuse_count ?? 1) > 1 ||
risk.password_strength < 3
);
}

View File

@@ -113,6 +113,12 @@ export class CipherView implements View, InitializerMetadata {
return this.passwordHistory && this.passwordHistory.length > 0;
}
get hasLoginPassword(): boolean {
return (
this.type === CipherType.Login && this.login?.password != null && this.login.password !== ""
);
}
get hasAttachments(): boolean {
return !!this.attachments && this.attachments.length > 0;
}

View File

@@ -1,11 +1,11 @@
import { mock } from "jest-mock-extended";
import { BehaviorSubject } from "rxjs";
import { BehaviorSubject, Observable } from "rxjs";
import type { CipherRiskOptions, CipherId, CipherRiskResult } from "@bitwarden/sdk-internal";
import type { CipherRiskOptions, CipherRiskResult } from "@bitwarden/sdk-internal";
import { asUuid } from "../../platform/abstractions/sdk/sdk.service";
import { MockSdkService } from "../../platform/spec/mock-sdk.service";
import { UserId } from "../../types/guid";
import { UserId, CipherId } from "../../types/guid";
import { CipherService } from "../abstractions/cipher.service";
import { CipherType } from "../enums/cipher-type";
import { CipherView } from "../models/view/cipher.view";
@@ -19,9 +19,9 @@ describe("DefaultCipherRiskService", () => {
let mockCipherService: jest.Mocked<CipherService>;
const mockUserId = "test-user-id" as UserId;
const mockCipherId1 = "cbea34a8-bde4-46ad-9d19-b05001228ab2";
const mockCipherId2 = "cbea34a8-bde4-46ad-9d19-b05001228ab3";
const mockCipherId3 = "cbea34a8-bde4-46ad-9d19-b05001228ab4";
const mockCipherId1 = "cbea34a8-bde4-46ad-9d19-b05001228ab2" as CipherId;
const mockCipherId2 = "cbea34a8-bde4-46ad-9d19-b05001228ab3" as CipherId;
const mockCipherId3 = "cbea34a8-bde4-46ad-9d19-b05001228ab4" as CipherId;
beforeEach(() => {
sdkService = new MockSdkService();
@@ -534,5 +534,56 @@ describe("DefaultCipherRiskService", () => {
// Verify password_reuse_map was called twice (fresh computation each time)
expect(mockCipherRiskClient.password_reuse_map).toHaveBeenCalledTimes(2);
});
it("should wait for a decrypted vault before computing risk", async () => {
const mockClient = sdkService.simulate.userLogin(mockUserId);
const mockCipherRiskClient = mockClient.vault.mockDeep().cipher_risk.mockDeep();
const cipher = new CipherView();
cipher.id = mockCipherId1;
cipher.type = CipherType.Login;
cipher.login = new LoginView();
cipher.login.password = "password1";
// Simulate the observable emitting null (undecrypted vault) first, then the decrypted ciphers
const cipherViewsSubject = new BehaviorSubject<CipherView[] | null>(null);
mockCipherService.cipherViews$.mockReturnValue(
cipherViewsSubject as Observable<CipherView[]>,
);
mockCipherRiskClient.password_reuse_map.mockReturnValue({});
mockCipherRiskClient.compute_risk.mockResolvedValue([
{
id: mockCipherId1 as any,
password_strength: 4,
exposed_result: { type: "NotChecked" },
reuse_count: 1,
},
]);
// Initiate the async call but don't await yet
const computePromise = cipherRiskService.computeCipherRiskForUser(
asUuid<CipherId>(mockCipherId1),
mockUserId,
true,
);
// Simulate a tick to allow the service to process the null emission
await new Promise((resolve) => setTimeout(resolve, 0));
// Now emit the actual decrypted ciphers
cipherViewsSubject.next([cipher]);
const result = await computePromise;
expect(mockCipherRiskClient.compute_risk).toHaveBeenCalledWith(
[expect.objectContaining({ password: "password1" })],
{
passwordMap: expect.any(Object),
checkExposed: true,
},
);
expect(result).toEqual(expect.objectContaining({ id: expect.anything() }));
});
});
});

View File

@@ -1,16 +1,17 @@
import { firstValueFrom, switchMap } from "rxjs";
import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
import { filterOutNullish } from "@bitwarden/common/vault/utils/observable-utilities";
import {
CipherLoginDetails,
CipherRiskOptions,
PasswordReuseMap,
CipherId,
CipherRiskResult,
CipherId as SdkCipherId,
} from "@bitwarden/sdk-internal";
import { SdkService, asUuid } from "../../platform/abstractions/sdk/sdk.service";
import { UserId } from "../../types/guid";
import { UserId, CipherId } from "../../types/guid";
import { CipherRiskService as CipherRiskServiceAbstraction } from "../abstractions/cipher-risk.service";
import { CipherType } from "../enums/cipher-type";
import { CipherView } from "../models/view/cipher.view";
@@ -52,7 +53,9 @@ export class DefaultCipherRiskService implements CipherRiskServiceAbstraction {
checkExposed: boolean = true,
): Promise<CipherRiskResult> {
// Get all ciphers for the user
const allCiphers = await firstValueFrom(this.cipherService.cipherViews$(userId));
const allCiphers = await firstValueFrom(
this.cipherService.cipherViews$(userId).pipe(filterOutNullish()),
);
// Find the specific cipher
const targetCipher = allCiphers?.find((c) => asUuid<CipherId>(c.id) === cipherId);
@@ -106,7 +109,7 @@ export class DefaultCipherRiskService implements CipherRiskServiceAbstraction {
.map(
(cipher) =>
({
id: asUuid<CipherId>(cipher.id),
id: asUuid<SdkCipherId>(cipher.id),
password: cipher.login.password!,
username: cipher.login.username,
}) satisfies CipherLoginDetails,