[PM-20041] Marking Task as complete (#14980)

* When saving a cipher, mark any associated security tasks as complete

* fix test error from encryption refactor

* hide security tasks that are associated with deleted ciphers (#15247)

* account for deleted ciphers for atRiskPasswordDescriptions

apps/browser/src/vault/popup/components/at-risk-callout/at-risk-password-callout.component.ts

@@ -1,10 +1,11 @@
 import { CommonModule } from "@angular/common";
 import { Component, inject } from "@angular/core";
 import { RouterModule } from "@angular/router";
-import { map, switchMap } from "rxjs";
+import { combineLatest, map, switchMap } from "rxjs";
 
 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
 import { getUserId } from "@bitwarden/common/auth/services/account.service";
+import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
 import { SecurityTaskType, TaskService } from "@bitwarden/common/vault/tasks";
 import { AnchorLinkDirective, CalloutModule } from "@bitwarden/components";
 import { I18nPipe } from "@bitwarden/ui-common";
@@ -16,10 +17,26 @@ import { I18nPipe } from "@bitwarden/ui-common";
 })
 export class AtRiskPasswordCalloutComponent {
   private taskService = inject(TaskService);
+  private cipherService = inject(CipherService);
   private activeAccount$ = inject(AccountService).activeAccount$.pipe(getUserId);
 
   protected pendingTasks$ = this.activeAccount$.pipe(
-    switchMap((userId) => this.taskService.pendingTasks$(userId)),
+    switchMap((userId) =>
-    map((tasks) => tasks.filter((t) => t.type === SecurityTaskType.UpdateAtRiskCredential)),
+      combineLatest([
+        this.taskService.pendingTasks$(userId),
+        this.cipherService.cipherViews$(userId),
+      ]),
+    ),
+    map(([tasks, ciphers]) =>
+      tasks.filter((t) => {
+        const associatedCipher = ciphers.find((c) => c.id === t.cipherId);
+
+        return (
+          t.type === SecurityTaskType.UpdateAtRiskCredential &&
+          associatedCipher &&
+          !associatedCipher.isDeleted
+        );
+      }),
+    ),
   );
 }

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

@@ -203,6 +203,20 @@ describe("AtRiskPasswordsComponent", () => {
       expect(items).toHaveLength(1);
       expect(items[0].name).toBe("Item 1");
     });
+
+    it("should not show tasks associated with deleted ciphers", async () => {
+      mockCiphers$.next([
+        {
+          id: "cipher",
+          organizationId: "org",
+          name: "Item 1",
+          isDeleted: true,
+        } as CipherView,
+      ]);
+
+      const items = await firstValueFrom(component["atRiskItems$"]);
+      expect(items).toHaveLength(0);
+    });
   });
 
   describe("pageDescription$", () => {
@@ -245,6 +259,19 @@ describe("AtRiskPasswordsComponent", () => {
           type: SecurityTaskType.UpdateAtRiskCredential,
         } as SecurityTask,
       ]);
+      mockCiphers$.next([
+        {
+          id: "cipher",
+          organizationId: "org",
+          name: "Item 1",
+        } as CipherView,
+        {
+          id: "cipher2",
+          organizationId: "org2",
+          name: "Item 2",
+        } as CipherView,
+      ]);
+
       const description = await firstValueFrom(component["pageDescription$"]);
       expect(description).toBe("atRiskPasswordsDescMultiOrgPlural");
     });

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

@@ -155,32 +155,35 @@ export class AtRiskPasswordsComponent implements OnInit {
           (t) =>
             t.type === SecurityTaskType.UpdateAtRiskCredential &&
             t.cipherId != null &&
-            ciphers[t.cipherId] != null,
+            ciphers[t.cipherId] != null &&
+            !ciphers[t.cipherId].isDeleted,
         )
         .map((t) => ciphers[t.cipherId!]),
     ),
   );
 
