[PM-20379] Fix At-risk password task permission bug (#17110)

* [PM-20379] Fix at risk password task permission checks * [PM-20379] Fix at risk password component specs * [PM-20379] Cleanup FIXMEs * [PM-20379] Update to OnPush * [PM-20379] Add tests for pendingTasks$ * [PM-20379] Reduce test boilerplate / redundancy * [PM-20379] Cleanup as any * [PM-20379] Remove redundant "should" language
2025-12-13 06:43:35 +00:00 · 2025-10-29 14:47:55 -07:00
parent 9fca0b0138
commit 51a557514f
4 changed files with 298 additions and 29 deletions
--- 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: `<ng-content></ng-content>`,
+  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<string | undefined>(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: `<ng-content></ng-content>`,
+  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<boolean | 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: "app-vault-icon",
  template: `<ng-content></ng-content>`,
+  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<CipherView | undefined>(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<Organization[]>([
@@ -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,
      ]);

--- 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!]),
--- 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",
--- 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
          );
        });