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 c58798d9d12..96c597113a5 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 @@ -1,4 +1,4 @@ -import { Component, Input } from "@angular/core"; +import { ChangeDetectionStrategy, Component, input } from "@angular/core"; import { ComponentFixture, TestBed } from "@angular/core/testing"; import { By } from "@angular/platform-browser"; import { mock } from "jest-mock-extended"; @@ -37,43 +37,32 @@ import { AtRiskCarouselDialogResult } from "../at-risk-carousel-dialog/at-risk-c import { AtRiskPasswordPageService } from "./at-risk-password-page.service"; import { AtRiskPasswordsComponent } from "./at-risk-passwords.component"; -// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush -// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @Component({ selector: "popup-header", template: ``, + changeDetection: ChangeDetectionStrategy.OnPush, }) class MockPopupHeaderComponent { - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input() pageTitle: string | undefined; - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input() backAction: (() => void) | undefined; + readonly pageTitle = input(undefined); + readonly backAction = input<(() => void) | undefined>(undefined); } -// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush -// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @Component({ selector: "popup-page", template: ``, + changeDetection: ChangeDetectionStrategy.OnPush, }) class MockPopupPageComponent { - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input() loading: boolean | undefined; + readonly loading = input(undefined); } -// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush -// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @Component({ selector: "app-vault-icon", template: ``, + changeDetection: ChangeDetectionStrategy.OnPush, }) class MockAppIcon { - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-signals - @Input() cipher: CipherView | undefined; + readonly cipher = input(undefined); } describe("AtRiskPasswordsComponent", () => { @@ -109,11 +98,15 @@ describe("AtRiskPasswordsComponent", () => { id: "cipher", organizationId: "org", name: "Item 1", + edit: true, + viewPassword: true, } as CipherView, { id: "cipher2", organizationId: "org", name: "Item 2", + edit: true, + viewPassword: true, } as CipherView, ]); mockOrgs$ = new BehaviorSubject([ @@ -235,6 +228,38 @@ describe("AtRiskPasswordsComponent", () => { organizationId: "org", name: "Item 1", isDeleted: true, + edit: true, + viewPassword: true, + } as CipherView, + ]); + + const items = await firstValueFrom(component["atRiskItems$"]); + expect(items).toHaveLength(0); + }); + + it("should not show tasks when cipher does not have edit permission", async () => { + mockCiphers$.next([ + { + id: "cipher", + organizationId: "org", + name: "Item 1", + edit: false, + viewPassword: true, + } as CipherView, + ]); + + const items = await firstValueFrom(component["atRiskItems$"]); + expect(items).toHaveLength(0); + }); + + it("should not show tasks when cipher does not have viewPassword permission", async () => { + mockCiphers$.next([ + { + id: "cipher", + organizationId: "org", + name: "Item 1", + edit: true, + viewPassword: false, } as CipherView, ]); @@ -288,11 +313,15 @@ describe("AtRiskPasswordsComponent", () => { id: "cipher", organizationId: "org", name: "Item 1", + edit: true, + viewPassword: true, } as CipherView, { id: "cipher2", organizationId: "org2", name: "Item 2", + edit: true, + viewPassword: true, } as CipherView, ]); 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 3eeb2d1917b..94fdb00f566 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,12 @@ import { CommonModule } from "@angular/common"; -import { Component, DestroyRef, inject, OnInit, signal } from "@angular/core"; +import { + Component, + DestroyRef, + inject, + OnInit, + signal, + ChangeDetectionStrategy, +} from "@angular/core"; import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; import { Router } from "@angular/router"; import { @@ -58,8 +65,6 @@ import { import { AtRiskPasswordPageService } from "./at-risk-password-page.service"; -// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush -// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @Component({ imports: [ PopupPageComponent, @@ -82,6 +87,7 @@ import { AtRiskPasswordPageService } from "./at-risk-password-page.service"; ], selector: "vault-at-risk-passwords", templateUrl: "./at-risk-passwords.component.html", + changeDetection: ChangeDetectionStrategy.OnPush, }) export class AtRiskPasswordsComponent implements OnInit { private taskService = inject(TaskService); @@ -158,6 +164,8 @@ export class AtRiskPasswordsComponent implements OnInit { t.type === SecurityTaskType.UpdateAtRiskCredential && t.cipherId != null && ciphers[t.cipherId] != null && + ciphers[t.cipherId].edit && + ciphers[t.cipherId].viewPassword && !ciphers[t.cipherId].isDeleted, ) .map((t) => ciphers[t.cipherId!]), diff --git a/libs/vault/src/services/at-risk-password-callout.service.spec.ts b/libs/vault/src/services/at-risk-password-callout.service.spec.ts index 47b83f4a903..5b687970c4b 100644 --- a/libs/vault/src/services/at-risk-password-callout.service.spec.ts +++ b/libs/vault/src/services/at-risk-password-callout.service.spec.ts @@ -28,6 +28,8 @@ class MockCipherView { constructor( public id: string, private deleted: boolean, + public edit: boolean = true, + public viewPassword: boolean = true, ) {} get isDeleted() { return this.deleted; @@ -65,33 +67,261 @@ describe("AtRiskPasswordCalloutService", () => { service = TestBed.inject(AtRiskPasswordCalloutService); }); + describe("pendingTasks$", () => { + it.each([ + { + description: + "returns tasks filtered by UpdateAtRiskCredential type with valid cipher permissions", + tasks: [ + { + id: "t1", + cipherId: "c1", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + { + id: "t2", + cipherId: "c2", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + ], + ciphers: [ + new MockCipherView("c1", false, true, true), + new MockCipherView("c2", false, true, true), + ], + expectedLength: 2, + expectedFirstId: "t1", + }, + { + description: "filters out tasks with wrong task type", + tasks: [ + { + id: "t1", + cipherId: "c1", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + { + id: "t2", + cipherId: "c2", + type: 999 as SecurityTaskType, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + ], + ciphers: [ + new MockCipherView("c1", false, true, true), + new MockCipherView("c2", false, true, true), + ], + expectedLength: 1, + expectedFirstId: "t1", + }, + { + description: "filters out tasks with missing associated cipher", + tasks: [ + { + id: "t1", + cipherId: "c1", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + { + id: "t2", + cipherId: "c-nonexistent", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + ], + ciphers: [new MockCipherView("c1", false, true, true)], + expectedLength: 1, + expectedFirstId: "t1", + }, + { + description: "filters out tasks when cipher edit permission is false", + tasks: [ + { + id: "t1", + cipherId: "c1", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + { + id: "t2", + cipherId: "c2", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + ], + ciphers: [ + new MockCipherView("c1", false, true, true), + new MockCipherView("c2", false, false, true), + ], + expectedLength: 1, + expectedFirstId: "t1", + }, + { + description: "filters out tasks when cipher viewPassword permission is false", + tasks: [ + { + id: "t1", + cipherId: "c1", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + { + id: "t2", + cipherId: "c2", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + ], + ciphers: [ + new MockCipherView("c1", false, true, true), + new MockCipherView("c2", false, true, false), + ], + expectedLength: 1, + expectedFirstId: "t1", + }, + { + description: "filters out tasks when cipher is deleted", + tasks: [ + { + id: "t1", + cipherId: "c1", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + { + id: "t2", + cipherId: "c2", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + ], + ciphers: [ + new MockCipherView("c1", false, true, true), + new MockCipherView("c2", true, true, true), + ], + expectedLength: 1, + expectedFirstId: "t1", + }, + ])("$description", async ({ tasks, ciphers, expectedLength, expectedFirstId }) => { + jest.spyOn(mockTaskService, "pendingTasks$").mockReturnValue(of(tasks)); + jest.spyOn(mockCipherService, "cipherViews$").mockReturnValue(of(ciphers)); + + const result = await firstValueFrom(service.pendingTasks$(userId)); + + expect(result).toHaveLength(expectedLength); + if (expectedFirstId) { + expect(result[0].id).toBe(expectedFirstId); + } + }); + + it("correctly filters mixed valid and invalid tasks", async () => { + const tasks: SecurityTask[] = [ + { + id: "t1", + cipherId: "c1", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + { + id: "t2", + cipherId: "c2", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + { + id: "t3", + cipherId: "c3", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + { + id: "t4", + cipherId: "c4", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + { + id: "t5", + cipherId: "c5", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + ]; + const ciphers = [ + new MockCipherView("c1", false, true, true), // valid + new MockCipherView("c2", false, false, true), // no edit + new MockCipherView("c3", true, true, true), // deleted + new MockCipherView("c4", false, true, false), // no viewPassword + // c5 missing + ]; + + jest.spyOn(mockTaskService, "pendingTasks$").mockReturnValue(of(tasks)); + jest.spyOn(mockCipherService, "cipherViews$").mockReturnValue(of(ciphers)); + + const result = await firstValueFrom(service.pendingTasks$(userId)); + + expect(result).toHaveLength(1); + expect(result[0].id).toBe("t1"); + }); + + it.each([ + { + description: "returns empty array when no tasks match filter criteria", + tasks: [ + { + id: "t1", + cipherId: "c1", + type: SecurityTaskType.UpdateAtRiskCredential, + status: SecurityTaskStatus.Pending, + } as SecurityTask, + ], + ciphers: [new MockCipherView("c1", true, true, true)], // deleted + }, + { + description: "returns empty array when no pending tasks exist", + tasks: [], + ciphers: [new MockCipherView("c1", false, true, true)], + }, + ])("$description", async ({ tasks, ciphers }) => { + jest.spyOn(mockTaskService, "pendingTasks$").mockReturnValue(of(tasks)); + jest.spyOn(mockCipherService, "cipherViews$").mockReturnValue(of(ciphers)); + + const result = await firstValueFrom(service.pendingTasks$(userId)); + + expect(result).toHaveLength(0); + }); + }); + describe("completedTasks$", () => { - it(" should return true if completed tasks exist", async () => { + it("returns true if completed tasks exist", async () => { const tasks: SecurityTask[] = [ { id: "t1", cipherId: "c1", type: SecurityTaskType.UpdateAtRiskCredential, status: SecurityTaskStatus.Completed, - } as any, + } as SecurityTask, { id: "t2", cipherId: "c2", type: SecurityTaskType.UpdateAtRiskCredential, status: SecurityTaskStatus.Pending, - } as any, + } as SecurityTask, { id: "t3", cipherId: "nope", type: SecurityTaskType.UpdateAtRiskCredential, status: SecurityTaskStatus.Completed, - } as any, + } as SecurityTask, { id: "t4", cipherId: "c3", type: SecurityTaskType.UpdateAtRiskCredential, status: SecurityTaskStatus.Completed, - } as any, + } as SecurityTask, ]; jest.spyOn(mockTaskService, "completedTasks$").mockReturnValue(of(tasks)); @@ -110,7 +340,7 @@ describe("AtRiskPasswordCalloutService", () => { jest.spyOn(mockCipherService, "cipherViews$").mockReturnValue(of([])); }); - it("should return false if banner has been dismissed", async () => { + it("returns false if banner has been dismissed", async () => { const state: AtRiskPasswordCalloutData = { hasInteractedWithTasks: true, tasksBannerDismissed: true, @@ -123,7 +353,7 @@ describe("AtRiskPasswordCalloutService", () => { expect(result).toBe(false); }); - it("should return true when has completed tasks, no pending tasks, and banner not dismissed", async () => { + it("returns true when has completed tasks, no pending tasks, and banner not dismissed", async () => { const completedTasks = [ { id: "t1", diff --git a/libs/vault/src/services/at-risk-password-callout.service.ts b/libs/vault/src/services/at-risk-password-callout.service.ts index d3af4f8421e..214a061399e 100644 --- a/libs/vault/src/services/at-risk-password-callout.service.ts +++ b/libs/vault/src/services/at-risk-password-callout.service.ts @@ -45,6 +45,8 @@ export class AtRiskPasswordCalloutService { return ( t.type === SecurityTaskType.UpdateAtRiskCredential && associatedCipher && + associatedCipher.edit && + associatedCipher.viewPassword && !associatedCipher.isDeleted ); });