-  protected pageDescription$ = this.activeUserData$.pipe(
+  protected pageDescription$ = combineLatest([this.activeUserData$, this.atRiskItems$]).pipe(
-    switchMap(({ tasks, userId }) => {
+    switchMap(([{ userId }, atRiskCiphers]) => {
-      const orgIds = new Set(tasks.map((t) => t.organizationId));
+      const orgIds = new Set(
+        atRiskCiphers.filter((c) => c.organizationId).map((c) => c.organizationId),
+      ) as Set<string>;
       if (orgIds.size === 1) {
         const [orgId] = orgIds;
         return this.organizationService.organizations$(userId).pipe(
           getOrganizationById(orgId),
           map((org) =>
             this.i18nService.t(
-              tasks.length === 1
+              atRiskCiphers.length === 1
                 ? "atRiskPasswordDescSingleOrg"
                 : "atRiskPasswordsDescSingleOrgPlural",
               org?.name,
-              tasks.length,
+              atRiskCiphers.length,
             ),
           ),
         );
       }
 
-      return of(this.i18nService.t("atRiskPasswordsDescMultiOrgPlural", tasks.length));
+      return of(this.i18nService.t("atRiskPasswordsDescMultiOrgPlural", atRiskCiphers.length));
     }),
   );
 

libs/vault/src/cipher-form/services/default-cipher-form.service.spec.ts

@@ -0,0 +1,161 @@
+import { TestBed } from "@angular/core/testing";
+import { mock } from "jest-mock-extended";
+import { of } from "rxjs";
+
+import { Account, AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+import { UserId } from "@bitwarden/common/types/guid";
+import {
+  CipherService,
+  EncryptionContext,
+} from "@bitwarden/common/vault/abstractions/cipher.service";
+import { Cipher } from "@bitwarden/common/vault/models/domain/cipher";
+import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view";
+import { LoginView } from "@bitwarden/common/vault/models/view/login.view";
+import { SecurityTaskType, TaskService } from "@bitwarden/common/vault/tasks";
+import { CipherType } from "@bitwarden/sdk-internal";
+
+import { CipherFormConfig } from "../abstractions/cipher-form-config.service";
+
+import { DefaultCipherFormService } from "./default-cipher-form.service";
+
+describe("DefaultCipherFormService", () => {
+  let service: DefaultCipherFormService;
+  let testBed: TestBed;
+  const cipherServiceMock = mock<CipherService>();
+
+  let markAsCompleteMock: jest.Mock;
+  let pendingTasks$: jest.Mock;
+
+  beforeEach(() => {
+    markAsCompleteMock = jest.fn().mockResolvedValue(undefined);
+    pendingTasks$ = jest.fn().mockReturnValue(of([]));
+    cipherServiceMock.encrypt.mockResolvedValue({} as EncryptionContext);
+
+    testBed = TestBed.configureTestingModule({
+      providers: [
+        { provide: CipherService, useValue: cipherServiceMock },
+        { provide: TaskService, useValue: { markAsComplete: markAsCompleteMock, pendingTasks$ } },
+        {
+          provide: AccountService,
+          useValue: { activeAccount$: of({ id: "user-1" as UserId } as Account) },
+        },
+        DefaultCipherFormService,
+      ],
+    });
+
+    service = testBed.inject(DefaultCipherFormService);
+  });
+
+  describe("markAssociatedTaskAsComplete", () => {
+    it("does not call markAsComplete when the cipher is not a login", async () => {
+      pendingTasks$.mockReturnValueOnce(
+        of([
+          {
+            type: SecurityTaskType.UpdateAtRiskCredential,
+            cipherId: "cipher-1",
+            userId: "user-1" as UserId,
+          },
+        ]),
+      );
+
+      const cardCipher = new CipherView();
+      cardCipher.type = CipherType.Card;
+      cardCipher.id = "cipher-1";
+
+      await service.saveCipher(cardCipher, {
+        originalCipher: new Cipher(),
+        admin: false,
+      } as CipherFormConfig);
+
+      expect(markAsCompleteMock).not.toHaveBeenCalled();
+    });
+
+    it("does not call markAsComplete when there is no associated credential tasks", async () => {
+      pendingTasks$.mockReturnValueOnce(of([]));
+
+      const originalCipher = new Cipher();
+      originalCipher.type = CipherType.Login;
+
+      const cipher = new CipherView();
+      cipher.type = CipherType.Login;
+      cipher.id = "cipher-1";
+      cipher.login = new LoginView();
+      cipher.login.password = "password123";
+
+      cipherServiceMock.decrypt.mockResolvedValue({
+        ...cipher,
+        login: {
+          ...cipher.login,
+          password: "newPassword123",
+        },
+      } as CipherView);
+
+      await service.saveCipher(cipher, {
+        originalCipher: originalCipher,
+        admin: false,
+      } as CipherFormConfig);
+
+      expect(markAsCompleteMock).not.toHaveBeenCalled();
+    });
+
+    it("does not call markAsComplete when the password has not changed", async () => {
+      pendingTasks$.mockReturnValueOnce(
+        of([
+          {
+            type: SecurityTaskType.UpdateAtRiskCredential,
+            cipherId: "cipher-1",
+            userId: "user-1" as UserId,
+          },
+        ]),
+      );
+
+      const cipher = new CipherView();
+      cipher.type = CipherType.Login;
+      cipher.id = "cipher-1";
+      cipher.login = new LoginView();
+      cipher.login.password = "password123";
+
+      cipherServiceMock.decrypt.mockResolvedValue(cipher);
+
+      await service.saveCipher(cipher, {
+        originalCipher: new Cipher(),
+        admin: false,
+      } as CipherFormConfig);
+
+      expect(markAsCompleteMock).not.toHaveBeenCalled();
+    });
+
+    it("calls markAsComplete when the cipher password has changed and there is an associated credential task", async () => {
+      pendingTasks$.mockReturnValueOnce(
+        of([
+          {
+            type: SecurityTaskType.UpdateAtRiskCredential,
+            cipherId: "cipher-1",
+            userId: "user-1" as UserId,
+          },
+        ]),
+      );
+
+      const cipher = new CipherView();
+      cipher.type = CipherType.Login;
+      cipher.id = "cipher-1";
+      cipher.login = new LoginView();
+      cipher.login.password = "password123";
+
+      cipherServiceMock.decrypt.mockResolvedValue({
+        ...cipher,
+        login: {
+          ...cipher.login,
+          password: "newPassword123",
+        },
+      } as CipherView);
+
+      await service.saveCipher(cipher, {
+        originalCipher: new Cipher(),
+        admin: false,
+      } as CipherFormConfig);
+
+      expect(markAsCompleteMock).toHaveBeenCalled();
+    });
+  });
+});

libs/vault/src/cipher-form/services/default-cipher-form.service.ts

@@ -1,13 +1,16 @@
 // FIXME: Update this file to be type safe and remove this and next line
 // @ts-strict-ignore
 import { inject, Injectable } from "@angular/core";
-import { firstValueFrom } from "rxjs";
+import { firstValueFrom, map } from "rxjs";
 
 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
 import { getUserId } from "@bitwarden/common/auth/services/account.service";
+import { UserId } from "@bitwarden/common/types/guid";
 import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
+import { CipherType } from "@bitwarden/common/vault/enums";
 import { Cipher } from "@bitwarden/common/vault/models/domain/cipher";
 import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view";
+import { SecurityTaskType, TaskService } from "@bitwarden/common/vault/tasks";
 
 import { CipherFormConfig } from "../abstractions/cipher-form-config.service";
 import { CipherFormService } from "../abstractions/cipher-form.service";
@@ -20,6 +23,7 @@ function isSetEqual(a: Set<string>, b: Set<string>) {
 export class DefaultCipherFormService implements CipherFormService {
   private cipherService: CipherService = inject(CipherService);
   private accountService: AccountService = inject(AccountService);
+  private taskService: TaskService = inject(TaskService);
 
   async decryptCipher(cipher: Cipher): Promise<CipherView> {
     const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
@@ -89,6 +93,8 @@ export class DefaultCipherFormService implements CipherFormService {
       }
     }
 
+    await this.markAssociatedTaskAsComplete(activeUserId, cipher, config);
+
     // Its possible the cipher was made no longer available due to collection assignment changes
     // e.g. The cipher was moved to a collection that the user no longer has access to
     if (savedCipher == null) {
@@ -97,4 +103,47 @@ export class DefaultCipherFormService implements CipherFormService {
 
     return await this.cipherService.decrypt(savedCipher, activeUserId);
   }
+
+  /**
+   * When a cipher has an associated pending `UpdateAtRiskCredential` task
+   * and the password has changed, mark the task as complete.
+   */
+  private async markAssociatedTaskAsComplete(
+    userId: UserId,
+    updatedCipher: CipherView,
+    config: CipherFormConfig,
+  ) {
+    const decryptedOriginalCipherCipher = await this.cipherService.decrypt(
+      config.originalCipher,
+      userId,
+    );
+
+    const associatedPendingTask = await firstValueFrom(
+      this.taskService
+        .pendingTasks$(userId)
+        .pipe(
+          map((tasks) =>
+            tasks.find(
+              (task) =>
+                task.type === SecurityTaskType.UpdateAtRiskCredential &&
+                task.cipherId === updatedCipher.id,
+            ),
+          ),
+        ),
+    );
+
+    const passwordHasChanged =
+      updatedCipher.type === CipherType.Login &&
+      updatedCipher.login.password &&
+      updatedCipher.login.password !== decryptedOriginalCipherCipher?.login?.password;
+
+    // When there is not an associated pending task or the password has not changed,
+    // no action needed-return early.
+    if (!associatedPendingTask || !passwordHasChanged) {
+      return;
+    }
+
+    // If the cipher is a login and the password has changed, mark the associated task as complete
+    await this.taskService.markAsComplete(associatedPendingTask.id, userId);
+  }
 }