From 700f54357c1212aac620916f6578e8838a08dfa6 Mon Sep 17 00:00:00 2001 From: Nick Krantz <125900171+nick-livefront@users.noreply.github.com> Date: Fri, 27 Jun 2025 16:04:51 -0500 Subject: [PATCH] [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 --- .../at-risk-password-callout.component.ts | 23 ++- .../at-risk-passwords.component.spec.ts | 27 +++ .../at-risk-passwords.component.ts | 17 +- .../default-cipher-form.service.spec.ts | 161 ++++++++++++++++++ .../services/default-cipher-form.service.ts | 51 +++++- 5 files changed, 268 insertions(+), 11 deletions(-) create mode 100644 libs/vault/src/cipher-form/services/default-cipher-form.service.spec.ts diff --git a/apps/browser/src/vault/popup/components/at-risk-callout/at-risk-password-callout.component.ts b/apps/browser/src/vault/popup/components/at-risk-callout/at-risk-password-callout.component.ts index 18482706272..3c3270e557c 100644 --- a/apps/browser/src/vault/popup/components/at-risk-callout/at-risk-password-callout.component.ts +++ b/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)), - map((tasks) => tasks.filter((t) => t.type === SecurityTaskType.UpdateAtRiskCredential)), + switchMap((userId) => + 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 + ); + }), + ), ); } 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 dae00ba6c2b..eaa10aba624 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 @@ -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"); }); 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 dc6712aa23f..1bfb65a15cc 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 @@ -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( - switchMap(({ tasks, userId }) => { - const orgIds = new Set(tasks.map((t) => t.organizationId)); + protected pageDescription$ = combineLatest([this.activeUserData$, this.atRiskItems$]).pipe( + switchMap(([{ userId }, atRiskCiphers]) => { + const orgIds = new Set( + atRiskCiphers.filter((c) => c.organizationId).map((c) => c.organizationId), + ) as Set; 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)); }), ); diff --git a/libs/vault/src/cipher-form/services/default-cipher-form.service.spec.ts b/libs/vault/src/cipher-form/services/default-cipher-form.service.spec.ts new file mode 100644 index 00000000000..3b2573b8f94 --- /dev/null +++ b/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(); + + 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(); + }); + }); +}); diff --git a/libs/vault/src/cipher-form/services/default-cipher-form.service.ts b/libs/vault/src/cipher-form/services/default-cipher-form.service.ts index 99f853d4c86..5228c85c3f7 100644 --- a/libs/vault/src/cipher-form/services/default-cipher-form.service.ts +++ b/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, b: Set) { 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 { 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); + } }